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 of union with conditional type not working since 4.3 #44382

Closed
pedro-pedrosa opened this issue Jun 2, 2021 · 5 comments
Closed

Type guard of union with conditional type not working since 4.3 #44382

pedro-pedrosa opened this issue Jun 2, 2021 · 5 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@pedro-pedrosa
Copy link

Bug Report

πŸ”Ž Search Terms

type guard conditional type

πŸ•— Version & Regression Information

  • This changed between versions 4.2.3 and 4.3.2

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

interface A<S> {
    a: S
}
interface B<S> {
    b: S
}
interface C<S> {
    c: S
}

type U1<S> = A<S> | B<S> | C<S>
function f1<S>(u: U1<S>): u is B<S> | C<S> {
    return false
}
function test1<S>(x: U1<S>) {
    if (!f1(x)) {
        x.a //OK
    }
}

type Cond<S> = S extends number ? B<S> : C<S>
type U2<S> = A<S> | Cond<S>
function f2<S>(u: U2<S>): u is Cond<S> {
    return false
}
function test2<S>(x: U2<S>) {
    if (!f2(x)) {
        x.a //ERROR
    }
}

πŸ™ Actual behavior

The type guard is unable to narrow the type of x to A

πŸ™‚ Expected behavior

The type guard should narrow the type of x to A because it is not assignable to Cond<S>

@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug Bug A bug in TypeScript and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Jun 10, 2021
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jun 10, 2021
@bbenezech
Copy link

@RyanCavanaugh Can this bug be prioritize for 4.3.4 please? πŸ™
It's a real regression over 4.2. Inferred types are not narrow-able anymore.

@RyanCavanaugh
Copy link
Member

This doesn't meet our bar for a patch release, but I'll move it to 4.4

@andrewbranch
Copy link
Member

andrewbranch commented Jul 23, 2021

This was caused by #43183, because the conditional type Cond<S> gets substituted for its constraint, B<S> | C<S>, in the type of x before it gets narrowed, so we are effectively trying to narrow from A<S> | B<S> | C<S>. Most of the time, that’s way more useful than trying to narrow with Cond<S>, because it means you can actually discriminate between the two branches of the conditional. Cond<S> just won’t relate with many useful types during narrowing. However, it will relate to itself, which is what your example is doing, but in 4.3 we can no longer tell that’s what’s happening since we substituted the constraint earlier on.

I don’t know if it would make sense for your real code @pedro-pedrosa, but a workaround would be to expand the conditional type to a union in your type predicate:

function f2<S>(u: A<S> | B<S> | C<S>): u is B<S> | C<S> {
    return false
}

The only way I can think to fix this without undoing the very useful improvements of #43183 would be to carry the original, non-substituted type through narrowing (in addition to the usually-more-narrowable constraint-substituted type), trying it if narrowing the constraint-substituted type had no effect. But that sounds like it could be expensive, for what feels like quite an edge case to me.

@pedro-pedrosa
Copy link
Author

I don’t know if it would make sense for your real code @pedro-pedrosa, but a workaround would be to expand the conditional type to a union in your type predicate:

Fortunately I was able to change it to u is A<S> because in my real example I wasn't exactly trying to figure out if my values where Cond<S>, I was trying to figure out if they were not Cond<S>. I didn't think of writing the guards this way because my real example is not just A | Cond, it has a few more types in that union other than A so it was just easier to do u is Cond<S> and negate it.

Thanks for the suggestion.

@andrewbranch andrewbranch added Needs Investigation This issue needs a team member to investigate its status. and removed Bug A bug in TypeScript labels Aug 20, 2021
@andrewbranch andrewbranch added the Rescheduled This issue was previously scheduled to an earlier milestone label Aug 20, 2021
@andrewbranch
Copy link
Member

Coming back to this, I think my earlier analysis was right, and given the existence of a reasonable workaround, I doubt we will want to take on the complexity and performance costs of doing something different here. This is very similar to #47337, which we discussed in a recent design meeting: getNarrowableTypeForReference did exactly what it’s supposed to do, but for reasons that are very hard to predict at the time it’s called, it would have turned out better if it returned the original type. @gabritto came up with a workable heuristic for #47337, but for this issue I don’t think there is any such heuristic, as evidenced by the existence of my workaround:

type Cond<S> = S extends number ? B<S> : C<S>
type U2<S> = A<S> | Cond<S>

+ function f2<S>(u: U2<S>): u is B<S> | C<S> {
- function f2<S>(u: U2<S>): u is Cond<S> {
    return false
}

function test2<S>(x: U2<S>) {
    if (!f2(x)) {
        x.a //ERROR
    }
}

Each of the two signatures highlighted in this diff is reasonable to write, but they work mutually exclusively based on whether getNarrowableTypeForReference returns A<S> | B<S> | C<S> or A<S> | Cond<S> for U2<S>. To β€œfix” the narrowing ability of one of these type guards by tweaking getNarrowableTypeForReference would be to break the other.

When we discussed #47337, @ahejlsberg noted that we’re doing with getNarrowableTypeForReference is only an approximation of what we should be doing in theory: rather than narrowing the type parameter itself, we ought to be narrowing its constraint. I think that theoretical solution would fix this issue too, but it was decided that was too complex. So, I think this can safely be called a design limitation. @ahejlsberg does that sound right to you, or did I miss something?

@andrewbranch andrewbranch removed this from the TypeScript 4.6.0 milestone Feb 3, 2022
@andrewbranch andrewbranch added Design Limitation Constraints of the existing architecture prevent this from being fixed and removed Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone labels Feb 3, 2022
@andrewbranch andrewbranch removed their assignment Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants