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.

Sign up for GitHub

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

Jump to bottom

Add normative changes from January 2023 plenary #4

Merged
merged 3 commits into from Feb 9, 2023
Merged

Add normative changes from January 2023 plenary #4

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66bf36b
Add normative changes from January 2023 plenary
rbuckton Feb 5, 2023
be6db19
Swap PrivateSet/Set to correct steps in context.access.set
rbuckton Feb 6, 2023
b978144
Address editor feedback
rbuckton Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
82 changes: 57 additions & 25 deletions spec.html
View file
Open in desktop
Expand Up @@ -24382,7 +24382,7 @@ <h1>Runtime Semantics: DecoratorListEvaluation ( ): either a normal completion c
<h1>
CreateDecoratorAccessObject (
_kind_: ~field~, ~method~, ~accessor~, ~getter~, or ~setter~,
_name_: a String or Private Name,
_name_: either a String, a Symbol, or a Private Name,
): an Object
</h1>
<dl class="header">
Expand All @@ -24392,20 +24392,27 @@ <h1>
<emu-alg>
1. Let _accessObj_ be OrdinaryObjectCreate(%Object.prototype%).
1. If _kind_ is ~field~, ~method~, ~accessor~, or ~getter~, then
1. Let _getterClosure_ be a new Abstract Closure with no parameters that captures _name_ and performs the following steps when called:
1. Let _o_ be the *this* value.
1. If _name_ is a String, return ? Get(_o_, _name_).
1. Else, return ? PrivateGet(_name_, _o_).
1. Let _getter_ be CreateBuiltinFunction(_getterClosure_, 0, *""*, &laquo; &raquo;).
1. Let _getterClosure_ be a new Abstract Closure with parameter (_obj_) that captures _name_ and performs the following steps when called:
1. If _obj_ is not an Object, throw a *TypeError* exception.
1. If _name_ is either a String or a Symbol, return ? Get(_obj_, _name_).
Copy link

Choose a reason for hiding this comment

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

The change to accept Symbols looks good, but just to confirm my understanding, that's a bugfix unrelated to the normative changes presented in committee, right?

(I have no problem with it going in with this PR, I just want to make sure I understand why it's here.)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, its a bugfix. Without it, we'd try to do a PrivateGet using a symbol and not a Private Name, since name is already declared to be a String, Symbol, or Private Name in the AO.

Copy link
Author

Choose a reason for hiding this comment

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

(I have no problem with it going in with this PR, I just want to make sure I understand why it's here.)

It's a bit of a drive-by bugfix since I needed to rewrite the algorithm steps for both get and set anyways and noticed the oversight.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, its a bugfix. Without it, we'd try to do a PrivateGet using a symbol and not a Private Name, since name is already declared to be a String, Symbol, or Private Name in the AO.

Correction: name could already be a String, Symbol, or Private Name per the name argument to the CreateDecoratorContextObject AO.

1. Else, return ? PrivateGet(_name_, _obj_).
1. Let _getter_ be CreateBuiltinFunction(_getterClosure_, 1, *""*, &laquo; &raquo;).
1. Perform ! CreateDataPropertyOrThrow(_accessObj_, *"get"*, _getter_).
1. If _kind_ is ~field~, ~accessor~, or ~setter~, then
1. Let _setterClosure_ be a new Abstract Closure with parameters (_value_) that captures _name_ and performs the following steps when called:
1. Let _o_ be the *this* value.
1. If _name_ is a String, perform ? PrivateSet(_name_, _o_, _value_).
1. Else, perform ? Set(_o_, _name_, _value_, *true*).
1. Let _setterClosure_ be a new Abstract Closure with parameters (_obj_, _value_) that captures _name_ and performs the following steps when called:
1. If _obj_ is not an Object, throw a *TypeError* exception.
1. If _name_ is either a String or a Symbol, perform ? PrivateSet(_name_, _obj_, _value_).
1. Else, perform ? Set(_obj_, _name_, _value_, *true*).
rbuckton marked this conversation as resolved.
Show resolved Hide resolved
1. Return *undefined*.
1. Let _setter_ be CreateBuiltinFunction(_setterClosure_, 1, *""*, &laquo; &raquo;).
1. Let _setter_ be CreateBuiltinFunction(_setterClosure_, 2, *""*, &laquo; &raquo;).
1. Perform ! CreateDataPropertyOrThrow(_accessObj_, *"set"*, _setter_).
1. Let _hasClosure_ be a new Abstract Closure with parameter (_obj_) that captures _name_ and performs the following steps when called:
1. If _obj_ is not an Object, throw a *TypeError* exception.
1. If _name_ is either a String or a Symbol, return ? HasProperty(_obj_, _name_).
1. If PrivateElementFind(_obj_, _name_) is not ~empty~, return *true*.
1. Return *false*.
1. Let _has_ be CreateBuiltinFunction(_hasClosure_, 1, *""*, &laquo; &raquo;).
1. Perform ! CreateDataPropertyOrThrow(_accessObj_, *"has"*, _has_).
1. Return _accessObj_.
</emu-alg>
</emu-clause>
Expand Down Expand Up @@ -25416,15 +25423,16 @@ <h1>
</emu-clause>

<emu-clause id="sec-runtime-semantics-bindingclassdeclarationevaluation" type="sdo">
<h1>Runtime Semantics: BindingClassDeclarationEvaluation ( ): either a normal completion containing a function object or an abrupt completion</h1>
<h1>
Runtime Semantics: BindingClassDeclarationEvaluation (
_decorators_: a List,
): either a normal completion containing a function object or an abrupt completion
</h1>
<dl class="header">
</dl>
<emu-grammar>ClassDeclaration : DecoratorList? `class` BindingIdentifier ClassTail</emu-grammar>
<emu-alg>
1. Let _className_ be StringValue of |BindingIdentifier|.
1. If |DecoratorList?| is present, then
1. Let _decorators_ be ? DecoratorListEvaluation of |DecoratorList|.
1. Else, let _decorators_ be a new empty List.
1. Let _value_ be ? ClassDefinitionEvaluation of |ClassTail| with arguments _className_, _className_, and _decorators_.
1. Set _value_.[[SourceText]] to the source text matched by |ClassDeclaration|.
1. Let _env_ be the running execution context's LexicalEnvironment.
Expand All @@ -25433,9 +25441,6 @@ <h1>Runtime Semantics: BindingClassDeclarationEvaluation ( ): either a normal co
</emu-alg>
<emu-grammar>ClassDeclaration : DecoratorList? `class` ClassTail</emu-grammar>
<emu-alg>
1. If |DecoratorList?| is present, then
1. Let _decorators_ be ? DecoratorListEvaluation of |DecoratorList|.
1. Else, let _decorators_ be a new empty List.
1. Let _value_ be ? ClassDefinitionEvaluation of |ClassTail| with arguments *undefined*, *"default"*, and _decorators_.
1. Set _value_.[[SourceText]] to the source text matched by |ClassDeclaration|.
1. Return _value_.
Expand All @@ -25449,7 +25454,10 @@ <h1>Runtime Semantics: BindingClassDeclarationEvaluation ( ): either a normal co
<h1>Runtime Semantics: Evaluation</h1>
<emu-grammar>ClassDeclaration : DecoratorList? `class` BindingIdentifier ClassTail</emu-grammar>
<emu-alg>
1. Perform ? BindingClassDeclarationEvaluation of this |ClassDeclaration|.
1. If |DecoratorList?| is present, then
1. Let _decorators_ be ? DecoratorListEvaluation of |DecoratorList|.
1. Else, let _decorators_ be a new empty List.
1. Perform ? BindingClassDeclarationEvaluation of this |ClassDeclaration| with argument _decorators_.
1. Return ~empty~.
</emu-alg>
<emu-note>
Expand Down Expand Up @@ -28653,9 +28661,9 @@ <h2>Syntax</h2>
`export` ExportFromClause FromClause `;`
`export` NamedExports `;`
`export` VariableStatement[~Yield, +Await]
`export` Declaration[~Yield, +Await]
DecoratorList[~Yield, +Await]? `export` Declaration[~Yield, +Await]
Copy link

Choose a reason for hiding this comment

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

I think it would be slightly easier to follow the grammar if it was defined like this:

`export` LexicalDeclaration[~Yield, +Await]
`export` HoistableDeclaration[~Yield, +Await]
`export` ClassDeclaration[~Yield, +Await, +Decorators]
DecoratorList[~Yield, +Await]? `export` ClassDeclaration[~Yield, +Await, ~Decorators]

`export` `default` HoistableDeclaration[~Yield, +Await, +Default]
`export` `default` ClassDeclaration[~Yield, +Await, +Decorators, +Default]
DecoratorList[~Yield, +Await]? `export` `default` ClassDeclaration[~Yield, +Await, +Decorators, ~Default]
`export` `default` [lookahead &notin; { `function`, `async` [no LineTerminator here] `function`, `class`, `@` }] AssignmentExpression[+In, ~Yield, +Await] `;`

because the grammar rules are in a single place and you don't have to read both the grammar and the syntactic early errors. However, the end result is effectively the same so avoiding the new production parameter as you proposed is also ok 👍

Copy link
Author

Choose a reason for hiding this comment

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

I considered that, and that was what I'd implemented originally in https://github.com/tc39/proposal-decorators/pull/95/files. The early error seemed a little easier to reason over given that a Decorators parameter doesn't go very deep.

Copy link
Author

Choose a reason for hiding this comment

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

There's also a bug in esmeta regarding multiple constraints on an RHS as would be needed here. That bug has since been fixed, but hasn't been released yet. I think there's still some work to go on the decorators PR before it will pass esmeta anyways, so it doesn't really matter too much either way.

Copy link

Choose a reason for hiding this comment

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

I think I would prefer to have a new UndecoratedClassDeclaration nonterminal which derives what ClassDeclaration currently does in ecma262, and then have ClassDeclaration derive DecoratorList? UndecoratedClassDeclaration. Then you can use UndecoratedClassDeclaration here, no parameters or early errors necessary.

Still, that's an editorial change which can be made later. This change is correct as-is. Only reason to do it now is if it would cause TS or Babel to use a different representation in their ASTs; I know sometimes people like to have the AST shapes stick close to the spec (though this seems like a case where you would not want that).

Copy link
Author

Choose a reason for hiding this comment

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

I don't know about Babel, but TypeScript won't introduce a new AST node to differentiate. We just store decorators as if they were modifiers now (a change we made in TS 4.9 to support decorators-after-export syntax), so export, default, declare, abstract, and decorators all end up in the same bucket. An UndecoratedClassDeclaration production feels more like a workaround for the grammar, much like cover grammars. I don't expect we would really tie any unique semantics to it, and it might even make handling Function.prototype.toString a bit more complicated.

Copy link

Choose a reason for hiding this comment

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

Eh, I disagree - using productions which precisely express what you actually want to accept is the normal use of a CFG, and grammar parameters and early errors are the workaround for when that's not practical.

Fair point about Function.prototype.toString, but I'd have to actually write it out to tell if it will actually be a problem.

`export` `default` HoistableDeclaration[~Yield, +Await, +Default]
`export` `default` ClassDeclaration[~Yield, +Await, +Default]
DecoratorList[~Yield, +Await]? `export` `default` ClassDeclaration[~Yield, +Await, +Default]
`export` `default` [lookahead &notin; { `function`, `async` [no LineTerminator here] `function`, `class`, `@` }] AssignmentExpression[+In, ~Yield, +Await] `;`

ExportFromClause :
Expand Down Expand Up @@ -28691,6 +28699,21 @@ <h1>Static Semantics: Early Errors</h1>
<emu-note>
<p>The above rule means that each ReferencedBindings of |NamedExports| is treated as an |IdentifierReference|.</p>
</emu-note>
<emu-grammar>ExportDeclaration : DecoratorList? `export` Declaration</emu-grammar>
<ul>
<li>
It is a Syntax Error if |DecoratorList| is present and |Declaration| is not |ClassDeclaration|.
</li>
<li>
It is a Syntax Error if |DecoratorList| is present and |Declaration| is a |ClassDeclaration| and the |DecoratorList| of that |ClassDeclaration| is present.
</li>
</ul>
<emu-grammar>ExportDeclaration : DecoratorList? `export` `default` ClassDeclaration</emu-grammar>
<ul>
<li>
It is a Syntax Error if |DecoratorList| is present and the |DecoratorList| of |ClassDeclaration| is present.
</li>
</ul>
</emu-clause>

<emu-clause id="sec-static-semantics-exportedbindings" oldids="sec-module-semantics-static-semantics-exportedbindings,sec-exports-static-semantics-exportedbindings" type="sdo">
Expand Down Expand Up @@ -29005,17 +29028,26 @@ <h1>Runtime Semantics: Evaluation</h1>
<emu-alg>
1. Return the result of evaluating |VariableStatement|.
</emu-alg>
<emu-grammar>ExportDeclaration : `export` Declaration</emu-grammar>
<emu-grammar>ExportDeclaration : DecoratorList? `export` Declaration</emu-grammar>
<emu-alg>
1. Return the result of evaluating |Declaration|.
1. If |DecoratorList?| is present, then
Copy link

Choose a reason for hiding this comment

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

Suggested change
1. If |DecoratorList?| is present, then
1. If |DecoratorList| is present, then

I personally am neutral on whether the ? should be included in general, but right now the prevailing style is that it is not. (See for example ForLoopEvaluation.)

Copy link
Author

Choose a reason for hiding this comment

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

I can change that, but there are other cases of |DecoratorList?| in the main PR we'll need to address at some point as well.

Copy link

@bakkot bakkot Feb 9, 2023 •
edited

Choose a reason for hiding this comment

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

Feel free to leave this as-is and address it as part of the main PR if you'd prefer, then. Either way.

1. Assert: |Declaration| is a |ClassDeclaration|.
1. Let _decorators_ be ? DecoratorListEvaluation of |DecoratorList|.
Copy link

Choose a reason for hiding this comment

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

Might be worth adding an assert here that |ClassDeclaration| does not have its own |DecoratorList| (and in the export default case as well, of course).

It's prevented by the early errors, but not by the mere structure of the nonterminals, so if you haven't noticed the early errors it seems like this is just discarding that |DecoratorList|.

1. Perform ? BindingClassDeclarationEvaluation of |ClassDeclaration| with argument _decorators_.
1. Return ~empty~.
1. Else,
1. Return the result of evaluating |Declaration|.
</emu-alg>
<emu-grammar>ExportDeclaration : `export` `default` HoistableDeclaration</emu-grammar>
<emu-alg>
1. Return the result of evaluating |HoistableDeclaration|.
</emu-alg>
<emu-grammar>ExportDeclaration : `export` `default` ClassDeclaration</emu-grammar>
<emu-grammar>ExportDeclaration : DecoratorList? `export` `default` ClassDeclaration</emu-grammar>
<emu-alg>
1. Let _value_ be ? BindingClassDeclarationEvaluation of |ClassDeclaration|.
1. If |DecoratorList?| is present, then
1. Let _decorators_ be ? DecoratorListEvaluation of |DecoratorList|.
1. Else, let _decorators_ be a new empty List.
1. Let _value_ be ? BindingClassDeclarationEvaluation of |ClassDeclaration| with argument _decorators_.
1. Let _className_ be the sole element of BoundNames of |ClassDeclaration|.
1. If _className_ is *"\*default\*"*, then
1. Let _env_ be the running execution context's LexicalEnvironment.
Expand Down