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

Control flow analysis for dependent function parameters "destructured" from a tuple type #46658

Closed
5 tasks done
Andarist opened this issue Nov 3, 2021 · 6 comments · Fixed by #47190
Closed
5 tasks done
Assignees
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@Andarist
Copy link
Contributor

Andarist commented Nov 3, 2021

Suggestion

🔍 Search Terms

List of keywords you searched for before creating this issue. Write them down here so that others can find this suggestion more easily and help provide feedback.

dependent arguments, parameters

This issue is very related to mine here, but I don't believe it's the same. That issue propose some bigger changes to TS - while I'd only like to "reuse" the, just landed, logic for the situation described below.

✅ Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

It's today possible to mimic dependent function parameters with tuples, spreads & co. Using such function parameters is clunky though because one can't destructure the arguments tuple without losing nice control flow analysis.

Since control flow analysis for destructured properties from unions has just landed on the main branch ( #46266 ), I think it would be great to extend the logic to cover this situation.

I've looked briefly at how this works for the newly introduced control flow analysis and I think the problem is that the information about the identifier coming from the arguments list is not propagated to getCandidateDiscriminantPropertyAccess. So it has no chance of handling this "correctly". If we could pass this info there and then check the signature type, if it's spread+tuple based then we could apply the same logic as for binding patterns.

📃 Motivating Example

I've added a test case for this here: Andarist@3e87dec

type Args = ['A', number] | ['B', string]

declare function fn10(cb: (...args: Args) => void): void

fn10((kind, data) => {
    if (kind === 'A') {
        data.toFixed();
    }
    if (kind === 'B') {
        data.toUpperCase();
    }
})

Currently, with this technique, one could do this to keep the control flow analysis working:

type Args = ['A', number] | ['B', string]

declare function fn10(cb: (...args: Args) => void): void

fn10((...args) => {
    if (args[0] === 'A') {
        args[1].toFixed();
    }
    if (args[0] === 'B') {
        args[1].toUpperCase();
    }
})

but this ain't super friendly to the user.

💻 Use Cases

In XState (a state machine library) we have actions (can be thought of as functions/callbacks) that can be declared like this:

assign((context, event) => {
  // this particular action can return a partial update to the context value
  return {}
})

Since XState is a state machine library some combinations of context & event types are impossible. With this feature in place, we could explore computing valid combinations and maybe (since the task ain't trivial) we could narrow down types of the context parameter based on different event.types. So for instance, there is a chance that this would become possible:

type Context = { userId: string | null }

assign((context, event) => {
  if (event.type === 'CHANGE_PASSWORD') {
    // `context` could be narrowed down to `{ userId: string }` if we could figure out how to create compute proper parameter tuples
  }
  return {}
})

cc @ahejlsberg

@MartinJohns
Copy link
Contributor

I think this is related: #35873

@Andarist
Copy link
Contributor Author

Andarist commented Nov 3, 2021

Yep, this looks related (but imho it's not the same) and i've already mentioned that in the original post.

@devanshj
Copy link

devanshj commented Nov 9, 2021

I guess someone should label this? (Reminding in case it got overlooked while triaging issues)

@andrewbranch
Copy link
Member

It hasn’t been overlooked, I’m just drowning in issues 😞

@devanshj
Copy link

devanshj commented Nov 9, 2021

Oh apologies! Take your time, and thanks for the work you do :)

@andrewbranch
Copy link
Member

andrewbranch commented Nov 9, 2021

It sounds like this could be used to solve Node-style callbacks, where we usually just type the err param as maybe-null and the payload param as always defined for convenience, even though that’s wrong:

fs.readFile("foo.txt", (err, data) => {
  if (err) {
    // `data` should be null here
    throw err;
  }
  // `data` should be a Buffer here
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants