1066227 - Parser support for ES6 ClassDefinitions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

(All this stuff will be Nightly-only for now.)

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 0: Refactor Parser::methodName out of Parser::objectLiteral

Comment 2

[Eric Faust [:efaust]]

Attachment: Rough start

There's a lot still left to play with here...

We need a plan for |constructor|. My thought is to initialize the first element of the NodeList to a PNK_NOP or some other placeholder, and then dynamically replace it with the body of |constructor| if it's defined in the class. We will eventually want similar machinery for |extends|. Is this too hackish?

There are also a bunch of little things like some of the error messages currently say "object litteral" when they should clearly talk about class bodies, and I'm not sure that the jsops line up with the right attributes on the generated properties. I just wanted to get some kind of prototype up for people to look at.

Jason, I would vastly appreciate your eyes and insights here.

Comment 3

[Eric Faust [:efaust]]

OK, so clearly there's two more things to consider about this patch:

1) It's probably important to get the name of the class to the next layer, so it should be involved in the PNK_CLASS node somehow. Oops :)

2) We are gonna want some kind of ternary structure, I think: one for the name, one for a box for the extension (if any) and the constructor (if any), and one for a list of the rest of the stuff. This is much better than packing everything into a single list. Maybe we are also gonna need one for statics? More on that later.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8494900 [details] [diff] [review]
Part 0: Refactor Parser::methodName out of Parser::objectLiteral

Review of attachment 8494900 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +7132,5 @@
>  
>  template <typename ParseHandler>
>  typename ParseHandler::Node
> +Parser<ParseHandler>::methodName(typename ParseHandler::Node literal, MutableHandleAtom atom,
> +                                 JSOp &op, bool &isGenerator)

This doesn't seem like the right name for this method. methodDeclarationOrPropertyName() maybe.

This is kind of a weird unit of code to carve out... It might be more natural to add a "mode" parameter, and include both object-literal-only `prop: value` parsing and class-decl-only `static` parsing, both conditional on the mode. But either way is fine with me.

::: js/src/frontend/Parser.h
@@ +627,5 @@
>      Node computedPropertyName(Node literal);
>      Node arrayInitializer();
>      Node newRegExp();
>  
> +    Node methodName(Node literal, MutableHandleAtom atom, JSOp &op, bool &isGenerator); 

trailing space character on this line

Comment 5

[Eric Faust [:efaust]]

Attachment: Less rough start

This is a more serious attempt. It still features some weird carvings, though. Perhaps we want a different manner of dividing up the work

Comment 6

[Eric Faust [:efaust]]

Attachment: Basic Reflect.parse support

No tests as yet, but visually the stuff coming out of Reflect.parse with classes isn't TOTALLY brain damaged, so that's mild to moderately encouraging.

Comment 7

[Eric Faust [:efaust]]

Attachment: Uh...maybe a random lexical binding in the outer scope?

What is this, I don't even. Is is too heinous to live?

Comment 8

[Eric Faust [:efaust]]

Attachment: One way to add a lexical binding to the outer scope

Confirmed that this adds enough binding to get shadowing errors. The question remains if the "refactor" of lexicalDeclaration is too ugly.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8519311 [details] [diff] [review]
One way to add a lexical binding to the outer scope

Review of attachment 8519311 [details] [diff] [review]:
-----------------------------------------------------------------

Would be nice if the big move-code-into-new-function bits here were a separate patch, not intermingled with substantive changes.  Harder to review this than the actual mini-changes that are the hard of this patch.

::: js/src/frontend/Parser.cpp
@@ +3885,5 @@
> +Parser<FullParseHandler>::checkAndPrepareLexical(bool isConst)
> +{
> +    /*
> +     * This is a lexcial declaration. We must be directly under a block per the
> +     * proposed ES4 specs, but not an implicit block created due to

lexical
...per ES6, but...

(Hate these deadwood ES4 references scattered through our code.)

@@ +4007,5 @@
> +     *
> +     * See 8.1.1.1.6 and the note in 13.2.1.
> +     *
> +     * FIXME global-level lets are still considered vars until
> +     * other bugs are fixed.

Make this bug 589199, please.

@@ +5811,5 @@
> +
> +    // Handle the silliness of global and body level lexical decls.
> +    ParseNode *classDn;
> +    if (pc->atGlobalLevel()) {
> +        data.initVarOrGlobalConst(JSOP_DEFVAR);

Add a // Bug 589199 will convert this to a lexical declaration. comment or so.

::: js/src/frontend/Parser.h
@@ +300,5 @@
>      //   function f1() { function f2() { } }
>      //   if (cond) { function f3() { if (cond) { function f4() { } } } }
>      //
>      bool atBodyLevel() { return !topStmt; }
> +    bool atGlobalLevel() { return !sc->isFunctionBox() && (topStmt == topScopeStmt); }

No need for parens, nor do we regularly include them.

@@ +570,5 @@
>                                  GeneratorKind generatorKind, JSOp Op);
>      bool classMethodDefinition(Node methodList, Node propname, FunctionType type,
>                                 FunctionSyntaxKind kind, GeneratorKind generatorKind, JSOp op);
>  
> +    bool checkAndPrepareLexical(bool isConst);

Hmm, callers shouldn't have a bool on hand for this, it should be an enum.  Someday.

Comment 10

[Eric Faust [:efaust]]

Attachment: Part 1: create a clean way to create lexical bindings

Comment 11

[Eric Faust [:efaust]]

Attachment: Part 2: Give objectLiteral a new name and argument to prepare for class parsing

Comment 12

[Eric Faust [:efaust]]

Attachment: Part 3: Parse ES6 ClassDefinitions (Nightly only)

Virtually none of this is behind the guard. The thinking is that it will be a reserved keyword if not JS_HAS_CLASSES, and so the rest of the code will be dead. There are a few more dead branches. I can #ifdef more stuff out, as deemed necessary.

Comment 13

[Eric Faust [:efaust]]

Attachment: Part 4: Reflect.parse support

Comment 14

[Eric Faust [:efaust]]

Attachment: Part 2: Give objectLiteral a new name and argument to prepare for class parsing

Better with the right patch ;)

Comment 15

[Eric Faust [:efaust]]

Attachment: Part 3: Parse ES6 ClassDefinitions (Nightly only)

Add a typeerror for having a default constructor in light of filing 1105463. This will be removed, eventually.

Comment 16

[Eric Faust [:efaust]]

Attachment: Part 1: Create a clean way to created lexical bindings at initalizer sites.

Factor out part of the lexical state initialization from lexicalDeclaration(), as well as renaming bindDestructuringVar to bindInitialized, before providing makeInitializedLexicalBinding().

Comment 17

[Eric Faust [:efaust]]

Attachment: Part 2: Rename objectLiteral() propertyList() in preparation for classes.

Comment 18

[Eric Faust [:efaust]]

Attachment: Part 1: Create a clean way to created lexical bindings at initalizer sites.

Comment 19

[Eric Faust [:efaust]]

Attachment: Part 3: Parse ES6 ClassStatements (Nightly Only)

Comment 20

[Eric Faust [:efaust]]

Attachment: Part 4: Reflect.parse support for ES6 ClassStatements

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539071 [details] [diff] [review]
Part 4: Reflect.parse support for ES6 ClassStatements

Review of attachment 8539071 [details] [diff] [review]:
-----------------------------------------------------------------

Needs tests. Clearing review for now.

::: js/src/jsreflect.cpp
@@ +2615,5 @@
> +                return false;
> +            methods.infallibleAppend(prop);
> +        }
> +
> +        return builder.classMethods(methods, &pn->pn_pos, dst);

Can we drop classMethods and make classStatement.body a plain old array?

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539068 [details] [diff] [review]
Part 1: Create a clean way to created lexical bindings at initalizer sites.

Review of attachment 8539068 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +3313,5 @@
>  }
>  
>  template <>
>  bool
> +Parser<FullParseHandler>::bindInitialized(BindData<FullParseHandler> *data, ParseNode *pn)

OK, so this is called "bindInitialized" because it creates a binding, then tweaks pn->pn_op so that the emitter emits the appropriate opcode to initialize the binding.

This is crazy and if you understand it now, it would be a good idea to change this code to be saner while you still have it swapped in. I think we agree that the binding itself and the assignment/initialization should be two separate things in the ParseNode tree.

If you're too busy for that, or if it's better to wait until the global lexical tier is done, then still, it'd be nice to have a comment on this method's declaration in Parser.h saying what it does and what the postconditions are on success.

@@ +3924,5 @@
> +bool
> +Parser<FullParseHandler>::checkAndPrepareLexical(bool isConst)
> +{
> +    /*
> +     * This is a lexcial declaration. We must be directly under a block per the

spelling: lexical

@@ +4029,5 @@
> +
> +template <>
> +ParseNode *
> +Parser<FullParseHandler>::makeInitializedLexicalBinding(HandlePropertyName name, bool isConst,
> +                                                        const TokenPos &pos)

This could use a method-comment too.

@@ +4064,5 @@
> +    /*
> +     * Parse body-level lets without a new block object. ES6 specs
> +     * that an execution environment's initial lexical environment
> +     * is the VariableEnvironment, i.e., body-level lets are in
> +     * the same environment record as vars.

This isn't true anymore, right?

@@ +4072,5 @@
> +     *
> +     * See 8.1.1.1.6 and the note in 13.2.1.
> +     *
> +     * FIXME global-level lets are still considered vars until
> +     * other bugs are fixed.

This is more to the point. Please rewrite the comment; I think a line or two would suffice.

::: js/src/frontend/Parser.h
@@ +300,5 @@
>      //   function f1() { function f2() { } }
>      //   if (cond) { function f3() { if (cond) { function f4() { } } } }
>      //
>      bool atBodyLevel() { return !topStmt; }
> +    bool atGlobalLevel() { return atBodyLevel() && !sc->isFunctionBox() && (topStmt == topScopeStmt); }

Needs a comment, I think.

Also `topStmt == topScopeStmt` is weird, since if we get there, we already established that topStmt is null. So how about just `!topScopeStmt`?

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539070 [details] [diff] [review]
Part 3: Parse ES6 ClassStatements (Nightly Only)

Review of attachment 8539070 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/FullParseHandler.h
@@ +291,5 @@
> +        return class_;
> +    }
> +    ParseNode *newClassMethodList(uint32_t begin) {
> +        ParseNode *methodList = new_<ListNode>(PNK_CLASSMETHODLIST, TokenPos(begin, begin + 1));
> +        return methodList;

Any reason the temporary variable can't be dropped, and just `return new_<ListNode>(...)`?

@@ +294,5 @@
> +        ParseNode *methodList = new_<ListNode>(PNK_CLASSMETHODLIST, TokenPos(begin, begin + 1));
> +        return methodList;
> +    }
> +    ParseNode *newClassNames(ParseNode *outer, ParseNode *inner, const TokenPos &pos) {
> +        return new_<ClassNames>(outer, inner, pos);

^^^^ (Like this?)

::: js/src/frontend/ParseNode.h
@@ +1316,5 @@
> +    /*
> +     * Method defintions often keep a name and function body that overlap,
> +     * so explicitly define the beginning and end here.
> +     */
> +    ClassMethod(ParseNode *name, ParseNode *body, JSOp op, bool isStatic)

Let's have a single Method class used for both classes and object literals. They're nearly identical in the grammar. We even use the same code to parse them.

Continuing to put stuff into the pn_op field is also sad; it'd be a little better to define more PNK_ constants instead, so we don't make it harder for some future brave person to remove pn_op. Or add a separate field alongside isStatic, with its own enum.

@@ +1339,5 @@
> +        return pn_u.binary.isStatic;
> +    }
> +};
> +
> +struct ClassNames : public BinaryNode {

This would be a great place for a comment explaining inner and outer bindings.

I guess I'll take for granted that we need the innerBinding for something later. (It seems like we'll need outerBinding in order to emit the right code to initialize it once we've built the constructor, but I can't think of anything we'll need the innerBinding for yet. I'll figure it out when we get to it, I'm sure.)

::: js/src/frontend/Parser.cpp
@@ +5800,5 @@
> +    TokenPos namePos = pos();
> +
> +    MUST_MATCH_TOKEN(TOK_LC, JSMSG_CURLY_BEFORE_CLASS);
> +
> +    Node outerBinding = makeInitializedLexicalBinding(name, false, namePos);

I think most specialized methods use concrete types instead of "Node", so this would be a ParseNode *.

@@ +8235,5 @@
> +    if (listType == ClassBody)
> +        return handler.addClassMethodDefinition(propList, propname, fn, op);
> +
> +    MOZ_ASSERT(listType == ObjectLiteral);
> +    return handler.addObjectMethodDefinition(propList, propname, fn, op);

As with ClassMethod above, my gut says unify addClassMethodDefinition and addObjectMethodDefinition. If this is super hard for some reason, let's talk about it.

::: js/src/frontend/TokenStream.cpp
@@ +1005,5 @@
>  {
> +    if (kw->tokentype == TOK_RESERVED
> +#ifndef JS_HAS_CLASSES
> +        || kw->tokentype == TOK_CLASS
> +#endif

Is this change necessary? Since TOK_CLASS is present in all versions, won't the "// Working keyword." block take care of it?

Comment 24

[Jason Orendorff [:jorendorff]]

Note: Despite my reviews, parts 1-3 can't land without the Reflect.parse support, which can't land without tests.

Superficially, this is because Reflect.parse crashes hard if it sees any kind of ParseNode it doesn't recognize. But really it's because new code shouldn't land without tests and Reflect.parse is how we test this.

Comment 25

[Eric Faust [:efaust]]

Attachment: Tests, as a first pass.

This builds on a part 4 with arrays used instead of classMethods, as suggested. If this looks sufficient, we should land the lot.

Comment 26

[Jason Orendorff [:jorendorff]]

Comment on attachment 8548473 [details] [diff] [review]
Tests, as a first pass.

Review of attachment 8548473 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ r++++++

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +1126,5 @@
> +    // Allow Semicolons in Class Definitions
> +    assertStmt("class Foo { constructor() { }; }", emptyFooClass);
> +
> +    // Allow more than one semicolon, even in otherwise trivial classses
> +    assertStmt("class Foo { ;;; constructor() { } }", emptyFooClass);

please add a sleeper test

    assertError("class Foo { ;;; }", TypeError);

to be converted to a real test later.

@@ +1133,5 @@
> +    setClassMethods(stmt, [simpleMethod("method", "method", false), simpleConstructor]);
> +    assertStmt("class Foo { method() { } constructor() { } }", stmt);
> +
> +    /* Generators */
> +    // No yield as a class name inside a generator

ok, but what about the other case, yield as a class name when not in a generator?

@@ +1138,5 @@
> +    assertError("function *foo() {\
> +                    class yield {\
> +                        constructor() { } \
> +                    }\
> +                 }", SyntaxError);

Use a template string here instead of backslashes?

@@ +1151,5 @@
> +    /* Strictness */
> +    // yield is a strict-mode keyword, and class definitions are always strict.
> +    assertError("class Foo { constructor() { var yield; } }", SyntaxError);
> +    // Beware of the strictness of computed property names. Here use bareword
> +    // deletion (a deprecated action) to check.

nice

@@ +1159,5 @@
> +    // Classes should bind lexically, so they should collide with other in-block
> +    // lexical bindings
> +    assertError("{ let Foo; class Foo { constructor() { } } }", TypeError);
> +    assertError("{ const Foo = 0; class Foo { constructor() { } } }", TypeError);
> +    assertError("{ class Foo { constructor() { } } class Foo { constructor() { } } }", TypeError);

For that last one, I feel like a template string would have made it clearer what's going on.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40d868c8e468
https://hg.mozilla.org/mozilla-central/rev/11610b63ed6d
https://hg.mozilla.org/mozilla-central/rev/168b7595a633
https://hg.mozilla.org/mozilla-central/rev/83834e66a119
https://hg.mozilla.org/mozilla-central/rev/1d986d70ad5f