Add normative changes from January 2023 plenary #4

rbuckton · 2023-02-05T23:53:32Z

This adds the normative changes from the January 2023 plenary:

Decorators are now also allowed before export
Decorators are not allowed both before and after export or export default
context.access.get.call(obj) -> context.access.get(obj)
context.access.set.call(obj, value) -> context.access.set(obj, value)
Adds context.access.has(obj)

Rather than using a production parameter like, this instead uses early errors to ensure a decorator list prior to export is only allowed for class declarations, and to ensure you do not have decorators both before and after export/export default.

rbuckton · 2023-02-05T23:54:58Z

@pzuraq, we should have these changes reviewed by the editors before merging.

nicolo-ribaudo · 2023-02-06T14:38:31Z

spec.html

@@ -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]


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 ∉ { `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 👍

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.

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.

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

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.

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.

spec.html

rbuckton · 2023-02-08T20:11:46Z

@syg, @michaelficarra, @bakkot: Would one of you mind reviewing these changes? I understand the baseline spec in tc39#2417 (which this PR is a diff against) probably needs updating as well, but an editor's review on these individual changes would help so this can be merged into the main PR and unblock microsoft/TypeScript#52582 and babel/babel#15405.

bakkot

Couple tiny comments, but LGTM otherwise. Feel free to merge once the ? and assert thing are addressed.

bakkot · 2023-02-08T20:29:29Z

spec.html

-          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_).


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

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.

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

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.

bakkot · 2023-02-08T20:30:52Z

spec.html

        <emu-alg>
-          1. Return the result of evaluating |Declaration|.
+          1. If |DecoratorList?| is present, then


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

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.

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

bakkot · 2023-02-08T20:35:09Z

spec.html

-          1. Return the result of evaluating |Declaration|.
+          1. If |DecoratorList?| is present, then
+            1. Assert: |Declaration| is a |ClassDeclaration|.
+            1. Let _decorators_ be ? DecoratorListEvaluation of |DecoratorList|.


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

bakkot · 2023-02-08T20:43:21Z

spec.html

@@ -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]


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

rbuckton · 2023-02-09T02:24:29Z

@pzuraq: I've addressed @bakkot's feedback. Can you merge this into your decorators branch so that it is applied to the main PR?

pzuraq · 2023-02-09T17:16:58Z

Yes, thanks again for taking this on @rbuckton, super appreciated!

rbuckton force-pushed the decorators-jan-2023 branch from 55d6710 to 1ddcce3 Compare February 6, 2023 14:27

rbuckton mentioned this pull request Feb 6, 2023

Implement decorators as presented at 2023-01 TC39 meeting babel/babel#15405

Merged

4 tasks

Add normative changes from January 2023 plenary

66bf36b

rbuckton force-pushed the decorators-jan-2023 branch from 1ddcce3 to 66bf36b Compare February 6, 2023 14:32

nicolo-ribaudo reviewed Feb 6, 2023

View reviewed changes

Swap PrivateSet/Set to correct steps in context.access.set

be6db19

rbuckton mentioned this pull request Feb 6, 2023

Decorators normative changes microsoft/TypeScript#52582

Merged

bakkot approved these changes Feb 8, 2023

View reviewed changes

pzuraq approved these changes Feb 8, 2023

View reviewed changes

Address editor feedback

b978144

pzuraq merged commit 7fbb55b into pzuraq:decorators Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add normative changes from January 2023 plenary #4

Add normative changes from January 2023 plenary #4

rbuckton commented Feb 5, 2023

rbuckton commented Feb 5, 2023

nicolo-ribaudo Feb 6, 2023

rbuckton Feb 6, 2023

rbuckton Feb 6, 2023

bakkot Feb 8, 2023

rbuckton Feb 9, 2023

bakkot Feb 9, 2023

rbuckton commented Feb 8, 2023

bakkot left a comment

bakkot Feb 8, 2023

rbuckton Feb 9, 2023

rbuckton Feb 9, 2023

rbuckton Feb 9, 2023

bakkot Feb 8, 2023

rbuckton Feb 9, 2023

bakkot Feb 9, 2023 •

edited

Loading

bakkot Feb 8, 2023

bakkot Feb 8, 2023

rbuckton commented Feb 9, 2023

pzuraq commented Feb 9, 2023

	1. If \|DecoratorList?\| is present, then
	1. If \|DecoratorList\| is present, then

Add normative changes from January 2023 plenary #4

Add normative changes from January 2023 plenary #4

Conversation

rbuckton commented Feb 5, 2023

rbuckton commented Feb 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton commented Feb 8, 2023

bakkot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakkot Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton commented Feb 9, 2023

pzuraq commented Feb 9, 2023

bakkot Feb 9, 2023 •

edited

Loading