Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial revision of reachability checks #4788

Merged
merged 25 commits into from
Nov 2, 2015
Merged

initial revision of reachability checks #4788

merged 25 commits into from
Nov 2, 2015

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Sep 14, 2015

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
Copy link
Contributor

basarat commented Sep 14, 2015

fancy 😎

return false;
}

switch(n.kind) {
Copy link
Member

Choose a reason for hiding this comment

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

Space after switch

* Returns true if node and its subnodes were successfully traversed.
* Returning false means that node was not examined and caller needs to dive into the node himself.
*/
function bindReachableStatement(n: Node): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

use the parameter name 'node' instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

const hasDefault = forEach(n.caseBlock.clauses, c => c.kind === SyntaxKind.DefaultClause);

// post switch state is unreachable if switch is exaustive (has a default case ) and does not have fallthrough from the last case
const postSwitchState = hasDefault && currentReachabilityState !== Reachability.Reachable ? Reachability.Unreachable : preSwitchState;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed offline

Copy link
Contributor

Choose a reason for hiding this comment

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

🍪 🍰

@jbondc
Copy link
Contributor

jbondc commented Sep 22, 2015

🍺 is it enabled by default? impact on performance?

@vladima
Copy link
Contributor Author

vladima commented Oct 14, 2015

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

@DanielRosenwasser
Copy link
Member

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

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

}

@vladima
Copy link
Contributor Author

vladima commented Oct 20, 2015

@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
Copy link
Member

@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
Copy link
Contributor Author

vladima commented Oct 20, 2015

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
Copy link
Contributor

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
Copy link
Contributor Author

vladima commented Oct 22, 2015

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

}

function popImplicitLabel(implicitLabelIndex: number, outerState: Reachability): void {
Debug.assert(labelStack.length === implicitLabelIndex + 1, `Label stack: ${labelStack.length}, index:${implicitLabelIndex}`);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, will fix it

@ahejlsberg
Copy link
Member

@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
Copy link
Member

👍

vladima added a commit that referenced this pull request Nov 2, 2015
initial revision of reachability checks
@vladima vladima merged commit fb15e9c into master Nov 2, 2015
@vladima 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
@glen-84
Copy link

glen-84 commented Jan 18, 2016

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.