1059908 - Method definitions and getter/setter functions should not be constructors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

ES6 method definitions except for GeneratorMethod should not be constructors.

Comment 2

[Tom S [:evilpie]]

Attachment: Merge FunctionType and FunctionSyntaxKind

I need to verify that this doesn't change any behavior, so just feedback. Passes all tests however. This makes it also easier to fix bug 1150855.

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8587967 [details] [diff] [review]
Merge FunctionType and FunctionSyntaxKind

I don't see anything immediately wrong with this. Personally, I would probably just have JSFunction::Accessor instead of Getter and Setter, since we don't care for delzaification, but since we are not short of kind space, we don't really care that much.

Interesting to see what, if any, toString or toSource behavior changes. Maybe you should look there, though I admit I don't see any obvious changes in the patch.

As an aside, maybe it would be good to have a NUM_KINDS, and a KIND_BIT_MASK and a static assert that we don't have more kinds than we have bits for. I consdiered tis the last time I was working there, but didn't bother with it.

At any rate, looks good. f=me

Comment 4

[Tom S [:evilpie]]

Interestingly I got a new test failure after rebasing.

class   {
                    constructor() { };
                     get  method()
                {
                    super.prop;
                }
                }
assertLocalStmt@js1_8_5/reflect-parse/PatternAsserts.js:31:28
assertLocalExpr@js1_8_5/reflect-parse/PatternAsserts.js:35:5
assertExpr@js1_8_5/reflect-parse/PatternAsserts.js:70:5
assertClassExpr@js1_8_5/reflect-parse/classes.js:36:9
assertClass@js1_8_5/reflect-parse/classes.js:41:9
doClassSuperPropAssert@js1_8_5/reflect-parse/classes.js:369:9
assertValidSuperProps@js1_8_5/reflect-parse/classes.js:330:1
assertValidSuperPropTypes@js1_8_5/reflect-parse/classes.js:344:1
assertValidClassSuperPropExtends@js1_8_5/reflect-parse/classes.js:374:1
assertValidClassSuperProps@js1_8_5/reflect-parse/classes.js:381:9
testClasses@js1_8_5/reflect-parse/classes.js:398:5
runtest@js1_8_5/reflect-parse/shell.js:59:9
@js1_8_5/reflect-parse/classes.js:449:5

js1_8_5/reflect-parse/PatternAsserts.js:31:27 SyntaxError: use of super property accesses only valid within methods or eval code within methods

Comment 5

[Tom S [:evilpie]]

Oh and "constructor" is a method, but should be constructable.

Comment 6

[Tom S [:evilpie]]

Attachment: v1 - Merge FunctionType and FunctionSyntaxKind

Right now there is no difference between Method and Constructor, especially because we can't tell the difference as all methods are constructors.

Comment 7

[Tom S [:evilpie]]

Attachment: v1 - Make getter/setter/method non-constructable

Comment 8

[Eric Faust [:efaust]]

Comment on attachment 8600405 [details] [diff] [review]
v1 - Merge FunctionType and FunctionSyntaxKind

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

Cool. See comments below.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +283,5 @@
>  
>      bool savedCallerFun = evalCaller && evalCaller->functionOrCallerFunction();
> +    bool allowSuperProperty = savedCallerFun && (evalCaller->functionOrCallerFunction()->isMethod() ||
> +                                                 evalCaller->functionOrCallerFunction()->isGetter() ||
> +                                                 evalCaller->functionOrCallerFunction()->isSetter());

I suspect there's gonna be a few places where we care about that kind of thing. Is it worth having a JSFunction::isMethodSyntax()?

::: js/src/frontend/ParseNode.h
@@ +1698,5 @@
> +    Expression,
> +    Statement,
> +    Arrow,
> +    Method,
> +    Constructor,

nit: Can we make this ClassConstructor instead? I would prefer to make it explicit. "Constructor" is in danger of getting overloaded.

::: js/src/frontend/Parser.cpp
@@ +8445,3 @@
>      /* NB: Getter function in { get x(){} } is unnamed. */
>      RootedPropertyName funName(context);
> +    if ((kind == Method || kind == Constructor) && tokenStream.isCurrentTokenType(TOK_NAME)) {

style: these are still one line, right? So they don't need braces.

::: js/src/jsfun.cpp
@@ +1008,5 @@
>          }
>      }
> +
> +    bool funIsMethodOrNonArrowLambda = (fun->isLambda() && !fun->isArrow()) || fun->isMethod() ||
> +                                        fun->isGetter() || fun->isSetter();

Actually, we can do a little better, here, maybe. We probably don't want the parens around methods or class constructors, but *did* want them around getters/setters. When I unified everything under isMethod(), I didn't have a way to differentiate, and so had to add parens to methods also. It's hard to know how much this matters, but we can now put it back the way Guptha had it originally, if we want.

::: js/src/jsfun.h
@@ +58,5 @@
>          RESOLVED_LENGTH  = 0x0800,  /* f.length has been resolved (see fun_resolve). */
>          RESOLVED_NAME    = 0x1000,  /* f.name has been resolved (see fun_resolve). */
>  
>          FUNCTION_KIND_SHIFT = 13,
> +        FUNCTION_KIND_MASK  = 0x7 << FUNCTION_KIND_SHIFT,

A static assert linking the number of members of FunctionKind and this mask would be nice, but maybe it's not worth it.

Comment 9

[Eric Faust [:efaust]]

Comment on attachment 8600414 [details] [diff] [review]
v1 - Make getter/setter/method non-constructable

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

This looks right to me. I am curious why the XXX? What are we afraid of? Did I miss some gotcha?

::: js/src/jit-test/tests/basic/bug653262.js
@@ +1,4 @@
>  with(evalcx(''))(function eval() {}, this.__defineGetter__("x", Function));
>  var i = 0;
>  var o;
> +print(x);

This debug print should not land.

::: js/src/jit/BaselineIC.cpp
@@ +9916,5 @@
>          if (op == JSOP_FUNAPPLY)
>              return true;
>  
>          // If callee is not an interpreted constructor, we have to throw.
> +        if (constructing && !fun->isConstructor()) // XXX

Why the XXX here? This looks innocuous to me.

::: js/src/jit/MacroAssembler.cpp
@@ +2357,5 @@
>      branchTest32(Assembler::Zero, scratch, Imm32(bits), label);
>  
> +    // Check if the CONSTRUCTOR bit is set.
> +    bits = IMM32_16ADJ(JSFunction::CONSTRUCTOR);
> +    branchTest32(Assembler::Zero, scratch, Imm32(bits), label);

This is so much better :)

::: js/src/jsfun.h
@@ +207,5 @@
>      }
>  
> +    // Make the function constructible.
> +    void setIsConstructor() {
> +        MOZ_ASSERT(!isConstructor());

Should this also assert about self-hostedness? This doesn't seem like an operation we are likely to want to be able to perform elsewhere.

::: js/src/vm/Interpreter.cpp
@@ +722,5 @@
>  
>      /* Invoke native functions. */
>      JSFunction* fun = &args.callee().as<JSFunction>();
> +    if (fun->isNative()) {
> +        MOZ_ASSERT_IF(construct, !fun->isConstructor()); // XXX ?

Again, confused. This also looks right to me.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd7da3aa49a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7b7d427d1c

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2fd7da3aa49a
https://hg.mozilla.org/mozilla-central/rev/9f7b7d427d1c

Comment 12

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/41#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Method_definitions_are_not_constructable