1066238 - Support 'static' MethodDefinitions in ES6 classes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Note that
- static is not a keyword
- static methods do not appear on instances
- static methods receive the constructor function object as 'this'

Comment 1

[Eric Faust [:efaust]]

Attachment: Parser support for "static"

Comment 2

[Eric Faust [:efaust]]

Attachment: Emitter support for "static"

Note that calling them will not produce a special |this| at this time, because we don't set home objects, since we can't tell if we have super bindings.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539080 [details] [diff] [review]
Parser support for "static"

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

Everything looks great.

Needs new Reflect.parse tests. Clearing r? for now.

::: js/src/frontend/Parser.cpp
@@ +7977,5 @@
> +
> +            if (ltok == TOK_NAME &&
> +                tokenStream.currentName() == context->names().static_)
> +            {
> +                isStatic = true;

Hate to say it, but I think the draft standard requires these to be parsed as methods with the name `static`:

    class x {
        static() {}
        get static() {}
        set static(x) {}
    }

Even keywords are legal as method names; in that light I guess it makes sense that `static` should be as well...

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539081 [details] [diff] [review]
Emitter support for "static"

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

Looks good. This doesn't generate super pretty bytecode, but it's not painful enough to justify adding another stack-manipulation opcode...

Needs lots of tests. Clearing the review flag for now.

I'll stop here for now. I'm not crazy about reviewing patches that are missing tests; none of it can land and if we're honest about what can be expected of a reviewer in this code, we have little reason to believe it's correct.

Comment 5

[Eric Faust [:efaust]]

Attachment: Tests for "static"

If this goes r+, then we should look at the rest again.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8554847 [details] [diff] [review]
Tests for "static"

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

Let's also have one short, super-basic end-to-end test for this:

    // basic static method test
    class X {
        static count() { return ++this.hits; }
    }
    X.hits = 0;
    assertEq(X.count(), 1);

    // A static method is just a function.
    assertEq(X.count instanceof Function, true);
    assertEq(X.count.length, 0);
    assertEq(X.count.bind({hits: 77})(), 78);

Of course the other tests cover most of this ground at a lower level, which is good too.

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +1127,5 @@
> +    // It's not an error to have a method named static, static, or not.
> +    setClassMethods(stmt, [simpleConstructor, simpleMethod("static", "method", false, [], false)]);
> +    assertStmt("class Foo{ constructor() { } static() { } }", stmt);
> +    setClassMethods(stmt, [simpleMethod("static", "method", false, [], true), simpleConstructor]);
> +    assertStmt("class Foo{ static static() { }; constructor() { } }");

Please also test that `static get()`, `get static()`, and `static get static()` all work; and that `get static x()` is a SyntaxError (...right?)

@@ +1138,5 @@
> +    assertError("class Foo { static set prototype(x) { }; constructor() { } }", SyntaxError);
> +
> +    // You are, however, allowed to have a CPN called prototype as a static
> +    setClassMethods(stmt, [simpleConstructor, emptyCPNMethod("prototype", true)]);
> +    assertStmt("class Foo { constructor() { }; static [\"prototype\"]() { } }", stmt);

But this is what the other test tests, right? It should fail?

Comment 7

[Jason Orendorff [:jorendorff]]

I think the other patches here must be old. Post updated ones for review?

Comment 8

[Jason Orendorff [:jorendorff]]

In reflect-parse.js, under "Clipped classes", please add tests for

   class X { static
   class X { static y
   class X { static *
   class X { static *y
   class X { static get
   class X { static get y

and whatever else seems to make sense.

Comment 9

[Eric Faust [:efaust]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> Comment on attachment 8554847 [details] [diff] [review]
> Tests for "static"

> > +
> > +    // You are, however, allowed to have a CPN called prototype as a static
> > +    setClassMethods(stmt, [simpleConstructor, emptyCPNMethod("prototype", true)]);
> > +    assertStmt("class Foo { constructor() { }; static [\"prototype\"]() { } }", stmt);
> 
> But this is what the other test tests, right? It should fail?

It is not an early error (nor could it easily be) to write such code with ComputedPropertyNames. It should throw when you try to evaluate the ClassDefinition because .prototype is neither writable nor configurable.

Comment 10

[Eric Faust [:efaust]]

Attachment: Parser support for "static"

Comment 11

[Eric Faust [:efaust]]

Attachment: Emitter support for "static"

Comment 12

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> In reflect-parse.js, under "Clipped classes", please add tests for
> 
>    class X { static
>    class X { static y
>    class X { static *
>    class X { static *y
>    class X { static get
>    class X { static get y
> 
> and whatever else seems to make sense.

This reminds me, it'd be great to add tests for unexpected tokens in various places, to make sure we're reporting them correctly.  jit-test/tests/basic/syntax-error-illegal-character.js is easy to extend for all this.  Ideally have enough tests, at a minimum, to cover every single place where you do a getToken, a peekToken, etc.

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8567338 [details] [diff] [review]
Parser support for "static"

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

::: js/src/frontend/Parser.cpp
@@ +8036,5 @@
> +
> +            if (ltok == TOK_NAME &&
> +                tokenStream.currentName() == context->names().static_)
> +            {
> +                isStatic = true;

Might be worth a comment:

            isStatic = true;  // unless it's a method called "static" (see `isStatic = false;` below)

...at your option.

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 8567340 [details] [diff] [review]
Emitter support for "static"

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6579,5 @@
>              continue;
>          }
>  
> +        bool extraPop = false;
> +        if (type == ClassBody && propdef->as<ClassMethod>().isStatic()) {

I'd prefer

    if (propdef->as<ClassMethod>().isStatic()) {
        MOZ_ASSERT(type == ClassBody);

assuming that's correct.

Comment 15

[Eric Faust [:efaust]]

(In reply to Jason Orendorff [:jorendorff] from comment #14)
> Comment on attachment 8567340 [details] [diff] [review]
> Emitter support for "static"
> 
> Review of attachment 8567340 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +6579,5 @@
> >              continue;
> >          }
> >  
> > +        bool extraPop = false;
> > +        if (type == ClassBody && propdef->as<ClassMethod>().isStatic()) {
> 
> I'd prefer
> 
>     if (propdef->as<ClassMethod>().isStatic()) {
>         MOZ_ASSERT(type == ClassBody);
> 
> assuming that's correct.

I don't think it is. This function is overloaded with nodes of Arity (don't tell Waldo I referred to it that way ;) ) PN_LIST. For Objects, it's PNK_OBJECT, which is a list of PNK_COLON or PNK_MUTATEPROTO nodes. For Classes, it's PNK_CLASSMETHODS, which is a list of type PNK_CLASSMETHOD, which is what we want to check. We are (admittedly implicitly) doing the following

if (type == ClassBody) {
    MOZ_ASSERT(pn->is<ClassMethod>());
    if (pn->as<ClassMethod().isStatic())
        ...
}

Would you prefer that we check the kind and assert the PropListType instead? It is not enough to just drop any check that we actually have a node for which isStatic() is relevant.

Comment 16

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5bda72dc35cb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca8f8762fd9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/682921d1eb3b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5bda72dc35cb
https://hg.mozilla.org/mozilla-central/rev/4ca8f8762fd9
https://hg.mozilla.org/mozilla-central/rev/682921d1eb3b

Comment 18

[Florian Scholz (Open Web Docs)]

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