Skip to content

Fixed an issue with destructured bindings from a generic union constraint not being narrowed correctly - #50221

Merged
Anders Hejlsberg (ahejlsberg) merged 2 commits into
microsoft:mainfrom
Andarist:fix/generic-param-dependent-destructured-variables
Sep 17, 2022
Merged

Fixed an issue with destructured bindings from a generic union constraint not being narrowed correctly#50221
Anders Hejlsberg (ahejlsberg) merged 2 commits into
microsoft:mainfrom
Andarist:fix/generic-param-dependent-destructured-variables

Conversation

@Andarist

Copy link
Copy Markdown
Contributor

Comment thread src/compiler/checker.ts
@@ -25762,10 +25762,11 @@ namespace ts {
if (!(links.flags & NodeCheckFlags.InCheckIdentifier)) {
links.flags |= NodeCheckFlags.InCheckIdentifier;
const parentType = getTypeForBindingElementParent(parent, CheckMode.Normal);

@Andarist Mateusz Burzyński (Andarist) Aug 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are receiving T here from here:
https://github.dev/microsoft/TypeScript/blob/71e852922888337ef51a0e48416034a94a6c34d9/src/compiler/checker.ts#L9030-L9034

I've compared the code run for the second case - where those variables were destructured into a const declaration in the function's body and not directly in the parameter position and I've discovered that in that case, the returned type was Action, from here:
https://github.dev/microsoft/TypeScript/blob/71e852922888337ef51a0e48416034a94a6c34d9/src/compiler/checker.ts#L9088-L9089
That in turn could be traced back to getNarrowableTypeForReference here:
https://github.dev/microsoft/TypeScript/blob/71e852922888337ef51a0e48416034a94a6c34d9/src/compiler/checker.ts#L26001

So I've just replicated the logic from there (just slightly simplified):
https://github.dev/microsoft/TypeScript/blob/71e852922888337ef51a0e48416034a94a6c34d9/src/compiler/checker.ts#L25747

In here, we are always interested in narrowable types so it made sense to me to just unconditionally resolve through getBaseConstraintOrType

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me although Anders Hejlsberg (@ahejlsberg) should take a look too.

…iables

# Conflicts:
#	tests/baselines/reference/dependentDestructuredVariables.js
@Andarist

Copy link
Copy Markdown
Contributor Author

Anders Hejlsberg (@ahejlsberg) friendly 🏓

@ahejlsberg
Anders Hejlsberg (ahejlsberg) merged commit a8e13f7 into microsoft:main Sep 17, 2022
@Andarist
Mateusz Burzyński (Andarist) deleted the fix/generic-param-dependent-destructured-variables branch September 17, 2022 22:30
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Milestone Bug PRs that fix a bug with a specific milestone

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

CFA for destructured discriminated unions failed with generic constraints

4 participants