Method definitions and getter/setter functions should not be constructors

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: anba, Assigned: evilpie)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
ES6 method definitions except for GeneratorMethod should not be constructors.

Updated

5 years ago
Duplicate of this bug: 1069402

Updated

5 years ago
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]

Updated

4 years ago
Summary: Method definitions should not be constructors → Method definitions and getter/setter functions should not be constructors
(Assignee)

Updated

4 years ago
Assignee: nobody → evilpies
(Assignee)

Comment 2

4 years ago
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.
Attachment #8587967 - Flags: feedback?(efaustbmo)
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
Attachment #8587967 - Flags: feedback?(efaustbmo) → feedback+
(Assignee)

Comment 4

4 years ago
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
(Assignee)

Comment 5

4 years ago
Oh and "constructor" is a method, but should be constructable.
(Assignee)

Comment 6

4 years ago
Right now there is no difference between Method and Constructor, especially because we can't tell the difference as all methods are constructors.
Attachment #8600405 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Attachment #8587967 - Attachment is obsolete: true
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.
Attachment #8600405 - Flags: review?(efaustbmo) → review+
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.
Attachment #8600414 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/mozilla-central/rev/2fd7da3aa49a
https://hg.mozilla.org/mozilla-central/rev/9f7b7d427d1c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1165794

Updated

4 years ago
See Also: → 1166950
You need to log in before you can comment on or make changes to this bug.