1169740 - Implement TDZ like this behavior for limited derived class constructors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

make TDZ-like this work for derived class constructors without eval or arrow functions.

Comment 1

[Eric Faust [:efaust]]

Attachment: Implement |this| and return value checks for derived class constructors.

This also enforces that the return value of a derived class constructor must be either undefined or an object.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8621427 [details] [diff] [review]
Implement |this| and return value checks for derived class constructors.

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

Looks great, but some suggestions and nits below.

::: js/src/jit/BaselineCompiler.cpp
@@ +3242,5 @@
>  
>  bool
>  BaselineCompiler::emitReturn()
>  {
> +    if (script->isDerivedClassConstructor()) {

I wonder if it'd be simpler to emit JSOP_CHECKRETURN or something in derived class constructors, and then have that op check the TOS value, and maybe also JSOP_CHECKTHIS if possible. WDYT?

If we changed from all-caps we could have JSOp::CheckDerivedThis or JSOp::CheckDerivedReturn...

@@ +3247,5 @@
> +        Label derivedDone, returnOK;
> +        masm.branchTestObject(Assembler::Equal, JSReturnOperand, &derivedDone);
> +        masm.branchTestUndefined(Assembler::Equal, JSReturnOperand, &returnOK);
> +
> +        // This is gonna smash JSReturnOperand, but we don't care, because it's

Nit: I'd prefer "This will smash" or "This is going to smash"; it sounds a bit more formal.

::: js/src/jit/CodeGenerator.cpp
@@ +10146,5 @@
>      masm.bind(&done);
>  }
>  
> +void
> +CodeGenerator::visitCheckReturn(LCheckReturn *ins)

Nit: LCheckReturn* ins

::: js/src/jit/IonBuilder.cpp
@@ +5927,5 @@
> +    if (target->nonLazyScript()->isDerivedClassConstructor()) {
> +        MOZ_ASSERT(target->isClassConstructor());
> +        MConstant* magic = MConstant::New(alloc(), MagicValue(JS_UNINITIALIZED_LEXICAL));
> +        current->add(magic);
> +        return magic;

This can be:

return constant(MagicValue(JS_UNINITIALIZED_LEXICAL));

::: js/src/jit/LIR-Common.h
@@ +7031,5 @@
>          return getOperand(0);
>      }
>  };
>  
> +class LCheckReturn : public LCallInstructionHelper<BOX_PIECES, 2 * BOX_PIECES, 0>

This should be LInstructionHelper (marking it as call instruction will cause unnecessary spills).

Also it has no output so the first BOX_PIECES should be 0.

::: js/src/jit/MIR.h
@@ +12754,5 @@
> +    }
> +    MDefinition* thisValue() const {
> +        return getOperand(1);
> +    }
> +};

We can give this an empty alias set.

::: js/src/vm/Stack.cpp
@@ +306,5 @@
> +            // so that people could know which constructor blew up, but we don't
> +            // actually have the class name, All the functions are called
> +            // 'constructor', which isn't very useful
> +            JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_UNINITIALIZED_THIS);
> +            return false;

I think it'd be nice to move ThrowUninitializedThis from VMFunctions.cpp to Interpreter.cpp, and do return ThrowUninitializedThis(...); here.

Then we have a single function responsible for throwing the error.

@@ +326,5 @@
> +
> +        if (!retVal.isUndefined()) {
> +            ReportValueError(cx, JSMSG_BAD_DERIVED_RETURN, JSDVG_IGNORE_STACK, retVal, nullptr);
> +            return false;
> +        }

Same here.

Comment 3

[Eric Faust [:efaust]]

Attachment: Rebased

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8658306 [details] [diff] [review]
Rebased

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

Looks good, r=me with comments below addressed.

I didn't check the tests, Jason is probably a better reviewer for those.

::: js/src/jit/BaselineCompiler.cpp
@@ +1277,5 @@
> +    FunctionInfo<ThrowUninitializedThisFn>(ThrowUninitializedThis);
> +
> +bool
> +BaselineCompiler::emitCheckThis()
> +{

It'd be good to call frame.assertSyncedStack() here, so we don't sync inside conditional code (the prepareVMCall below is "inside" the branchTestMagic branch) and end up with incorrect frame state.

@@ +1301,5 @@
>          frame.push(R0);
>          return true;
>      }
>  
> +    if (script->isDerivedClassConstructor() && !emitCheckThis())

Then here we can call syncStack(0) before we call emitCheckThis

@@ +3269,5 @@
>  bool
>  BaselineCompiler::emitReturn()
>  {
> +    if (script->isDerivedClassConstructor()) {
> +        Label derivedDone, returnOK;

And here at the start of this if-block.

::: js/src/jit/BaselineIC.cpp
@@ +9989,5 @@
>          masm.push(masm.extractObject(R1, ExtractTemp0));
>          if (!callVM(CreateThisInfoBaseline, masm))
>              return false;
>  
>          // Return of CreateThis must be an object.

Nit: fix the comment

::: js/src/jit/VMFunctions.cpp
@@ +1262,5 @@
>      return false;
>  }
>  
> +bool
> +ThrowUninitializedThis(JSContext* cx)

I think it'd be nice to move these 2 Throw* functions to vm/Interpreter.cpp and then have both the JIT code and the interpreter call them. The more we can share the better.

::: js/src/vm/Stack.cpp
@@ +228,5 @@
>      if (fun()->needsCallObject() && !initFunctionScopeObjects(cx))
>          return false;
>  
> +    if (isConstructing()) {
> +        if (script->isDerivedClassConstructor()) {

This if-else appears also in RunState::maybeCreateThisForConstructor and in jit::CreateThis. Maybe factor it out like:

bool
CreateThisForConstructor(JSContext* cx, HandleFunction callee, bool singleton, MutableHandleValue res)
{
    if (callee->nonLazyScript()->isDerivedClassConstructor()) {
        res.setMagic(...);
        return true;
    }
    ...
}

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8658306 [details] [diff] [review]
Rebased

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

::: js/src/js.msg
@@ +103,5 @@
>  MSG_DEF(JSMSG_TERMINATED,              1, JSEXN_ERR, "Script terminated by timeout at:\n{0}")
>  MSG_DEF(JSMSG_PROTO_NOT_OBJORNULL,     1, JSEXN_TYPEERR, "{0}.prototype is not an object or null")
>  MSG_DEF(JSMSG_CANT_CALL_CLASS_CONSTRUCTOR, 0, JSEXN_TYPEERR, "class constructors must be invoked with |new|")
>  MSG_DEF(JSMSG_DISABLED_DERIVED_CLASS,  1, JSEXN_INTERNALERR, "{0} temporarily disallowed in derived class constructors")
> +MSG_DEF(JSMSG_UNINITIALIZED_THIS,      0, JSEXN_TYPEERR, "illegal use of uninitialized |this|")

Eh, don't use "illegal" in error messages. Take your pick:
    "'this' is uninitialized"
    "'this' must be initialized by calling super()"
    "derived class constructor must call super() before using 'this'"

@@ +104,5 @@
>  MSG_DEF(JSMSG_PROTO_NOT_OBJORNULL,     1, JSEXN_TYPEERR, "{0}.prototype is not an object or null")
>  MSG_DEF(JSMSG_CANT_CALL_CLASS_CONSTRUCTOR, 0, JSEXN_TYPEERR, "class constructors must be invoked with |new|")
>  MSG_DEF(JSMSG_DISABLED_DERIVED_CLASS,  1, JSEXN_INTERNALERR, "{0} temporarily disallowed in derived class constructors")
> +MSG_DEF(JSMSG_UNINITIALIZED_THIS,      0, JSEXN_TYPEERR, "illegal use of uninitialized |this|")
> +MSG_DEF(JSMSG_BAD_DERIVED_RETURN,      1, JSEXN_TYPEERR, "derived class constructor returned illegal value {0}")

Please change "illegal" to "invalid".

::: js/src/jsfun.h
@@ +511,5 @@
>          u.n.jitinfo = data;
>      }
>  
> +    bool isDerivedClassConstructor() {
> +        bool derived = false;;

Double semicolon here - and drop the `= false`?

::: js/src/tests/ecma_6/Class/derivedConstructorDisabled.js
@@ +30,3 @@
>  
> +// Remove the assertion and add super() when super() is implemented!
> +assertThrownErrorContains(() => g.eval("new class foo extends null { constructor() { debugger; } }"), "|this|");

Reminder to change this when you change the error message.

::: js/src/tests/ecma_6/Class/derivedConstructorInlining.js
@@ +12,5 @@
> +    new foo();
> +}
> +
> +for (let i = 0; i < 1100; i++)
> +    assertThrownErrorContains(intermediate, "|this|");

and here

::: js/src/tests/ecma_6/Class/derivedConstructorTDZExplicitThis.js
@@ +2,5 @@
> +
> +class foo extends null {
> +    constructor() {
> +        // Just bareword |this| is DCEd by the BytecodeEmitter. Your guess as
> +        // to why we think this is a good idea is as good as mine. In order to

Whoa, if BytecodeEmitter::checkSideEffects considers `this` effect-free even though it may throw, that's a bug. Don't work around it here --- fix and test it. Or file a follow-up blocking es6, but isn't it super easy to fix?

As to the "why" question, my guess is the original point of detecting useless expression-statements was to warn about them. Then the author (surely Brendan) figured he might as well drop them from the bytecode too.

::: js/src/tests/ecma_6/Class/derivedConstructorTDZOffEdge.js
@@ +4,5 @@
> +    }
> +}
> +
> +for (let i = 0; i < 1100; i++)
> +    assertThrownErrorContains(() => new foo(), "|this|");

and here

::: js/src/tests/ecma_6/Class/derivedConstructorTDZReturnUndefined.js
@@ +6,5 @@
> +    }
> +}
> +
> +for (let i = 0; i < 1100; i++) {
> +    print(i);

stray print() left in!

@@ +7,5 @@
> +}
> +
> +for (let i = 0; i < 1100; i++) {
> +    print(i);
> +    assertThrownErrorContains(() => new foo(), "|this|");

and here with the error message

::: js/src/vm/Stack.cpp
@@ +296,5 @@
> +        if (thisValue().isMagic(JS_UNINITIALIZED_LEXICAL)) {
> +            // TODO: It would be really useful to put the function name here
> +            // so that people could know which constructor blew up, but we don't
> +            // actually have the class name, All the functions are called
> +            // 'constructor', which isn't very useful

Fix the punctuation and capitalization, and cite bug 883377. The .name of a class is supposed to be the class name, not "constructor".

Comment 6

[Eric Faust [:efaust]]

Attachment: Address Nit

Address the combined nit about handling the TODO left behind, and also factoring ThrowUninitalizedThis into Interpreter.*

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8671664 [details] [diff] [review]
Address Nit

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

Cromulent enough, but it'd be nice to pass in AbstractFrame* or something, that clearly indicates execution frame, versus HandleFunction that could be any random function from some goofy caller.  Just general belt-and-suspenders.  If it's too tricky to do, this works tho.

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a59b5b0139b4