Repository navigation
CFA Assertions Not Active on Implicitly-Typed Function Calls #33580
Description
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.
Activity
jack-williams commentedon Sep 24, 2019
I think this is working as intended, and the issue is that you are missing explicit type annotations.
This makes the error go away:
I will just put this here to annoy people.
molisani commentedon Sep 24, 2019
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 commentedon Sep 24, 2019
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 commentedon Sep 24, 2019
Alright, thanks for the quick and informative response Jack Williams (@jack-williams) and Ryan Cavanaugh (@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.
[-]CFA Assertions Fail on Externally-Invoked Class Methods[/-][+]CFA Assertions Not Active on Implicitly-Typed Function Calls[/+]ahejlsberg commentedon Sep 25, 2019
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 commentedon Sep 26, 2019
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 commentedon Sep 26, 2019
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 commentedon Sep 26, 2019
Ok, with two issues reported in short order I'm convinced this merits an error as suggested by Jack Williams (@jack-williams). I'm changing this back to a bug.
ahejlsberg commentedon Sep 26, 2019
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 commentedon Sep 27, 2019
Makes sense - I overlooked the general changes you made to reachability analysis beyond
assert.