initial revision of reachability checks

[vladima]

this PR supersedes #1287.
Checking unreachable code in the compiler uncovered 2 bugs, both of the same origin:

function foo() {
    return bar; / at this point x cannot be used yet because of TDZ
    let x: number; 
    function bar() {
        if (!x) x = 0;
        return ++x;
    }
}

Currently reachability checks are placed in binder but ultimately I'd like to be separate actions that are executed on the same pass - split walking and processing logic

[basarat]

fancy 😎

[DanielRosenwasser]

Space after switch

[yuit]

use the parameter name 'node' instead

[vladima]

ok

[yuit]

I don't understand reachability of switch-statement. Why being exhaustive prevent post switch to be reachable?

[vladima]

discussed offline

[yuit]

🍪 🍰

[jbondc]

🍺 is it enabled by default? impact on performance?

[vladima]

@ahejlsberg I've addressed your comments, can you please take another look to see if everything looks good from your perspective?

[DanielRosenwasser]

@vladima noImplicitReturns doesn't seem to catch the following function:

function foo(x: number) {
    if (x < 0) return 42;

}

[vladima]

@DanielRosenwasser noImplicitReturns is enabled if return type is specified

function foo(x: number): number {
    if (x < 0) return 42;

}

v2m@v2m-ThinkPad-W520:~/sources/TypeScript$ node built/local/tsc.js test.ts --noImplicitReturns
test.ts(1,26): error TS7030: Not all code paths return a value.

[DanielRosenwasser]

@vladima yes, I wasn't sure if that was intentional or not. I think that's a little too subtle. I would assume you are more likely to forget a type annotation as you are an explicit return, and the switch is "no implicit returns".

[vladima]

currently implemented behavior is aligned with our existing approach to report errors on missing return or throw only if user explicitly opted in (by using return type annotation). However I do see how it might be subtle especially now it is kind of double opt-in (first via command line switch and second - by explicitly specifying return type) - this behavior can always be adjusted

[alexeagle]

This is fantastic! We love preventing bugs.

I'm curious if the team has discussed how to classify which static analysis will be included in the compiler, vs. what would be in something like tslint.
Our experience at Google is that optional tooling is underused and much less effective. (we're writing a paper on it which I hope to share soon - cc @eaftan).
As a trivial example, we found a handful of occurrences of a bug like:

if (condition);
  throw new IllegalState();

in Google's code, and this is very easily prevented by disallowing "empty" if statements with no then/else blocks.

Do you imagine adding many more compilerOptions like the new --noImplicitReturns added in this PR?

[vladima]

@ahejlsberg can you please check if your time measurements got better with last commit?

[ahejlsberg]

Since we don't remove Debug.assert calls in retail code, this will always perform expensive number-to-string and string allocation/concatenation operations.

[vladima]

makes sense, will fix it

[ahejlsberg]

@vladima Just ran my RWC Monaco tests. With the latest changes we're now consistently a little bit faster than 1.6 even with reachability checks. Good stuff.

[ahejlsberg]

👍

[glen-84]

What about the tsconfig.json schema? Shouldn't it be updated?