Fix parsing of private fields #566
Conversation
Codecov Report
@@ Coverage Diff @@
## master #566 +/- ##
=======================================
Coverage 98.15% 98.15%
=======================================
Files 22 22
Lines 3625 3625
Branches 1012 1012
=======================================
Hits 3558 3558
Misses 24 24
Partials 43 43
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #566 +/- ##
=======================================
Coverage 98.14% 98.14%
=======================================
Files 22 22
Lines 3663 3663
Branches 1023 1023
=======================================
Hits 3595 3595
Misses 25 25
Partials 43 43
Continue to review full report at Codecov.
|
| equals(p) { return this.#x === p.#x && this.#y === p.#y } | ||
|
|
||
| toString() { return `Point<${ this.#x },${ this.#y }>` } | ||
| } |
| @@ -1,4 +1,4 @@ | |||
| class Foo { | |||
| #p = x | |||
| [#m] () {} | |||
There was a problem hiding this comment.
This would have been fine actually, besides the method. This looks totally valid to me:
class A {
#x = 1 // private field
[#x] = 2 // public field
}There was a problem hiding this comment.
That public field/method definition on the third line should throw a ReferenceError at parse time, as the private name is unresolvable. But, yes, totally fine from a narrow grammar perspective, if Babylon doesn't enforce Static Semantics: Early Errors.
There was a problem hiding this comment.
Have you created a test for this?
There was a problem hiding this comment.
I think my example should throw, by looking at it again.
What I meant is this:
class A {
#x = 1
[this.#x] = 2
//or
#y = this.#x
}But these are also early errors?
There was a problem hiding this comment.
#4 should be a syntax error, as # needs to be followed by an IdentifierName.\
[this.#x] should be a ReferenceError when the class declaration is executed, as #x is "in scope" as a name during the evaluation of computed property names, but then #x cannot possibly be present on this.
There was a problem hiding this comment.
Upps, the #4 was not intended to be there. Basically I wanted to show initializing a private field from another private field.
|
@littledan should review the expression, we might be allowing more use cases than the spec intended |
| @@ -1,4 +1,4 @@ | |||
| class Foo { | |||
| #p = x | |||
| [#m] () {} | |||
There was a problem hiding this comment.
That public field/method definition on the third line should throw a ReferenceError at parse time, as the private name is unresolvable. But, yes, totally fine from a narrow grammar perspective, if Babylon doesn't enforce Static Semantics: Early Errors.
| @@ -0,0 +1,4 @@ | |||
| class Foo { | |||
| #"p" = x | |||
| #2 = y | |||
There was a problem hiding this comment.
It'd be nice to break this out into a separate test file, so you can check that each of them is a syntax error.
| @@ -0,0 +1,4 @@ | |||
| class Foo { | |||
| #p = x | |||
| #m () {} | |||
There was a problem hiding this comment.
Good to have a test for this; I hope we can remove it soon :)
|
@diervo Looks like the key for a ClassMethod is an Expression. This should be the same as whatever the key of a class method is. With hindsight, Babel might've gone for a more restrictive choice that more closely encodes the grammar, so that transforms can operate with a little more confidence and determinism, but Expression is a reasonable choice that expresses the possibilities. |
The computed key is not part of the spec. key for ClassProperties is an Expression Do not parse computed and literal keys for PrivateClassProperties
Do not parse computed and literal keys for PrivateClassProperties. By looking at the spec it should always be an Identifier afaics. This also removes the computed key as it will always be false, and it is not mentioned in the spec.
so this should not work I think:
Little unrelated:
I also noticed that our spec and types say, that key for ClassProperties (non-private) is an Identifier, but the current official spec says it is:
StringLiteral | NumericLiteral | Identifier | ComputerPropertyso I went withExpression, same as ClassMethod.This all parses fine and I think it is correct: