Skip to content

Preserve type refinements in closures created past last assignment #56908

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 14 commits into from
Jan 9, 2024
84 changes: 67 additions & 17 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ import {
getContainingClassExcludingClassDecorators,
getContainingClassStaticBlock,
getContainingFunction,
getContainingFunctionDeclaration,
getContainingFunctionOrClassStaticBlock,
getDeclarationModifierFlagsFromSymbol,
getDeclarationOfKind,
Expand Down Expand Up @@ -667,7 +668,6 @@ import {
isOutermostOptionalChain,
isParameter,
isParameterDeclaration,
isParameterOrCatchClauseVariable,
isParameterPropertyDeclaration,
isParenthesizedExpression,
isParenthesizedTypeNode,
Expand Down Expand Up @@ -27451,7 +27451,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
case SyntaxKind.Identifier:
if (!isThisInTypeQuery(node)) {
const symbol = getResolvedSymbol(node as Identifier);
return isConstantVariable(symbol) || isParameterOrCatchClauseVariable(symbol) && !isSymbolAssigned(symbol);
return isConstantVariable(symbol) || isParameterOrLetOrCatchVariable(symbol) && !isSymbolAssigned(symbol);
}
break;
case SyntaxKind.PropertyAccessExpression:
Expand Down Expand Up @@ -28728,18 +28728,24 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

// Check if a parameter or catch variable is assigned anywhere
function isSymbolAssigned(symbol: Symbol) {
if (!symbol.valueDeclaration) {
return !isPastLastAssignment(symbol, /*location*/ undefined);
}

// Return true if there are no assignments to the given symbol or if the given location
// is past the last assignment to the symbol.
function isPastLastAssignment(symbol: Symbol, location: Node | undefined) {
const parent = findAncestor(symbol.valueDeclaration, isAssignmentMarkingContainer);
if (!parent) {
return false;
}
const parent = getRootDeclaration(symbol.valueDeclaration).parent;
const links = getNodeLinks(parent);
if (!(links.flags & NodeCheckFlags.AssignmentsMarked)) {
links.flags |= NodeCheckFlags.AssignmentsMarked;
if (!hasParentWithAssignmentsMarked(parent)) {
markNodeAssignments(parent);
}
}
return symbol.isAssigned || false;
return !symbol.lastAssignmentPos || location && symbol.lastAssignmentPos < location.pos;
}

// Check if a parameter or catch variable (or their bindings elements) is assigned anywhere
Expand All @@ -28757,27 +28763,71 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function hasParentWithAssignmentsMarked(node: Node) {
return !!findAncestor(node.parent, node => (isFunctionLike(node) || isCatchClause(node)) && !!(getNodeLinks(node).flags & NodeCheckFlags.AssignmentsMarked));
return !!findAncestor(node.parent, node => isAssignmentMarkingContainer(node) && !!(getNodeLinks(node).flags & NodeCheckFlags.AssignmentsMarked));
}

function isAssignmentMarkingContainer(node: Node) {
return isFunctionLikeDeclaration(node) || isBlock(node) || isSourceFile(node) || isForStatement(node) || isForInOrOfStatement(node) || isCatchClause(node);
}

// For all assignments within the given root node, record the last assignment source position for all
// referenced parameters, let variables, and catch variables. When assignments occur in nested functions,
// record Number.MAX_VALUE as the assignment position. When assignments occur in compound statements,
// record the ending source position of the compound statement as the assignment position (this is more
// conservative than full control flow analysis, but requires only a single walk over the AST).
function markNodeAssignments(node: Node) {
if (node.kind === SyntaxKind.Identifier) {
if (isAssignmentTarget(node)) {
const symbol = getResolvedSymbol(node as Identifier);
if (isParameterOrCatchClauseVariable(symbol)) {
symbol.isAssigned = true;
}
let statementEnd: number | undefined;
markAssignments(node);
function markAssignments(node: Node) {
switch (node.kind) {
case SyntaxKind.Identifier:
if (isAssignmentTarget(node)) {
const symbol = getResolvedSymbol(node as Identifier);
if (isParameterOrLetOrCatchVariable(symbol) && symbol.lastAssignmentPos !== Number.MAX_VALUE) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to ensure that if declaringFunction is a SourceFile, that it's also a module? Otherwise you can't be certain that the scope isn't global.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's a good point. For top-level declarations, we should only preserve refinements for const variables and non-exported mutable variables declared in external modules.

const referencingFunction = getContainingFunctionDeclaration(node) || getSourceFileOfNode(node);
const declaringFunction = getContainingFunctionDeclaration(symbol.valueDeclaration!) || getSourceFileOfNode(node);
symbol.lastAssignmentPos = referencingFunction === declaringFunction ? statementEnd ?? node.pos : Number.MAX_VALUE;
}
}
return;
case SyntaxKind.VariableStatement:
case SyntaxKind.ExpressionStatement:
case SyntaxKind.IfStatement:
case SyntaxKind.DoStatement:
case SyntaxKind.WhileStatement:
case SyntaxKind.ForStatement:
case SyntaxKind.ForInStatement:
case SyntaxKind.ForOfStatement:
case SyntaxKind.WithStatement:
case SyntaxKind.SwitchStatement:
case SyntaxKind.TryStatement:
case SyntaxKind.ClassDeclaration:
const saveStatementEnd = statementEnd;
statementEnd ??= node.end;
forEachChild(node, markAssignments);
statementEnd = saveStatementEnd;
return;
case SyntaxKind.InterfaceDeclaration:
case SyntaxKind.TypeAliasDeclaration:
case SyntaxKind.EnumDeclaration:
return;
}
if (!isTypeNode(node)) {
forEachChild(node, markAssignments);
}
}
else {
forEachChild(node, markNodeAssignments);
}
}
}

function isConstantVariable(symbol: Symbol) {
return symbol.flags & SymbolFlags.Variable && (getDeclarationNodeFlagsFromSymbol(symbol) & NodeFlags.Constant) !== 0;
}

function isParameterOrLetOrCatchVariable(symbol: Symbol) {
const declaration = symbol.valueDeclaration && getRootDeclaration(symbol.valueDeclaration);
return !!(declaration && (isParameter(declaration) ||
isVariableDeclaration(declaration) && (declaration.parent.kind === SyntaxKind.CatchClause || declaration.parent.flags & NodeFlags.Let)));
}

function parameterInitializerContainsUndefined(declaration: ParameterDeclaration): boolean {
const links = getNodeLinks(declaration);

Expand Down Expand Up @@ -29134,7 +29184,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
while (
flowContainer !== declarationContainer && (flowContainer.kind === SyntaxKind.FunctionExpression ||
flowContainer.kind === SyntaxKind.ArrowFunction || isObjectLiteralOrClassExpressionMethodOrAccessor(flowContainer)) &&
(isConstantVariable(localOrExportSymbol) && type !== autoArrayType || isParameter && !isSymbolAssigned(localOrExportSymbol))
(isConstantVariable(localOrExportSymbol) && type !== autoArrayType || (isParameterOrLetOrCatchVariable(localOrExportSymbol)) && isPastLastAssignment(localOrExportSymbol, node))
) {
flowContainer = getControlFlowContainer(flowContainer);
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5826,7 +5826,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 */ lastAssignmentPos?: number; // Last node that assigns value to symbol
Copy link
Member

Choose a reason for hiding this comment

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

Consider placing this next to isReferenced, since it's always definitely assigned right after in the Symbol constructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

/** @internal */ assignmentDeclarationMembers?: Map<number, Declaration>; // detected late-bound assignment declarations associated with the symbol
}

Expand Down
13 changes: 1 addition & 12 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8178,7 +8178,7 @@ function Symbol(this: Symbol, flags: SymbolFlags, name: __String) {
this.exportSymbol = undefined;
this.constEnumOnlyModule = undefined;
this.isReferenced = undefined;
this.isAssigned = undefined;
this.lastAssignmentPos = undefined;
(this as any).links = undefined; // used by TransientSymbol
}

Expand Down Expand Up @@ -10346,17 +10346,6 @@ export function isInfinityOrNaNString(name: string | __String): boolean {
return name === "Infinity" || name === "-Infinity" || name === "NaN";
}

/** @internal */
export function isCatchClauseVariableDeclaration(node: Node) {
return node.kind === SyntaxKind.VariableDeclaration && node.parent.kind === SyntaxKind.CatchClause;
}

/** @internal */
export function isParameterOrCatchClauseVariable(symbol: Symbol) {
const declaration = symbol.valueDeclaration && getRootDeclaration(symbol.valueDeclaration);
return !!declaration && (isParameter(declaration) || isCatchClauseVariableDeclaration(declaration));
}

/** @internal */
export function isFunctionExpressionOrArrowFunction(node: Node): node is FunctionExpression | ArrowFunction {
return node.kind === SyntaxKind.FunctionExpression || node.kind === SyntaxKind.ArrowFunction;
Expand Down
12 changes: 1 addition & 11 deletions tests/baselines/reference/controlFlowAliasing.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@ controlFlowAliasing.ts(112,13): error TS2339: Property 'foo' does not exist on t
Property 'foo' does not exist on type '{ kind: "bar"; bar: number; }'.
controlFlowAliasing.ts(115,13): error TS2339: Property 'bar' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
Property 'bar' does not exist on type '{ kind: "foo"; foo: string; }'.
controlFlowAliasing.ts(134,13): error TS2339: Property 'foo' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
Property 'foo' does not exist on type '{ kind: "bar"; bar: number; }'.
controlFlowAliasing.ts(137,13): error TS2339: Property 'bar' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
Property 'bar' does not exist on type '{ kind: "foo"; foo: string; }'.
controlFlowAliasing.ts(154,19): error TS2339: Property 'foo' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
Property 'foo' does not exist on type '{ kind: "bar"; bar: number; }'.
controlFlowAliasing.ts(157,19): error TS2339: Property 'bar' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
Expand All @@ -28,7 +24,7 @@ controlFlowAliasing.ts(280,5): error TS2448: Block-scoped variable 'a' used befo
controlFlowAliasing.ts(280,5): error TS2454: Variable 'a' is used before being assigned.


==== controlFlowAliasing.ts (15 errors) ====
==== controlFlowAliasing.ts (13 errors) ====
// Narrowing by aliased conditional expressions

function f10(x: string | number) {
Expand Down Expand Up @@ -187,15 +183,9 @@ controlFlowAliasing.ts(280,5): error TS2454: Variable 'a' is used before being a
const isFoo = obj.kind === 'foo';
if (isFoo) {
obj.foo; // Not narrowed because obj is mutable
~~~
!!! error TS2339: Property 'foo' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
!!! error TS2339: Property 'foo' does not exist on type '{ kind: "bar"; bar: number; }'.
}
else {
obj.bar; // Not narrowed because obj is mutable

Choose a reason for hiding this comment

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

I guess these comments are outdated now

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll fix those.

~~~
!!! error TS2339: Property 'bar' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
!!! error TS2339: Property 'bar' does not exist on type '{ kind: "foo"; foo: string; }'.
}
}

Expand Down
4 changes: 4 additions & 0 deletions tests/baselines/reference/controlFlowAliasing.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -371,11 +371,15 @@ function f25(arg: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
>isFoo : Symbol(isFoo, Decl(controlFlowAliasing.ts, 131, 9))

obj.foo; // Not narrowed because obj is mutable
>obj.foo : Symbol(foo, Decl(controlFlowAliasing.ts, 129, 32))
>obj : Symbol(obj, Decl(controlFlowAliasing.ts, 130, 7))
>foo : Symbol(foo, Decl(controlFlowAliasing.ts, 129, 32))
}
else {
obj.bar; // Not narrowed because obj is mutable
>obj.bar : Symbol(bar, Decl(controlFlowAliasing.ts, 129, 63))
>obj : Symbol(obj, Decl(controlFlowAliasing.ts, 130, 7))
>bar : Symbol(bar, Decl(controlFlowAliasing.ts, 129, 63))
}
}

Expand Down
12 changes: 6 additions & 6 deletions tests/baselines/reference/controlFlowAliasing.types
Original file line number Diff line number Diff line change
Expand Up @@ -441,15 +441,15 @@ function f25(arg: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
>isFoo : boolean

obj.foo; // Not narrowed because obj is mutable
>obj.foo : any
>obj : { kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }
>foo : any
>obj.foo : string
>obj : { kind: "foo"; foo: string; }
>foo : string
}
else {
obj.bar; // Not narrowed because obj is mutable
>obj.bar : any
>obj : { kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }
>bar : any
>obj.bar : number
>obj : { kind: "bar"; bar: number; }
>bar : number
}
}

Expand Down
5 changes: 1 addition & 4 deletions tests/baselines/reference/implicitConstParameters.errors.txt
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
implicitConstParameters.ts(38,27): error TS18048: 'x' is possibly 'undefined'.
implicitConstParameters.ts(44,27): error TS18048: 'x' is possibly 'undefined'.


==== implicitConstParameters.ts (2 errors) ====
==== implicitConstParameters.ts (1 errors) ====
function doSomething(cb: () => void) {
cb();
}
Expand Down Expand Up @@ -41,8 +40,6 @@ implicitConstParameters.ts(44,27): error TS18048: 'x' is possibly 'undefined'.
x = "abc"; // causes x to be considered non-const
if (x) {
doSomething(() => x.length);
~
!!! error TS18048: 'x' is possibly 'undefined'.
}
}

Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/implicitConstParameters.types
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ function f4(x: string | undefined) {
>doSomething : (cb: () => void) => void
>() => x.length : () => number
>x.length : number
>x : string | undefined
>x : string
>length : number
}
}
Expand Down
Loading