Overriding an accessor with a property causes runtime error when emitting ESNExt

[thw0rted]

TypeScript Version: 3.3.3333 and whatever is on Playground

Search Terms: subclass getter accessor

Code

class Animal {
    public get legs(): number { return -1; }
    constructor(public name: string) { }
    move(distanceInMeters: number = 0) {
        console.log(`${this.name} moved ${distanceInMeters}m.`);
    }
}

class Snake extends Animal {
    public get legs(): number { return 0; }    // Correct implementation of abstract superclass property
    constructor(name: string) { super(name); }
    move(distanceInMeters = 5) {
        console.log("Slithering...");
        super.move(distanceInMeters);
    }
}

class Horse extends Animal {
    public readonly legs: number;    // Not sure if this "should" be a valid redeclaration
    constructor(name: string) {
        super(name);
        this.legs = 4;    // Works when using helper code to emulate pre-ES6 classes, fails with actual ES6
    }
    move(distanceInMeters = 45) {
        console.log("Galloping...");
        super.move(distanceInMeters);
    }
}

let sam = new Snake("Sammy the Python");
let tom: Animal = new Horse("Tommy the Palomino");

sam.move();
tom.move(34);

Expected behavior: Compiler should flag the redeclaration of the getter member as a non-accessor (normal) property, at least when emitting ES6.

Actual behavior: Compiler is fine, but assignment (this.legs = 4) fails at runtime with error about assigning to a member with getter but no setter.

Playground Link: You can paste the above into Playground to see that there are no compile-time errors but the emitted JS does not demonstrate the error because as far as I can tell, you can't tell Playground to emit ES6. (That would be a nice feature; maybe I should put in a ticket for that?)

Related Issues: none that I could find

ETA: I mistakenly had this with an abstract getter to start with. This does not show the buggy behavior because ES6 doesn't have a concept of abstract, so the getter isn't actually defined in the generated parent class at all. The updated code above has an implementation for the getter in the base class and should show the behavior I'm talking about -- I compile it with target: 'esnext' but I imagine it should work with target: 'es2015' also.

[AlCalzone]

In this modified playground with target ES2015 it does not fail:

https://typescript-play.js.org/?target=2#code/IYIwzgLgTsDGEAJYBthjAgggOwJYFthkEBvAKAUoQAcBXEZXWBUSGeBAcwFNFlvOYABQBKAFwJstfCG5QA3BSqwA9tja14KqELoMmk4Pm4S2ubJxGkEAXyWV8KgG7chAE1yRg2WNwCS2ACyvHJgElIycggAvAgADFbkVMlIamAq-AB0yCqcQgAGACQkEAAWnpnYRtw2CI4ubgjFHl4+-kEhUGA2+Jn5IorJdnZkKGgYAMpVANbcCNwAHhDc2G4YOAREpPY09IzMPHwCwuKS0rJQ1lC8tFDY8fK2yQD0zwgAwtrXHATU-MbYCDACC4NQIFQAMxY4GgcEQYFo1DkY3QNCgKiRUAgAE8dqp1NBNBBtEIqsZTNBzJZrAjMaTqgNbDt6q4WkC2gFgssujEEABWRI7ZL49JZHJ5ABEE0YZTkVMyColAyFVFpckyLPcnnZvk5nTAyqGZBGKIwAAltGA5otlqt1nhCMQklQ9PsENdgG41MhsQh+IJwuc5I8qK8EAA5FTw25zXBQsqeBASsClFS0ZBuCUIWQsBBOIi4RrXNzcMYwEFqPFpQlaHRkkwIMwWQUpVWIuT04yG1sIBNgbLHXkAFhDlDDAHVtNMMAB3UorBC0MBUhDz5CY1Il3sqeb4dPAubUa4AWgAohMAGxIVDobhgAA0CAhwFwyFnuDKLHgtC254vOzsZJNTZbxdQ6bkMFiIcBW2HtUnUDJuGyXIhAlABxIgcmoeVFW7Vs1SgDVnFZbVQPaLlQjwpkRn4eEjF5bBuBnBApmAWZUImIx8F9WUEAABWxMo1CVRRaO3fAJA2R0GKYhALS6VwJQAFRUfBuN7ed+KIVTzBUESyDIMAjCIlxREUYlek1ABmIcRCAA

[j-oliveras]

I cannot get the error (neither using 3.3.3333 or newer version): I get the same generated result as the link posted by AlCalzone.

Can you post your tsconfig.json and the generated code?

[thw0rted]

I made a mistake in the original reproduction code -- I condensed an example from a more complex problem that I thought would be representative but mistakenly thought that my base class had an abstract member. The base class getter is actually implemented, so I updated the example above to correct it. The bug does occur in the updated version, tested by running through tsc then executing the output in node.

The node error output is

TypeError: Cannot set property legs of #<Animal> which has only a getter

[thw0rted]

Since you asked, I just copied in a tsconfig from another project:

{
    "compilerOptions": {
        "alwaysStrict": true,
        "allowSyntheticDefaultImports": true,
        "emitDecoratorMetadata": true,
        "esModuleInterop": true,
        "experimentalDecorators": true,
        "lib": [
            "es2016",
            "dom"
        ],
        "module": "esnext",
        "moduleResolution": "node",
        "noImplicitAny": true,
        "noImplicitThis": true,
        "noUnusedLocals": true,
        "sourceMap": true,
        "strictNullChecks": true,
        "target": "esnext"
    },
}

and the output looks like

"use strict";
class Animal {
    constructor(name) {
        this.name = name;
    }
    get legs() { return -1; }
    move(distanceInMeters = 0) {
        console.log(`${this.name} moved ${distanceInMeters}m.`);
    }
}
class Snake extends Animal {
    get legs() { return 0; } // Correct implementation of abstract superclass property
    constructor(name) { super(name); }
    move(distanceInMeters = 5) {
        console.log("Slithering...");
        super.move(distanceInMeters);
    }
}
class Horse extends Animal {
    constructor(name) {
        super(name);
        this.legs = 4; // Works when using helper code to emulate pre-ES6 classes, fails with actual ES6
    }
    move(distanceInMeters = 45) {
        console.log("Galloping...");
        super.move(distanceInMeters);
    }
}
let sam = new Snake("Sammy the Python");
let tom = new Horse("Tommy the Palomino");
sam.move();
tom.move(34);
//# sourceMappingURL=main.js.map

[thw0rted]

I just looked at AlCalzone's link. That's very handy thanks! If you make the same change there, giving Animal#legs an implementation, then run it, you'll see an error similar to the one I posted above. Playing with the options there, it looks like this isn't actually specific to ES6 / native class output. It happens with any target. So, what I thought was a class issue, is actually because TS can't tell the difference between accessor and "normal" properties. I have the sinking feeling there might already been an issue for this, and the answer might be "we can't fix it".

[weswigham]

Looks like our es6-classes-but-no-class-fields downlevel emit needs a slight change - we need to overwrite the getter/setter on the superclass with the property defined (rather than completely erasing the field). This is probably best done as a followup to #30467 and #30829 as part of overhauling our field semantics to match the spec a bit better.

[fatcerberus]

@thw0rted

You can paste the above into Playground to see that there are no compile-time errors but the emitted JS does not demonstrate the error because as far as I can tell, you can't tell Playground to emit ES6. (That would be a nice feature; maybe I should put in a ticket for that?)

Until that feature gets added to the official playground, you could use this playground instead which has that feature:
https://typescript-play.js.org/

[thw0rted]

Does milestone modification count as "activity"? I don't want the stale-bot to eat this, it bit me again today.

[rbuckton]

@sandersn would this be related to the work we were discussing for the -useDefineForClassFields to error for the case where a field declaration overrides an accessor? We currently report an error only when -useDefineForClassFields is set. Should we change the behavior to always issue an error in this case?

[sandersn]

My current plan is to start issuing these as suggestions with a codefix when class fields reach stage 4. However, the code is error-prone and suspect, so it might be better to

1. Always report an error.
2. Provide a codefix.

In addition, pretty large corpora of code haven't shown more than a few instances of the error. That means that a codefix is lower priority since the error on its own is sufficient for new code that's being written.

@rbuckton We have plenty of time before 3.9, so maybe we should just turn the error on and see if we run into problems. What do you think?

[rbuckton]

We've been reporting this as an error since at least 4.0. I think we can close this.