Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Decorator Metadata #10

Open
wants to merge 18 commits into
base: decorators
Choose a base branch
from
Open

Adds Decorator Metadata #10

wants to merge 18 commits into from

Conversation

pzuraq
Copy link
Owner

@pzuraq pzuraq commented Mar 7, 2023

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 March 5, 2023 22:12
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.
pzuraq and others added 7 commits April 12, 2023 04:27
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
spec.html Outdated
@@ -25194,39 +25210,43 @@ <h1>
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%.
Copy link

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%?

Copy link
Owner Author

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* }).
Copy link

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 ?.

Copy link
Owner Author

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

spec.html Outdated
@@ -25194,39 +25210,43 @@ <h1>
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).
Copy link

@syg syg May 10, 2023

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?

Copy link

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.

Copy link
Owner Author

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.

Copy link

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?

Copy link
Owner Author

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.

spec.html Outdated
@@ -30619,6 +30628,7 @@ <h1>Properties of the Function Prototype Object</h1>
<li>does not have a *"prototype"* property.</li>
<li>has a *"length"* property whose value is *+0*<sub>𝔽</sub>.</li>
<li>has a *"name"* property whose value is the empty String.</li>
<li>has a @@metadata property whose value is *null*, which is non-writable and non-configurable to prevent tampering that could be used to globally add metadata to all classes.</li>
Copy link

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.

Copy link
Owner Author

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!

Copy link

@bakkot bakkot left a comment

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 @@ <h1>
_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~,
Copy link

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.

Copy link
Owner Author

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 @@ <h1>Function.prototype [ @@hasInstance ] ( _V_ )</h1>
<p>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.</p>
<p>The value of the *"name"* property of this method is *"[Symbol.hasInstance]"*.</p>
</emu-clause>

<emu-clause id="sec-function.prototype-@@metadata">
<h1>Symbol.prototype [ @@metadata ]</h1>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<h1>Symbol.prototype [ @@metadata ]</h1>
<h1>Function.prototype [ @@metadata ]</h1>

<h1>Symbol.prototype [ @@metadata ]</h1>
<p>The initial value of the @@metadata property is *null*.</p>
<p>This property has the attributes { [[Writable]]: *false*, [[Enumerable]]: *false*, [[Configurable]]: *false* }.</p>
<p>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.</p>
Copy link

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 @@ <h1>
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?| is present, 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.

Not that class extends null {} actually works for construction, but won't this mean that @dec class extends null {} will throw?

Copy link

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?| is present, let _metadataParent_ be ? Get(_constructorParent_, @@metadata).
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants