Adds Decorator Metadata by pzuraq · Pull Request #10 · pzuraq/ecma262 (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation21 Commits18 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Adds the spec implementation for the current iteration of Decorator Metadata. Since this builds off the decorators proposal, this PR is currently being made against that proposals PR. Once that proposal is merged into the spec fully, this branch will be rebased and a new PR will be made.
Chris Garrett and others added 6 commits
Updates
- Refactor to use ClassElementDefinition more extensively
- Separate parsing class elements and defining them so that we can control the order of definition and decorator application
- Extract and centralize class element decoration into a single place
- Fix completions in ClassDefinitionEvaluation (need to reset the env after each possible abrupt completion).
- Refactor adding private methods to instance to pull directly from
Class.[[Elements]], so we don't have multiple sources of truth for the elements that are defined on a class.Class.[[Elements]]is the canonical source of truth after ClassDefinitionEvaluation, and any operations afterward such as instantiation should base themselves on that. - Refactor to use anonymous function syntax.
Remove dynamic [[HomeObject]] from decorator return values
…allable
Throw an error if the value passed to addInitializer is not callable
Set the name of the addInitializer function
Remove setFunctionName from decorator application
Call decorators with the natural this value
| @@ -25194,39 +25210,43 @@ |
|---|
| 1. Set the running execution context's LexicalEnvironment to _env_. |
| 1. Let _instanceExtraInitializers_ be a new empty List. |
| 1. Let _staticExtraInitializers_ be a new empty List. |
| 1. Let _metadataParent_ be ? Get(_constructorParent_, @@metadata). |
| 1. If _metadataParent_ is *undefined*, let _metadataParent_ be %Object.prototype%. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we bottom out in null instead of %Object.prototype%?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and updated, thanks for calling that out!
| 1. If _newF_ is an abrupt completion, then |
|---|
| 1. Set the running execution context's PrivateEnvironment to _outerPrivateEnvironment_. |
| 1. Return ? _result_. |
| 1. Set _F_ to _newF_.[[Value]]. |
| 1. Perform ! DefinePropertyOrThrow(_F_, @@metadata, PropertyDescriptor { [[Value]]: _metadataObj_, [[Writable]]: *true*, [[Enumerable]]: *false*, [[Configurable]]: *true* }). |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the class object might have gotten swapped out by a class decorator (or the class could have been mutated), which might cause this to fail in a variety of ways. So this needs to be ?.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point, I'll revise this asap
| @@ -25194,39 +25210,43 @@ |
|---|
| 1. Set the running execution context's LexicalEnvironment to _env_. |
| 1. Let _instanceExtraInitializers_ be a new empty List. |
| 1. Let _staticExtraInitializers_ be a new empty List. |
| 1. Let _metadataParent_ be ? Get(_constructorParent_, @@metadata). |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So _constructorParent_ gets defaulted to %Function.prototype% when there is no extends clause. Which means someone can assign to Function.prototype[Symbol.metadata] to be the default parent for all metadata. That seems... not good? Should we do this Get only if |ClassHeritage?| is present?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like making the Get conditional. Alternatively, we could make Function.prototype[Symbol.metadata] nonconfigurable-nonwritable null.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, one thing I'm realizing here is that all classes get Symbol.metadata, I wonder if we should add the metadata object in cases where decorators are applied, so that it's more performant? If so, then nonconfigurable-nonwritable null would make more sense, otherwise we could do the Get conditional.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intuition is that classes without any decorators don't have a metadata object. But we should probably talk about this in committee?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that was my original intention, but I just realized on reading that I hadn't made that part optional in the spec. We can discuss in committee.
@@ -30619,6 +30628,7 @@ Properties of the Function Prototype Object |
|---|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be its own section - we only really do "name" and "length" like this.
Compare, for example, GeneratorFunction.prototype [ @@toStringTag ]. (Except of course it should be non-configurable, and then it can be just "the value" rather than "the initial value".)
I'm neutral on whether to include the explanation in that section as well or just leave it out entirely.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was looking for something similar and couldn't find it, I'll update!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the lint failure and my comment (which does not need to block advancement).
| @@ -24319,6 +24330,7 @@ |
|---|
| _name_: a String, a Symbol, or a Private Name, |
| _initializers_: a List of function objects, |
| _decorationState_: a Record with a field [[Finished]] (a Boolean), |
| _metadataObj_: an Object or ~empty~, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't actually be ~empty~, can it? We only call this method if there are decorators.
That would be better handled by making this just "an Object" and adding an Assert: _metadataObj_ is not ~empty~ at the callsites.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's a good point, I can add conditionals there as well.
@@ -30724,6 +30755,13 @@ Function.prototype [ @@hasInstance ] ( _V_ ) |
|---|
This property is non-writable and non-configurable to prevent tampering that could be used to globally expose the target function of a bound function. |
The value of the *"name"* property of this method is *"[Symbol.hasInstance]"*. |
Symbol.prototype [ @@metadata ] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <h1>Symbol.prototype [ @@metadata ]</h1> |
|---|
| <h1>Function.prototype [ @@metadata ]</h1> |
Symbol.prototype [ @@metadata ] |
|---|
The initial value of the @@metadata property is *null*. |
This property has the attributes { [[Writable]]: *false*, [[Enumerable]]: *false*, [[Configurable]]: *false* }. |
This property is non-writable and non-configurable to prevent tampering that could be used to globally expose the target function of a bound function. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a copy/paste error?
| @@ -25240,39 +25261,49 @@ |
|---|
| 1. Set the running execution context's LexicalEnvironment to _env_. |
| 1. Let _instanceExtraInitializers_ be a new empty List. |
| 1. Let _staticExtraInitializers_ be a new empty List. |
| 1. Let _metadataObj_ be ~empty~. |
| 1. If _hasDecorators_ is *true*, then |
| 1. If |ClassHeritage? |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that class extends null {} actually works for construction, but won't this mean that @dec class extends null {} will throw?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, that seems better then doing what class extends null {} does (until we're able to fix extending null)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case, I'd maybe call it out in some sort of editorial note to indicate that the behavior is intended.
| 1. If _hasDecorators_ is *true*, then |
|---|
| 1. If |ClassHeritage? |
| 1. Else, let _metadataParent_ be *null*. |
| 1. Set _metadataObj_ to OrdinaryObjectCreate(_metadataParent_). |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OrdinaryObjectCreate only accepts an object or null, but _constructorParent_[@@metadata] could be any ECMAScript value.
Probably we shuold:
- if
_constructorParent_[@@metadata]is undefined, fallback to null - if it's a different primitive, throw a TypeError
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe always throw, given that there is the Function.prototype[Symbol.metadata] fallback and thus undefined must have been explicitly set.
Reviewers
