Skip to content

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

Closed

Description

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!

Activity

  1. DanielRosenwasser commented on Nov 30, 2015

    @DanielRosenwasser
    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.

  2. weswigham commented on Dec 1, 2015

    @weswigham
    Member

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

  3. weswigham commented on Dec 2, 2015

    @weswigham
    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 Anders Hejlsberg (@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.

  4. weswigham commented on Dec 3, 2015

    @weswigham
    Member

    Remo H. Jansen (@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.

  5. added
    By DesignDeprecated - use "Working as Intended" or "Design Limitation" instead
    Breaking ChangeWould introduce errors in existing code
    and removed
    Breaking ChangeWould introduce errors in existing code
    on Dec 3, 2015
  6. remojansen commented on Dec 3, 2015

    @remojansen
    ContributorAuthor

    Wesley Wigham (@weswigham) 👍 thanks for that

  7. locked and limited conversation to collaborators on Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    BugA bug in TypeScriptBy DesignDeprecated - use "Working as Intended" or "Design Limitation" insteadFix AvailableA PR has been opened for this issue

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions