Control flow analysis for destructured discriminated unions #46266
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at dd07cdc. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at dd07cdc. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at dd07cdc. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at dd07cdc. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at dd07cdc. You can monitor the build here. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@DanielRosenwasser Here they are:Comparison Report - main..46266
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot
added
For Milestone Bug
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 76fbe25. You can monitor the build here. Update: The results are in! |
JoostK
reviewed
Oct 8, 2021
| const declaration = symbol.valueDeclaration; | ||
| if (declaration && isBindingElement(declaration) && !declaration.initializer && !declaration.dotDotDotToken && declaration.parent.elements.length >= 2) { | ||
| const parent = declaration.parent.parent; | ||
| if (parent.kind === SyntaxKind.VariableDeclaration && getCombinedNodeFlags(declaration) && NodeFlags.Const || parent.kind === SyntaxKind.Parameter) { |
There was a problem hiding this comment.
Small typo here:
Suggested change
| if (parent.kind === SyntaxKind.VariableDeclaration && getCombinedNodeFlags(declaration) && NodeFlags.Const || parent.kind === SyntaxKind.Parameter) { | |
| if (parent.kind === SyntaxKind.VariableDeclaration && getCombinedNodeFlags(declaration) & NodeFlags.Const || parent.kind === SyntaxKind.Parameter) { | |
|
@typescript-bot perf test this faster |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at cf546e8. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at cf546e8. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - main..46266
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here they are:Comparison Report - main..46266
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
5 tasks
|
This is super exciting, thanks for working on this @ahejlsberg! I just ran into this today when working with |
mprobst
pushed a commit
to mprobst/TypeScript
that referenced
this pull request
Jan 10, 2022
…t#46266) * CFA for dependent variables destructured from discriminated union * Accept new baselines * Add tests * Limit calls to isSymbolAssigned * Fix wrong operator
|
@DanielRosenwasser maybe it's worth adding this feature to the 4.6 beta blog post: The blog post does not mention object restructuring, only tuple/union destructuring, but I checked the 4.6 nightly playground and it works with objects too now, which is highly anticipated by React users (more than the example showcased in the blog post IMHO) Edit: oups actually I'm wrong it's not yet 100% good enough for React, and the following still fails: import React from "react";
type ItemData =
| { kind: 'user', name: string }
| { kind: 'company', id: string }
function Item({ kind, ...data }: ItemData) {
if (kind === 'user') {
<div>{data.id}</div>
}
if (kind === 'company') {
<div>{data.name}</div>
}
return null;
}But the following now works: type ItemData2 =
| { kind: 'user', data: {name: string} }
| { kind: 'company', data: {id: string} }
function Item2({ kind, data }: ItemData2) {
if (kind === 'user') {
<div>{data.name}</div>
}
if (kind === 'company') {
<div>{data.id}</div>
}
return null;
}Still worth highlighting that in the post :) |
|
Seems like the difference there is the spread, right? Okay, that's a good call, we must have missed that. I will likely add that to the RC post. |
|
Awesome! Thank you so much❤️ |
|
Just a question of understanding: Is the support for the spread syntax planned for a bugfix release or for a minor version? Or is it currently not being planned to be changed / given priority? |
This was referenced Aug 6, 2022
Open
|
I've noticed that this feature doesn't seem to play nice with default values: Is this also covered by #46680 or is there another reason it doesn't work? |
|
@50an6xy06r6n that feels like a bug |
|
It's a design limitation, but one we could consider removing. We only do CFA for dependent destructured parameters when they're declared without initializers because it adds complexity to consider the contribution the initializer might make to the inferred declared type of the parameter. |
|
@ahejlsberg intuitively it feels like it should work. In my case (react), the way props work means you have to pass parameters as an object, though I realized that there's other ways to set the default value. |
|
This also does not work when destructuring a nested discriminated union type. See playground link here. I assume this is because of this line in the PR description:
Since the object is nested, the parent type being destructured is not the discriminated union. Still, it's a shame this does not work. |
|
I am currently running into issues with this (or a related) analysis. I have a union |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements control flow analysis for dependent parameters and variables declared by destructuring discriminated unions. Specifically, when non-rest binding elements are declared as
constvariables or const-like parameters (parameters for which there are no assignments in the function body) and the parent type for the destructuring is a discriminated union type, conditional checks for variables destructured from discriminant properties now affect the types of other variables declared in the same destructuring.Some examples:
Fixes #10830.
Fixes #35283.
Fixes #38020.
Fixes #46143.