Skip to content

Avoid dependent parameters narrowings if any declared symbol of the parameter is assigned to #56313

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28684,6 +28684,20 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return symbol.isAssigned || false;
}

// Check if a parameter or catch variable (or their bindings elements) is assigned anywhere
function isSomeSymbolAssigned(rootDeclaration: Node) {
Debug.assert(isVariableDeclaration(rootDeclaration) || isParameter(rootDeclaration));
return isSomeSymbolAssignedWorker(rootDeclaration.name);
}

function isSomeSymbolAssignedWorker(node: BindingName): boolean {
if (node.kind === SyntaxKind.Identifier) {
return isSymbolAssigned(getSymbolOfDeclaration(node.parent as Declaration));
}

return some(node.elements, e => e.kind !== SyntaxKind.OmittedExpression && isSomeSymbolAssignedWorker(e.name));
}

function hasParentWithAssignmentsMarked(node: Node) {
return !!findAncestor(node.parent, node => (isFunctionLike(node) || isCatchClause(node)) && !!(getNodeLinks(node).flags & NodeCheckFlags.AssignmentsMarked));
}
Expand Down Expand Up @@ -28867,7 +28881,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const parentType = getTypeForBindingElementParent(parent, CheckMode.Normal);
const parentTypeConstraint = parentType && mapType(parentType, getBaseConstraintOrType);
links.flags &= ~NodeCheckFlags.InCheckIdentifier;
if (parentTypeConstraint && parentTypeConstraint.flags & TypeFlags.Union && !(rootDeclaration.kind === SyntaxKind.Parameter && isSymbolAssigned(symbol))) {
if (parentTypeConstraint && parentTypeConstraint.flags & TypeFlags.Union && !(rootDeclaration.kind === SyntaxKind.Parameter && isSomeSymbolAssigned(rootDeclaration))) {
const pattern = declaration.parent;
const narrowedType = getFlowTypeOfReference(pattern, parentTypeConstraint, parentTypeConstraint, /*flowContainer*/ undefined, location.flowNode);
if (narrowedType.flags & TypeFlags.Never) {
Expand Down Expand Up @@ -28907,7 +28921,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const contextualSignature = getContextualSignature(func);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the change to isSymbolAssigned behavior, to aggregate isAssigned at the root declaration level, might not be the correct thing to do here, for this particular usage of isSymbolAssigned, as evidenced by this breaking change: #56313 (comment).

Maybe we want to keep marking symbols with isAssigned, and also aggregate that info at the root declaration level, since getNarrowedTypeOfSymbol needs the aggregate info but checkIdentifier needs the symbol info.

Here's a more minimal repro of the break linked above:

function ff({ a, b }: { a: string | undefined, b: () => void }) {
  if (a !== undefined) {
    b = () => {
      const x: string = a;
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, I really need to start reading through all of the existing comments at once instead of reading through them one by one. It would save me a few minutes here 😅

if (contextualSignature && contextualSignature.parameters.length === 1 && signatureHasRestParameter(contextualSignature)) {
const restType = getReducedApparentType(instantiateType(getTypeOfSymbol(contextualSignature.parameters[0]), getInferenceContext(func)?.nonFixingMapper));
if (restType.flags & TypeFlags.Union && everyType(restType, isTupleType) && !isSymbolAssigned(symbol)) {
if (restType.flags & TypeFlags.Union && everyType(restType, isTupleType) && !some(func.parameters, isSomeSymbolAssigned)) {
const narrowedType = getFlowTypeOfReference(func, restType, restType, /*flowContainer*/ undefined, location.flowNode);
const index = func.parameters.indexOf(declaration) - (getThisParameter(func) ? 1 : 0);
return getIndexedAccessType(narrowedType, getNumberLiteralType(index));
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5811,7 +5811,7 @@ export interface Symbol {
/** @internal */ constEnumOnlyModule: boolean | undefined; // True if module contains only const enums or other modules with only const enums
/** @internal */ isReferenced?: SymbolFlags; // True if the symbol is referenced elsewhere. Keeps track of the meaning of a reference in case a symbol is both a type parameter and parameter.
/** @internal */ isReplaceableByMethod?: boolean; // Can this Javascript class property be replaced by a method symbol?
/** @internal */ isAssigned?: boolean; // True if the symbol is a parameter with assignments
/** @internal */ isAssigned?: boolean; // True if the symbol is a parameter with assignments
/** @internal */ assignmentDeclarationMembers?: Map<number, Declaration>; // detected late-bound assignment declarations associated with the symbol
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -443,4 +443,35 @@ dependentDestructuredVariables.ts(431,15): error TS2322: Type 'number' is not as
!!! error TS2322: Type 'number' is not assignable to type 'never'.
}
}

// https://github.com/microsoft/TypeScript/issues/56312

function parameterReassigned1([x, y]: [1, 2] | [3, 4]) {
if (Math.random()) {
x = 1;
}
if (y === 2) {
x; // 1 | 3
}
}

function parameterReassigned2([x, y]: [1, 2] | [3, 4]) {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x; // 1 | 3
}
}

// https://github.com/microsoft/TypeScript/pull/56313#discussion_r1416482490

const parameterReassignedContextualRest1: (...args: [1, 2] | [3, 4]) => void = (x, y) => {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x; // 1 | 3
}
}

60 changes: 60 additions & 0 deletions tests/baselines/reference/dependentDestructuredVariables.js
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,37 @@ function tooNarrow([x, y]: [1, 1] | [1, 2] | [1]) {
const shouldNotBeOk: never = x; // Error
}
}

// https://github.com/microsoft/TypeScript/issues/56312

function parameterReassigned1([x, y]: [1, 2] | [3, 4]) {
if (Math.random()) {
x = 1;
}
if (y === 2) {
x; // 1 | 3
}
}

function parameterReassigned2([x, y]: [1, 2] | [3, 4]) {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x; // 1 | 3
}
}

// https://github.com/microsoft/TypeScript/pull/56313#discussion_r1416482490

const parameterReassignedContextualRest1: (...args: [1, 2] | [3, 4]) => void = (x, y) => {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x; // 1 | 3
}
}


//// [dependentDestructuredVariables.js]
Expand Down Expand Up @@ -766,6 +797,32 @@ function tooNarrow([x, y]) {
const shouldNotBeOk = x; // Error
}
}
// https://github.com/microsoft/TypeScript/issues/56312
function parameterReassigned1([x, y]) {
if (Math.random()) {
x = 1;
}
if (y === 2) {
x; // 1 | 3
}
}
function parameterReassigned2([x, y]) {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x; // 1 | 3
}
}
// https://github.com/microsoft/TypeScript/pull/56313#discussion_r1416482490
const parameterReassignedContextualRest1 = (x, y) => {
if (Math.random()) {
y = 2;
}
if (y === 2) {
x; // 1 | 3
}
};


//// [dependentDestructuredVariables.d.ts]
Expand Down Expand Up @@ -916,3 +973,6 @@ declare class Client {
declare const bot: Client;
declare function fz1([x, y]: [1, 2] | [3, 4] | [5]): void;
declare function tooNarrow([x, y]: [1, 1] | [1, 2] | [1]): void;
declare function parameterReassigned1([x, y]: [1, 2] | [3, 4]): void;
declare function parameterReassigned2([x, y]: [1, 2] | [3, 4]): void;
declare const parameterReassignedContextualRest1: (...args: [1, 2] | [3, 4]) => void;
68 changes: 68 additions & 0 deletions tests/baselines/reference/dependentDestructuredVariables.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -1100,3 +1100,71 @@ function tooNarrow([x, y]: [1, 1] | [1, 2] | [1]) {
}
}

// https://github.com/microsoft/TypeScript/issues/56312

function parameterReassigned1([x, y]: [1, 2] | [3, 4]) {
>parameterReassigned1 : Symbol(parameterReassigned1, Decl(dependentDestructuredVariables.ts, 432, 1))
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 436, 31))
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 436, 33))

if (Math.random()) {
>Math.random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))
>Math : Symbol(Math, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.core.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --))
>random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))

x = 1;
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 436, 31))
}
if (y === 2) {
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 436, 33))

x; // 1 | 3
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 436, 31))
}
}

function parameterReassigned2([x, y]: [1, 2] | [3, 4]) {
>parameterReassigned2 : Symbol(parameterReassigned2, Decl(dependentDestructuredVariables.ts, 443, 1))
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 445, 31))
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 445, 33))

if (Math.random()) {
>Math.random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))
>Math : Symbol(Math, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.core.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --))
>random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))

y = 2;
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 445, 33))
}
if (y === 2) {
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 445, 33))

x; // 1 | 3
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 445, 31))
}
}

// https://github.com/microsoft/TypeScript/pull/56313#discussion_r1416482490

const parameterReassignedContextualRest1: (...args: [1, 2] | [3, 4]) => void = (x, y) => {
>parameterReassignedContextualRest1 : Symbol(parameterReassignedContextualRest1, Decl(dependentDestructuredVariables.ts, 456, 5))
>args : Symbol(args, Decl(dependentDestructuredVariables.ts, 456, 43))
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 456, 80))
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 456, 82))

if (Math.random()) {
>Math.random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))
>Math : Symbol(Math, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.core.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --))
>random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))

y = 2;
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 456, 82))
}
if (y === 2) {
>y : Symbol(y, Decl(dependentDestructuredVariables.ts, 456, 82))

x; // 1 | 3
>x : Symbol(x, Decl(dependentDestructuredVariables.ts, 456, 80))
}
}

84 changes: 84 additions & 0 deletions tests/baselines/reference/dependentDestructuredVariables.types
Original file line number Diff line number Diff line change
Expand Up @@ -1263,3 +1263,87 @@ function tooNarrow([x, y]: [1, 1] | [1, 2] | [1]) {
}
}

// https://github.com/microsoft/TypeScript/issues/56312

function parameterReassigned1([x, y]: [1, 2] | [3, 4]) {
>parameterReassigned1 : ([x, y]: [1, 2] | [3, 4]) => void
>x : 1 | 3
>y : 2 | 4

if (Math.random()) {
>Math.random() : number
>Math.random : () => number
>Math : Math
>random : () => number

x = 1;
>x = 1 : 1
>x : 1 | 3
>1 : 1
}
if (y === 2) {
>y === 2 : boolean
>y : 2 | 4
>2 : 2

x; // 1 | 3
>x : 1 | 3
}
}

function parameterReassigned2([x, y]: [1, 2] | [3, 4]) {
>parameterReassigned2 : ([x, y]: [1, 2] | [3, 4]) => void
>x : 1 | 3
>y : 2 | 4

if (Math.random()) {
>Math.random() : number
>Math.random : () => number
>Math : Math
>random : () => number

y = 2;
>y = 2 : 2
>y : 2 | 4
>2 : 2
}
if (y === 2) {
>y === 2 : boolean
>y : 2 | 4
>2 : 2

x; // 1 | 3
>x : 1 | 3
}
}

// https://github.com/microsoft/TypeScript/pull/56313#discussion_r1416482490

const parameterReassignedContextualRest1: (...args: [1, 2] | [3, 4]) => void = (x, y) => {
>parameterReassignedContextualRest1 : (...args: [1, 2] | [3, 4]) => void
>args : [1, 2] | [3, 4]
>(x, y) => { if (Math.random()) { y = 2; } if (y === 2) { x; // 1 | 3 }} : (x: 1 | 3, y: 2 | 4) => void
>x : 1 | 3
>y : 2 | 4

if (Math.random()) {
>Math.random() : number
>Math.random : () => number
>Math : Math
>random : () => number

y = 2;
>y = 2 : 2
>y : 2 | 4
>2 : 2
}
if (y === 2) {
>y === 2 : boolean
>y : 2 | 4
>2 : 2

x; // 1 | 3
>x : 1 | 3
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//// [tests/cases/compiler/narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts] ////

=== narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts ===
function ff({ a, b }: { a: string | undefined, b: () => void }) {
>ff : Symbol(ff, Decl(narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts, 0, 0))
>a : Symbol(a, Decl(narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts, 0, 13))
>b : Symbol(b, Decl(narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts, 0, 16))
>a : Symbol(a, Decl(narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts, 0, 23))
>b : Symbol(b, Decl(narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts, 0, 46))

if (a !== undefined) {
>a : Symbol(a, Decl(narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts, 0, 13))
>undefined : Symbol(undefined)

b = () => {
>b : Symbol(b, Decl(narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts, 0, 16))

const x: string = a;
>x : Symbol(x, Decl(narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts, 3, 11))
>a : Symbol(a, Decl(narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts, 0, 13))
}
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//// [tests/cases/compiler/narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts] ////

=== narrowRefinedConstLikeParameterBIndingElementNameInInnerScope.ts ===
function ff({ a, b }: { a: string | undefined, b: () => void }) {
>ff : ({ a, b }: { a: string | undefined; b: () => void;}) => void
>a : string | undefined
>b : () => void
>a : string | undefined
>b : () => void

if (a !== undefined) {
>a !== undefined : boolean
>a : string | undefined
>undefined : undefined

b = () => {
>b = () => { const x: string = a; } : () => void
>b : () => void
>() => { const x: string = a; } : () => void

const x: string = a;
>x : string
>a : string
}
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// @strict: true
// @noEmit: true

function ff({ a, b }: { a: string | undefined, b: () => void }) {
if (a !== undefined) {
b = () => {
const x: string = a;
}
}
}
Loading