Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extending from Error doesn't work when emitting ES5 #10166

Closed
mcclure opened this issue Aug 5, 2016 · 15 comments
Closed

Extending from Error doesn't work when emitting ES5 #10166

mcclure opened this issue Aug 5, 2016 · 15 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed Fixed A PR has been merged for this issue

Comments

@mcclure
Copy link

mcclure commented Aug 5, 2016

I am using Typescript 1.8.10. I am using Node v4.4.7 as my primary means of executing Javascript.

Consider this very simple code (I will refer to this later as "code snippet 1"):

class GenConstructFail extends Error {} let x = new GenConstructFail("Test one two"); console.log(x.message)

I run this with tsc test.ts && node test.js. My expected behavior is that this will print "Test one two". My observed behavior is that it prints only a blank line.

There have been issues filed about this before, the ones I found seem to all link back to #5069 where project member @mhegazy closed the issue and explained "The issue is with how these built in types are hand[led] by the engine. They are extensible as per the es6 spec, but do not think the engines are there yet." However, I believe this is not the correct way to think about the issue.

If I look at the ES2015 spec, I find:

19.5.1 The Error Constructor

The Error constructor is the %Error% intrinsic object and the initial value of the Error property of the global object. When Error is called as a function rather than as a constructor, it creates and initializes a new Error object. Thus the function call Error(…) is equivalent to the object creation expression new Error(…) with the same arguments.

The Error constructor is designed to be subclassable. It may be used as the value of an extends clause of a class definition. Subclass constructors that intend to inherit the specified Error behaviour must include a super call to the Error constructor to create and initialize subclass instances with a [[ErrorData]] internal slot.

The last paragraph here is important. The ES2015 spec requires that Error be subclassable using extends and super(). However, it does not specify Error should be subclassable by other means. Here is what tsc emits when run with the default target of ES5 (I will refer to this later as "code snippet 2"):

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 GenConstructFail = (function (_super) {
    __extends(GenConstructFail, _super);
    function GenConstructFail(m) {
        _super.call(this, m);
    }
    return GenConstructFail;
}(Error));
var x = new GenConstructFail("Test one two");
console.log(x.message);

I do not fully understand what the Typescript __extends function is doing, but the generated code for GenConstructCall appears to revolve around expecting it can invoke call on the _super object, in this case Error. However, call is a method on Function.prototype and Error does not have Function.prototype in its prototype chain. Error is an intrinsic object, its prototype is also an intrinsic object (defined in section 19.5.2.1 of the spec), and the operations supported on Error, its constructor, and its prototype are all explicitly enumerated. call is not among them.

So in other words, if the code Typescript generates in ES5 mode does not behave correctly with regard to constructor parameters, it is not because the engines have not yet caught up to ES6, it is because Typescript has generated code which is neither correct ES5 nor correct ES6.

In fact, in my testing, I find that ES6 engines that fully support "code snippet 1" above when simply evaluated as code, do not support "code snippet 2" (the Typescript-transpiled-to-ES5 version of the same code). I tested with Chrome version "51.0.2704.106 m" (up to date) and also with "Microsoft Edge 25.10586.0.0 / Microsoft EdgeHTML 13.10586"; in both cases, code snippet 1 printed "Test one two" and code snippet 2 printed a blank line. (A person I talked to on Twitter tested with Node 5.4.1 and saw these same results.) I also found that in both Chrome and Edge, Error("test") produced an Error object with the message "test" (this is a special behavior guaranteed by section 19.5.1.1 of the ES2015 spec) but Error.call("test") produced an Error object with a blank message.

So, to summarize: Typescript's ES5 target generates an invalid constructor when Error is extended, it is doing this predictably and 100% of the time, and it is invalid because of the Javascript spec and not because of any particular engine limitation. This should be considered a bug.

Moreover, it seems Typescript could be easily modified to fix this. The problem is that .call invocation. Consider this alternate version of code snippet 1 (let's call this "code snippet 3"):

class GenConstructFail extends Error { constructor(m:string) { super(); this.message = m } }
let x = new GenConstructFail("TEST THREE FOUR"); console.log(x.message)

Here is the Javascript tsc emits for code snippet 3 (let's call this "code snippet 4"):

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 GenConstructFail = (function (_super) {
    __extends(GenConstructFail, _super);
    function GenConstructFail(m) {
        _super.call(this);
        this.message = m;
    }
    return GenConstructFail;
}(Error));
var x = new GenConstructFail("TEST THREE FOUR");
console.log(x.message);

The problem with "code snippet 2", at least with V8 and Edge, is that call() is swallowing the arguments to the constructor (which is after all not a real constructor). Assigning the fields after the call() avoids this problem and the code prints "TEST THREE FOUR" even on my old copy of Node 4.4.7.

My suggested fix is that Typescript should detect when it is subclassing Error or one of the built-in Error subclasses (all intrinsics) while in ES5 mode, and in this case generate a constructor which instead of assuming call() works simply takes the arguments to super() and assigns them to this after call(). (And maybe it would be better to find a way to avoid call() altogether if you can, since I see nothing in the spec to guarantee it even exists on Error.)

@DanielRosenwasser
Copy link
Member

It's late and I need to go to sleep, but after having experimented with this, I believe that this issue is related to or a duplicate of #7574.

@DanielRosenwasser
Copy link
Member

Basically the reason I say it's a duplicate is because if we simply capture the value of the call to super and return it, you get the correct behavior. Try this out:

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 GenConstructFail = (function (_super) {
    __extends(GenConstructFail, _super);
    function GenConstructFail() {
        var SUPER_RESULT = _super.apply(this, arguments) || this;
        // substitute any use of 'this' with 'SUPER_RESULT'
        return SUPER_RESULT;
    }
    return GenConstructFail;
}(Error));
var x = new GenConstructFail("Test one two");
console.log(x.message);

@DanielRosenwasser DanielRosenwasser added the Duplicate An existing issue was already created label Aug 6, 2016
@DanielRosenwasser
Copy link
Member

In any case, thanks for filing because it shows that this is another good use-case.

@mcclure
Copy link
Author

mcclure commented Aug 6, 2016

Daniel-- I don't think this is a duplicate of #7574, although it is clearly a related problem. At least, your sample code does not work, so if this is the code that a fix for #7574 would produce then it would not resolve this bug.

If I run your code ("code snippet 5"), I find that x.message does have the desired value on both current Chrome and Edge. However, let's say I add two more lines:

console.log(x instanceof Error)
console.log(x instanceof GenConstructFail)

On both Chrome and Edge, x instanceof Error prints true and x instanceof GenConstructFail prints false. So you have fixed the message bug, but created a new bug which is possibly worse because killing x.message results only in lost data and breaking instanceof is likely to result in actual changes to control flow (because program logic decisions are more likely to be based on instanceof than the contents of message).

The reason your sample code does not work is that code snippet 5, just as much as code snippet 2, depends on undefined behavior of Error. Error is to my reading of the spec not guaranteed to have either call or apply and therefore the behavior of call and apply on Error could do nearly anything. In practice (in Chrome and Edge-- other ES2015 engines maybe not) it appears Error.call(this, args) ignores all of its arguments and returns a new Error(), whereas Error.apply(this, args) ignores the this argument, honors args and returns a new Error with "args" as its arguments. So in code snippet 2, the Error.call(this, args) alone on a line has literally no effect and so args are just lost, but this has already been primed with a correct prototype chain so at least instanceof works; and in code snippet 5 the Error.apply(this, args) does (at least in Chrome and Edge) successfully get the arguments into Error, but the previous value of this and therefore the prototype chain are lost.

I believe that the actual bug in this particular issue is that Error is unique and cannot be treated like a constructor or function, and this is an additional issue on top of what #7574 is trying to solve.

@DanielRosenwasser DanielRosenwasser removed the Duplicate An existing issue was already created label Sep 4, 2016
@DanielRosenwasser
Copy link
Member

Having gotten the opportunity to revisit this in investigating #7574, I now see the issue. However, I'm not sure what we can do here. I think we'd have to do a special check to correctly set the prototypes on certain classes.

@DanielRosenwasser DanielRosenwasser added the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Sep 4, 2016
@DanielRosenwasser
Copy link
Member

I'll bring this up with @bterlson at some point soon.

@mcclure
Copy link
Author

mcclure commented Sep 4, 2016

At the moment, I am working around this by never inheriting from Error, and instead exclusively inheriting from this class:

class CustomError extends Error {
    constructor(message:string) { super() ; this.message = message }
}

This does seem to demonstrate that working around fake-constructor entities is easy, assuming you have the mechanism in place to identify the situations where you need to work around...

@falsandtru
Copy link
Contributor

falsandtru commented Nov 9, 2016

@DanielRosenwasser @mcclure How is this?

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 'm';
    };
    return C;
}(Error));
console.log(new C().message); // 'error'
console.log(new C().m()); // 'm'
console.log(new C() instanceof Error); // true
console.log(new C() instanceof C); // true

This is included in #12123

@e-cloud
Copy link
Contributor

e-cloud commented Nov 28, 2016

Is there no progress or solution on this topic? It's blocking some of my code like #12097.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Dec 6, 2016

Technically this will be fixed in TypeScript 2.1; however, a different part of your code may no longer work. message will now be correctly initialized, however, the prototype chain won't be linked up correctly. See the write-up documented here.

@mhegazy mhegazy removed the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Dec 6, 2016
@mhegazy mhegazy closed this as completed Dec 6, 2016
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Dec 6, 2016
@mhegazy mhegazy added this to the TypeScript 2.1.3 milestone Dec 6, 2016
@mhegazy mhegazy added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Dec 6, 2016
@eschwartz
Copy link

eschwartz commented Dec 28, 2016

This workaround worked for me:

interface IErrorConstructor {
  new (message:string):Error;
}

const CustomError = function(message:string) {
  Error.call(this, message);
  Error.captureStackTrace(this);

  this.message = message;
  this.name = this.constructor.name;
} as any as IErrorConstructor;
CustomError.prototype = Object.create(Error.prototype);

class MyError extends CustomError {}

const err = new MyError('something bad happened!');
err instanceof MyError;   // true
err.message;  // 'something bad happened'
err.name;  // 'MyError'

@tuxslayer
Copy link

I wonder why compiler cannot generate the code like this...

@shirakaba
Copy link

shirakaba commented Jun 14, 2017

@eschwartz Will your workaround work when targeting ES5?

Edit: As far as I can tell, it does!

@edaniels
Copy link

What's the status of this? This took us a while to find out what was going when switching our targets from ES2015 to ES5.

@opterion
Copy link

Hi! Why don't just make the solution like in babel-plugin-transform-builtin-extend?
And anyone can enable the option if he agree to not support IE<=10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

10 participants