Restore class field tests: public, private (+static) #1660

rwaldron · 2018-07-30T22:45:30Z

Most of this was removed previously, but can now be restored and updated.

This is a work in progress

I've tagged @leobalter and @spectranaut for review, but I'm not actually ready for review yet.

rwaldron · 2018-08-01T20:58:11Z

@anba If you have any opportunity to review, I always appreciate your time and efforts :)

rwaldron · 2018-08-06T18:03:44Z

This now contains tests for grammar that's blocked by tc39/proposal-static-class-features#37

I just assume that the issue will be resolved and the syntax will match what is shown in the readme of the proposal.

anba · 2018-08-08T15:14:47Z

@anba If you have any opportunity to review, I always appreciate your time and efforts :)

I haven't read any of the test files, but when executing them, I got the following errors. :-)

test/language/expressions/class/fields-after-same-line-gen-rs-static-method-privatename-identifier-alt.js
etc.

Assignment to #stored, but #stored is never defined.

test/language/expressions/class/fields-after-same-line-gen-static-private-methods.js
etc.

Declares static #y(){}, but then assigns to #y, which is not allowed, because private name methods are non-writable.
This means the test always throws a TypeError when executing this.#y = value;.

test/language/expressions/class/fields-computed-name-propname-constructor.js
test/language/expressions/class/fields-computed-name-static-propname-constructor.js
etc.

Invalid tests, PropName(ComputedPropertyName) is always empty.
https://tc39.github.io/ecma262/#sec-object-initializer-static-semantics-propname

test/language/expressions/class/static-private-fields-proxy-default-handler-throws.js

Needs to invoke the method: C.x -> C.x().
And for the proxy: P.x -> P.x()

test/language/expressions/class/syntax-error-grammar-field-def-has-initializer-no-sc.js
etc.

Test expects x = [] is an early syntax error.
I assume "no-sc" means "no-super-call" (?), but no super-call is present in the test files.

test/language/expressions/class/syntax-error-grammar-field-no-initializer-with-method.js
etc.

Test expects x <newline> m(){} is an early syntax error.
No idea where this restriction is supposed to come from, unless ASI shouldn't apply at all for class fields?

test/language/expressions/class/syntax-error-grammar-fields.js
test/language/expressions/class/syntax-error-grammar-privatename.js
test/language/expressions/class/syntax-error-grammar-privatenames.js

Missing error flags in YAML block.

Complete errors with stacktraces -> https://gist.github.com/anba/9045b8ce023eabd9cb90920c4427923b

rwaldron · 2018-08-10T15:29:45Z

@anba Thank! I'll check on these today :)

rwaldron · 2018-08-10T15:51:32Z

Assignment to #stored, but #stored is never defined.

This was corrected in a recent push to the WIP branch.

Declares static #y(){}, but then assigns to #y, which is not allowed, because private name methods are non-writable.

This means the test always throws a TypeError when executing this.#y = value;.

This was corrected in a recent push to the WIP branch.

test/language/expressions/class/fields-computed-name-propname-constructor.js
test/language/expressions/class/fields-computed-name-static-propname-constructor.js
etc.

Invalid tests, PropName(ComputedPropertyName) is always empty.

https://tc39.github.io/ecma262/#sec-object-initializer-static-semantics-propname

Dropped

test/language/expressions/class/static-private-fields-proxy-default-handler-throws.js

Needs to invoke the method: C.x -> C.x().

And for the proxy: P.x -> P.x()

Will be fixed in next push

test/language/expressions/class/syntax-error-grammar-field-def-has-initializer-no-sc.js
etc.

Test expects x = [] is an early syntax error.

I assume "no-sc" means "no-super-call" (?), but no super-call is present in the test files.

"no semi-colon" ;)

...But I'm seeing now that some of these are missing the offending ClassElement or FieldDefinition necessary for the syntax error exception. I'm going to work on those as well.

test/language/expressions/class/syntax-error-grammar-field-no-initializer-with-method.js
etc.

Test expects x m(){} is an early syntax error.

No idea where this restriction is supposed to come from, unless ASI shouldn't apply at all for class fields?

This was corrected in a recent push to the WIP branch.

test/language/expressions/class/syntax-error-grammar-fields.js
test/language/expressions/class/syntax-error-grammar-privatename.js
test/language/expressions/class/syntax-error-grammar-privatenames.js

Missing error flags in YAML block.

These don't exist anymore, so I guess "problem solved!". I will check to make sure the missing YAML didn't surface in some other files.

leobalter

All the issues I'm finding are not blockers for moving on. The tests are looking correct.

leobalter · 2018-08-16T19:23:51Z

src/class-fields/grammar-field-classelementname-initializer-alt.case

+\u{6F} = 3;
+℘ = 4;      // DO NOT CHANGE THE NAME OF THIS FIELD
+ZW_‌_NJ = 5; // DO NOT CHANGE THE NAME OF THIS FIELD
+ZW_‍_J = 6;  // DO NOT CHANGE THE NAME OF THIS FIELD


the most lovable variable names in the world.

leobalter · 2018-08-16T19:25:09Z

src/class-fields/grammar-fields-same-line-error.case

+// This code is governed by the BSD license found in the LICENSE file.
+
+/*---
+desc: SyntaxError


I wish the description could be a bit longer here

leobalter · 2018-08-16T19:27:01Z

src/class-fields/grammar-privatename-error.case

+// This code is governed by the BSD license found in the LICENSE file.
+
+/*---
+desc: No space allowed between sigial and IdentifierName


typo, sigial

leobalter · 2018-08-16T19:35:31Z

src/class-fields/initializer-error/cls-decl-fields-comp-name.template

+var x = "string";
+class C {
+  [x] = /*{ initializer }*/;
+}


thanks for fixing this

leobalter · 2018-08-16T19:40:28Z

src/class-fields/rs-static-async-generator-method-privatename-identifier-alt.case

+---*/
+
+//- fields
+static async #$(value) {


It's so weird that we are matching proposals to have static private methods, but in any case, bring it on.

leobalter · 2018-08-16T19:40:59Z

src/class-fields/rs-static-async-method-privatename-identifier.case

+
+template: productions
+flags: [async]
+features: [class-static-methods-private]


I'm glad we have proper flags for these

anba · 2018-08-17T11:37:53Z

Updated list of test failures: https://gist.github.com/anba/42051c5232bec1ac5bfde560995ac586

src/class-fields/rs-static-async-generator-method-privatename-identifier-alt.case

Uses async, but instead needs to use async* to declare async generator methods.

test/language/expressions/class/fields-after-same-line-gen-rs-static-generator-method-privatename-identifier-alt.js (multiple files with the same error)

Needs to use yield* if delegating yield semantics are intended.
(The test assumes (function*() { yield function*(){ yield 0; }()})() === 1 is true, which is obviously incorrect.)

test/language/expressions/class/fields-after-same-line-gen-static-private-methods.js (multiple files with the same error)

Has assignment to private name method, which is not allowed and throws a TypeError at runtime.

test/language/expressions/class/fields-after-same-line-static-async-gen-rs-static-async-generator-method-privatename-identifier.js (multiple files with the same error)

Calls $DONE() multiple times.

test/language/statements/class/syntax-invalid-grammar-field-def-has-initializer-no-sc-error.js (multiple files with the same error)

Test incorrectly assumes x = [] <line-break> y; is a SyntaxError.

leobalter · 2018-08-20T17:27:48Z

Test incorrectly assumes x = [] y; is a SyntaxError.

That's a reading from @rwaldron of the proposed syntax:

  ClassElement :
    MethodDefinition
    static MethodDefinition
    FieldDefinition ;
    ;

where this reads the sc char in FieldDefinition ; as not optional. While I understand his PoV, I agree the grammar is beat by other parts like the do-while statement which has a sc in a point known for ASI.

That grammar formatting might need work, but this would be a side-tracking for this PR.

If you want to stick to the bikeshedding, we could think on possible alternatives for the trailing ; that is a clear signal for separating fields with ; if they are in the same line. The alternative I have in mind is ridiculously verbose, thou:

ClassElement :
  MethodDefinition
  static MethodDefinition
  FieldDefinitionList
  ;

FieldDeclarationList :
  FieldDeclarationOthers_opt FieldDeclaration

FieldDeclarationOthers :
  FieldDeclarationList [no LineTerminator here ] ; [no LineTerminator here ] FieldDeclaration

I'm not sure if that really works as the idea is to prevent multiple fields on the same line without a separator, but that's also kind weird as we can stack multiple methods with a field in the same line: class C { m1() {} m2() {} }.

As I stated before, I wish the sc could have avoided as an element of ClassElement, but that's a battle lost since ES2015.

leobalter · 2018-08-20T17:35:12Z

@anba I tried to fix all the parts you mentioned. Can you help me there with another review, please?

leobalter · 2018-08-20T19:04:40Z

If it helps, I ran the tests using es6draft and found no errors on the tests for this PR.

# Conflicts: # src/class-fields/init-err-contains-super.case

# Conflicts: # src/class-fields/eval-err-contains-arguments.case # src/class-fields/eval-err-contains-supercall-1.case # src/class-fields/eval-err-contains-supercall-2.case # src/class-fields/eval-err-contains-supercall.case # src/class-fields/eval-err-contains-superproperty-1.case # src/class-fields/eval-err-contains-superproperty-2.case

# Conflicts: # src/class-fields/propname-constructor.case # src/class-fields/propname-error/cls-decl-computed-name.template # src/class-fields/propname-error/cls-decl-literal-name.template # src/class-fields/propname-error/cls-decl-string-name.template # src/class-fields/propname-error/cls-expr-computed-name.template # src/class-fields/propname-error/cls-expr-literal-name.template # src/class-fields/propname-error/cls-expr-string-name.template

# Conflicts: # src/class-fields/propname-error/cls-decl-variable-name.template # src/class-fields/propname-error/cls-expr-variable-name.template

# Conflicts: # src/class-fields/default/cls-decl-after-same-line-async-gen.template # src/class-fields/default/cls-decl-after-same-line-async-method.template # src/class-fields/default/cls-decl-after-same-line-gen.template # src/class-fields/default/cls-decl-after-same-line-method.template # src/class-fields/default/cls-decl-after-same-line-static-async-gen.template # src/class-fields/default/cls-decl-after-same-line-static-async-method.template # src/class-fields/default/cls-decl-after-same-line-static-gen.template # src/class-fields/default/cls-decl-after-same-line-static-method.template # src/class-fields/default/cls-decl-multiple-definitions.template # src/class-fields/default/cls-decl-multiple-stacked-definitions.template # src/class-fields/default/cls-decl-new-no-sc-line-method.template # src/class-fields/default/cls-decl-new-sc-line-generator.template # src/class-fields/default/cls-decl-new-sc-line-method.template # src/class-fields/default/cls-decl-regular-definitions.template # src/class-fields/default/cls-decl-same-line-generator.template # src/class-fields/default/cls-decl-same-line-method.template # src/class-fields/default/cls-decl-wrapped-in-sc.template # src/class-fields/default/cls-expr-after-same-line-async-gen.template # src/class-fields/default/cls-expr-after-same-line-async-method.template # src/class-fields/default/cls-expr-after-same-line-gen.template # src/class-fields/default/cls-expr-after-same-line-method.template # src/class-fields/default/cls-expr-after-same-line-static-async-gen.template # src/class-fields/default/cls-expr-after-same-line-static-async-method.template # src/class-fields/default/cls-expr-after-same-line-static-gen.template # src/class-fields/default/cls-expr-after-same-line-static-method.template # src/class-fields/default/cls-expr-multiple-definitions.template # src/class-fields/default/cls-expr-multiple-stacked-definitions.template # src/class-fields/default/cls-expr-new-no-sc-line-method.template # src/class-fields/default/cls-expr-new-sc-line-generator.template # src/class-fields/default/cls-expr-new-sc-line-method.template # src/class-fields/default/cls-expr-regular-definitions.template # src/class-fields/default/cls-expr-same-line-generator.template # src/class-fields/default/cls-expr-same-line-method.template # src/class-fields/default/cls-expr-wrapped-in-sc.template # src/class-fields/private-names.case

# Conflicts: # test/language/statements/class/classelementname-abrupt-completion.js # test/language/statements/class/fielddefinition-initializer-abrupt-completion.js # test/language/statements/class/super-fielddefinition-initializer-abrupt-completion.js

- needs to use async* to declare async generator methods. - Needs to use yield* if delegating yield semantics are intended. - Has assignment to private name method, which is not allowed and throws a TypeError at runtime. - handle multiple $DONE calls - fix calls to private methods

leobalter · 2018-08-20T20:41:33Z

Travis CI is getting me some weird results as it appears to be comparing with another commit. I'm rebasing this locally and rolling a force push to see if anything changes for Travis.

In any case, I have a local backup of the previous commits from this branch.

leobalter · 2018-08-20T21:16:31Z

The last commits should fix the problem. I had templates using the same path value. Thanks @jugglinmike for helping me finding the bug.

anba · 2018-08-21T12:55:45Z

If it helps, I ran the tests using es6draft and found no errors on the tests for this PR.

Hmm, the current version on GH is probably missing some fixes. With my local development version, I still get the following failures: https://gist.github.com/anba/25a51ad514b91870bdcb89f3ad478ed4

Most of the failures happen because rs-static-async-generator-method-privatename-identifier.case passes the value 1 to yield* (there's yield * await value with value = 1).

leobalter · 2018-08-21T16:18:08Z

@anba I think 7ea8043 should fix this. Please let me know.

anba · 2018-08-21T16:35:47Z

There seems to be a similar issue in rs-static-generator-method-privatename-identifier-alt.case and rs-static-generator-method-privatename-identifier.case (maybe more, I've stopped after those two files).

Additionally there are still some invalid property name tests:

test/language/expressions/class/fields-computed-name-static-propname-prototype.js
test/language/statements/class/fields-computed-name-propname-constructor.js
test/language/statements/class/fields-computed-name-static-computed-var-propname-constructor.js
test/language/statements/class/fields-computed-name-static-computed-var-propname-prototype.js
test/language/statements/class/fields-computed-name-static-propname-constructor.js
test/language/statements/class/fields-computed-name-static-propname-prototype.js
test/language/statements/class/privatefields-string-name-propname-constructor.js

anba · 2018-08-21T20:10:02Z

I see only a single remaining failure (test/language/statements/class/privatefields-string-name-propname-constructor.js incorrectly assume ["#constructor"] is not valid). 👍

…-constructor-err.js

leobalter · 2018-08-21T20:26:25Z

I see only a single remaining failure (test/language/statements/class/privatefields-string-name-propname-constructor.js incorrectly assume ["#constructor"] is not valid).

Thanks! I was still working on it, but I wanted to understand what that file was doing. In the process I even found a duplicate for checking privatenames of #constructor

leobalter · 2018-08-21T20:34:54Z

@anba I believe this will be good to go for a final review. Thank you so much for all the support.

anba · 2018-08-21T20:44:37Z

Zero failures with the latest commit!

leobalter · 2018-08-21T20:47:11Z

I'm getting a pretty positive feedback from parts I can test from other engines, so I'm honestly assuming this is good to ship for now. I'd be happy to address any eventual bug if we find something.

rwaldron requested review from leobalter and spectranaut and removed request for leobalter July 30, 2018 22:45

littledan mentioned this pull request Aug 6, 2018

[WIP] Early Errors > Grammar #1343 #1651

Closed

6 tasks

rwaldron force-pushed the restore-spectranaut-tests branch from 1156999 to a24bba2 Compare August 6, 2018 18:23

rwaldron force-pushed the restore-spectranaut-tests branch from 83a999e to c0509ff Compare August 10, 2018 16:04

leobalter approved these changes Aug 16, 2018

View reviewed changes

leobalter changed the title ~~WIP: Restore class field tests: public, private (+static)~~ Restore class field tests: public, private (+static) Aug 20, 2018

leobalter mentioned this pull request Aug 20, 2018

Add test for private accessor methods #1680

Closed

Valerie R Young and others added 11 commits August 20, 2018 16:38

class fields: add early errors for argument and super

f2d76e6

# Conflicts: # src/class-fields/init-err-contains-super.case

Add private fields tests of early errors and small fix

ac5e65a

Fixes for forbidden property names

d361aaa

# Conflicts: # src/class-fields/propname-error/cls-decl-variable-name.template # src/class-fields/propname-error/cls-expr-variable-name.template

Class Fields: Updating frontmatter

90e8859

Features: add class-static-* feature names

c72926a

Class Fields: Updating frontmatter in src/class-fields/*.case

ac7bc64

Class Fields: Remove templates that cause erroneous tests to be created.

3996741

rwaldron and others added 8 commits August 20, 2018 16:38

Class Fields: static method productions

b386947

Generate tests

331a396

Generate tests

c5a1a4a

Fix cases for ASI

d4fc8d6

Generate tests

9680930

missing get method

2ec2a00

Generate tests

de1bbd6

leobalter force-pushed the restore-spectranaut-tests branch from bdd4cc5 to de1bbd6 Compare August 20, 2018 20:41

leobalter added 2 commits August 20, 2018 17:12

fix templates path

345b933

Generate tests

9a7fa8c

leobalter added 2 commits August 21, 2018 12:06

rewrite values for yield star

7ea8043

Generate tests

dacc69a

leobalter added 3 commits August 21, 2018 12:43

Other cases of yield * with iterable values

0cc5e35

Generate tests

3690d98

Fix tests for computed names in class fields - ctor and prototype

45a3187

leobalter added 2 commits August 21, 2018 16:11

Fix tests for #constructor

e25e53c

Remove duplicate of test/language/statements/class/fields-privatename…

dbcd388

…-constructor-err.js

Fix generated tests

fa36c45

leobalter merged commit d296cc9 into tc39:master Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore class field tests: public, private (+static) #1660

Restore class field tests: public, private (+static) #1660

rwaldron commented Jul 30, 2018 •

edited

Loading

rwaldron commented Aug 1, 2018

rwaldron commented Aug 6, 2018

anba commented Aug 8, 2018

rwaldron commented Aug 10, 2018

rwaldron commented Aug 10, 2018

leobalter left a comment

leobalter Aug 16, 2018

leobalter Aug 16, 2018

leobalter Aug 16, 2018

leobalter Aug 16, 2018

leobalter Aug 16, 2018

leobalter Aug 16, 2018

anba commented Aug 17, 2018

leobalter commented Aug 20, 2018

leobalter commented Aug 20, 2018

leobalter commented Aug 20, 2018

leobalter commented Aug 20, 2018 •

edited

Loading

leobalter commented Aug 20, 2018

anba commented Aug 21, 2018

leobalter commented Aug 21, 2018

anba commented Aug 21, 2018

anba commented Aug 21, 2018

leobalter commented Aug 21, 2018

leobalter commented Aug 21, 2018

anba commented Aug 21, 2018

leobalter commented Aug 21, 2018 •

edited

Loading

Restore class field tests: public, private (+static) #1660

Restore class field tests: public, private (+static) #1660

Conversation

rwaldron commented Jul 30, 2018 • edited Loading

rwaldron commented Aug 1, 2018

rwaldron commented Aug 6, 2018

anba commented Aug 8, 2018

rwaldron commented Aug 10, 2018

rwaldron commented Aug 10, 2018

leobalter left a comment

Choose a reason for hiding this comment

leobalter Aug 16, 2018

Choose a reason for hiding this comment

leobalter Aug 16, 2018

Choose a reason for hiding this comment

leobalter Aug 16, 2018

Choose a reason for hiding this comment

leobalter Aug 16, 2018

Choose a reason for hiding this comment

leobalter Aug 16, 2018

Choose a reason for hiding this comment

leobalter Aug 16, 2018

Choose a reason for hiding this comment

anba commented Aug 17, 2018

leobalter commented Aug 20, 2018

leobalter commented Aug 20, 2018

leobalter commented Aug 20, 2018

leobalter commented Aug 20, 2018 • edited Loading

leobalter commented Aug 20, 2018

anba commented Aug 21, 2018

leobalter commented Aug 21, 2018

anba commented Aug 21, 2018

anba commented Aug 21, 2018

leobalter commented Aug 21, 2018

leobalter commented Aug 21, 2018

anba commented Aug 21, 2018

leobalter commented Aug 21, 2018 • edited Loading

rwaldron commented Jul 30, 2018 •

edited

Loading

leobalter commented Aug 20, 2018 •

edited

Loading

leobalter commented Aug 21, 2018 •

edited

Loading