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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from Sep 17, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5184941
Fixed an issue with destructured bindings from a generic union constr…
Andarist Aug 8, 2022
de6c91f
Merge branch 'main' into fix/generic-param-dependent-destructured-var…
Andarist Aug 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Expand Up @@ -25762,10 +25762,11 @@ namespace ts {
if (!(links.flags & NodeCheckFlags.InCheckIdentifier)) {
links.flags |= NodeCheckFlags.InCheckIdentifier;
const parentType = getTypeForBindingElementParent(parent, CheckMode.Normal);
Copy link
Contributor Author

@Andarist Andarist Aug 8, 2022 •
edited

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

const parentTypeConstraint = parentType && mapType(parentType, getBaseConstraintOrType);
links.flags &= ~NodeCheckFlags.InCheckIdentifier;
if (parentType && parentType.flags & TypeFlags.Union && !(parent.kind === SyntaxKind.Parameter && isSymbolAssigned(symbol))) {
if (parentTypeConstraint && parentTypeConstraint.flags & TypeFlags.Union && !(parent.kind === SyntaxKind.Parameter && isSymbolAssigned(symbol))) {
const pattern = declaration.parent;
const narrowedType = getFlowTypeOfReference(pattern, parentType, parentType, /*flowContainer*/ undefined, location.flowNode);
const narrowedType = getFlowTypeOfReference(pattern, parentTypeConstraint, parentTypeConstraint, /*flowContainer*/ undefined, location.flowNode);
if (narrowedType.flags & TypeFlags.Never) {
return neverType;
}
Expand Down
24 changes: 22 additions & 2 deletions tests/baselines/reference/dependentDestructuredVariables.errors.txt
View file
Open in desktop
@@ -1,5 +1,5 @@
tests/cases/conformance/controlFlow/dependentDestructuredVariables.ts(314,5): error TS7022: 'value1' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.
tests/cases/conformance/controlFlow/dependentDestructuredVariables.ts(314,5): error TS7031: Binding element 'value1' implicitly has an 'any' type.
tests/cases/conformance/controlFlow/dependentDestructuredVariables.ts(334,5): error TS7022: 'value1' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.
tests/cases/conformance/controlFlow/dependentDestructuredVariables.ts(334,5): error TS7031: Binding element 'value1' implicitly has an 'any' type.


==== tests/cases/conformance/controlFlow/dependentDestructuredVariables.ts (2 errors) ====
Expand Down Expand Up @@ -39,6 +39,26 @@ tests/cases/conformance/controlFlow/dependentDestructuredVariables.ts(314,5): er
}
}

// repro #50206
function f13<T extends Action>({ kind, payload }: T) {
if (kind === 'A') {
payload.toFixed();
}
if (kind === 'B') {
payload.toUpperCase();
}
}

function f14<T extends Action>(t: T) {
const { kind, payload } = t;
if (kind === 'A') {
payload.toFixed();
}
if (kind === 'B') {
payload.toUpperCase();
}
}

type Action2 =
| { kind: 'A', payload: number | undefined }
| { kind: 'B', payload: string | undefined };
Expand Down
40 changes: 40 additions & 0 deletions tests/baselines/reference/dependentDestructuredVariables.js
View file
Open in desktop
Expand Up @@ -35,6 +35,26 @@ function f12({ kind, payload }: Action) {
}
}

// repro #50206
function f13<T extends Action>({ kind, payload }: T) {
if (kind === 'A') {
payload.toFixed();
}
if (kind === 'B') {
payload.toUpperCase();
}
}

function f14<T extends Action>(t: T) {
const { kind, payload } = t;
if (kind === 'A') {
payload.toFixed();
}
if (kind === 'B') {
payload.toUpperCase();
}
}

type Action2 =
| { kind: 'A', payload: number | undefined }
| { kind: 'B', payload: string | undefined };
Expand Down Expand Up @@ -420,6 +440,24 @@ function f12({ kind, payload }) {
payload; // never
}
}
// repro #50206
function f13({ kind, payload }) {
if (kind === 'A') {
payload.toFixed();
}
if (kind === 'B') {
payload.toUpperCase();
}
}
function f14(t) {
const { kind, payload } = t;
if (kind === 'A') {
payload.toFixed();
}
if (kind === 'B') {
payload.toUpperCase();
}
}
function f20({ kind, payload }) {
if (payload) {
if (kind === 'A') {
Expand Down Expand Up @@ -662,6 +700,8 @@ declare type Action = {
declare function f10({ kind, payload }: Action): void;
declare function f11(action: Action): void;
declare function f12({ kind, payload }: Action): void;
declare function f13<T extends Action>({ kind, payload }: T): void;
declare function f14<T extends Action>(t: T): void;
declare type Action2 = {
kind: 'A';
payload: number | undefined;
Expand Down