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

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

Closed
NoelAbrahams opened this issue Jan 18, 2015 · 9 comments
Closed

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

NoelAbrahams opened this issue Jan 18, 2015 · 9 comments
Labels
Breaking Change Would introduce errors in existing code Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@NoelAbrahams
Copy link

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
Copy link
Member

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.

@ahejlsberg ahejlsberg added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Jan 19, 2015
@NoelAbrahams
Copy link
Author

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 danquirk added Suggestion An idea for TypeScript and removed By Design Deprecated - use "Working as Intended" or "Design Limitation" instead labels Feb 6, 2015
@danquirk
Copy link
Member

danquirk commented Feb 6, 2015

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
Copy link
Member

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

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

Overload Resolution

! https://github.com/basiliskjs/basilisk/blob/master/src/basilisk.ts#L1274
! https://github.com/ChaosinaCan/OperaExtOptions2/blob/master/js/lib/options-page.ts#L431
! https://github.com/egret-labs/egret-core/blob/master/src/egret/net/URLVariables.ts#L107
! https://github.com/galadhremmin/RTJS/blob/master/RTJS/UI/Widgets/SelectWidget.ts#L36
X https://github.com/intermine/intermine-apps-a/blob/master/choose-list/app/views/disposable.ts#L19
X https://github.com/jedmao/linez/blob/master/lib/StringFinder.ts#L9
! https://github.com/jodymgustafson/TSprite/blob/master/TSprite/CanvasPanel.ts#L52
! https://github.com/justindujardin/pow-core/blob/master/source/rect.ts#L132
! https://github.com/jvlppm/pucpr-webgl-terrain/blob/master/Jv.Games.WebGL/Scene.ts#L39
X https://github.com/kazuki/video-codec.js/blob/master/openh264_worker.ts#L46
X https://github.com/Khan/khan-windows/blob/master/KhanAcademy/js/utilities.ts#L59
X https://github.com/kidomah/tscheme/blob/master/src/s_expression.ts#L36
X https://github.com/kinoh/TypeMath/blob/master/TypeMath/app.ts#L44
X https://github.com/madmaw/LD29/blob/master/src/main/ts/GB/Transformer/XSL/StandardXSLTransformer.ts#L13
X https://github.com/TobiaszCudnik/mars-robots/blob/master/src/position.ts#L10
X https://github.com/mistaecko/skylark-framework/blob/master/skylark/src/skylark/display/DisplayObjectContainer.ts#L58
X https://github.com/mordra/CoTWjs/blob/master/js/engines/GraphicsEngine.ts#L371
X https://github.com/robertknight/passcards/blob/master/lib/base/streamutil.ts#L14
X https://github.com/sajjad-shirazy/Khesht/blob/master/khesht/utils.ts#L137
X https://github.com/solver/foundation-js/blob/master/solver.lab/ObjectUtils.ts#L123
! https://github.com/tae-jun/slms/blob/master/client/src/global/directives/ngContextMenu.ts#L60

Runtime Validation

+ https://github.com/aldore/typescript-documentation/blob/master/samples/win8/encyclopedia/Encyclopedia/js/topic.ts#L141
! https://github.com/dannymarsland/romeo/blob/master/assets/typescript/Application.ts#L44
! https://github.com/michal-minich/Efekt/blob/master/Sources/parser.ts#L311
X https://github.com/turbulenz/turbulenz_engine/blob/master/tslib/capturegraphicsdevice.ts#L1432
! https://github.com/vvakame/prh/blob/master/lib/utils/regexp.ts#L23
X https://github.com/winjs/winjs/blob/master/tests/Repeater/RepeaterEditingTests.ts#L94
+ https://github.com/wsick/nullstone/blob/master/src/errors/AggregateError.ts#L14
! https://github.com/xlexi/alientube/blob/master/TypeScript/CommentField.ts#L24
! https://github.com/xperiments/Pulsar/blob/master/src/pulsar/textures/DynamicTextureAtlas.ts#L101

Inverted Polymorphism

X https://github.com/ACReeser/ecopico/blob/master/ecopico.ts#L380
X https://github.com/chromolens/chromolens/blob/master/src/gff3.ts#L1161
X https://github.com/DragonBones/DragonBonesJS/blob/master/src/core/armature/Slot.ts#L214
X https://github.com/wcandillon/jsoniq/blob/master/lib/updates/composition/DeleteFromObjectComposition.ts#L19

Reflection

X https://github.com/horiuchi/dtsgenerator/blob/master/lib/generator.ts#L26
+ https://github.com/jodymgustafson/TSprite/blob/master/UnitTests/TSTest.ts#L330
+ https://github.com/xcap2000/TypeScriptFramework/blob/master/TypeScriptFramework/Tests/TestRunner.ts#L73

Other

X https://github.com/spatools/koui/blob/master/src/contextmenu.ts#L204
! https://github.com/ukyo/niconico-audio-extractor/blob/master/dist/js/vendor/mp4.js/src/parser.descr.ts#L89

@RyanCavanaugh
Copy link
Member

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
Copy link
Member

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

@tinganho
Copy link
Contributor

tinganho commented Oct 3, 2015

I will try to tackle this issue.

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Nov 7, 2015
@mhegazy mhegazy modified the milestones: TypeScript 1.8, Community Nov 7, 2015
@mhegazy mhegazy added the Breaking Change Would introduce errors in existing code label Nov 7, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Nov 7, 2015

@mhegazy
Copy link
Contributor

mhegazy commented Nov 7, 2015

thanks @tinganho!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants