Skip to content

Type guard not working as expected in function overloading with union types #5826

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

Closed
remojansen opened this issue Nov 30, 2015 · 5 comments
Closed
Labels
Bug A bug in TypeScript By Design Deprecated - use "Working as Intended" or "Design Limitation" instead Fix Available A PR has been opened for this issue

Comments

@remojansen
Copy link
Contributor

class Square {
    length : number; 
    constructor(length : number) {
        this.length = length;
    }
}

class Rectangle {
    length : number;
    width : number;
    constructor(length : number, width : number) {
        this.length = length;
        this.width = width;
    }
}

class Triangle {
    base : number
    height : number;
    constructor(base : number, height : number) {
        this.base = base;
        this.height = height;
    }
}

The following works as expected:

namespace function_overloading_with_any {

    function calculateArea(shape : Square) : number;    // overloaded signature
    function calculateArea(shape : Rectangle) : number; // overloaded signature
    function calculateArea(shape : Triangle) : number;  // overloaded signature
    function calculateArea(shape : any) : number        // implementation signature
    {
        if(shape instanceof Square) {
            return shape.length * shape.length;
        }
        else if(shape instanceof Rectangle) {
            return shape.length * shape.width;
        }
        else if(shape instanceof Triangle) {
            return (shape.base * shape.height) / 2;
        }
    }


    var square = new Square(5); 
    var rectangle = new Rectangle(3, 5); 
    var triangle = new Triangle(3, 5);

    calculateArea(square);  
    calculateArea(rectangle); 
    calculateArea(triangle); 
}

However, when I use an union type instead of anyin the implementation signature:

namespace chapter02.function_overloading.with_union {

    function calculateArea(shape : Square) : number;
    function calculateArea(shape : Rectangle) : number;
    function calculateArea(shape : Triangle) : number;
    function calculateArea(shape : (Square | Rectangle | Triangle)) : number {
        if(shape instanceof Square) {
            return shape.length * shape.length;
        }
        else if(shape instanceof Rectangle) {
            return shape.length * shape.width; // Error!
        }
        else if(shape instanceof Triangle) {
            return (shape.base * shape.height) / 2;
        }
    }

    var square = new Square(5); 
    var rectangle = new Rectangle(3, 5); 
    var triangle = new Triangle(3, 5);  

    calculateArea(square); 
    calculateArea(rectangle); 
    calculateArea(triangle);
}

I get a compilation error:

error TS2339: Property 'length' does not exist on type 'Triangle'.
error TS2339: Property 'width' does not exist on type 'Triangle'.

Looks like the type guard is not working as expected. I can use an if instead of if else to fix the error:

    function calculateArea(shape : Square) : number;
    function calculateArea(shape : Rectangle) : number;
    function calculateArea(shape : Triangle) : number;
    function calculateArea(shape : (Square | Rectangle | Triangle)) : number {
        if(shape instanceof Square) {
            return shape.length * shape.length;
        }
        if(shape instanceof Rectangle) {
            return shape.length * shape.width; // OK!
        }
        if(shape instanceof Triangle) {
            return (shape.base * shape.height) / 2;
        }
    }

I'm using TypeScript 1.8.0-dev.20151129 with gulp-typescript:

gulp.task('build', ['lint'], function() {

    var tsProject = ts.createProject('tsconfig.json', {
        typescript: require('typescript')
    });

    var compile = ts(tsProject);

    return gulp.src("src/**/**.ts")
                .pipe(compile)
                .js.pipe(gulp.dest("src/"));
});

And the following tsconfig.json compilation:

{
  "compilerOptions": {
    "target": "es5",
    "module": "umd",
    "moduleResolution": "node",
    "isolatedModules": false,
    "jsx": "react",
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "declaration": false,
    "noImplicitAny": false,
    "removeComments": true,
    "noLib": false,
    "preserveConstEnums": true,
    "suppressImplicitAnyIndexErrors": true
  }
}

The source code that I used to discover this issue can be found here.

Thanks!

@DanielRosenwasser
Copy link
Member

I think this is happening because we try to eliminate all types in the union that are assignable to Square in the first else. That's very incorrect because Rectangle is assignable to Square, but it has nothing to do with its instance identity.

@DanielRosenwasser DanielRosenwasser added the Bug A bug in TypeScript label Nov 30, 2015
@weswigham weswigham self-assigned this Nov 30, 2015
@weswigham
Copy link
Member

I think this may be a regression - in the playground, there's no errors for this example.

@mhegazy mhegazy added this to the TypeScript 1.8 milestone Dec 2, 2015
@weswigham
Copy link
Member

I think I've figured out why this is a regression - when I swapped narrowing to work from outside in rather than inside out (lol) in #5442 so empty sets could correctly propagate down and compound/nested type guards would work as expected, at the same time I made it so behaviors like this became visible. We had always been narrowing with the logic/spec saying this should happen, but since we'd narrow by the if within the else, then the else, we'd narrow Square | Rectangle | Triangle to Rectangle and then just return that subtype again at the higher level (since Rectangle wouldn't narrow any further - by old logic the expression !(instanceof Square) would have resulted in an intervening empty set which resulted in the type at that step just getting reused (Rectangle)).

So fixing a set of other bugs exposed this one. :D

While the new behavior is a change, the behavior described in the post, as Daniel says, is by design. Since Rectangle is a Square, instanceof Square removes Rectangle from the union, as instanceof type guards act the same way as user-defined type guards - structurally.

This is actually problematic - instanceof at runtime is highly nominal, it will actually only catch instances of the RHS constructor or its prototypical inheritors - structural similarity doesn't play into it, and because of our ignorance of this, we do inaccurate narrowing. We could add a nominal relationship to use with instanceof fairly easily, but it is fairly against the philosophy of TS - we're actively trying to remove the only place where we've accidentally introduced nominality.

So @ahejlsberg, this behavior stays (since it's correct, according to our spec - it was incorrect before) unless we want to add a nominal relationship to use with instanceof.

@weswigham
Copy link
Member

@remojansen as a workaround while we figure this out (or if it just sticks around), you can either brand your classes with dummy members (_square: void, _rectangle: void, _triangle: void) so they're not structurally assignable or reorder your if/else if chains so that the more specific types (eg, Rectangle) are first.

@weswigham weswigham added By Design Deprecated - use "Working as Intended" or "Design Limitation" instead Breaking Change Would introduce errors in existing code and removed Breaking Change Would introduce errors in existing code labels Dec 3, 2015
@remojansen
Copy link
Contributor Author

@weswigham 👍 thanks for that

@mhegazy mhegazy closed this as completed Dec 8, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Nov 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript By Design Deprecated - use "Working as Intended" or "Design Limitation" instead Fix Available A PR has been opened for this issue
Projects
None yet
Development

No branches or pull requests

5 participants