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.

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

Normative: Add public and private class fields, static class features, private methods & getters/setters #1668

Open
wants to merge 63 commits into
base: master
from

Conversation

@Ms2ger
Copy link
Contributor

commented Aug 14, 2019

This includes changes from https://tc39.es/proposal-static-class-features/.

@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

This is on top of #1667; see Ms2ger/ecma262@private-methods...static-fields for just these changes.

@ljharb

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I’m confused; #1655 should contain all of this - “static class features” were merged back into the main proposal.

@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

I’m confused; #1655 should contain all of this - “static class features” were merged back into the main proposal.

Not in https://github.com/tc39/proposal-class-fields, as far as I can tell.

@Ms2ger Ms2ger force-pushed the Ms2ger:static-fields branch from f79ae75 to 28147fe Aug 15, 2019

@ljharb

This comment was marked as resolved.

Copy link
Member

commented Aug 15, 2019

Yes, the specs in the proposal repos weren’t merged yet, but the features were, and are now a single proposal - and thus should be combined into a single PR.

@Ms2ger

This comment was marked as resolved.

Copy link
Contributor Author

commented Aug 16, 2019

I'm still not sure what makes you say that, but if you want a combined PR, this is it.

@ljharb

This comment was marked as resolved.

Copy link
Member

commented Aug 16, 2019

The proposal champion was supposed to have combined them a long time ago; since there’s only one class fields proposal - static and instance, public and private - a single PR is appropriate. If this is the one you want reviewed for that proposal, so be it.

@littledan

This comment was marked as resolved.

Copy link
Member

commented Sep 2, 2019

@ljharb I don't understand your concern (if one exists). This is the combined version. @Ms2ger and I are collaborating on this project, and this is the appropriate PR to review together. I don't plan to propose this for Stage 4 until all three proposals meet the Stage 4 requirements.

@littledan

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

I've removed the "needs tests" label from this PR and the related ones as the tests are checked into test262.

@littledan littledan added the has tests label Sep 2, 2019

@ljharb

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

@littledan I’m looking for one PR to review for class fields; i see #1655, #1667, and this one.

@littledan

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

Please review this one for the union of all three proposals.

jmdyck and others added 30 commits Sep 5, 2019
Simplify assertion.
Saying "an Object that has a [[PrivateFieldValues]] internal slot"
implies that there are objects without that slot,
but the spec has already said that there aren't.
(Every object has that slot.)
Delete "internal" from "internal Private Names"
It doesn't appear to signify anything.
PrivateEnvironment is for Private Names from |ClassElement|s
not just the ones from |FieldDefinition|s.
The name passed to SetFunctionName can now be a Private Name
Acknowledge that in preamble + assertion.
Simplify the StringValue of PrivateIdentifier
by reusing the StringValue of IdentifierName.
Change _F_ to _func_
_F_ doesn't exist. I *think* you mean _func_, but please check.
Two calls to FunctionCreate were missing the new _privateScope_ arg
I'm guessing it should just be
the PrivateEnvironment of the running execution context.
Please check.
Change a bunch of |PropertyName| to |ClassElementName|
in algorithms associated with productions that had that change.
Delete call to SetFunctionName
because that will now happen in DefineOrdinaryMethod.
Tweak grammatical parameters in 'FieldDefinition' production
First, in a RHS, just 'In' is invalid, you need either +In or ~In.
You probably want +In.

Second, [~Yield, ~Await] is valid, but I suspect it's wrong.
Every other use of Initializer is happy with [?Yield, ?Await],
and I don't see a reason why this particular one would need to force [~Yield, ~Await].

Please check.
Add a missing case for IsStatic.
(I assume it should be *true* for `static` FieldDefinition `;`)
Change "Lexical Environment" to "LexicalEnvironment"
With the space, it's a type name.
Without the space, it's a component of an execution context.
Change "is not ~empty~" to "is a ClassFieldDefinition Record"
For one thing, ClassElementEvaluation never returns ~empty~.
For another, it might return things that aren't ~empty~
that you don't want entering this block.
So use a more appropriate condition for the block.
Insert an assertion re the type of _privateName_
(because GetBindingValue can return a variety of things,
and the fact that it's a Private Name is important to the validity
of extracting its [[Description]] on the next line.)
Change some _privateEnv_ to _privEnv_
"Let _privateEnv_ be _privateEnv_" doesn't make any sense.
Presumably the intent was to introduce a "loop variable"
so as not to change the value of _privateEnv_,
which is referenced later in the algorithm.
So use a different name.
The bindings aren't in _privEnv_, they're in _privEnv_'s EnvironmentR…
…ecord

I still don't like the wording of this step
(a binding isn't "named _N_")
but I don't have a good alternative to suggest.
Update some step-number references in Annex B
due to inserted steps in the main-body algorithms.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.