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

In switch case, narrow base constraint of type #21483

Closed
wants to merge 2 commits into from

Conversation

sandersn
Copy link
Member

Fixes #20375

Previously, the type itself was narrowed, which was not useful for T extends 'a' | 'b' | 'c' and other type parameters that extend literal unions. Note that the previous behaviour was technically correct: at the function declaration, you can't prove that all branches of the case will be used because the type parameter may be instantiated with a smaller union (like T='a' | 'b', in the example above). But the unused cases don't hurt anything, and people expect narrowing to work here.

For example:

declare var n: never;
function f<T extends 'a' | 'b' | 'c'>(t: T): void {
    switch (t) {
        case 'a': n = t; break; // t should be 'a'
        case 'b': n = t; break; // should be 'b'
        case 'c': n = t; break; // should be 'c'
        default: n = t; break;  // should be never
    }
}

Narrowing is technically incorrect, by analogy with the non-type-parameter example:

declare var n: never;
function f(t: 'a' | 'b'): void {
    switch (t) {
        case 'a': n = t; break;
        case 'b': n = t; break;
        case 'c': n = t; break; // 'c' is unused; 'c' *might* be unused in the type parameter case
        default: n = t; break;
    }
}

Previously, the type itself was narrowed, which was not useful for `T
extends 'a' | 'b' | 'c'` and other type parameters that extend literal
unions. Note that the previous behaviour was technically correct: at the
function declaration, you can't prove that all branches of the case will
be used because the type parameter may be instantiated with a smaller
union (like T='a' | 'b', in the example above). But the unused cases
don't hurt anything, and people expect narrowing to work here.
@sandersn
Copy link
Member Author

@ahejlsberg points out that the type of t will be 'a' | 'b' | 'c' after the switch statement, when it should be T extends 'a' | 'b' | 'c'.

@jack-williams
Copy link
Collaborator

jack-williams commented Feb 21, 2018

@sandersn Sorry for dropping in out of nowhere. I had been working on typeof narrowing in switch (#21957) and it raised some questions relevant to this. (These questions are just for me to get a better understanding of the type system, I'm not trying to get involved with the review of this PR)

Is there a reason that the narrowed types are just the literals as opposed to an intersection type? So you would have:

declare var n: never;
function f<T extends 'a' | 'b' | 'c'>(t: T): void {
    switch (t) {
        case 'a': n = t; break; // t : T & 'a'
        case 'b': n = t; break; // t : T & 'b'
        case 'c': n = t; break; // t : T & 'c'
        default: n = t; break;  // t : never
    }
}

Also, am I correct in saying that if does not narrow literal constraints currently either?

Is there consideration into extending something like filterType to do the filter over the constraints of a type parameter in the same way? Currently I think it treats a constraint of union type as an atomic unit so the only options are either retaining the original type parameter, or filtering it out completely.

@sandersn
Copy link
Member Author

@jack-williams Narrowing, strictly speaking, only removes types from a union. The exception is instanceof checks, but those apply to classes with an inheritance relationship, not to unions of string literal types. That's the problem with this PR, in fact; it substitutes T with its constraint 'a' | 'b' | 'c' in order to make narrowing work. But then when t is reconstituted from the individual cases after the switch, it will have type 'a' | 'b' | 'c', losing the fact that it is actually T. Your solution has a similar problem, with the resulting post-switch type being T & 'a' | T & 'b' | T & 'c'.

You could solve this problem by introducing a "narrowed type parameter" type such that a switch on T extends 'a' | 'b' | 'c' narrows to T extends 'a'. But this is a lot of work because we also need rules to union these types: T extends 'a' | T extends 'b' | T extends 'c' === T extends 'a' | 'b' | 'c'. We don't have anything like those rules today. (Strictly speaking, you might not need a new kind of type, but the compiler would still need a good bit of knowledge about narrowed type parameters.)

(if narrows in the same way as switch, but with just two cases.)

@jack-williams
Copy link
Collaborator

@sandersn Thanks for the detailed explanation.

Is typeof and if another special case of this narrowing? For example:

function test<T extends number | string | boolean>(x: T) {
    if (typeof x === 'number') {
        x // has type T & number
    }
    else if (typeof x === 'string') {
        x // has type T & string
    } else if (typeof x === 'boolean') {
        x // has type T & boolean
    } else {
        x // has type T extends number | string | boolean, should probably be never
    }
}

I think that T extends A is not equivalent to T & A, but are there reasons why it's ok for an intersection here, but not in the case of the literal types in a switch? Is the problem of the return type being T & 'a' | T & 'b' | T & 'c' the primary issue with this approach?

You could solve this problem by introducing a "narrowed type parameter" type such that a switch on T extends 'a' | 'b' | 'c' narrows to T extends 'a'. But this is a lot of work because we also need rules to union these types: T extends 'a' | T extends 'b' | T extends 'c' === T extends 'a' | 'b' | 'c'.

Yes, from my short and rather uninformed look into this it doesn't seem like an easy fix. I'm wondering if it's possible to special case this just at the points of narrowing, rather than making the checker aware in general. So before narrowing a switch'd type, explode T extends A | B into T extends A | T extends B, and before returning the narrowed type fold any occurrences of T extends A | T extends B back into T extends A | B. I have a feeling that this has alot of dark corners and performance issues..

@sandersn
Copy link
Member Author

Oops. I forgot about the narrowing-as-assertion. Yes, for a type parameter, index type, or conditional type T, typeof will use intersection if the target type is assignable to the constraint of T (eg in test, number ==> number | string | boolean). However, narrowing-as-assertion doesn't alter or replace the declared type; it just adds to it. Also note (1) the post-if type is still T (2) this is an assertion, not a true narrowing, so the else case isn't never, it's the declared type.

For your PR, I think switch (typeof x) should use narrowing-as-assertion in the same way as if (typeof x === ...). As for fixing the narrowing of constrained type parameters, your approach would probably work, but it would be at least a medium-size change — a lot bigger than the one-line change in this PR.

@jack-williams
Copy link
Collaborator

However, narrowing-as-assertion doesn't alter or replace the declared type; it just adds to it. Also note (1) the post-if type is still T (2) this is an assertion, not a true narrowing, so the else case isn't never, it's the declared type.

This description makes things clearer, thanks. I think what I was doing was (sort-of) in this spirit. I was technically only adding to the type, but the thing I added was the constraint narrowed by the assertion, rather than the assertion directly (so T extends 3 | true) would narrow under typeof x === 'number'. In the else case I think the reason it doesn't narrow to never is because the assert facts (not-equal ones) don't apply to unions in constraints like they do directly to unions, as you say. My solution was apply the facts to the constraint (getting never) and then adding that to the type to get T & never.

For your PR, I think switch (typeof x) should use narrowing-as-assertion in the same way as if (typeof x === ...).

Agreed. I had this behaviour before and will bring it back. I'll change it then ping some of the team so they know when to look at it. There isn't any point reviewing it until I have the right behaviour.

As for fixing the narrowing of constrained type parameters, your approach would probably work, but it would be at least a medium-size change — a lot bigger than the one-line change in this PR.

Definitely! Again, I wasn't trying to suggest that the PR should be done in style xyz. I just wanted to pick your brains as it was relevant to switch + typeof.

@weswigham
Copy link
Member

weswigham commented Feb 21, 2018

@sandersn Rather than narrowing to the constraint (which removes the relationship with the type variable from the type), can we leverage substitution types and effectively narrow to Extract<TInput, TCase>? That way the constraint is narrowed but it remains a type-variable if the input was a type variable. I have an outstanding self-issue to change flow control for nonnull assertions to behave similarly.

@typescript-bot
Copy link
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@heyitsaamir
Copy link

@sandersn sorry to pop in, but was there ever a resolve to this? Found this issue for my current problem but doesn't look like this fix ever made it in

@sandersn
Copy link
Member Author

sandersn commented Mar 3, 2020

@heyitsaamir Not as far as I remember. My best memory of this PR is that it started off a bit experimental, ran into problems, and all we could think of was even more experimental approaches. So I think #20375 is quite a difficult problem that requires more than an easy fix.

@jakebailey jakebailey deleted the narrow-constraint-of-type-in-switch branch November 7, 2022 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

narrowing in switches doesnt work on constrained unions
5 participants