ecma262 icon indicating copy to clipboard operation
ecma262 copied to clipboard

Add Class and Class Element Decorators and `accessor` Keyword

Open pzuraq opened this issue 3 years ago • 17 comments

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.

pzuraq avatar May 26 '21 23:05 pzuraq

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

jmdyck avatar May 28 '21 13:05 jmdyck

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

jmdyck avatar May 30 '21 15:05 jmdyck

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

ljharb avatar May 30 '21 16:05 ljharb

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

jmdyck avatar May 30 '21 16:05 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.)

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

pzuraq avatar Dec 26 '21 04:12 pzuraq

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.

jmdyck avatar Dec 26 '21 15:12 jmdyck

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 https://github.com/tc39/proposal-decorators/issues/424#issuecomment-1032828707

  • 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 avatar Mar 03 '22 15:03 pzuraq

@ljharb @jmdyck I believe I've addressed all the open issues so far, including the SDO and chain production changes. Let me know if it all looks good!

pzuraq avatar Mar 06 '22 02:03 pzuraq

Latest round of feedback addressed!

pzuraq avatar Mar 07 '22 15:03 pzuraq

If you rebase to main, I can give you another PR of changes.

jmdyck avatar Mar 10 '22 21:03 jmdyck

Updates:

  • Changed initialize to init, per https://github.com/tc39/proposal-decorators/issues/446
  • Added state for tracking decorator application status, and added an error when calling context methods after decorator has been applied, per https://github.com/tc39/proposal-decorators/issues/447
  • Addressed outstanding feedback

@jmdyck rebased, would love another round of fixes/updates 😄

pzuraq avatar Mar 12 '22 21:03 pzuraq

@jmdyck rebased, would love another round of fixes/updates smile

Okay, PR at https://github.com/pzuraq/ecma262/pull/3

jmdyck avatar Mar 14 '22 16:03 jmdyck

Adding a note for myself: Need to handle abrupt completions for decorator context objects, if the decorator throws an error we need to still mark the addInitializer and getMetadata/setMetadata such that they aren't usable.

pzuraq avatar Mar 28 '22 15:03 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.

pzuraq avatar Mar 06 '23 19:03 pzuraq

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 https://github.com/doehyunbaek/ecma262/commit/8a2b51d858aed89a45503d99b780d3d39c538ba2 actually solves the issue.

doehyunbaek avatar Mar 08 '23 01:03 doehyunbaek

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

pzuraq avatar Feb 02 '24 18:02 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.

pzuraq avatar Feb 02 '24 21:02 pzuraq