Skip to content

Make type narrowing for destructured discriminated unions work for more types #59657

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

Open
6 tasks done
nekolab opened this issue Aug 16, 2024 · 1 comment Β· May be fixed by #59745
Open
6 tasks done

Make type narrowing for destructured discriminated unions work for more types #59657

nekolab opened this issue Aug 16, 2024 · 1 comment Β· May be fixed by #59745
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Milestone

Comments

@nekolab
Copy link

nekolab commented Aug 16, 2024

πŸ” Search Terms

discriminant union, ref, control flow guard, type narrowing

βœ… Viability Checklist

⭐ Suggestion

type Ref<T> = { value: T }
type Data =
    | { ready: Ref<true>, payload: string }
    | { ready: Ref<false>, payload: null }

declare const data: Data
const { ready, payload } = data

if (ready.value) {
    payload    // <== currently inferred as "string | null" but should be "string"
}

Treat types like Ref<T> as a discriminant property in a union or find a way to narrow the type of payload

πŸ“ƒ Motivating Example

This is a very common use case in the Vue Pinia state store library, millions of projects use this library and have code like

const store = useDataStore()
const { ready, payload } = storeToRefs(store)

If we can improve this type narrowing behavior, the narrowed payload type can helps developer write safer code than before

// before, Non-null assertion everywhere
if (ready.value) {
    payload.xxxx()     // <=== false alert raised by typescript and developers have to use ?. or ! to avoid it
    payload?.xxxx()    // <=== ?. is unnecessary, generates dead code and brings cognitive confusion
    xxxxx(payload!)
}
xxxxx(payload!)        // <=== copied from the if block and forget to remove the ! mark, cannot receive alert from typescript

// after, everything works fine
if (ready.value) {
    payload.xxxx()
    xxxxx(payload)
}
xxxxx(payload)         // received the null check protection from typescript

πŸ’» Use Cases

More detailed playground link

The use cases is actually shown in the motivating example.

I've dig into the checker.ts for some time and here's my findings

  1. getDiscriminantPropertyAccess cannot treat ready as a discriminant property now because it needs to check CheckFlags.Discriminant which implies CheckFlags.HasLiteralType. It's a pretty strict check and as its name describes, Ref<T> has no chance to pass this check.
  2. I'm not sure is it possible for relaxing the discriminate requirements but it seems to be a bad idea after some search. Fix discriminant property checkΒ #29110 is what I found but it's a really old PR so maybe time changes now
  3. If we cannot solve it by using discriminant property narrowing, as a newbie to the typescript project, I just tried to debug the checker and have another idea
interface Ref<T> { value: T }
type ToRefs<T> = { [K in keyof T]: Ref<T[K]> }
function toRefs<T>(o: T): ToRefs<T> {
    return {} as any
}

interface DataPrepared {
    ready: true
    payload: string
}

interface DataNotPrepared {
    ready: false
    payload: null
}

type Data = DataPrepared | DataNotPrepared

declare const data: Data
const { ready, payload } = toRefs(data)

function isDataReady(d: Data): d is DataPrepared {
  return d.ready.value
}

if (isDataReady(data)) {
  ready.value     // <=== inferred as boolean but should be true
  payload.value   // <=== inferred as "string | null" but should be string
}

function assertDataReady(d: Data): asserts d is DataPrepared {}

if (ready.value) {
  assertDataReady(data)
  ready.value     // <=== inferred as true which is expected but it's narrowed by other code path
  payload.value   // <=== inferred as "string | null" but should be string
}

Can we use type predicates or assert function to add more information to payload's flow list? If it's possible, maybe we can do following steps while examine the payload

  1. check payload's symbol, if its declaration is a BindingPattern
  2. check the flow list for payload, if the narrowed data is the initializer of payload's declaration
  3. narrow the payload based on the narrowed data
  4. maybe it's gibberish but hope it helps
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases labels Aug 16, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Aug 16, 2024
@nekolab
Copy link
Author

nekolab commented Aug 17, 2024

πŸ”ˆ I'm working on the "alternative solution" which aims to use type predicts or assert function to narrow the type of the destructured variables.

My findings:

declare const data: Data
const { ready, payload } = toRefs(data)

if (isDataPrepared(data): data is DataPrepared) {
    ready.value
    // ^ narrow |ready| based on asserted |data| because it shares same symbol with argument |data| in |toRefs|
}

Now I can check the initializer is toRefs(data) while checking the identifier ready. In ready's flow list, it's not difficult to infer the argument data in isDataPrepared(data) is DataPrepared.

My idea is to add more logic to narrowTypeByCallExpression, while the reference is a call expression and has an argument overlap with the callExpression, we can re-calculate the return type of the reference.

So my current question is: Is it possible to narrow a CallExpression toRefs(data) with the given argument type data? I've checked the checkCallExpression, and the argument type narrowing seems only to happen with the identifier's flow, it seems not possible to specify a flow to checkCallExpression and tell it to use the flow to narrow the type.

Wish I can hear some advice here, thanks

Update: Found this doc https://github.com/microsoft/TypeScript/wiki/Reference-Checker-Inference#type-parameter-inference

nekolab added a commit to nekolab/TypeScript that referenced this issue Aug 25, 2024
This PR introduces a method to narrow the types of destructured
variables through control flow analysis without relying on the
discriminant member.

When the initializer of the `ObjectBindingPattern` is available,
we attempt to narrow its type, which in turn helps further refine
the type of the destructured variable.

Partially fixes microsoft#59657
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants