Skip to content

Fix constant reference check in CFA #44762

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

Merged
DanielRosenwasser merged 5 commits into from
Jun 28, 2021
Merged

Fix constant reference check in CFA #44762

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12d5296
Check entire access path is constant when narrowing by inlining
ahejlsberg Jun 26, 2021
19eba22
Add tests
ahejlsberg Jun 26, 2021
d535af6
Accept new baselines
ahejlsberg Jun 26, 2021
6f86803
Added test cases for parameter properties.
DanielRosenwasser Jun 28, 2021
3b12c04
Accepted baselines.
DanielRosenwasser Jun 28, 2021
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
29 changes: 21 additions & 8 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22975,7 +22975,20 @@ namespace ts {
}
}

function getFlowTypeOfReference(reference: Node, declaredType: Type, initialType = declaredType, isConstant?: boolean, flowContainer?: Node) {
function isConstantReference(node: Node): boolean {
switch (node.kind) {
case SyntaxKind.Identifier:
const symbol = getResolvedSymbol(node as Identifier);
return isConstVariable(symbol) || !!symbol.valueDeclaration && getRootDeclaration(symbol.valueDeclaration).kind === SyntaxKind.Parameter && !isParameterAssigned(symbol);
case SyntaxKind.PropertyAccessExpression:
case SyntaxKind.ElementAccessExpression:
// The resolvedSymbol property is initialized by checkPropertyAccess or checkElementAccess before we get here.
Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser Jun 28, 2021

Choose a reason for hiding this comment

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

Would be nice if there was a way to assert this easily.

return isConstantReference((node as AccessExpression).expression) && isReadonlySymbol(getNodeLinks(node).resolvedSymbol || unknownSymbol);
}
return false;
}

function getFlowTypeOfReference(reference: Node, declaredType: Type, initialType = declaredType, flowContainer?: Node) {
let key: string | undefined;
let isKeySet = false;
let flowDepth = 0;
Expand Down Expand Up @@ -24020,11 +24033,11 @@ namespace ts {
case SyntaxKind.Identifier:
// When narrowing a reference to a const variable, non-assigned parameter, or readonly property, we inline
// up to five levels of aliased conditional expressions that are themselves declared as const variables.
if (isConstant && !isMatchingReference(reference, expr) && inlineLevel < 5) {
if (!isMatchingReference(reference, expr) && inlineLevel < 5) {
const symbol = getResolvedSymbol(expr as Identifier);
if (isConstVariable(symbol)) {
const declaration = symbol.valueDeclaration;
if (declaration && isVariableDeclaration(declaration) && !declaration.type && declaration.initializer) {
if (declaration && isVariableDeclaration(declaration) && !declaration.type && declaration.initializer && isConstantReference(reference)) {
inlineLevel++;
const result = narrowType(type, declaration.initializer, assumeTrue);
inlineLevel--;
Expand Down Expand Up @@ -24370,12 +24383,12 @@ namespace ts {
const isOuterVariable = flowContainer !== declarationContainer;
const isSpreadDestructuringAssignmentTarget = node.parent && node.parent.parent && isSpreadAssignment(node.parent) && isDestructuringAssignmentTarget(node.parent.parent);
const isModuleExports = symbol.flags & SymbolFlags.ModuleExports;
const isConstant = isConstVariable(localOrExportSymbol) && getTypeOfSymbol(localOrExportSymbol) !== autoArrayType || isParameter && !isParameterAssigned(localOrExportSymbol);
// 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.
while (isConstant && flowContainer !== declarationContainer && (flowContainer.kind === SyntaxKind.FunctionExpression ||
flowContainer.kind === SyntaxKind.ArrowFunction || isObjectLiteralOrClassExpressionMethod(flowContainer))) {
while (flowContainer !== declarationContainer && (flowContainer.kind === SyntaxKind.FunctionExpression ||
flowContainer.kind === SyntaxKind.ArrowFunction || isObjectLiteralOrClassExpressionMethod(flowContainer)) &&
(isConstVariable(localOrExportSymbol) && type !== autoArrayType || isParameter && !isParameterAssigned(localOrExportSymbol))) {
flowContainer = getControlFlowContainer(flowContainer);
}
// We only look for uninitialized variables in strict null checking mode, and only when we can analyze
Expand All @@ -24390,7 +24403,7 @@ namespace ts {
const initialType = assumeInitialized ? (isParameter ? removeOptionalityFromDeclaredType(type, declaration as VariableLikeDeclaration) : type) :
type === autoType || type === autoArrayType ? undefinedType :
getOptionalType(type);
const flowType = getFlowTypeOfReference(node, type, initialType, isConstant, flowContainer);
const flowType = getFlowTypeOfReference(node, type, initialType, flowContainer);
// A variable is considered uninitialized when it is possible to analyze the entire control flow graph
// from declaration to use, and when the variable's declared type doesn't include undefined but the
// control flow based type does include undefined.
Expand Down Expand Up @@ -27608,7 +27621,7 @@ namespace ts {
getControlFlowContainer(node) === getControlFlowContainer(prop.valueDeclaration)) {
assumeUninitialized = true;
}
const flowType = getFlowTypeOfReference(node, propType, assumeUninitialized ? getOptionalType(propType) : propType, prop && isReadonlySymbol(prop));
const flowType = getFlowTypeOfReference(node, propType, assumeUninitialized ? getOptionalType(propType) : propType);
if (assumeUninitialized && !(getFalsyFlags(propType) & TypeFlags.Undefined) && getFalsyFlags(flowType) & TypeFlags.Undefined) {
error(errorNode, Diagnostics.Property_0_is_used_before_being_assigned, symbolToString(prop!)); // TODO: GH#18217
// Return the declared type to reduce follow-on errors
Expand Down
66 changes: 53 additions & 13 deletions tests/baselines/reference/controlFlowAliasing.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,34 @@
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(61,13): error TS2339: Property 'foo' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(59,13): error TS2322: Type 'string | number' is not assignable to type 'string'.
Type 'number' is not assignable to type 'string'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(74,13): error TS2322: Type 'string | number' is not assignable to type 'string'.
Type 'number' is not assignable to type 'string'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(91,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; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(64,13): error TS2339: Property 'bar' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(94,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; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(71,13): error TS2339: Property 'foo' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(101,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; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(74,13): error TS2339: Property 'bar' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(104,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; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(82,13): error TS2339: Property 'foo' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(112,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; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(85,13): error TS2339: Property 'bar' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
tests/cases/conformance/controlFlow/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; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(104,13): error TS2339: Property 'foo' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
tests/cases/conformance/controlFlow/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; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(107,13): error TS2339: Property 'bar' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
tests/cases/conformance/controlFlow/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; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(124,19): error TS2339: Property 'foo' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
tests/cases/conformance/controlFlow/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; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(127,19): error TS2339: Property 'bar' does not exist on type '{ kind: "foo"; foo: string; } | { kind: "bar"; bar: number; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(157,19): 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; }'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(207,13): error TS2322: Type 'string | number' is not assignable to type 'string'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(237,13): error TS2322: Type 'string | number' is not assignable to type 'string'.
Type 'number' is not assignable to type 'string'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(210,13): error TS2322: Type 'string | number' is not assignable to type 'number'.
tests/cases/conformance/controlFlow/controlFlowAliasing.ts(240,13): error TS2322: Type 'string | number' is not assignable to type 'number'.
Type 'string' is not assignable to type 'number'.


==== tests/cases/conformance/controlFlow/controlFlowAliasing.ts (12 errors) ====
==== tests/cases/conformance/controlFlow/controlFlowAliasing.ts (14 errors) ====
// Narrowing by aliased conditional expressions

function f10(x: string | number) {
Expand Down Expand Up @@ -72,6 +76,42 @@ tests/cases/conformance/controlFlow/controlFlowAliasing.ts(210,13): error TS2322
return notUndefined ? x : 0;
}

function f15(obj: { readonly x: string | number }) {
const isString = typeof obj.x === 'string';
if (isString) {
let s: string = obj.x;
}
}

function f16(obj: { readonly x: string | number }) {
const isString = typeof obj.x === 'string';
obj = { x: 42 };
if (isString) {
let s: string = obj.x; // Not narrowed because of is assigned in function body
~
!!! error TS2322: Type 'string | number' is not assignable to type 'string'.
!!! error TS2322: Type 'number' is not assignable to type 'string'.
}
}

function f17(obj: readonly [string | number]) {
const isString = typeof obj[0] === 'string';
if (isString) {
let s: string = obj[0];
}
}

function f18(obj: readonly [string | number]) {
const isString = typeof obj[0] === 'string';
obj = [42];
if (isString) {
let s: string = obj[0]; // Not narrowed because of is assigned in function body
~
!!! error TS2322: Type 'string | number' is not assignable to type 'string'.
!!! error TS2322: Type 'number' is not assignable to type 'string'.
}
}

function f20(obj: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
const isFoo = obj.kind === 'foo';
if (isFoo) {
Expand Down
64 changes: 64 additions & 0 deletions tests/baselines/reference/controlFlowAliasing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,36 @@ function f14(x: number | null | undefined): number | null {
return notUndefined ? x : 0;
}

function f15(obj: { readonly x: string | number }) {
const isString = typeof obj.x === 'string';
if (isString) {
let s: string = obj.x;
}
}

function f16(obj: { readonly x: string | number }) {
const isString = typeof obj.x === 'string';
obj = { x: 42 };
if (isString) {
let s: string = obj.x; // Not narrowed because of is assigned in function body
}
}

function f17(obj: readonly [string | number]) {
const isString = typeof obj[0] === 'string';
if (isString) {
let s: string = obj[0];
}
}

function f18(obj: readonly [string | number]) {
const isString = typeof obj[0] === 'string';
obj = [42];
if (isString) {
let s: string = obj[0]; // Not narrowed because of is assigned in function body
}
}

function f20(obj: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
const isFoo = obj.kind === 'foo';
if (isFoo) {
Expand Down Expand Up @@ -256,6 +286,32 @@ function f14(x) {
var notUndefined = x !== undefined;
return notUndefined ? x : 0;
}
function f15(obj) {
var isString = typeof obj.x === 'string';
if (isString) {
var s = obj.x;
}
}
function f16(obj) {
var isString = typeof obj.x === 'string';
obj = { x: 42 };
if (isString) {
var s = obj.x; // Not narrowed because of is assigned in function body
}
}
function f17(obj) {
var isString = typeof obj[0] === 'string';
if (isString) {
var s = obj[0];
}
}
function f18(obj) {
var isString = typeof obj[0] === 'string';
obj = [42];
if (isString) {
var s = obj[0]; // Not narrowed because of is assigned in function body
}
}
function f20(obj) {
var isFoo = obj.kind === 'foo';
if (isFoo) {
Expand Down Expand Up @@ -413,6 +469,14 @@ declare function f11(x: unknown): void;
declare function f12(x: string | number | boolean): void;
declare function f13(x: string | number | boolean): void;
declare function f14(x: number | null | undefined): number | null;
declare function f15(obj: {
readonly x: string | number;
}): void;
declare function f16(obj: {
readonly x: string | number;
}): void;
declare function f17(obj: readonly [string | number]): void;
declare function f18(obj: readonly [string | number]): void;
declare function f20(obj: {
kind: 'foo';
foo: string;
Expand Down
Loading