Using this in the constructor of a class using a CLR base type throws with null

[christianrondeau]

Here's how to reproduce:

    [Fact]
    public void ShouldSupportThisWithClrBaseClass()
    {
        _engine.AddModule("base-module", builder => builder.ExportType<TestType>());
        _engine.AddModule("my-module", @"
import { TestType } from 'base-module';
export class MyClass extends TestType {
    constructor() {
        this.x = 1;
    }
}");
        var ns = _engine.ImportModule("my-module");
        var ctor = ns.Get("MyClass");
        var instance = _engine.Construct(ctor); // This throws

        Assert.Equal(1, instance.Get("x").AsInteger());
    }
    public class TestType {}

Here's the stack trace:

Jint.Runtime.JavaScriptException
null
   at MyClass () my-module:5:9
   at my-module:6:16

This, however, works:

    [Fact]
    public void ShouldSupportThis()
    {
        _engine.AddModule("my-module", @"export class MyClass { constructor() { this.x = 1; } }");
        var ns = _engine.ImportModule("my-module");
        var ctor = ns.Get("MyClass");
        var instance = _engine.Construct(ctor);

        Assert.Equal(1, instance.Get("x").AsInteger());
    }

I'm not 100% sure if this is by design, but if it is I believe a better error than null would be welcome :)

Reproduced on commit b07562e6

[christianrondeau]

A quick debug seems to show this sets the _constructorKind to Derived in ClassDefinition.BuildConstructor:

F._constructorKind = _superClass is null ? ConstructorKind.Base : ConstructorKind.Derived;

And in ScriptFunctionInstance.Construct there's this:

            var thisArgument = Undefined;

            if (kind == ConstructorKind.Base)
            {
                thisArgument = OrdinaryCreateFromConstructor(
                    newTarget,
                    static intrinsics => intrinsics.Object.PrototypeObject,
                    static (engine, realm, _) => new ObjectInstance(engine));
            }

            var calleeContext = PrepareForOrdinaryCall(newTarget);

            if (kind == ConstructorKind.Base)
            {
                OrdinaryCallBindThis(calleeContext, thisArgument);
            }

So it looks by design. @lahma if I may, does this seem obvious to you? I imagine there's a reason why there was code written to make subclasses fail like that.

Here's a much simpler repro without modules:

    [Fact]
    public void ShouldSupportThisInSubclass()
    {
        var engine = new Engine();
        engine.Evaluate(@"
class MyClass1 {
}

class MyClass2 extends MyClass1 {
    constructor() {
    }
}
const x = new MyClass2();
");
    }

[lahma]

Well I can say for certain that failing with null is always bad. When running the last code example in Node console I get:

ReferenceError: Must call super constructor in derived class before accessing 'this' or returning from derived constructor

So I think the error message should be improved, especially for this "common case" which people can run into.

[christianrondeau]

You are right, this was an error on my end. These both work fine:

        [Fact]
        public void CanConstructClrExtendedClass()
        {
            var engine = new Engine();

            engine.SetValue("TestClass", TypeReference.CreateTypeReference<TestClass>(engine));
            engine.Execute("class ExtendedType extends TestClass { constructor() { super(); this.a = 1; } }");
            // For some reason here I cannot use engine.GetValue("ExtendedType") to get the constructor
            var ctor = engine.Evaluate("ExtendedType");
            var obj = engine.Construct(ctor);
            
            Assert.Equal(1, obj.Get("a"));
        }

    [Fact]
    public void ShouldSupportThisWithClrBaseClass()
    {
        _engine.AddModule("base-module", builder => builder.ExportType<TestType>());
        _engine.AddModule("my-module", @"
import { TestType } from 'base-module';
export class ExtendedType  extends TestType { constructor() { super(); this.a = 1; } }");
        var ns = _engine.ImportModule("my-module");
        var ctor = ns.Get("ExtendedType");
        var instance = _engine.Construct(ctor);

        Assert.Equal(1, instance.Get("a"));
    }

It requires a little bit of mental gymnastics to get the right calls but in the end, it makes sense, mostly :)

Should I close this call or change it to "improve the error message"? I've noticed FunctionEnvironmentRecord.GetThisBinding() could get the change, though maybe the error could be thrown sooner too.

-            ExceptionHelper.ThrowReferenceError(_engine.ExecutionContext.Realm, (string) null);
+            ExceptionHelper.ThrowReferenceError(_engine.ExecutionContext.Realm, "this"); // or better

[lahma]

I think this issue and summary is fine. Just need to change code to detect conditions (this access and exit without super call).