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

ahejlsberg · 2019-09-26T21:17:31Z

With this PR we issue errors when calls to assertion functions are written in a manner that isn't recognized by control flow analysis (see #32695 for the specific requirements). For example:

class Test {
    assert(value: unknown): asserts value {}
}

function f20(x: unknown) {
    const assert1 = (value: unknown): asserts value => {}
    assert1(typeof x === "string");  // Error
    const assert2: (value: unknown) => asserts value = () => {};
    assert2(typeof x === "string");  // Ok
    const t1 = new Test();
    t1.assert(typeof x === "string");  // Error
    const t2: Test = new Test();
    t2.assert(typeof x === "string");  // Ok
    const getAssert = () => assert1;
    getAssert()(typeof x === "string");  // Error
}

The reported errors are:

t.ts(7,5): error TS2775: Assertions require every name in the call target to be declared with an explicit type annotation.

t.ts(11,5): error TS2775: Assertions require every name in the call target to be declared with an explicit type annotation.

t.ts(15,5): error TS2776: Assertions require the call target to be an identifier or qualified name.

Fixes #33580.

ahejlsberg · 2019-09-27T01:12:24Z

@typescript-bot run dt
@typescript-bot test this

typescript-bot · 2019-09-27T01:12:29Z

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at a13f862. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-09-27T01:12:29Z

Heya @ahejlsberg, I've started to run the extended test suite on this PR at a13f862. You can monitor the build here. It should now contribute to this PR's status checks.

DanielRosenwasser · 2019-09-27T07:51:32Z

@typescript-bot user test this
@typescript-bot pack this

typescript-bot · 2019-09-27T07:51:36Z

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at a13f862. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-09-27T07:51:36Z

Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at a13f862. You can monitor the build here. It should now contribute to this PR's status checks.

DanielRosenwasser

I think these error messages are very confusing, and we need to do as much as we can to keep it short and make it actionable.

DanielRosenwasser · 2019-09-27T07:39:09Z

src/compiler/diagnosticMessages.json

@@ -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.": {


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.

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.

Mentioning control flow isn't necessary for fixing the error -- and presumably most people understand the concept of assertion better than control flow anyway.

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.

We know that the expression has type annotations because hasTypePredicateOrNeverReturnType(signature) is true.

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. : :

DanielRosenwasser · 2019-09-27T07:42:00Z

tests/baselines/reference/assertionTypePredicates1.errors.txt

+        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.


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

typescript-bot · 2019-09-27T08:21:40Z

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

jack-williams · 2019-09-27T12:13:38Z

A couple of candidate messages.

assert1(typeof x === "string");  // Error
// Assertion calls must be explicitly typed.
// Identifier 'assert1' lacks an explicit type annotation and is not subject to control flow analysis.

getAssert()(typeof x === "string");  // Error
// Assertion calls must be explicitly resolved.
// Function expression is not an identifier or qualified-name and is not subject to control flow analysis.

sandersn · 2019-09-27T16:24:43Z

src/compiler/diagnosticMessages.json

@@ -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.": {


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.

Mentioning control flow isn't necessary for fixing the error -- and presumably most people understand the concept of assertion better than control flow anyway.

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.

We know that the expression has type annotations because hasTypePredicateOrNeverReturnType(signature) is true.

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. : :

sandersn · 2019-09-27T16:27:09Z

Oh, I didn't read @jack-williams comments until after commenting. Basically, what he said. =)

DanielRosenwasser · 2019-09-27T17:16:50Z

Mentioning control flow isn't necessary for fixing the error

Agreed!

"This assertion call" is implied by the error span.

Yes and no - I find it easier to just have the error message tell me what's wrong rather than reconstructing the intent from the span as well.

Wording this categorically also allows us to flip the message back to positive.

The suggestions both of you have are much more digestible, and made it clear to me exactly what the error check is. I only have nits at this point. :)

What do you think of:

Assertion functions can only be called when they were declared with an explicit type annotation.
Related: This is the declaration that must be annotated.

and

Assertion functions can only be called when referenced by an identifier or qualified name.

?

sandersn · 2019-09-27T17:26:10Z

No, I like the shorter wording better. The longer message only improves the precision of the wording, but won't help people (especially naive people) track down the problem.

"A declaration with an explicit type annotation" is shorter and therefore clearer than "When they [functions] were declared with an explicit type annotation", and much more flexible because it's a noun phrase. Just use it as the object of the sentence!
I guess that you ended up with "Assertion functions" because of "can only be called", which you got because of "when they were declared ...". Whether that reconstruction is right or not, it makes it sound like "an assertion function" could be different from "an assertion", but we don't actually have those two concepts. So we might as well call the one concept we do have "an assertion".

…functions

DanielRosenwasser · 2019-09-27T22:27:22Z

tests/baselines/reference/assertionTypePredicates1.errors.txt

+        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.


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

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

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.

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

ahejlsberg · 2019-09-27T22:27:57Z

Latest commits exclude never-returning functions from check that they match CFA requirements and improve error messages, per discussion in today's design meeting.

Error messages now improved

weswigham · 2019-09-27T23:31:18Z

src/compiler/utilities.ts

@@ -4062,6 +4062,12 @@ namespace ts {
        return node.kind === SyntaxKind.Identifier || isPropertyAccessEntityNameExpression(node);
    }

+    export function isDottedName(node: Expression): boolean {


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

mgcrea · 2020-01-16T19:14:51Z

Strangely, with 3.8-beta (or 3.7-stable), this will trigger the Assertions require every name in the call target to be declared with an explicit type annotation. ts(2775) error:

export const assert = (value: unknown, message?: string): asserts value => {
  if (!value) {
    throw new AssertionError({message});
  }
};

While this does work properly:

export const assert: (value: unknown, message?: string) => asserts value = (value: unknown, message?: string) => {
  if (!value) {
    throw new AssertionError({message});
  }
};

Is this working as intended?

ahejlsberg added 3 commits September 26, 2019 13:51

Error on assertion and non-returning function calls that aren't CFA-ed

8619bff

Add tests

894cc63

Accept new baselines

0595d61

ahejlsberg requested review from RyanCavanaugh and sandersn and removed request for RyanCavanaugh September 26, 2019 21:18

Handle alias symbols in getTypeOfDottedName

a13f862

DanielRosenwasser reviewed Sep 27, 2019

View reviewed changes

typescript-bot mentioned this pull request Sep 27, 2019

🤖 User test baselines have changed for fix33580 #33632

Closed

sandersn previously requested changes Sep 27, 2019

View reviewed changes

ahejlsberg added 5 commits September 27, 2019 15:05

Revise error messages + related spans + no errors on never-returning …

4648d6a

…functions

Fix error message

02395a9

Accept new baselines

56e0c6b

Update tests

34f6e68

Accept new baselines

e5af71e

DanielRosenwasser reviewed Sep 27, 2019

View reviewed changes

ahejlsberg requested a review from sandersn September 27, 2019 22:44

weswigham reviewed Sep 27, 2019

View reviewed changes

ahejlsberg changed the title ~~Error when assertion and never-returning function calls aren't CFA'd~~ Error when assertion function calls aren't CFA'd Sep 28, 2019

RyanCavanaugh added this to the TypeScript 3.7.0 milestone Sep 30, 2019

RyanCavanaugh approved these changes Sep 30, 2019

View reviewed changes

ahejlsberg merged commit ff5d38a into master Sep 30, 2019

jcalz mentioned this pull request Oct 17, 2019

asserts x is type doesn't work when using arrow functions #34523

Closed

DanielRosenwasser mentioned this pull request Oct 18, 2019

Design Meeting Notes, 9/27/2019 #34573

Closed

jcalz mentioned this pull request Oct 20, 2019

TS 3.7: unlike x is T, asserts x is T cannot close over generics defined in outer scopes #34596

Open

targos mentioned this pull request Dec 19, 2019

fix(response): use never return type for abort method adonisjs/http-server#13

Merged

mlhaufe mentioned this pull request Feb 21, 2020

Assertion functions don't work when defined as methods #36931

Closed

earthlyreason mentioned this pull request Feb 23, 2020

Use TypeScript assertion function signatures avajs/ava#2411

Closed

DanielRosenwasser mentioned this pull request Apr 2, 2020

I hope the type checker to report an error when I call a function which return never. #37747

Closed

jsoverson mentioned this pull request May 18, 2020

added node type, exported expression and statement types shapesecurity/shift-ast-js#59

Merged

DanielRosenwasser deleted the fix33580 branch July 30, 2020 21:58

DanielRosenwasser mentioned this pull request Jul 30, 2020

Methods that always throw aren't always recognized as a return from a function #39839

Closed

DanielRosenwasser mentioned this pull request Jul 26, 2021

Allow narrowing for any left-hand in operand #45152

Merged

andrewbranch mentioned this pull request Sep 2, 2021

Type narrowing not work with inferred function arguments #45671

Closed

jcalz mentioned this pull request Sep 21, 2021

Feature Request/Question: Lift the explicit type requirement in assertions for participation in CFA #45385

Open

5 tasks

ZachHaber mentioned this pull request Aug 16, 2023

Make assertions throw avajs/ava#3201

Closed

acuarica mentioned this pull request Dec 30, 2023

Support return type assertions for steps acuarica/evm#60

Open

danvk mentioned this pull request May 13, 2024

Infer AssertsIdentifier type predicates #58495

Open

jcalz mentioned this pull request Aug 21, 2024

Unions of assertion functions do not act as assertion functions #59707

Open

ruanyf mentioned this pull request Sep 23, 2024

断言函数用于函数表达式时，ts 报错， wangdoc/typescript-tutorial#119

Closed

jcalz mentioned this pull request Oct 4, 2024

Assertion signature on generics doesn't narrow #60130

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ahejlsberg commented Sep 26, 2019 •

edited

Loading

ahejlsberg commented Sep 27, 2019

typescript-bot commented Sep 27, 2019 •

edited

Loading

typescript-bot commented Sep 27, 2019 •

edited

Loading

DanielRosenwasser commented Sep 27, 2019

typescript-bot commented Sep 27, 2019 •

edited

Loading

typescript-bot commented Sep 27, 2019 •

edited

Loading

DanielRosenwasser left a comment

DanielRosenwasser Sep 27, 2019

sandersn Sep 27, 2019

DanielRosenwasser Sep 27, 2019

typescript-bot commented Sep 27, 2019

jack-williams commented Sep 27, 2019

sandersn Sep 27, 2019

sandersn commented Sep 27, 2019

DanielRosenwasser commented Sep 27, 2019

sandersn commented Sep 27, 2019

DanielRosenwasser Sep 27, 2019

ahejlsberg Sep 27, 2019

DanielRosenwasser Oct 1, 2019

ahejlsberg Oct 1, 2019

ahejlsberg commented Sep 27, 2019

weswigham Sep 27, 2019

mgcrea commented Jan 16, 2020 •

edited

Loading

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

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

Conversation

ahejlsberg commented Sep 26, 2019 • edited Loading

ahejlsberg commented Sep 27, 2019

typescript-bot commented Sep 27, 2019 • edited Loading

typescript-bot commented Sep 27, 2019 • edited Loading

DanielRosenwasser commented Sep 27, 2019

typescript-bot commented Sep 27, 2019 • edited Loading

typescript-bot commented Sep 27, 2019 • edited Loading

DanielRosenwasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Sep 27, 2019

jack-williams commented Sep 27, 2019

Choose a reason for hiding this comment

sandersn commented Sep 27, 2019

DanielRosenwasser commented Sep 27, 2019

sandersn commented Sep 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahejlsberg commented Sep 27, 2019

Choose a reason for hiding this comment

mgcrea commented Jan 16, 2020 • edited Loading

ahejlsberg commented Sep 26, 2019 •

edited

Loading

typescript-bot commented Sep 27, 2019 •

edited

Loading

typescript-bot commented Sep 27, 2019 •

edited

Loading

typescript-bot commented Sep 27, 2019 •

edited

Loading

typescript-bot commented Sep 27, 2019 •

edited

Loading

mgcrea commented Jan 16, 2020 •

edited

Loading