ecma262
ecma262 copied to clipboard
Add Class and Class Element Decorators and `accessor` Keyword
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.
Rather than make a ton of "suggested changes" here, I've made a PR against the pzuraq:decorators branch.
Since Decorators are only used with Class definitions, would it make sense to put the Decorators clause within the Class Definitions clause?
They’re only used with classes for now- there’s lots of interest in follow up proposals to add them in other places.
(I did look for follow-up proposals, but only found a couple at stage 0.)
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?
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.
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?
@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!
Latest round of feedback addressed!
If you rebase to main, I can give you another PR of changes.
Updates:
- Changed
initialize
toinit
, 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 😄
@jmdyck rebased, would love another round of fixes/updates smile
Okay, PR at https://github.com/pzuraq/ecma262/pull/3
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.
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.
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.
PR has been updated and rebased, and all changes have been addressed!
@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.