Skip to content
New issue

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

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

Already on GitHub? # to your account

Simple control flow analysis #1287

Closed
wants to merge 9 commits into from
Closed

Simple control flow analysis #1287

wants to merge 9 commits into from

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Nov 27, 2014

This PR adds a basic control flow analysis to typescript compiler. Now it can detect unreachable code, implicit returns of undefined in functions, unused labels, fall- through between cases in switch statements. Some of these checks can be used in #393 or #274.

cfa

Disclaimer: This PR is not intended to be checked in as-is. What I rather want is to use it as a starting point to discuss:

  • difference between warnings and errors. now all semantic errors that are produced by the compler can be treated as warnings - they do not block emit. However visually they are always presented as errors which might be confusing, I.e. I would expect message about unreachable code to be more like recommendation (presented using green squigglies or shadowed code) instead of being aggressively marked as error
  • error suppression story - global and local

@@ -4,6 +4,7 @@
/// <reference path="parser.ts"/>
/// <reference path="binder.ts"/>
/// <reference path="emitter.ts"/>
/// <reference path="controlflow.ts"/>
Copy link
Member

Choose a reason for hiding this comment

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

Consider controlFlow.ts, but I like this too.

@dekajp dekajp mentioned this pull request Nov 30, 2014
return newLen - 1;
}

function setFinalStateAtLabel(mergedStates: ControlFlowState, outerState: ControlFlowState, name: Identifier): void {
Copy link
Member

Choose a reason for hiding this comment

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

Leave a comment to explain that name is optional for the case of implicit labels.

hasDefault = hasDefault || c.kind === SyntaxKind.DefaultClause;
setState(startState);
forEach(c.statements, check);
if (c.statements.length && currentState === ControlFlowState.Reachable) {
Copy link
Member

Choose a reason for hiding this comment

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

Consider c.statements.length > 0

@JsonFreeman
Copy link
Contributor

👍

Awesome job!

@DanielRosenwasser
Copy link
Member

We should put this back on the radar again.

@NoelAbrahams
Copy link

We should put this back on the radar again.

👍

@awerlang
Copy link

IMO, it overlaps with TSLint.

Regarding this, I'm concerned with the fact that I cannot use more mature JSHint or ESLint. Perhaps running them on the output (haven't tried that though).
As a TS user, I would prefer that it becomes a full superset of ES6, so I could have back other linters in my workflow.

@vladima
Copy link
Contributor Author

vladima commented Sep 14, 2015

closed in favor of #4788

@vladima vladima closed this Sep 14, 2015
@mhegazy mhegazy deleted the cfa branch September 14, 2015 21:42
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants