initial revision of reachability checks by vladima · Pull Request #4788 · microsoft/TypeScript

vladima · 2015-09-14T21:07:34Z

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 · 2015-09-14T23:46:03Z

fancy 😎

DanielRosenwasser · 2015-09-15T04:28:29Z

src/compiler/binder.ts

Space after switch

yuit · 2015-09-16T18:24:23Z

src/compiler/binder.ts

use the parameter name 'node' instead

yuit · 2015-09-16T21:32:18Z

src/compiler/binder.ts

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

discussed offline

jbondc · 2015-09-22T13:16:47Z

🍺 is it enabled by default? impact on performance?

vladima · 2015-10-14T04:45:13Z

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

DanielRosenwasser · 2015-10-20T04:51:07Z

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

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

}

vladima · 2015-10-20T05:40:31Z

@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 · 2015-10-20T06:33:15Z

@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 · 2015-10-20T16:11:00Z

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 · 2015-10-22T10:43:07Z

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 · 2015-10-22T16:04:48Z

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

ahejlsberg · 2015-10-22T16:20:46Z

src/compiler/binder.ts

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

makes sense, will fix it

ahejlsberg · 2015-10-22T16:27:59Z

@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 · 2015-11-02T19:33:14Z

👍

initial revision of reachability checks

…eScript#4788 (reachability checks)

glen-84 · 2016-01-18T11:13:09Z

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

initial revision of reachability checks in binder

b62ef0d

msftclas added the cla-not-required label Sep 14, 2015

vladima mentioned this pull request Sep 14, 2015

Simple control flow analysis #1287

Closed

DanielRosenwasser reviewed Sep 15, 2015
View reviewed changes

src/compiler/binder.ts Outdated

Copy link

Member

DanielRosenwasser Sep 15, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space after switch

vladima added 2 commits September 15, 2015 10:36

addressed PR feedback

beb1aa3

merge with master

9aeb0f8

yuit reviewed Sep 16, 2015
View reviewed changes

adidahiya mentioned this pull request Sep 16, 2015

deprecate no-unreachable once implemented in TS compiler palantir/tslint#661

Closed

vladima added 2 commits September 16, 2015 12:43

addressed PR feedback

682c14c

merge with master

69321a0

yuit reviewed Sep 16, 2015
View reviewed changes

merge with master

4711f0e

vladima added 2 commits October 16, 2015 13:13

Merge branch 'master' into reachabilityChecks

7b12617

Merge branch 'master' into reachabilityChecks

f9eaed7

merge with master

f96980d

make binder singleton, inline bindWithReachabilityChecks

2779352

ahejlsberg reviewed Oct 22, 2015
View reviewed changes

vladima added 3 commits October 22, 2015 09:50

defer allocation of error message text in binder

7d09f26

merge with master

d2a11b5

merge with master

3f11c0b

vladima added a commit that referenced this pull request Nov 2, 2015

Merge pull request #4788 from Microsoft/reachabilityChecks

fb15e9c

initial revision of reachability checks

vladima merged commit fb15e9c into master Nov 2, 2015

vladima deleted the reachabilityChecks branch November 2, 2015 22:55

mjohnsonengr added a commit to mjohnsonengr/atom-typescript that referenced this pull request Nov 15, 2015

Added new tsconfig validation for options introduced in microsoft/Typ…

9b043c7

…eScript#4788 (reachability checks)

mjohnsonengr mentioned this pull request Nov 15, 2015

Support new tsconfig options for reachability checks TypeStrong/atom-typescript#725

Merged

vladima mentioned this pull request Nov 25, 2015

Show bind errors in js files #5785

Closed

mhegazy mentioned this pull request Dec 9, 2015

Compiler warnings / errors for common automatic semi-colon insertion (ASI) issues #2575

Closed

mhegazy mentioned this pull request Feb 22, 2016

Suggestion: Control flow analysis #2854

Closed

2 tasks

aluanhaddad mentioned this pull request Jun 5, 2016

New rule: ensure that all code paths return a value palantir/tslint#580

Closed

roymiloh mentioned this pull request Nov 21, 2016

remove unreachable code rule wix-incubator/tslint-config-wix#1

Merged

ashishsc mentioned this pull request Mar 27, 2017

Convert index.js to TypeScript evancohen/sonus#35

Open

microsoft locked and limited conversation to collaborators Jun 19, 2018

Conversation

vladima commented Sep 14, 2015

Uh oh!

basarat commented Sep 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbondc commented Sep 22, 2015

Uh oh!

vladima commented Oct 14, 2015

Uh oh!

DanielRosenwasser commented Oct 20, 2015

Uh oh!

vladima commented Oct 20, 2015

Uh oh!

DanielRosenwasser commented Oct 20, 2015

Uh oh!

vladima commented Oct 20, 2015

Uh oh!

alexeagle commented Oct 22, 2015

Uh oh!

vladima commented Oct 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahejlsberg commented Oct 22, 2015

Uh oh!

ahejlsberg commented Nov 2, 2015

Uh oh!

glen-84 commented Jan 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants