Closed Bug 1066238 Opened 7 years ago Closed 7 years ago

Support 'static' MethodDefinitions in ES6 classes

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jorendorff, Assigned: efaust)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(3 files, 2 obsolete files)

Note that
- static is not a keyword
- static methods do not appear on instances
- static methods receive the constructor function object as 'this'
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Attached patch Parser support for "static" (obsolete) — Splinter Review
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8539080 - Flags: review?(jorendorff)
Attached patch Emitter support for "static" (obsolete) — Splinter Review
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.
Attachment #8539081 - Flags: review?(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...
Attachment #8539080 - Flags: review?(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.
Attachment #8539081 - Flags: review?(jorendorff)
Depends on: 1124362
If this goes r+, then we should look at the rest again.
Attachment #8554847 - Flags: review?(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?
Attachment #8554847 - Flags: review?(jorendorff) → review+
I think the other patches here must be old. Post updated ones for review?
Flags: needinfo?(efaustbmo)
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.
(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.
Attachment #8539080 - Attachment is obsolete: true
Attachment #8567338 - Flags: review?(jorendorff)
Attachment #8539081 - Attachment is obsolete: true
Flags: needinfo?(efaustbmo)
Attachment #8567340 - Flags: review?(jorendorff)
(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 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.
Attachment #8567338 - Flags: review?(jorendorff) → review+
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.
Attachment #8567340 - Flags: review?(jorendorff) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/5bda72dc35cb
https://hg.mozilla.org/mozilla-central/rev/4ca8f8762fd9
https://hg.mozilla.org/mozilla-central/rev/682921d1eb3b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.