CFA Assertions Not Active on Implicitly-Typed Function Calls

[molisani]

TypeScript Version: 3.7.0-insiders.20190923 (this tgz from this comment)

Search Terms: asserts cfa control flow analysis

Code

class Foo {
}

interface BarChild {
    parent: Bar;
}

class Bar {
    static staticAdd(f: Foo): asserts f is Foo & BarChild {}
    add(f: Foo): asserts f is Foo & BarChild {}
    innerTest() {
        const innerFoo = new Foo();
        this.add(innerFoo);
        innerFoo.parent;  // this compiles successfully
    }
}

function main() {
    const foo1 = new Foo();
    const bar1 = new Bar();
    bar1.add(foo1);
    foo1.parent;  // this is an ERROR - Property 'parent' does not exist on type 'Foo'. ts(2339)

    const foo2 = new Foo();
    Bar.staticAdd(foo2);
    foo2.parent;  // this compiles successfully
}

The Bar.add assertion works when called inside another method of Bar but fails when called externally from main. I added a static function assertion to show that assertion still works otherwise in main.

Expected behavior: In main after bar1.add(foo1), foo1 should be of type Foo & BarChild.

Actual behavior: In main after bar1.add(foo1), foo1 is still type Foo.

Playground Link: I've duplicated the sample code above in this playground link, but at the time of posting the nightly version does not have these changes.

Related Issues: #32695 This feature was merged to master yesterday, so instead of commenting on a closed PR I opened a new issue.

[jack-williams]

I think this is working as intended, and the issue is that you are missing explicit type annotations.

A function call is analyzed as an assertion call or never-returning call when ….
each identifier in the function name references an entity with an explicit type, and

This makes the error go away:

const bar1: Bar = new Bar();

^{I will just put this here to annoy people.}

[molisani]

Ah, thanks for the link to the comment. That explains what is going on.

Was it a specific decision to require explicit type annotations to "activate" assertions, or is it a technical restriction? Regardless, it would be nice to not have to add otherwise-unnecessary type annotations to get this to work.

[RyanCavanaugh]

It's a technical restriction, otherwise control flow would have to do an unbounded number of passes to determine if a given function ever returned

[molisani]

Alright, thanks for the quick and informative response @jack-williams and @RyanCavanaugh. While I do agree with Jack's comment on the PR that this will catch a lot of people, feel free to handle this issue however you see fit.

[ahejlsberg]

I'm going to mark this working as intended, although we should probably discuss whether to issue errors when assertion functions and non-returning functions are used in a manner that doesn't get picked up by control flow analysis. See #32695 (comment).

[jcalz]

Yeah, an error with a corresponding annotation quick-fix would hopefully mitigate this? Or something to stem the expected tide of duplicates of this issue being filed and the analogous SO questions.

[jack-williams]

My concern with an error would be libraries upgrading to use assertions and clients breaking because they didn't use a type annotation on previously OK code.

[ahejlsberg]

Ok, with two issues reported in short order I'm convinced this merits an error as suggested by @jack-williams. I'm changing this back to a bug.

[ahejlsberg]

My concern with an error would be libraries upgrading to use assertions and clients breaking because they didn't use a type annotation on previously OK code.

I'm not concerned with issuing errors on non-CFA-recognized assertion function calls since assertions are new feature and no existing code uses them.

The fact that we now report reachability errors following calls to never-returning functions is already a breaking change, so also issuing errors when such functions are called in a manner that CFA doesn't recognized doesn't materially change the situation.

[jack-williams]

Makes sense - I overlooked the general changes you made to reachability analysis beyond assert.