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

if/typeof guards on union type's uncommon properties #21944

Closed
massimonewsuk opened this issue Feb 14, 2018 · 5 comments
Closed

if/typeof guards on union type's uncommon properties #21944

massimonewsuk opened this issue Feb 14, 2018 · 5 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@massimonewsuk
Copy link

massimonewsuk commented Feb 14, 2018

TypeScript Version: 2.7.0

Search Terms:
typeof union narrowing properties

Code

interface Bird {
    fly: () => any;
    layEggs: () => any;
}

interface Fish {
    swim: () => any;
    layEggs: () => any;
}

function Move(pet: Fish | Bird) {
    if (pet.swim) {
        pet.swim();
    } else {
        pet.fly();
    }
    
    if (typeof pet.swim === "function") {
        pet.swim();
    } else {
        pet.fly();
    }

    if ("swim" in pet) {
        pet.swim();
    } else {
        pet.fly();
    }
}

Expected behavior:
I expect the code above to work without any errors at all. I feel it's all reasonable.

Actual behavior:
I get an error for the first 2 if/else branches, where I am accessing the property swim. The last branch works fine. I'm aware of the possibility of having a user-defined type guard function, but I don't want to have to do that.

Playground Link:
https://www.typescriptlang.org/play/index.html#src=interface%20Bird%20%7B%0D%0A%20%20%20%20fly%3A%20()%20%3D%3E%20any%3B%0D%0A%20%20%20%20layEggs%3A%20()%20%3D%3E%20any%3B%0D%0A%7D%0D%0A%0D%0Ainterface%20Fish%20%7B%0D%0A%20%20%20%20swim%3A%20()%20%3D%3E%20any%3B%0D%0A%20%20%20%20layEggs%3A%20()%20%3D%3E%20any%3B%0D%0A%7D%0D%0A%0D%0Afunction%20Move(pet%3A%20Fish%20%7C%20Bird)%20%7B%0D%0A%20%20%20%20if%20(pet.swim)%20%7B%0D%0A%20%20%20%20%20%20%20%20pet.swim()%3B%0D%0A%20%20%20%20%7D%20else%20%7B%0D%0A%20%20%20%20%20%20%20%20pet.fly()%3B%0D%0A%20%20%20%20%7D%0D%0A%20%20%20%20%0D%0A%20%20%20%20if%20(typeof%20pet.swim%20%3D%3D%3D%20%22function%22)%20%7B%0D%0A%20%20%20%20%20%20%20%20pet.swim()%3B%0D%0A%20%20%20%20%7D%20else%20%7B%0D%0A%20%20%20%20%20%20%20%20pet.fly()%3B%0D%0A%20%20%20%20%7D%0D%0A%0D%0A%20%20%20%20if%20(%22swim%22%20in%20pet)%20%7B%0D%0A%20%20%20%20%20%20%20%20pet.swim()%3B%0D%0A%20%20%20%20%7D%20else%20%7B%0D%0A%20%20%20%20%20%20%20%20pet.fly()%3B%0D%0A%20%20%20%20%7D%0D%0A%7D%0D%0A

Related Issues:
Couldn't find any

@massimonewsuk massimonewsuk changed the title typeof on union property's type if/typeof guards on union type's uncommon properties Feb 14, 2018
@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Feb 14, 2018
@RyanCavanaugh
Copy link
Member

The danger here is that you write

interface Bird {
    fly: () => any;
    layEggs: () => any;
}

interface Fish {
    swim: () => any;
    layEggs: () => any;
    gills: number;
}

const puffin = {
    fly: () => undefined,
    swim: () => undefined,
    layEggs: () => undefined
}

Move(puffin);
function Move(pet: Fish | Bird) {
    if (pet.swim) {
        pet.gills; // undefined, danger
    }
}

We don't want to make property access appear safe unless it actually is; property access in an if conditional isn't distinguished from access elsewhere.

The in operator gets its own treatment; this operator doesn't restrict either of its operands because by nature it's a dynamic test. It acts as a type guard in a sort of "use at your own risk" kind of way. The baseline assumption here is that in is the user-defined type guard you would have written anyway so we may as well let it through (even though it's subject to the same error as above).

@massimonewsuk
Copy link
Author

I understand that this is working as intended, as per the current TypeScript documentation. I actually prefer this approach to anything else that's currently available because "property" in object is always written as a type guard. However, if statements are used for all sorts. I do think typeof is mostly used for type guarding too though, so maybe that could be treated like in...

Anyway, playing devil's advocate or purist or whatever you like, I'll continue...

I logged an issue with Flow facebook/flow#5816 featuring the same code example, and I feel like their approach (whilst also not intuitive) is more "safe".

In my mind, the following 3 alternative approaches sound relatively consistent:

  1. Disallow any type narrowing (to either Fish or Bird) without an explicit user-defined type guard (I don't think "property" in object counts as that)

  2. Allow some kind of "type expanding" (if that's even a term)... so you can test for specific properties that we know might be available (but obviously not properties that aren't available in Fish & Bird). This would prevent you from authoring the code you posted, because we only know "swim" exists.

    • We would obviously allow the typeof pet.swim === "function" approach
    • We might also allow the if (pet.swim) approach
      The problem with this approach is how far do we go with the "type expanding"? If we want to stay as true as possible, then we shouldn't trust the parameters/return type of Fish.swim (or even the fact it's a function, if we only tested using if (fish.swim))... We might also end up changing the behaviour of the "property" in object to be consistent.
  3. Treat if and typeof as full-on type-narrowing operations which are allowed to access properties that only exist in Fish & Bird, meaning the whole of my original example would work without errors. It would be most consistent with the existing behaviour of "property" in object. However, it does have the downside that the example you posted of the puffin would fail at runtime. Maybe we could have some sort of linting option that says hey you checked for "swim" but you're accessing "gills". Do you want to check for "gills" instead?.

@RyanCavanaugh
Copy link
Member

Disallow any type narrowing (to either Fish or Bird) without an explicit user-defined type guard (I don't think "property" in object counts as that)

This was the behavior prior to #10485. The change was well-received; I don't think this option is a "local maximum".

2 ...

This is #21732

Treat if and typeof as full-on type-narrowing operations which are allowed to access properties that only exist in Fish & Bird ... some sort of linting option

Clippy-style "linting" isn't really a thing we do because there's not an ergonomic way to dismiss it and say "No I did mean to do what I did there".

@denis-sokolov
Copy link

Idiomatic JavaScript is, arguably, duck typing: if (obj.foo).

if ('foo' in obj) is not good enough to allow convenient undefined values (also see #13195). Thus the resulting code is mildly annoyingly too long: if ('foo' in obj && obj.foo). Here the latter condition is a superset of the former, and thus the former looks unneeded.

I’d love for the property check to receive equivalent treatment to the in operator. To paraphrase yourself, the baseline assumption here is that if (obj.foo) is the user-defined type guard I write anyway so we may as well let it through (even though it's subject to this error).

@typescript-bot
Copy link
Collaborator

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants