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.

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
merged 9 commits into from
Sep 30, 2019
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8619bff
Error on assertion and non-returning function calls that aren't CFA-ed
ahejlsberg Sep 26, 2019
894cc63
Add tests
ahejlsberg Sep 26, 2019
0595d61
Accept new baselines
ahejlsberg Sep 26, 2019
a13f862
Handle alias symbols in getTypeOfDottedName
ahejlsberg Sep 27, 2019
4648d6a
Revise error messages + related spans + no errors on never-returning …
ahejlsberg Sep 27, 2019
02395a9
Fix error message
ahejlsberg Sep 27, 2019
56e0c6b
Accept new baselines
ahejlsberg Sep 27, 2019
34f6e68
Update tests
ahejlsberg Sep 27, 2019
e5af71e
Accept new baselines
ahejlsberg Sep 27, 2019
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 10 additions & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17205,7 +17205,7 @@ namespace ts {
if (!(node.flags & NodeFlags.InWithStatement)) {
switch (node.kind) {
case SyntaxKind.Identifier:
const symbol = getResolvedSymbol(<Identifier>node);
const symbol = getExportSymbolOfValueSymbolIfExported(getResolvedSymbol(<Identifier>node));
return getExplicitTypeOfSymbol(symbol.flags & SymbolFlags.Alias ? resolveAlias(symbol) : symbol);
case SyntaxKind.ThisKeyword:
return getExplicitThisType(node);
Expand Down Expand Up @@ -23416,6 +23416,15 @@ 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 | TypeFlags.Never) && hasTypePredicateOrNeverReturnType(signature)) {
if (!isDottedName(node.expression)) {
error(node.expression, Diagnostics.Control_flow_effects_of_calls_to_assertion_and_never_returning_functions_are_reflected_only_when_the_function_expression_is_an_identifier_or_qualified_name);
}
else if (!getEffectsSignature(node)) {
error(node.expression, Diagnostics.Control_flow_effects_of_calls_to_assertion_and_never_returning_functions_are_reflected_only_when_every_variable_or_property_referenced_in_the_function_expression_is_declared_with_an_explicit_type_annotation);
}
}
let jsAssignmentType: Type | undefined;
if (isInJSFile(node)) {
const decl = getDeclarationOfExpando(node);
Expand Down
8 changes: 8 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2726,6 +2726,14 @@
"category": "Error",
"code": 2774
},
"Control flow effects of calls to assertion and never-returning functions are reflected only when every variable or property referenced in the function expression is declared with an explicit type annotation.": {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what this is telling a user. Is it something like the following?

This assertion call cannot be properly analyzed because not every variable and property used in the current function has an explicit type annotation.

This 'never'-returning function call cannot be properly analyzed because not every variable and property used in the current function has an explicit type annotation.

Along with

This assertion call cannot be properly analyzed because the function being called is not a qualified name.

This 'never'-returning function call cannot be properly analyzed because the function being called is not a qualified name.

Copy link
Member

Choose a reason for hiding this comment

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

  1. I think that "explicitly never-returning function" is really just another kind of assertion. We could introduce the concept of guard = "explicitly never-returning function" if it's too ambiguous.
  2. Mentioning control flow isn't necessary for fixing the error -- and presumably most people understand the concept of assertion better than control flow anyway.
  3. As for @DanielRosenwasser's wording: "This assertion call" is implied by the error span. The fact that it's an assertion is implied by the error span+error wording. So neither of those is necessary. Wording this categorically also allows us to flip the message back to positive.
Assertions must be an identifier or qualified name.

Assertions must reference a declaration with an explicit type annotation.
  Related: This is the declaration that should be annotated.
  1. We know that the expression has type annotations because hasTypePredicateOrNeverReturnType(signature) is true.
  2. If we want to handle the case where there is more than one declaration, the message can be
Assertions must reference declaration(s) with an explicit type annotation.
  Related: This is one declaration that must be annotated.
  Related: This is one declaration that must be annotated.
  :  
  :

"category": "Error",
"code": 2775
},
"Control flow effects of calls to assertion and never-returning functions are reflected only when the function expression is an identifier or qualified-name.": {
"category": "Error",
"code": 2776
},
"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
"code": 4000
Expand Down
6 changes: 6 additions & 0 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
22 changes: 21 additions & 1 deletion tests/baselines/reference/assertionTypePredicates1.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: Control flow effects of calls to assertion and never-returning functions are reflected only when every variable or property referenced in the function expression is declared with an explicit type annotation.
tests/cases/conformance/controlFlow/assertionTypePredicates1.ts(131,5): error TS2776: Control flow effects of calls to assertion and never-returning functions are reflected only when the function expression is an identifier or qualified-name.
tests/cases/conformance/controlFlow/assertionTypePredicates1.ts(133,5): error TS2775: Control flow effects of calls to assertion and never-returning functions are reflected only when every variable or property referenced in the function expression is 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[];

Expand Down Expand Up @@ -147,4 +150,21 @@ 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: Control flow effects of calls to assertion and never-returning functions are reflected only when every variable or property referenced in the function expression is declared with an explicit type annotation.
Copy link
Member

Choose a reason for hiding this comment

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

Which variable doesn't have an annotation? Can you give a related span?

const a = [assert];
a[0](typeof x === "string"); // Error
~~~~
!!! error TS2776: Control flow effects of calls to assertion and never-returning functions are reflected only when the function expression is an identifier or qualified-name.
const t1 = new Test();
t1.assert(typeof x === "string"); // Error
~~~~~~~~~
!!! error TS2775: Control flow effects of calls to assertion and never-returning functions are reflected only when every variable or property referenced in the function expression is declared with an explicit type annotation.
const t2: Test = new Test();
t2.assert(typeof x === "string");
}

22 changes: 22 additions & 0 deletions tests/baselines/reference/assertionTypePredicates1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}

15 changes: 14 additions & 1 deletion tests/baselines/reference/neverReturningFunctions1.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ tests/cases/conformance/controlFlow/neverReturningFunctions1.ts(139,9): error TS
tests/cases/conformance/controlFlow/neverReturningFunctions1.ts(141,5): error TS7027: Unreachable code detected.
tests/cases/conformance/controlFlow/neverReturningFunctions1.ts(148,9): error TS7027: Unreachable code detected.
tests/cases/conformance/controlFlow/neverReturningFunctions1.ts(153,5): error TS7027: Unreachable code detected.
tests/cases/conformance/controlFlow/neverReturningFunctions1.ts(159,5): error TS2775: Control flow effects of calls to assertion and never-returning functions are reflected only when every variable or property referenced in the function expression is declared with an explicit type annotation.
tests/cases/conformance/controlFlow/neverReturningFunctions1.ts(160,5): error TS2776: Control flow effects of calls to assertion and never-returning functions are reflected only when the function expression is an identifier or qualified-name.


==== tests/cases/conformance/controlFlow/neverReturningFunctions1.ts (23 errors) ====
==== tests/cases/conformance/controlFlow/neverReturningFunctions1.ts (25 errors) ====
function fail(message?: string): never {
throw new Error(message);
}
Expand Down Expand Up @@ -225,6 +227,17 @@ 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(); // Error
~~~~
!!! error TS2775: Control flow effects of calls to assertion and never-returning functions are reflected only when every variable or property referenced in the function expression is declared with an explicit type annotation.
f[0](); // Error
~~~~
!!! error TS2776: Control flow effects of calls to assertion and never-returning functions are reflected only when the function expression is an identifier or qualified-name.
}

// Repro from #33582

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

function f43() {
const fail = (): never => { throw new Error(); };
const f = [fail];
fail(); // Error
f[0](); // Error
}

// Repro from #33582

export interface Component<T extends object = any> {
Expand Down Expand Up @@ -375,6 +382,12 @@ function f42(x) {
}
x; // Unreachable
}
function f43() {
var fail = function () { throw new Error(); };
var f = [fail];
fail(); // Error
f[0](); // Error
}
var Component = registerComponent('test-component', {
schema: {
myProperty: {
Expand Down
Loading