Skip to content

Union not properly narrowed in false side of type conditional #52172

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

Closed
wbt opened this issue Jan 9, 2023 · 11 comments
Closed

Union not properly narrowed in false side of type conditional #52172

wbt opened this issue Jan 9, 2023 · 11 comments
Labels
Duplicate An existing issue was already created

Comments

@wbt
Copy link

wbt commented Jan 9, 2023

Bug Report

🔎 Search Terms

Union narrowing conditional false

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about common 'bugs' that aren't bugs
  • Nightly version as of test time: v5.0.0-dev.20230109

⏯ Playground Link

Playground link with relevant code

💻 Code

(Note: naming conventions may be a bit inconsistent in this toy example with a smaller, more general-knowledge, domain)

//Types widely used throughout application; expansion would be incorrect
type G7Capital = {
    CA: 'Ottawa'; FR: 'Paris'; DE: 'Berlin'; IT: 'Rome';
    JP: 'Tokyo'; UK: 'London'; US: 'Washington DC'; EU: 'Brussels';
}
type G7Abbreviation = keyof G7Capital;
//Type used only in a few functions for handling a legacy edge case:
type G8Capital<A extends G7Abbreviation | 'RU'> = (
    //Error ts(2536): Type 'A' cannot be used to index type 'G7Capital'.
    //but on the side of the conditional where A is found, 'RU' *should be*
    //narrowed out (it is not: that's the bug reported here)
    //and that would make A === G7Abbreviation === keyof G7Capital
    //which can be used to index 'G7Capital' so there should be no error.
    A extends 'RU' ? 'Moscow' : G7Capital[A]
    //A workaround is flipping the conditional order,
    //but it's not intiutive to try this, expected to behave the same:
    //A extends G7Abbreviation ? G7Capital[A] : 'Moscow'
);

🙁 Actual behavior

In its last uncommented instance, A is still of type G7Abbreviation | 'RU' instead of just G7Abbreviation producing error ts(2536): Type 'A' cannot be used to index type 'G7Capital'.

🙂 Expected behavior

On the side of the conditional where A is found, 'RU' should be narrowed out (it is not: that's the bug reported here) and that would make A === G7Abbreviation === keyof G7Capital which can be used to index 'G7Capital' so there should be no error.
Also, flipping the conditional sequence as seen in the workaround should not make a substantive difference in any conditional expression where either side of the conditional can be described with relative ease.

Deduplication discussion

Not a duplicate of #44401 as it is not fixed by #44771, nor of #44382 as it did not change between versions 4.2.3 and 4.3.2. This is also not a duplicate of issues around control flow analysis, especially with the optional chaining or ternary operators in control-flow analysis of JavaScript.

@RyanCavanaugh
Copy link
Member

We'd need negated types to be able to handle this properly, since the correct type to give A in the false branch is still a type parameter. Changing it to a concrete G7Abbreviation would break (many) other things.

Without loss of generality you can write G7Capital[A & G7Abbreviation]

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jan 9, 2023
@wbt
Copy link
Author

wbt commented Jan 10, 2023

When an incoming type param is of the form A extends B | C (for notation convenience below, type D = B | C) and you have an extension of form A extends B ? T : F I would expect that occurrences of A in expression T are treated as B and occurrences of A in expression F are treated as Exclude<D, B>. I don't think negated types are needed for that.

To be more explicit, in this particular case I would also then expect that Exclude<D, B> would be equivalent to Exclude<B | C, B> which would further be equivalent to C.

@RyanCavanaugh
Copy link
Member

Understood that that was your expectation. That is not what happens.

@wbt
Copy link
Author

wbt commented Jan 10, 2023

I understand that is not what happens; the difference between what I think is a reasonable expectation other developers would likely share (if it's not, some description of why it's not a reasonable expectation would be appreciated!) and the reality of what happens is reported here as a bug.

@fatcerberus
Copy link

fatcerberus commented Jan 11, 2023

I'm not convinced this needs negated types, at least not for the simple union case; it feels like it might be enough to simply re-constrain the type parameter in the false branch of the conditional, e.g. from A extends G7Abbreviation | 'RU' to A extends G7Abbreviation. As this already works:

type G8Capital<A extends G7Abbreviation | 'RU'> =
    A extends 'RU' ? 'Moscow' : G7Capital[Exclude<A, 'RU'>];

so if the compiler could automatically apply that Exclude to the constraint of A it would solve this, I think.

@wbt
Copy link
Author

wbt commented Jan 11, 2023

I should also clarify that while in this demonstration example, the generic type parameter constraint (D) is a union of a union and a constant, some of the motivating examples in actual code have two unions.
Keeping the same example domain, here is a slightly less minimal example which demonstrates this:

type G7Capital = {
    CA: 'Ottawa'; FR: 'Paris'; DE: 'Berlin'; IT: 'Rome';
    JP: 'Tokyo'; UK: 'London'; US: 'Washington DC'; EU: 'Brussels';
}
type NonG7Capital = {
    AU: 'Canberra'; CN: 'Beijing'; IN: 'New Delhi'; RU: 'Moscow'; //...
}
type G7Abbreviation = keyof G7Capital;
type NonG7Abbreviation = keyof NonG7Capital;
type CountryAbbreviation = G7Abbreviation | NonG7Abbreviation;
type CountryCapital<A extends CountryAbbreviation> = (
    //Error ts(2536): Type 'A' cannot be used to index type 'NonG7Capital'.
    //but on the side of the conditional where A is found, 
    //it *should be* narrowed to Exclude<CountryAbbreviation, G7Abbreviation>
    //(it is not: that's the bug reported here)
    //and that would make A === NonG7Abbreviation === keyof NonG7Capital
    //which can be used to index 'NonG7Capital' so there should be no error.
    A extends G7Abbreviation ? G7Capital[A] : NonG7Capital[A]
    //Here, the uninutitive workaround of flipping the conditional order
    //doesn't work because both are unions:
    //A extends NonG7Abbreviation ? NonG7Capital[A] : G7Capital[A]
)

Fixing the issue reported above in the way @fatcerberus describes would also fix this case.
I don't think this requires having negated types and I don't think this Issue should be closed as a duplicate of nothing.

@wbt
Copy link
Author

wbt commented Jan 11, 2023

As another note, introducing negated types wouldn't automatically fix this issue either - someone would still have to go in and change the definition for how the type parameter is constrained in the false branch, just as they would to add Exclude.

@RyanCavanaugh
Copy link
Member

simply re-constrain the type parameter in the false branch of the conditional, e.g. from A extends G7Abbreviation | 'RU' to A extends G7Abbreviation. As this already works:

Sure. "re-constraining" isn't really a thing, though. That'd be a separate can of worms; it might be good to collate some use cases for that behavior if it'd be useful elsewhere.

@wbt
Copy link
Author

wbt commented Jan 13, 2023

Maybe there's another term instead of "re-constraining" that is applied to further constrain A in the true side of the conditional (to the intersection of its original limits and the type it extends for that conditional), and a similar process could be applied on the false side.

@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@wbt
Copy link
Author

wbt commented Jan 16, 2023

I don't think this should be CLOSED WONTFIX (as a duplicate of nothing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants