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.

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

Preserve type refinements in closures created past last assignment #56908

Merged
merged 14 commits into from
Jan 9, 2024
122 changes: 102 additions & 20 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,6 @@ import {
isOutermostOptionalChain,
isParameter,
isParameterDeclaration,
isParameterOrCatchClauseVariable,
isParameterPropertyDeclaration,
isParenthesizedExpression,
isParenthesizedTypeNode,
Expand Down Expand Up @@ -27481,7 +27480,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) || isParameterOrMutableLocalVariable(symbol) && !isSymbolAssigned(symbol);
}
break;
case SyntaxKind.PropertyAccessExpression:
Expand Down Expand Up @@ -28760,18 +28759,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, isFunctionOrSourceFile);
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 @@ -28789,27 +28794,98 @@ 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 => isFunctionOrSourceFile(node) && !!(getNodeLinks(node).flags & NodeCheckFlags.AssignmentsMarked));
}

function isFunctionOrSourceFile(node: Node) {
return isFunctionLikeDeclaration(node) || isSourceFile(node);
}

// For all assignments within the given root node, record the last assignment source position for all
// referenced parameters and mutable local variables. When assignments occur in nested functions or
// references occur in export specifiers, 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;
switch (node.kind) {
case SyntaxKind.Identifier:
if (isAssignmentTarget(node)) {
const symbol = getResolvedSymbol(node as Identifier);
if (isParameterOrMutableLocalVariable(symbol) && symbol.lastAssignmentPos !== Number.MAX_VALUE) {
const referencingFunction = findAncestor(node, isFunctionOrSourceFile);
const declaringFunction = findAncestor(symbol.valueDeclaration, isFunctionOrSourceFile);
symbol.lastAssignmentPos = referencingFunction === declaringFunction ? extendAssignmentPosition(node, symbol.valueDeclaration!) : Number.MAX_VALUE;
}
}
}
return;
case SyntaxKind.ExportSpecifier:
const exportDeclaration = (node as ExportSpecifier).parent.parent;
if (!(node as ExportSpecifier).isTypeOnly && !exportDeclaration.isTypeOnly && !exportDeclaration.moduleSpecifier) {
const symbol = resolveEntityName((node as ExportSpecifier).propertyName || (node as ExportSpecifier).name, SymbolFlags.Value, /*ignoreErrors*/ true, /*dontResolveAlias*/ true);
if (symbol && isParameterOrMutableLocalVariable(symbol)) {
symbol.lastAssignmentPos = Number.MAX_VALUE;
}
}
return;
case SyntaxKind.InterfaceDeclaration:
case SyntaxKind.TypeAliasDeclaration:
case SyntaxKind.EnumDeclaration:
return;
}
else {
forEachChild(node, markNodeAssignments);
if (isTypeNode(node)) {
return;
}
forEachChild(node, markNodeAssignments);
}

// Extend the position of the given assignment target node to the end of any intervening variable statement,
// expression statement, compound statement, or class declaration occurring between the node and the given
// declaration node.
function extendAssignmentPosition(node: Node, declaration: Declaration) {
let pos = node.pos;
while (node && node.pos > declaration.pos) {
switch (node.kind) {
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:
pos = node.end;
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jan 9, 2024

Choose a reason for hiding this comment

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

I don't know if it's worth noting that LabeledStatement isn't covered here. I don't think that it really matters because its end position should be identical to any of the other cases.

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, no special handling needed for LabeledStatement.

}
node = node.parent;
}
return pos;
}

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

function isParameterOrMutableLocalVariable(symbol: Symbol) {
// Return true if symbol is a parameter, a catch clause variable, or a mutable local variable
const declaration = symbol.valueDeclaration && getRootDeclaration(symbol.valueDeclaration);
return !!declaration && (
isParameter(declaration) ||
isVariableDeclaration(declaration) && (isCatchClause(declaration.parent) || isMutableLocalVariableDeclaration(declaration))
);
}

function isMutableLocalVariableDeclaration(declaration: VariableDeclaration) {
// Return true if symbol is a non-exported and non-global `let` variable
return !!(declaration.parent.flags & NodeFlags.Let) && !(
getCombinedModifierFlags(declaration) & ModifierFlags.Export ||
declaration.parent.parent.kind === SyntaxKind.VariableStatement && isGlobalSourceFile(declaration.parent.parent.parent)
);
}

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

Expand Down Expand Up @@ -29160,13 +29236,19 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const isModuleExports = symbol.flags & SymbolFlags.ModuleExports;
const typeIsAutomatic = type === autoType || type === autoArrayType;
const isAutomaticTypeInNonNull = typeIsAutomatic && node.parent.kind === SyntaxKind.NonNullExpression;
// When the control flow originates in a function expression or arrow function and we are referencing
// a const variable or parameter from an outer function, we extend the origin of the control flow
// analysis to include the immediately enclosing function.
// When the control flow originates in a function expression, arrow function, method, or accessor, and
// we are referencing a closed-over const variable or parameter or mutable local variable past its last
// assignment, we extend the origin of the control flow analysis to include the immediately enclosing
// control flow container.
while (
flowContainer !== declarationContainer && (flowContainer.kind === SyntaxKind.FunctionExpression ||
flowContainer.kind === SyntaxKind.ArrowFunction || isObjectLiteralOrClassExpressionMethodOrAccessor(flowContainer)) &&
(isConstantVariable(localOrExportSymbol) && type !== autoArrayType || isParameter && !isSymbolAssigned(localOrExportSymbol))
flowContainer !== declarationContainer && (
flowContainer.kind === SyntaxKind.FunctionExpression ||
flowContainer.kind === SyntaxKind.ArrowFunction ||
isObjectLiteralOrClassExpressionMethodOrAccessor(flowContainer)
) && (
isConstantVariable(localOrExportSymbol) && type !== autoArrayType ||
isParameterOrMutableLocalVariable(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 @@ -5825,8 +5825,8 @@ export interface Symbol {
/** @internal */ exportSymbol?: Symbol; // Exported symbol associated with this 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 */ lastAssignmentPos?: number; // Source position of last node that assigns value to symbol
/** @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 */ assignmentDeclarationMembers?: Map<number, Declaration>; // detected late-bound assignment declarations associated with the symbol
}

Expand Down
8 changes: 1 addition & 7 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 @@ -10351,12 +10351,6 @@ 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
16 changes: 3 additions & 13 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 @@ -186,16 +182,10 @@ controlFlowAliasing.ts(280,5): error TS2454: Variable 'a' is used before being a
let obj = arg;
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; }'.
obj.foo;
}
else {
obj.bar; // Not narrowed because obj is mutable
~~~
!!! 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; }'.
obj.bar;
}
}

Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/controlFlowAliasing.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,10 @@ function f25(arg: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
let obj = arg;
const isFoo = obj.kind === 'foo';
if (isFoo) {
obj.foo; // Not narrowed because obj is mutable
obj.foo;
}
else {
obj.bar; // Not narrowed because obj is mutable
obj.bar;
}
}

Expand Down Expand Up @@ -423,10 +423,10 @@ function f25(arg) {
var obj = arg;
var isFoo = obj.kind === 'foo';
if (isFoo) {
obj.foo; // Not narrowed because obj is mutable
obj.foo;
}
else {
obj.bar; // Not narrowed because obj is mutable
obj.bar;
}
}
function f26(outer) {
Expand Down
8 changes: 6 additions & 2 deletions tests/baselines/reference/controlFlowAliasing.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -370,12 +370,16 @@ function f25(arg: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
if (isFoo) {
>isFoo : Symbol(isFoo, Decl(controlFlowAliasing.ts, 131, 9))

obj.foo; // Not narrowed because obj is mutable
obj.foo;
>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;
>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
16 changes: 8 additions & 8 deletions tests/baselines/reference/controlFlowAliasing.types
Original file line number Diff line number Diff line change
Expand Up @@ -440,16 +440,16 @@ function f25(arg: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
if (isFoo) {
>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;
>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;
>obj.bar : number
>obj : { kind: "bar"; bar: number; }
>bar : number
}
}

Expand Down
7 changes: 2 additions & 5 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 @@ -38,11 +37,9 @@ implicitConstParameters.ts(44,27): error TS18048: 'x' is possibly 'undefined'.
}

function f4(x: string | undefined) {
x = "abc"; // causes x to be considered non-const
x = "abc";
if (x) {
doSomething(() => x.length);
~
!!! error TS18048: 'x' is possibly 'undefined'.
}
}

Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/implicitConstParameters.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ function f3(x: string | undefined) {
}

function f4(x: string | undefined) {
x = "abc"; // causes x to be considered non-const
x = "abc";
if (x) {
doSomething(() => x.length);
}
Expand Down Expand Up @@ -88,7 +88,7 @@ function f3(x) {
}
}
function f4(x) {
x = "abc"; // causes x to be considered non-const
x = "abc";
if (x) {
doSomething(function () { return x.length; });
}
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/implicitConstParameters.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ function f4(x: string | undefined) {
>f4 : Symbol(f4, Decl(implicitConstParameters.ts, 32, 1))
>x : Symbol(x, Decl(implicitConstParameters.ts, 34, 12))

x = "abc"; // causes x to be considered non-const
x = "abc";
>x : Symbol(x, Decl(implicitConstParameters.ts, 34, 12))

if (x) {
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/implicitConstParameters.types
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ function f4(x: string | undefined) {
>f4 : (x: string | undefined) => void
>x : string | undefined

x = "abc"; // causes x to be considered non-const
x = "abc";
>x = "abc" : "abc"
>x : string | undefined
>"abc" : "abc"
Expand All @@ -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
Loading