Add Class and Class Element Decorators and accessor Keyword

[pzuraq]

This PR includes the spec changes for the current iteration of the decorators proposal. This proposal is still in progress, currently in stage 2, and the PR is mainly being opened for reviewing and iteration purposes.

[jmdyck]

Rather than make a ton of "suggested changes" here, I've made a PR against the pzuraq:decorators branch.

[jmdyck]

Since Decorators are only used with Class definitions, would it make sense to put the Decorators clause within the Class Definitions clause?

[ljharb]

They’re only used with classes for now- there’s lots of interest in follow up proposals to add them in other places.

[jmdyck]

(I did look for follow-up proposals, but only found a couple at stage 0.)

[jmdyck]

I believe the following SDOs need to be modified to handle the new syntax for ClassDeclaration, ClassExpression, and/or ClassElement:

• BoundNames
• ClassElementKind
• ComputedPropertyContains
• Early Errors
• IsConstantDeclaration
• IsFunctionDefinition
• IsStatic
• PrivateBoundIdentifiers
• PropName

(Note that currently, 4 of the 5 RHSs of ClassElement are chain productions, and so often have implicit definitions for SDOs, but they aren't chain productions in this PR, and so will need explicit definitions.)

[pzuraq]

I believe the following SDOs need to be modified to handle the new syntax for ClassDeclaration, ClassExpression, and/or ClassElement:

• BoundNames
• ClassElementKind
• ComputedPropertyContains
• Early Errors
• IsConstantDeclaration
• IsFunctionDefinition
• IsStatic
• PrivateBoundIdentifiers
• PropName

(Note that currently, 4 of the 5 RHSs of ClassElement are chain productions, and so often have implicit definitions for SDOs, but they aren't chain productions in this PR, and so will need explicit definitions.)

@jmdyck regarding this comment, I'm not quite sure I understand what needs to be done here. Why do these productions need to be updated exactly?

[jmdyck]

Consider the production ClassElement : FieldDefinition in the status quo. It's a chain production, so (e.g.) the SDO ComputedPropertyContains doesn't need to have a definition for that production; instead we just chain down to FieldDefinition, for which ComputedPropertyContains does have an explicit definition.

But this PR replaces that production with ClassElement : DecoratorList? FieldDefinition, which is not a chain production, so there needs to be an explicit definition of ComputedPropertyContains for that production.

And likewise for the other alternatives of ClassElement that now have DecoratorList?, and likewise for the other SDOs that reach ClassElement.

[Jack-Works]

For an initializer x = 123 it will return a function with the proper env and private env.

function () {
    = 123
}

The result function does not have a valid Body (the Initializer is passed in).

[pzuraq]

Is this still an issue? Not sure I understood it, I would think that this is a syntax error

[pzuraq]

Still working on addressing the feedback items, but I made a number of updates so far and wanted to get those updates up so folks could start reviewing sooner rather than later. Updates since last draft:

• Separate parsing class elements and defining them so that we can control the order of definition and decorator application. This also allows us to avoid large numbers of optional arguments for various SDOs, instead the SDOs are now responsible for returning a ClassElementDefinition record, and the invoker of the SDO is then responsible for actually decorating and defining the element.

• Run field decorators after method decorators, per Is metadata too confusing? proposal-decorators#424 (comment)

• Extract and centralize class element decoration into a single abstract operation, so we don't have to worry about keeping it in sync in several different places.

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

  NOTE: This does mean that the abstract operation for instantiating private methods on the instance has to "run" and filter out the private methods from all of the other elements per-instantiation in the spec, but my assumption here was that the exact details of the spec wouldn't need to be followed and implementations could, for instance, filter out and store the private method definitions at class definition time and then copy them onto the instance all at once. If we need to make a note to that effect, we can, and if it's important for the spec to run the AO only at definition time for some reason we can do that, it's just a bit more complicated.

• Refactor to use anonymous function syntax.

Open Todos

• Update SDOs that were previously relying on chain productions for items that no longer have them
• Final review pass for formatting, linting, and overall correctness

Sidenote: It would be really nice to put comments inline in the spec to clarify the thinking for a specific portion, unsure if that's a feature anyone would be interested in?

[pzuraq]

PR has been rebased, I squashed the existing commits to make rebasing easier overall. The only new commit contains non-normative fixes. The remaining issues open in this thread are all normative I believe, and I will be opening up separate PRs for each of them so we can discuss at the upcoming plenary.

[doehyunbaek]

Hi, esmeta typecheck is failing because InitializeInstanceElements, which calls InitializePrivateMethods, expects to return throw completion, as exmplified here.

InitializePrivateMethods, on the other hand, declares it might return abrupt completion, which is not a subtype of throw completion, as seen here.

Changing the return type of InitializePrivateMethods, as this commit doehyunbaek@8a2b51d actually solves the issue.

[pzuraq]

PR has been updated and rebased, and all changes have been addressed!

[pzuraq]

@jmdyck all issues resolved I believe, except the one that @ljharb disagreed on. Happy to change it either way, just want to make sure it's the right way.

[ljharb]

Suggested change

	The DecoratorDefinition type is a Record used in the specification of
	decorators.
	The DecoratorDefinition type is a Record used in the specification of decorators.

[ljharb]

i think we soft-wrap pretty consistently, not hard-wrap

[ljharb]

aside for the editors: kind of feels like we need a SpecValueToString() AO or something for cases like this. i think atomics has one too.

[lpardosixtosMs]

I understand that steps 6 through 9 define a no-op if there is already a private accessor pair with a setter and a getter and the same key, is this correct? If so, could we reword 7.c and 7.d? It seems a little strange to use _e_ and _existing_ in the same line when _existing_ was set to _e_ in the previous step.