ES5 class rewriting incompatible with frozen intrinsics

[kriskowal]

Bug Report

When TSC targets ES5, classes that extend other classes generate JS that throws a Cannot assign to read only property 'constructor' of object TypeError when the intrinsics, particularly the Function.prototype, are frozen. This affects programs that are freezing their intrinsics to protect against supply chain attacks, particularly using SES.

🔎 Search Terms

• constructor
• frozen

🕗 Version & Regression Information

I identified this issue in the published code of external-editor 3.1.0, which is using TypeScript 3.5.2 https://github.com/mrkmg/node-external-editor/blob/e1070a8295b8ebbafa7b29802ae0e2da0fad47c0/tsconfig.json

In the playground, the output is consistent for the very oldest and very newest versions of TypeScript when targeting ES5.

⏯ Playground Link

Playground link with relevant code

💻 Code

Object.freeze(Function);
Object.freeze(Function.prototype);
class Foo {
    constructor() {
    }
}
class Bar extends Foo {
    constructor() {
    }
}

🙁 Actual behavior

This code throws an exception like:

TypeError: Cannot assign to read only property 'constructor' of object ''

🙂 Expected behavior

This should not throw an exception. This can be achieved by using Object.defineProperty(prototype, 'constructor', {value: constructor}) instead of prototype.constructor = constructor. This is a symptom of the so-called property override mistake, in the design of ES5.

[erights]

Attn @DanielRosenwasser @rbuckton

[RyanCavanaugh]

This can be achieved by using Object.defineProperty(prototype, 'constructor', {value: constructor})

Our helper emit is designed to work in ES3 as well, where defineProperty is not available. I'm not sure that other alternatives are available; you can provide your own __extends helper that uses that approach if you're not running in ES3 environments.

[kriskowal]

There’s no way to discriminate the output based on ES5 vs ES3?

[kriskowal]

Would it be an option to feature-test Object.defineProperty to discriminate at runtime?

[RyanCavanaugh]

There’s no way to discriminate the output based on ES5 vs ES3?

This just confuses the situation, because this is a global helper with "first in wins" semantics (to enable you to provide your own, as mentioned) - so if an ES3-targeted library loads before an ES5-targeted library, which is a totally legal situation, then the ES3 version of the helper is going to win and you're back where you started.

[kriskowal]

Would it work if the ES3 and ES5 helpers were identical:

function __extends(x, y) {
  if (Object.defineProperty) {
    Object.defineProperty(x, 'constructor', {value: y});
  } else {
    x.constructor = y;
  }
}

[DanielRosenwasser]

We do that in __makeTemplateObject, __createBinding, and __setModuleDefault for what it's worth.

[rbuckton]

We do that in __makeTemplateObject, __createBinding, and __setModuleDefault for what it's worth.

I took a look and we do that wrong in __makeTemplateObject, unfortunately. For __createBinding and __setModuleDefault we check for Object.create (which is good), but we check for Object.defineProperty in __makeTemplateObject. The problem with testing for Object.defineProperty is that IE8's Object.defineProperty could only be used on DOM nodes, however IE8 did not support Object.create.

So, we need to fix __makeTemplateObject, and the __extends helper should probably be:

var __extends = (this && this.__extends) || (function () {
    var extendStatics = function (d, b) {
        extendStatics = Object.setPrototypeOf ||
            ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
            function (d, b) { for (var p in b) if (Object.prototype.hasOwnProperty.call(b, p)) d[p] = b[p]; };
        return extendStatics(d, b);
    };
    var setConstructor = function (f, d) {
        setConstructor = Object.create ?
            function (f, d) { Object.defineProperty(f, "constructor", { value: d, writable: true, configurable: true }); } :
            function (f, d) { f.constructor = d; };
        setConstructor(f, d);
    };
    return function (d, b) {
        if (typeof b !== "function" && b !== null)
            throw new TypeError("Class extends value " + String(b) + " is not a constructor or null");
        extendStatics(d, b);
        function __() { setConstructor(this, d); }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    };
})();

[kumavis]

@rbuckton should this

setConstructor = Object.create ?

be

setConstructor = Object.defineProperty ?

[kumavis]

oh i see you explained exactly why you did this 🤦 I suppose adding a comment would help clarify its intentional. 😸

[erights]

Hi @rbuckton any progress on #43450 (comment) ?