Incorrect transpiled constructor of derived classes

[falsandtru]

• Missing own class methods
• Missing relationship with own class (Extending from Error doesn't work when emitting ES5 #10166)

TypeScript Version: master

Code

class C extends Error {
    constructor() {
        super('error');
    }
    m() {
    }
}
console.log(new C().message);
console.log(new C().m);
console.log(new C() instanceof Error);
console.log(new C() instanceof C);

Expected behavior:

var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var C = (function (_super) {
    __extends(C, _super);
    function C() {
        var _that = _super.call(this, 'error');
        if (_that) {
            _that.__proto__ = this;
        }
        var _this = _that || this;
        return _this;
    }
    C.prototype.m = function () {
    };
    return C;
}(Error));
console.log(new C().message); // 'error'
console.log(new C().m); // [Function]
console.log(new C() instanceof Error); // true
console.log(new C() instanceof C); // true

Actual behavior:

var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var C = (function (_super) {
    __extends(C, _super);
    function C() {
        return _super.call(this, 'error') || this;
    }
    C.prototype.m = function () {
    };
    return C;
}(Error));
console.log(new C().message); // 'error'
console.log(new C().m); // undefined
console.log(new C() instanceof Error); // true
console.log(new C() instanceof C); // false

[HerringtonDarkholme]

Babel has the same behavior... babel/babel#3083

And babel only supports extending builtin object as option plugin. https://www.npmjs.com/package/babel-plugin-transform-builtin-extend

Buth babel's fix is quite problematic as __proto__ is not universally supported and is specified as legacy feature.

[e-cloud]

@falsandtru, one thing should be mentioned is that your expected behavior will generate a weird prototype chain.

However, since it's hard to be fully compatible. That's acceptable temporarily.

[HerringtonDarkholme]

extending Error is quite common in JavaScript land. This would break many libraries, say Angular2.

https://github.com/angular/angular/blob/745e10e6d2ea9097b7ec650ae54cea91e3d193f2/modules/%40angular/facade/src/errors.ts#L16

It turns out Angular also have special handling for property lost.
And would be very hard to debug and discover. Maybe we can just list all native constructor to skip emitting var _this = _super.call(this, 'error') || this.

[falsandtru]

__proto__ is not universally supported

I think so, but this solution possibly has no regression. So TypeScript can fix this bug using this solution. Additionally, the latest stable version doesn't have this bug. So this bug will be the large regression, and it is too large to release as a stable version.

[falsandtru]

Ah, not true. Probably there is no regression for 2.1.0 (RC), but this solution makes some regressions for 2.0.8 (stable) at least on IE10 or below.

[HerringtonDarkholme]

@falsandtru I found another issue.

class A {
    constructor() {
        return {}
    }
}

class B extends A {}

var b = new B()

b instanceof A // should be false

Manually setting __proto__ is still problematic when returning a non primitive value.

Also, the current behavior still have another problem that when a primitive value is return, subclass properties are not correctly set. The following example can be run in both node and tsc. But their results are different.

class A {
    constructor() {
        return 123
    }
}

class B extends A {
    constructor() {
        super()
        this['prop'] = 13
    }
}

var b = new B()

console.log(b['prop']) // tsc prints undefined, node prints 13

As a comparison, Babel will log b['prop'] as 13.

The most proper way I can conceive is

1. adding a runtime check for _super.apply(this, arguments), assign it to _this iff. it has type of object or function
2. if subclass extends builtin object like Error and Array, _this should always be this

@DanielRosenwasser What's your opinion?

[falsandtru]

Of course, this issue is not restricted to builtin objects. But probably we don't need to consider primitive values now because seems like it is not supported by the spec. However, if TS users incorrectly use primitive values, the current code like _super.call(this, ...) || this will make some different behaviors.

class A {
    constructor() {
        return 1
    }
}
console.log(new A()) // node prints A, not 1

[HerringtonDarkholme]

Returning primitive is supported in JS spec. Babel supports this by _possibleConstructorReturn helper.

Primitive value will only influence inherited class. In the example above. TS also prints A because new A will return this instead if the return value is primitive. Spec here.

And inheriting is also fine before #10762 because TS used to ignore super constructor's return value.

Now, for code like class A {constructor() {return 123}}; class B extends A {}, new B will still return this object because _super.call(this, ...) || this evaluates to a primitive. Yet additional operations in child class constructor is lost.

[falsandtru]

Thanks for the info, but I did run my previous example without transpiling. If you are right, why I cannot to get the primitive value result in that example?

[HerringtonDarkholme]

The rule of thumb is: new Ctor will never get a primitive return value. But Ctor.apply(this, arguments) can return a primitive value.

The first rule is specified by 9.2.2 in ECMA262.

If result.[[type]] is return, then
If Type(result.[[value]]) is Object, return NormalCompletion(result.[[value]]).
If kind is "base", return NormalCompletion(thisArgument).

So, even if A return a number, new A() will return this object.

The second rule applies to transpiled body.

var A = (function () {
    function A() {
        return 123;
    }
    return A;
}());
var B = (function (_super) {
    __extends(B, _super);
    function B() {
        var _this = _super.apply(this, arguments) || this; // here, _super.apply(this, arguments) return 123
        _this.prop = 123; 
        return _this; // _this is primitive value 1232
    }
    return B;
}(A));

In the transpiled js, _super.apply will return 123, so B's constructor will return primitive. Then rule 1 comes to play, which makes new B a this object where prop is never assigned.

This example might be contrived. But there might non trivial code return value that relies on external data (a http call, a database access). In those scenarios this bug might be very hard to find.

[falsandtru]

I understand, it is one of the case. But I think the handling of primitives is not very important in this issue. It can be included, but it can be also a separated issue. So I didn't include it in this issue's description.

[falsandtru]

As a note, I found another bug. Compiler must not create return statements oneself. It makes another bug when inheriting 2 times or more. So a more expected code is:

var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var C = (function (_super) {
    __extends(C, _super);
    function C() {
        var _that = _super.call(this, 'error');
        if (_that) {
            _that.__proto__ = this;
        }
        var _this = _that || this;
        // do something
        if (_that) {
            return _this;
        }
    }
    C.prototype.m = function () {
    };
    return C;
}(Error));
console.log(new C().message); // 'error'
console.log(new C().m); // [Function]
console.log(new C() instanceof Error); // true
console.log(new C() instanceof C); // true

[HerringtonDarkholme]

I really appreciate your effort to couple with extending native object. But it turns out very very hard for transpiler. For example, Babel does not support native Error at all! Angular2 uses custom getter/setter to mock native behavior.

Setting proto to this is cyclic, which, I would argue, introduces more problem than it solves.

Compiler must not create return statements yourself

Does this mean code below is not permitted?

class A {
  constructor() { return this; }
}

class B extends A {}

[falsandtru]

Does this mean code below is not permitted?

No, I don't block any source code. I mean generated return statements which are not written in source code have an unexpected influence on _super.call(this, ...) || this.

add: without when _super.call(this, ...) returns any object. Transpiled class constructor needs to return this object. Then compiler needs to add return statements oneself as I wrote the example code in the previous comment #12123 (comment).

[falsandtru]

@ahejlsberg @sandersn @vladima @RyanCavanaugh @DanielRosenwasser @mhegazy Can you resolve these problems before releasing 2.1? I believe TypeScript must not publish these problems via stable versions. At least these problems will break some code because of missing subclass methods.

[DanielRosenwasser]

The thing is that extending Error and the like were previously broken as well, but in different ways. As a workaround, you can manually patch the prototype in your constructor, though I know that this is a less-than-satisfying answer.

@falsandtru can you elaborate on the issue you mentioned when inheriting two levels deep or more?

[DanielRosenwasser]

@HerringtonDarkholme what is your use case for returning a primitive type from the constructor of your class?

[HerringtonDarkholme]

@DanielRosenwasser Handling primitive return value is not for real use but for bug catching.

If some one return value in constructor from external sources like server call, it will misbehave, of course. But the point here is the misbehavior in TypeScript should also follow ES spec.

[falsandtru]

As I said in #12123 (comment), this problem is not restricted to builtin objects. All subclasses which return a truthy value lose their methods. It will affect not a few users. So it need to fix natively.

About "two levels", I found the unexpected generated return statements only in subclasses. However, I don't investigate the details yet.

[HerringtonDarkholme]

All subclasses which return a truthy value lose their methods.

@falsandtru Yes, they should lose methods.

In node 6:

class A {constructor() {return {}}};
(new A) instanceof A // false

[falsandtru]

Sorry, I confused about the two different cases.

class B {
    constructor() {
        return {};
    }
}
class D extends B {
    m() {
    }
}
console.log(new D().m); // undefined, this method is lost possibly correctly.

class D extends Error {
    m() {
    }
}
console.log(new D().m); // undefined, this method is lost incorrectly.

So I take back my words. This problem is restricted to a few cases such as builtin objects.

[aluanhaddad]

It is worth noting that in say, recent versions of chrome, extending an ES6 class like Map will throw on the call to super.apply.

Now code compiled for older browsers breaks on newer browsers. I guess this is a chicken and egg problem because ES6 uses new.target to resolve these ambiguities but I think that is a "special" value that must be provided by ES6 runtimes.

I realize that this is tangentially related at best... but I thought it worth bringing up.

[falsandtru]

@aluanhaddad Also see #11304.

[falsandtru]

Merge into #10166

[devpreview]

@DanielRosenwasser

It's solution doesn't not work on TypeScript v2.7.2:

class FooError extends Error {
    constructor(m: string) {
        super(m);

        // Set the prototype explicitly.
        Object.setPrototypeOf(this, FooError.prototype);
    }

    sayHello() {
        return "hello " + this.message;
    }
}

Compile error:

TSError: ⨯ Unable to compile TypeScript
src/main/object-mapper-error.ts (24,16): Property 'setPrototypeOf' does not exist on type 'ObjectConstructor'. Did you mean 'getPrototypeOf'? (2551)

[devpreview]

My solution:

tsconfig.json:

{
  "compilerOptions": {
    "target": "es2017",
    "lib": [
      "es2017"
    ]
  }
}

custom-error.ts:

export class CustomError extends Error {

    public readonly name: string;
    public readonly message: string;
    public readonly stack?: string;
    public readonly cause?: Error;

    public constructor(message?: string, cause?: Error) {
        super(message);
        this.name = this.constructor.name;
        this.message = message || 'Unexpected error';
        Error.captureStackTrace(this, this.constructor);
        this.cause = cause;
    }

}

[christianbradley]

My dudes. Have you heard the good news about factory functions?

function CustomError<T extends object>(
    message: string,
    properties: T,
    ctor: Function = CustomError
): Error & T {
    const error = Error(message)
    Error.captureStackTrace(error, ctor)
    return Object.assign(error, { name: ctor.name }, properties)
}

interface InvalidFoo extends Error {
  code: 1000
  data: { foo: string }
}

function InvalidFoo(data: InvalidFoo['data']): InvalidFoo {
    return CustomError(`Invalid foo: ${foo}`, { code: 1000, data: { foo } }, InvalidFoo)
}

[Yaojian]

My solution here

• ensure the Error.captureStackTrace is called only from the most concrete class

CustomError

export interface IErrorConstructor {
    new(...args: any[]): Error;
}

// tslint:disable:max-line-length
/**
 * Workaround for custom errors when compiling typescript targeting 'ES5'.
 * see: https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
 * @param {CustomError} error
 * @param newTarget the value of `new.target`
 * @param {Function} errorType
 */
// tslint:enable:max-line-length
export function fixError(error: Error, newTarget: IErrorConstructor, errorType: IErrorConstructor) {
    Object.setPrototypeOf(error, errorType.prototype);

    // when an error constructor is invoked with the `new` operator
    if (newTarget === errorType) {
        error.name = newTarget.name;

        // exclude the constructor call of the error type from the stack trace.
        if (Error.captureStackTrace) {
            Error.captureStackTrace(error, errorType);
        } else {
            const stack = new Error(error.message).stack;
            if (stack) {
                error.stack = fixStack(stack, `new ${newTarget.name}`);
            }
         }
    }
}


export function fixStack(stack: string, functionName: string) {
    if (!stack) return stack;
    if (!functionName) return stack;

    // exclude lines starts with:  "  at functionName "
    const exclusion: RegExp = new RegExp(`\\s+at\\s${functionName}\\s`);

    const lines = stack.split('\n');
    const resultLines = lines.filter((line) => !line.match(exclusion));
    return resultLines.join('\n');
}

export class CustomError extends Error {
    constructor(message: string) {
        super(message);
        fixError(this, new.target, CustomError);
    }
}

unit tests

describe('error-utils', () => {
    const msg = 'something-wrong';

    describe('fixStack', () => {
        function createError(): string {
            return new Error('f').stack!;
        }

        it('should exclude names from stack', () => {
            const s = createError();
            console.log(s);
            expect(s).toMatch(createError.name);

            const fixed = fixStack(s, createError.name);
            expect(fixed).not.toMatch(createError.name);
        });
    });

    describe('CustomError', () => {
        it('should be instanceof CustomError and ancestor types', () => {
            const e = new CustomError(msg);
            expect(e).toBeInstanceOf(CustomError);
            expect(e).toBeInstanceOf(Error);
        });
        it('should exclude error constructors from stack', () => {
            const e = new CustomError(msg);
            const s = e.stack;
            expect(s).toMatch(msg);
            expect(s).not.toMatch(`new CustomError`);
        });
        it('should call `captureStackTrace` only once.', () => {
            const spy = jest.spyOn(Error, 'captureStackTrace');
            try {
                // tslint:disable-next-line:no-unused-expression
                new CustomError(msg);
                expect(spy).toHaveBeenCalledTimes(1);

                // the constructorOpt argument
                expect(spy.mock.calls[0][1]).toBe(CustomError);
            } finally {
                spy.mockRestore();
            }
        });
    });
    describe('DerivedError', () => {

        class DerivedError extends CustomError {
            constructor(message: string) {
                super(message);
                fixError(this, new.target, DerivedError);
            }
        }

        it('should be instanceof CustomError and ancestor types', () => {
            const e = new DerivedError(msg);
            expect(e).toBeInstanceOf(DerivedError);
            expect(e).toBeInstanceOf(CustomError);
            expect(e).toBeInstanceOf(Error);
        });
        it('should exclude error constructors from stack', () => {
            const e = new DerivedError(msg);
            const s = e.stack;
            expect(s).toMatch(msg);
            console.log(s);
            expect(s).not.toMatch(`new CustomError`);
            expect(s).not.toMatch(`new DerivedError`);
        });
        it('should call `captureStackTrace` only once.', () => {
            const spy = jest.spyOn(Error, 'captureStackTrace');
            try {
                // tslint:disable-next-line:no-unused-expression
                new DerivedError(msg);
                expect(spy).toHaveBeenCalledTimes(1);

                // the constructorOpt argument
                expect(spy.mock.calls[0][1]).toBe(DerivedError);
            } finally {
                spy.mockRestore();
            }
        });
    });
});

[TomaszWaszczyk]

@mhegazy Could you describe what you mean by "alternatively, you can just set this.proto instead of using Object.setPrototypeOf`". I am facing the issue having TypeScript 3.0.3.