Type guard with instanceof fails to narrow type in `else` block

NoelAbrahams commented 9 years ago

Hi,

TS Version: 1.4

In the following:

class Bar {
    bar ='bar';
}

class Foo {
    foo = 100;
}

function f1(x: Bar|Foo) {

    if(x instanceof Bar){
        console.log(x.bar);
    }
    else {
        console.log(x.foo); // Error: inferred as Bar|Foo
    }   
}
 f1(new Bar()); // bar
 f1(new Foo()); // 100

is there any reason why in the else block we are failing to narrow the type to Foo?

ahejlsberg commented 9 years ago

This is by design. The expression x instanceof Bar tests whether x references an instance that was created by the Bar constructor function. But because of structural typing, even when the result is false we can't conclusively say that x isn't compatible with the Bar type. Indeed, you could write

f1({ bar = "hello" });

and it would be a perfectly fine instance of the Bar type.

User-defined type guard functions (#1007) would be trusted to deliver a conclusive result, but they're not implemented yet.

NoelAbrahams commented 9 years ago

even when the result is false we can't conclusively say that x isn't compatible with the Bar type. Indeed, you could write

f1({ bar : "hello" });

and it would be a perfectly fine instance of the Bar type.

Structurally yes, but in practice we don't expect them to be compatible at all:

var bar = { bar : 'bar' };
if(bar instanceof Bar){
    // Not going to happen
}

I would vote to have the else block narrowed in the original example, because that would extend the usefulness of the instanceof checks.

I'm still not sure about user-defined type guard functions. The proposed syntax looks foreign and I'm not very happy with introducing code just to coax the type system into doing things. It would make sense to get more feedback regarding how useful it will be in practice - once people have become more familiar with union types.

danquirk commented 9 years ago

Gonna leave this as a suggestion for the moment and see how type guards play out for a bit to find the right balance of soundness vs practicality.

RyanCavanaugh commented 9 years ago

General patterns we see here in the wild

Overload resolution - given x, see if it's instanceof one overload, else assume we were invoked on the other overload
Runtime validation - throw if x is not instanceof the expected argument type
Inverted polymorphism - switch on x instanceof foo / bar / baz and adopt different behavior that is not dependent on the actual differing shapes of foo/bar/baz

Error coding:

!: Fails to do the right thing when presented with a "structural match". For example:

    export function show(element: any) {
        if (element instanceof Array) {
            for (var i = 0; i < element.length; i++) {
                element[i].style.display = 'block';
            }
        } else {
            element.style.display = 'block';
        }
    }

X: Throws an explicit exception because it treatsinstanceof clauses as definitive. Example:

    public disposeOf(obj: any): void {
        if (obj instanceof Array) {
            obj.forEach(this.disposeOf);
        } else {
            if ('dispose' in obj && typeof(<DisposableView> obj.dispose) == 'function') {
                obj.dispose();
            } else {
                throw 'Cannot dispose of this object';
            }
        }
    }

+: Code that, by inspection, is necessarily dealing with things that pass instanceof checks (for example, document.getElementById('foo') instanceof HTMLDivElement

:rocket: : Code that does an instanceof check, then does something plausibly correct in the else clause. There are no instances of this.

Overload Resolution

Runtime Validation

RyanCavanaugh commented 9 years ago

Tagging this as something that needs discussion. Based on available evidence, no one writes an instanceof expression and treats it as a non-exhaustive check for that type. We shouldn't interfere with that assumption for the sake of the 0% of programmers who are doing this "correctly".

RyanCavanaugh commented 9 years ago

Approved. We should remove union type constituents that are assignable to the return type of the instanceof operand's construct signatures.

tinganho commented 9 years ago

I will try to tackle this issue.

mhegazy commented 9 years ago

We need a plurp for this in https://github.com/Microsoft/TypeScript-wiki/blob/master/Breaking-Changes.md

mhegazy commented 9 years ago

thanks @tinganho!

microsoft / TypeScript

Type guard with instanceof fails to narrow type in `else` block #1719

Overload Resolution

Runtime Validation

Inverted Polymorphism

Reflection

Other