Control flow based type guards

[ivogabe]

As discussed in #5168, here's my PR that adds flow based type guards. This is still work in process. The compiler can compile itself, but one of the tests doesn't terminate.

Related issues: #1764, #1769, #2388

Here's a quick overview of the approach I took. During binding, every node gets references to flow markers that preceded the node in the control flow. A flow marker is either a type guard or some node, such as an identifier or a while loop. This represents the control flow graph.

In checker.ts, the type of a variable is calculated as the union of the types after the previous occurrences. The type after a node is in case of an assignment narrowed by the assigned type (see below) and otherwise the same as before the node.

At an assignment x = y, where y can be any expression, the following happens. If the type of x is a union type, the constituent parts are filtered. The local type of x will be the union of all constituent parts to which y is assignable. If this yields an empty union type, the initial type of x is used instead. If x is not a union type, the local type after the assignment will be the initial type.

Since the incoming flow of a catch block can come from anywhere in the try block, I decided to fall back to the initial type as a precaution. I can relax that rule if desired, for instance by looking at assignments in the try block.

Todo:

• Fix test where compiler doesn't terminate
• Add tests
• Clean up & comments
• Fix bug: let x: string | number = "" doesn't narrow to string, but x = "" does.
• Fix bug: with x = ""; for (;x;) { x; }, narrowing is removed inside the for (because of recursion), but x = ""; for(;;) { x; } works as expected.

~~Questions:

1. Not every Node has an id property (at runtime it is undefined). Is that correct or a bug? I currently use flowIndex (which I will remove later on) in getPreviousOccurencies, but id would be better.
2. One test currently doesn't terminate. The process aborts because it's out of memory (even when I increase the old space to 8 GB). Given that the call stack contains createFileDiagnostic, I think the compiler is trying to create an infinite amount of diagnostics.. Does someone have an idea why that's happening?~~

[weswigham]

1. Not every Node has an id property (at runtime it is undefined). Is that correct or a bug? I currently use flowIndex (which I will remove later on) in getPreviousOccurencies, but id would be better.

Use the function getNodeId to get node id's - if the node has yet to be assigned one, this function assigns it one.

[weswigham]

I don't see this brand applied anywhere - is there a reason for this?

[ivogabe]

If a type guard returns false, it is expected that the argument is not of the specified type. However, isDeclarationName can return false when name is an identifier. This wasn't an issue, as isDeclarationName was never used in an if statement with an else. However, with control flow based type guards, this wasn't working anymore. The then-block of an if statement contained a return, thus the statements after the if were the else branch of the if. It might not be the best way to solve this, but it's working..

[weswigham]

After a bit of sniffing with your code, I think the (or one of the?) test(s) which hangs is tests/cases/conformance/parser/ecmascript5/parserRealSource7.ts (which I think is a chunk of code form the original compiler). The preCollectFuncDeclTypes function within the file is enough (on its own) to make the test hit the memory ceiling. It seems to spend a lot of cycles re-narrowing the ast cast-and-assignment-to-funcDecl in that function (and in some other places, too).

[weswigham]

This type isn't used anywhere, AFAIK.

[ivogabe]

You're right, I forgot to remove it during refactoring.

[ivogabe]

It took a while, but I think most things are working now. I have updated the PR for the dotted names type guards and this type guards. null and undefined worked without extra work. I haven't used the control flow graph for variable assignment checking, but that can be easily added later on.

I think that two minor problems: handling of a try/catch block and getTypeOfSymbolAtLocation. After a try/catch, narrowing breaks and types of variables are set to their initial types. Better would be to use the type before the try if the variable is not assigned within the try, otherwise the initial type; or to take the union of the type before the try with all types of assignments in the try/catch.

The following code compiles, but gives the wrong type in VSCode, with this custom build. When I hover the last x, it says string | number. I suspect this has something to do with getTypeOfSymbolAtLocation, but I'm not sure what's happening. This seems to fail with double type guards. The tests show this too, in tests/baselines/reference/typeGuardsInIfStatement.types.

let x: string | boolean | number;
if (typeof x === "string") {}
else if (typeof x === "boolean") {}
else x.toExponential;

Since most work has been finished, can someone review my code?

[jods4]

For try...catch and try..finally it's very useful to keep the narrowed type of symbols that do not change type in the block, including those that are assigned, so that the following works:

let x: number | null;
x = 4; // from here x: number
try {
  x = 7;
  throw new Error();
} 
finally {
  x.toString(); // <-- should be ok, it would be annoying if x reverts to number|null
}

For symbols, which change type inside the block I am unsure whether it's worth the effort. The union of all their types across the block would be correct but it can get hard to reason about, for IMHO little benefit. I think reverting to their initial type would be OK for a first release, it can always be improved later if people have motivating examples of code where it would be nice.

[mhegazy]

this functionality should be handled by #8010.

Big thanks to @ivogabe for sharing this work. it has been the inspiration and guide for the work in #8010. thanks again @ivogabe!