1066233 - Support ES6 ClassExpressions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

These come in two flavors: named and unnamed.

Comment 1

[Eric Faust [:efaust]]

Attachment: Parser support for ES6 ClassExpressions

Comment 2

[Eric Faust [:efaust]]

Attachment: Emitter support for ES6 ClassExpressions

Comment 3

[Eric Faust [:efaust]]

Attachment: Reflect.parse support for ES6 ClassExpressions

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539088 [details] [diff] [review]
Parser support for ES6 ClassExpressions

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

Clearing r? on a bunch of patches that need tests.

Comment 5

[Eric Faust [:efaust]]

Attachment: Test support for ES6 ClassExpressions

This patch looks big because it's just rehashing all the tests we have already. As always, if you like this, you'll love the rest.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8560055 [details] [diff] [review]
Test support for ES6 ClassExpressions

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

Great tests. I thought I'd be able to come up with some new twist on inner bindings but I got nothing.

Do we have tests regarding what's allowed as a class name? These are all SyntaxErrors:

    class x.y { constructor() {} }
    class []  { constructor() {} }
    class {x} { constructor() {} }
    class for { constructor() {} }
    function* f() { class yield { constructor () {} } }

What about this?

    class extends extends null { constructor () {} }
    (class extends { constructor() {} })

I'm not sure about 'let' and 'static'; please make sure we have tests for them as well.

::: js/src/tests/ecma_6/Class/classPrototype.js
@@ +2,5 @@
>  
>  // The prototype of a class is a non-writable, non-configurable, non-enumerable data property.
>  class a { constructor() { } }
> +var b = class { constructor() { } };
> +for (test of [a,b]) {

Can you throw a 'let' in the for-loop head, just so this isn't assigning to the global variable 'test' that contains the source code of the test? It works as-is, it just seems... too weird to leave like that.

::: js/src/tests/ecma_6/Class/innerBinding.js
@@ +5,5 @@
>  if (classesEnabled()) {
>  
> +// XXXefaust Because we currently try to do assignment to const as an early
> +// error, sometimes, maybe, this is almost sometimes perhaps a syntax error.
> +// It is specced to be a TypeError.

So awful.

@@ +21,4 @@
>  /*
>  var test = `
>  class Foo { constructor() { }; tryBreak() { Foo = 4; } }
> +for (result of [Foo, class Bar { constructor() { }; tryBreak() { Bar = 4 } }])

for (let result of

Good habit to get into.

@@ +26,4 @@
>  
>  {
>      class foo { constructor() { }; tryBreak() { foo = 4; } }
> +    for (result of [foo, class bar { constructor() { }; tryBreal() { bar = 4 } }])

Same here.

Comment 7

[Jason Orendorff [:jorendorff]]

New patches r?me, please!

Comment 8

[Eric Faust [:efaust]]

Attachment: Parser support for ES6 ClassExpressions

Comment 9

[Eric Faust [:efaust]]

Attachment: Emitter support for ES6 ClassExpressions

Comment 10

[Eric Faust [:efaust]]

Attachment: Reflect.parse support for ES6 ClassExpressions

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8567364 [details] [diff] [review]
Parser support for ES6 ClassExpressions

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

Sweet.

::: js/src/frontend/ParseNode.cpp
@@ +390,5 @@
>              stack->push(pn->pn_kid3);
>          return PushResult::Recyclable;
>        }
>  
> +      // classes might have an optional node for the heritage, as well as the names

This wasn't clear to me. How about:

    // pn_kid1 (names) and pn_kid2 (heritage) can be null.

::: js/src/frontend/Parser.cpp
@@ +5912,4 @@
>  
>      JS_ALWAYS_TRUE(setLocalStrictMode(savedStrictness));
>  
> +    return handler.newClass(nameNode, classHeritage, methodsOrBlock);

The location information in Reflect.parse("class ...") is wrong, both start and end.

    js> Reflect.parse("class Foo {\n    constructor(){}\n}\n").body[0].loc
    ({start:{line:1, column:6}, end:{line:1, column:11}, source:null})

Please fix this in a follow-up bug.

To fix start, do what Parser<>::ifStatement() does.

To fix end, I think you just need to make sure methodsOrBlock has correct location information, because TernaryNode's constructor copies location information from kid nodes. Maybe setLexicalScopeBody needs to copy location information from the 2nd argument to the 1st argument.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8567365 [details] [diff] [review]
Emitter support for ES6 ClassExpressions

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6942,5 @@
> +    MOZ_ASSERT(!!names == methodsOrBlock->isKind(PNK_LEXICALSCOPE));
> +
> +    ParseNode *methodList = methodsOrBlock->isKind(PNK_LEXICALSCOPE) ?
> +                            methodsOrBlock->pn_expr                  :
> +                            methodsOrBlock;

You could assert that this node is the expected kind. (your call)

@@ +6952,5 @@
>              constructor = mn->pn_right;
>              break;
>          }
>      }
>      // For now, no default constructors

blank line before this comment (pre-existing style nit)

@@ +7020,5 @@
> +
> +    if (shouldPopResult) {
> +        if (Emit1(cx, bce, JSOP_POP) < 0)
> +            return false;
> +    }

This JSOP_POP could be emitted right after EmitLexicalInitialization, and the variable `shouldPopResult` could be deleted.

Comment 13

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8ec511e8f40a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd61b0194f1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8cb8a753c6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f2eb8a8f407a

Comment 14

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd391a15a670

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ec511e8f40a
https://hg.mozilla.org/mozilla-central/rev/ffd61b0194f1
https://hg.mozilla.org/mozilla-central/rev/7c8cb8a753c6
https://hg.mozilla.org/mozilla-central/rev/f2eb8a8f407a
https://hg.mozilla.org/mozilla-central/rev/fd391a15a670

Comment 16

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/class
(marked as Nightly only)