Incorrect transpiled constructor of derived classes

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

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.

falsandtru (@falsandtru)

falsandtru (@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.

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.

__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.

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.

update: sync with merge changes from last commit

falsandtru (@falsandtru)

falsandtru (@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

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

Daniel Rosenwasser (@DanielRosenwasser) What's your opinion?

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

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.

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?

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.

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.

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

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 {}

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).

Great!

fix: test error due to Typescript limitation

Daniel Rosenwasser (@DanielRosenwasser)

Daniel Rosenwasser (@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)

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;
    }

}

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)
}

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();
            }
        });
    });
});

Mohamed Hegazy (@mhegazy)

Mohamed Hegazy (@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.

Add ts-invariant package.

Work around TypeScript's failure to extend Error correctly.

Remove unhelpful and slightly buggy enhanceErrorWithDocument function.

Navigation Menu

Repository navigation

Incorrect transpiled constructor of derived classes #12123

21 remaining items

My solution here

CustomError

unit tests

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Incorrect transpiled constructor of derived classes #12123

Description

Activity

HerringtonDarkholme commented on Nov 9, 2016

e-cloud commented on Nov 9, 2016

HerringtonDarkholme commented on Nov 9, 2016

falsandtru commented on Nov 9, 2016

falsandtru commented on Nov 9, 2016

HerringtonDarkholme commented on Nov 10, 2016

falsandtru commented on Nov 10, 2016

HerringtonDarkholme commented on Nov 10, 2016

falsandtru commented on Nov 10, 2016

HerringtonDarkholme commented on Nov 10, 2016

falsandtru commented on Nov 10, 2016

falsandtru commented on Nov 10, 2016

HerringtonDarkholme commented on Nov 10, 2016

falsandtru commented on Nov 10, 2016

21 remaining items

falsandtru commented on Dec 7, 2016

devpreview commented on Feb 23, 2018

devpreview commented on Feb 23, 2018

christianbradley commented on Mar 9, 2018

Yaojian commented on Jun 6, 2018

My solution here

CustomError

unit tests

TomaszWaszczyk commented on Sep 19, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions