Skip to content

Error when assertion function calls aren't CFA'd #33622

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 9 commits into from
Sep 30, 2019
Merged
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
6 changes: 0 additions & 6 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
@@ -1287,12 +1287,6 @@ namespace ts {
activeLabels!.pop();
}

function isDottedName(node: Expression): boolean {
return node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.ThisKeyword ||
node.kind === SyntaxKind.PropertyAccessExpression && isDottedName((<PropertyAccessExpression>node).expression) ||
node.kind === SyntaxKind.ParenthesizedExpression && isDottedName((<ParenthesizedExpression>node).expression);
}

function bindExpressionStatement(node: ExpressionStatement): void {
bind(node.expression);
// A top level call expression with a dotted function name and at least one argument
40 changes: 29 additions & 11 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
@@ -17191,30 +17191,38 @@ namespace ts {
getEffectiveTypeAnnotationNode(declaration as VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature));
}

function getExplicitTypeOfSymbol(symbol: Symbol) {
return symbol.flags & (SymbolFlags.Function | SymbolFlags.Method | SymbolFlags.Class | SymbolFlags.ValueModule) ||
symbol.flags & (SymbolFlags.Variable | SymbolFlags.Property) && isDeclarationWithExplicitTypeAnnotation(symbol.valueDeclaration) ?
getTypeOfSymbol(symbol) : undefined;
function getExplicitTypeOfSymbol(symbol: Symbol, diagnostic?: Diagnostic) {
if (symbol.flags & (SymbolFlags.Function | SymbolFlags.Method | SymbolFlags.Class | SymbolFlags.ValueModule)) {
return getTypeOfSymbol(symbol);
}
if (symbol.flags & (SymbolFlags.Variable | SymbolFlags.Property)) {
if (isDeclarationWithExplicitTypeAnnotation(symbol.valueDeclaration)) {
return getTypeOfSymbol(symbol);
}
if (diagnostic && symbol.valueDeclaration) {
addRelatedInfo(diagnostic, createDiagnosticForNode(symbol.valueDeclaration, Diagnostics._0_is_declared_here, symbolToString(symbol)));
}
}
}

// We require the dotted function name in an assertion expression to be comprised of identifiers
// that reference function, method, class or value module symbols; or variable, property or
// parameter symbols with declarations that have explicit type annotations. Such references are
// resolvable with no possibility of triggering circularities in control flow analysis.
function getTypeOfDottedName(node: Expression): Type | undefined {
function getTypeOfDottedName(node: Expression, diagnostic: Diagnostic | undefined): Type | undefined {
if (!(node.flags & NodeFlags.InWithStatement)) {
switch (node.kind) {
case SyntaxKind.Identifier:
const symbol = getResolvedSymbol(<Identifier>node);
return getExplicitTypeOfSymbol(symbol.flags & SymbolFlags.Alias ? resolveAlias(symbol) : symbol);
const symbol = getExportSymbolOfValueSymbolIfExported(getResolvedSymbol(<Identifier>node));
return getExplicitTypeOfSymbol(symbol.flags & SymbolFlags.Alias ? resolveAlias(symbol) : symbol, diagnostic);
case SyntaxKind.ThisKeyword:
return getExplicitThisType(node);
case SyntaxKind.PropertyAccessExpression:
const type = getTypeOfDottedName((<PropertyAccessExpression>node).expression);
const type = getTypeOfDottedName((<PropertyAccessExpression>node).expression, diagnostic);
const prop = type && getPropertyOfType(type, (<PropertyAccessExpression>node).name.escapedText);
return prop && getExplicitTypeOfSymbol(prop);
return prop && getExplicitTypeOfSymbol(prop, diagnostic);
case SyntaxKind.ParenthesizedExpression:
return getTypeOfDottedName((<ParenthesizedExpression>node).expression);
return getTypeOfDottedName((<ParenthesizedExpression>node).expression, diagnostic);
}
}
}
@@ -17227,7 +17235,7 @@ namespace ts {
// expressions are potential type predicate function calls. In order to avoid triggering
// circularities in control flow analysis, we use getTypeOfDottedName when resolving the call
// target expression of an assertion.
const funcType = node.parent.kind === SyntaxKind.ExpressionStatement ? getTypeOfDottedName(node.expression) :
const funcType = node.parent.kind === SyntaxKind.ExpressionStatement ? getTypeOfDottedName(node.expression, /*diagnostic*/ undefined) :
node.expression.kind !== SyntaxKind.SuperKeyword ? checkNonNullExpression(node.expression) :
undefined;
const signatures = getSignaturesOfType(funcType && getApparentType(funcType) || unknownType, SignatureKind.Call);
@@ -23416,6 +23424,16 @@ namespace ts {
if (returnType.flags & TypeFlags.ESSymbolLike && isSymbolOrSymbolForCall(node)) {
return getESSymbolLikeTypeForNode(walkUpParenthesizedExpressions(node.parent));
}
if (node.kind === SyntaxKind.CallExpression && node.parent.kind === SyntaxKind.ExpressionStatement &&
returnType.flags & TypeFlags.Void && getTypePredicateOfSignature(signature)) {
if (!isDottedName(node.expression)) {
error(node.expression, Diagnostics.Assertions_require_the_call_target_to_be_an_identifier_or_qualified_name);
}
else if (!getEffectsSignature(node)) {
const diagnostic = error(node.expression, Diagnostics.Assertions_require_every_name_in_the_call_target_to_be_declared_with_an_explicit_type_annotation);
getTypeOfDottedName(node.expression, diagnostic);
}
}
let jsAssignmentType: Type | undefined;
if (isInJSFile(node)) {
const decl = getDeclarationOfExpando(node);
8 changes: 8 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
@@ -2726,6 +2726,14 @@
"category": "Error",
"code": 2774
},
"Assertions require every name in the call target to be declared with an explicit type annotation.": {
"category": "Error",
"code": 2775
},
"Assertions require the call target to be an identifier or qualified name.": {
"category": "Error",
"code": 2776
},
"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
"code": 4000
6 changes: 6 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
@@ -4062,6 +4062,12 @@ namespace ts {
return node.kind === SyntaxKind.Identifier || isPropertyAccessEntityNameExpression(node);
}

export function isDottedName(node: Expression): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

BTW, will this need to be updated for optional chains? Should a t1?.assert(b) be a thing?

return node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.ThisKeyword ||
node.kind === SyntaxKind.PropertyAccessExpression && isDottedName((<PropertyAccessExpression>node).expression) ||
node.kind === SyntaxKind.ParenthesizedExpression && isDottedName((<ParenthesizedExpression>node).expression);
}

export function isPropertyAccessEntityNameExpression(node: Node): node is PropertyAccessEntityNameExpression {
return isPropertyAccessExpression(node) && isEntityNameExpression(node.expression);
}
24 changes: 23 additions & 1 deletion tests/baselines/reference/assertionTypePredicates1.errors.txt
Original file line number Diff line number Diff line change
@@ -5,9 +5,12 @@ tests/cases/conformance/controlFlow/assertionTypePredicates1.ts(121,15): error T
tests/cases/conformance/controlFlow/assertionTypePredicates1.ts(122,15): error TS1228: A type predicate is only allowed in return type position for functions and methods.
tests/cases/conformance/controlFlow/assertionTypePredicates1.ts(123,15): error TS1228: A type predicate is only allowed in return type position for functions and methods.
tests/cases/conformance/controlFlow/assertionTypePredicates1.ts(124,15): error TS1228: A type predicate is only allowed in return type position for functions and methods.
tests/cases/conformance/controlFlow/assertionTypePredicates1.ts(129,5): error TS2775: Assertions require every name in the call target to be declared with an explicit type annotation.
tests/cases/conformance/controlFlow/assertionTypePredicates1.ts(131,5): error TS2776: Assertions require the call target to be an identifier or qualified name.
tests/cases/conformance/controlFlow/assertionTypePredicates1.ts(133,5): error TS2775: Assertions require every name in the call target to be declared with an explicit type annotation.


==== tests/cases/conformance/controlFlow/assertionTypePredicates1.ts (7 errors) ====
==== tests/cases/conformance/controlFlow/assertionTypePredicates1.ts (10 errors) ====
declare function isString(value: unknown): value is string;
declare function isArrayOfStrings(value: unknown): value is string[];

@@ -147,4 +150,23 @@ tests/cases/conformance/controlFlow/assertionTypePredicates1.ts(124,15): error T
~~~~~~~~~~~~~~~~~~~~~~
!!! error TS1228: A type predicate is only allowed in return type position for functions and methods.
}

function f20(x: unknown) {
const assert = (value: unknown): asserts value => {}
assert(typeof x === "string"); // Error
~~~~~~
!!! error TS2775: Assertions require every name in the call target to be declared with an explicit type annotation.
!!! related TS2728 tests/cases/conformance/controlFlow/assertionTypePredicates1.ts:128:11: 'assert' is declared here.
Copy link
Member

Choose a reason for hiding this comment

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

'{0}' must be annotated would be a lot nicer 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

It would, but do we really want yet another diagnostic?

Copy link
Member

Choose a reason for hiding this comment

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

I know you already merged this, but for the future, diagnostics are cheap! They bear very little overhead for the <20 of us on the core team compared to at least hundreds of thousands of TypeScript users.

Copy link
Member Author

Choose a reason for hiding this comment

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

We discussed in the team room and agreed that the existing message is fine, so I didn't change it.

const a = [assert];
a[0](typeof x === "string"); // Error
~~~~
!!! error TS2776: Assertions require the call target to be an identifier or qualified name.
const t1 = new Test();
t1.assert(typeof x === "string"); // Error
~~~~~~~~~
!!! error TS2775: Assertions require every name in the call target to be declared with an explicit type annotation.
!!! related TS2728 tests/cases/conformance/controlFlow/assertionTypePredicates1.ts:132:11: 't1' is declared here.
const t2: Test = new Test();
t2.assert(typeof x === "string");
}

22 changes: 22 additions & 0 deletions tests/baselines/reference/assertionTypePredicates1.js
Original file line number Diff line number Diff line change
@@ -124,6 +124,17 @@ declare class Wat {
get p2(): asserts this is string;
set p2(x: asserts this is string);
}

function f20(x: unknown) {
const assert = (value: unknown): asserts value => {}
assert(typeof x === "string"); // Error
const a = [assert];
a[0](typeof x === "string"); // Error
const t1 = new Test();
t1.assert(typeof x === "string"); // Error
const t2: Test = new Test();
t2.assert(typeof x === "string");
}


//// [assertionTypePredicates1.js]
@@ -250,6 +261,16 @@ var Test2 = /** @class */ (function (_super) {
}
return Test2;
}(Test));
function f20(x) {
var assert = function (value) { };
assert(typeof x === "string"); // Error
var a = [assert];
a[0](typeof x === "string"); // Error
var t1 = new Test();
t1.assert(typeof x === "string"); // Error
var t2 = new Test();
t2.assert(typeof x === "string");
}


//// [assertionTypePredicates1.d.ts]
@@ -287,3 +308,4 @@ declare class Wat {
get p2(): asserts this is string;
set p2(x: asserts this is string);
}
declare function f20(x: unknown): void;
43 changes: 43 additions & 0 deletions tests/baselines/reference/assertionTypePredicates1.symbols
Original file line number Diff line number Diff line change
@@ -357,3 +357,46 @@ declare class Wat {
>x : Symbol(x, Decl(assertionTypePredicates1.ts, 123, 11))
}

function f20(x: unknown) {
>f20 : Symbol(f20, Decl(assertionTypePredicates1.ts, 124, 1))
>x : Symbol(x, Decl(assertionTypePredicates1.ts, 126, 13))

const assert = (value: unknown): asserts value => {}
>assert : Symbol(assert, Decl(assertionTypePredicates1.ts, 127, 9))
>value : Symbol(value, Decl(assertionTypePredicates1.ts, 127, 20))
>value : Symbol(value, Decl(assertionTypePredicates1.ts, 127, 20))

assert(typeof x === "string"); // Error
>assert : Symbol(assert, Decl(assertionTypePredicates1.ts, 127, 9))
>x : Symbol(x, Decl(assertionTypePredicates1.ts, 126, 13))

const a = [assert];
>a : Symbol(a, Decl(assertionTypePredicates1.ts, 129, 9))
>assert : Symbol(assert, Decl(assertionTypePredicates1.ts, 127, 9))

a[0](typeof x === "string"); // Error
>a : Symbol(a, Decl(assertionTypePredicates1.ts, 129, 9))
>x : Symbol(x, Decl(assertionTypePredicates1.ts, 126, 13))

const t1 = new Test();
>t1 : Symbol(t1, Decl(assertionTypePredicates1.ts, 131, 9))
>Test : Symbol(Test, Decl(assertionTypePredicates1.ts, 76, 1))

t1.assert(typeof x === "string"); // Error
>t1.assert : Symbol(Test.assert, Decl(assertionTypePredicates1.ts, 78, 12))
>t1 : Symbol(t1, Decl(assertionTypePredicates1.ts, 131, 9))
>assert : Symbol(Test.assert, Decl(assertionTypePredicates1.ts, 78, 12))
>x : Symbol(x, Decl(assertionTypePredicates1.ts, 126, 13))

const t2: Test = new Test();
>t2 : Symbol(t2, Decl(assertionTypePredicates1.ts, 133, 9))
>Test : Symbol(Test, Decl(assertionTypePredicates1.ts, 76, 1))
>Test : Symbol(Test, Decl(assertionTypePredicates1.ts, 76, 1))

t2.assert(typeof x === "string");
>t2.assert : Symbol(Test.assert, Decl(assertionTypePredicates1.ts, 78, 12))
>t2 : Symbol(t2, Decl(assertionTypePredicates1.ts, 133, 9))
>assert : Symbol(Test.assert, Decl(assertionTypePredicates1.ts, 78, 12))
>x : Symbol(x, Decl(assertionTypePredicates1.ts, 126, 13))
}

63 changes: 63 additions & 0 deletions tests/baselines/reference/assertionTypePredicates1.types
Original file line number Diff line number Diff line change
@@ -436,3 +436,66 @@ declare class Wat {
>x : void
}

function f20(x: unknown) {
>f20 : (x: unknown) => void
>x : unknown

const assert = (value: unknown): asserts value => {}
>assert : (value: unknown) => asserts value
>(value: unknown): asserts value => {} : (value: unknown) => asserts value
>value : unknown

assert(typeof x === "string"); // Error
>assert(typeof x === "string") : void
>assert : (value: unknown) => asserts value
>typeof x === "string" : boolean
>typeof x : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"
>x : unknown
>"string" : "string"

const a = [assert];
>a : ((value: unknown) => asserts value)[]
>[assert] : ((value: unknown) => asserts value)[]
>assert : (value: unknown) => asserts value

a[0](typeof x === "string"); // Error
>a[0](typeof x === "string") : void
>a[0] : (value: unknown) => asserts value
>a : ((value: unknown) => asserts value)[]
>0 : 0
>typeof x === "string" : boolean
>typeof x : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"
>x : unknown
>"string" : "string"

const t1 = new Test();
>t1 : Test
>new Test() : Test
>Test : typeof Test

t1.assert(typeof x === "string"); // Error
>t1.assert(typeof x === "string") : void
>t1.assert : (value: unknown) => asserts value
>t1 : Test
>assert : (value: unknown) => asserts value
>typeof x === "string" : boolean
>typeof x : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"
>x : unknown
>"string" : "string"

const t2: Test = new Test();
>t2 : Test
>new Test() : Test
>Test : typeof Test

t2.assert(typeof x === "string");
>t2.assert(typeof x === "string") : void
>t2.assert : (value: unknown) => asserts value
>t2 : Test
>assert : (value: unknown) => asserts value
>typeof x === "string" : boolean
>typeof x : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"
>x : unknown
>"string" : "string"
}

8 changes: 8 additions & 0 deletions tests/baselines/reference/neverReturningFunctions1.errors.txt
Original file line number Diff line number Diff line change
@@ -225,6 +225,14 @@ tests/cases/conformance/controlFlow/neverReturningFunctions1.ts(153,5): error TS
!!! error TS7027: Unreachable code detected.
}

function f43() {
const fail = (): never => { throw new Error(); };
const f = [fail];
fail(); // No effect (missing type annotation)
f[0](); // No effect (not a dotted name)
f;
}

// Repro from #33582

export interface Component<T extends object = any> {
15 changes: 15 additions & 0 deletions tests/baselines/reference/neverReturningFunctions1.js
Original file line number Diff line number Diff line change
@@ -154,6 +154,14 @@ function f42(x: number) {
x; // Unreachable
}

function f43() {
const fail = (): never => { throw new Error(); };
const f = [fail];
fail(); // No effect (missing type annotation)
f[0](); // No effect (not a dotted name)
f;
}

// Repro from #33582

export interface Component<T extends object = any> {
@@ -375,6 +383,13 @@ function f42(x) {
}
x; // Unreachable
}
function f43() {
var fail = function () { throw new Error(); };
var f = [fail];
fail(); // No effect (missing type annotation)
f[0](); // No effect (not a dotted name)
f;
}
var Component = registerComponent('test-component', {
schema: {
myProperty: {
Loading