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

Simple control flow analysis #1287

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Jakefile
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ var tscFile = path.join(builtLocalDirectory, compilerFilename);
compileFile(tscFile, compilerSources, [builtLocalDirectory, copyright].concat(compilerSources), [copyright], /*useBuiltCompiler:*/ false);

var servicesFile = path.join(builtLocalDirectory, "typescriptServices.js");
compileFile(servicesFile, servicesSources, [builtLocalDirectory, copyright].concat(servicesSources), [copyright], /*useBuiltCompiler:*/ true);
compileFile(servicesFile, servicesSources, [builtLocalDirectory, copyright].concat(servicesSources), [copyright], /*useBuiltCompiler:*/ false);

// Local target to build the compiler and services
desc("Builds the full compiler and services");
Expand Down
7 changes: 5 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.


module ts {
var nextSymbolId = 1;
Expand Down Expand Up @@ -6111,7 +6112,8 @@ module ts {

function checkFunctionExpressionBody(node: FunctionExpression) {
if (node.type) {
checkIfNonVoidFunctionHasReturnExpressionsOrSingleThrowStatment(node, getTypeFromTypeNode(node.type));
checkControlFlowOfFunction(node, getTypeFromTypeNode(node.type) !== voidType, error);
// checkIfNonVoidFunctionHasReturnExpressionsOrSingleThrowStatment(node, getTypeFromTypeNode(node.type));
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented out lines.

}
if (node.body.kind === SyntaxKind.FunctionBlock) {
checkSourceElement(node.body);
Expand Down Expand Up @@ -7229,7 +7231,8 @@ module ts {

checkSourceElement(node.body);
if (node.type && !isAccessor(node.kind)) {
checkIfNonVoidFunctionHasReturnExpressionsOrSingleThrowStatment(node, getTypeFromTypeNode(node.type));
checkControlFlowOfFunction(node, getTypeFromTypeNode(node.type) !== voidType, error);
// checkIfNonVoidFunctionHasReturnExpressionsOrSingleThrowStatment(node, getTypeFromTypeNode(node.type));
}

// If there is no body and no explicit return type, then report an error.
Expand Down
316 changes: 316 additions & 0 deletions src/compiler/controlflow.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,316 @@
/// <reference path="types.ts"/>

module ts {
export function checkControlFlowOfFunction(decl: FunctionLikeDeclaration, noImplicitReturns: boolean, error: (n: Node, message: DiagnosticMessage, arg0?: any) => void) {
if (!decl.body || decl.body.kind !== SyntaxKind.FunctionBlock) {
return;
}

var finalState = checkControlFlow(decl.body, error);
if (noImplicitReturns && finalState === ControlFlowState.Reachable) {
var errorNode: Node = decl.name || decl;
error(errorNode, Diagnostics.Not_all_code_paths_return_a_value);
}
}

export function checkControlFlowOfBlock(block: Block, error: (n: Node, message: DiagnosticMessage, arg0?: any) => void) {
checkControlFlow(block, error);
}

function checkControlFlow(decl: Node, error: (n: Node, message: DiagnosticMessage, arg0?: any) => void): ControlFlowState {
var currentState = ControlFlowState.Reachable;

function setState(newState: ControlFlowState) {
currentState = newState;
}

function or(s1: ControlFlowState, s2: ControlFlowState): ControlFlowState {
if (s1 === ControlFlowState.Reachable || s2 === ControlFlowState.Reachable) {
return ControlFlowState.Reachable;
}
if (s1 === ControlFlowState.ReportedUnreachable && s2 === ControlFlowState.ReportedUnreachable) {
return ControlFlowState.ReportedUnreachable;
}
return ControlFlowState.Unreachable;
Copy link
Member

Choose a reason for hiding this comment

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

Even when Uninitialized? Or is it certain that you never call this on Uninitialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latter one. Uninitialized is only used to mark initial state at labels. Prior merging states setFinalStateAtLabel tests if state at label is Uninitialized and if this is true - does not do merging.

Copy link
Member

Choose a reason for hiding this comment

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

Then consider an assertion, but this looks fine.

}

function verifyReachable(n: Node): void {
Copy link
Member

Choose a reason for hiding this comment

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

reportAsUnreachable or reportUnreachable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

semantic of this function is: "check current state and report error if current state is unreachable" so I would say that reportIfUnreachable is more appropriate here. However honestly I don't see much difference between "report if unreachable" and "ensure that reachable"

Copy link
Member

Choose a reason for hiding this comment

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

reportIfUnreachable is actually a lot better; "verify" implies that there is some more elaborate checking, and doesn't really imply the act of reporting.

if (currentState === ControlFlowState.Unreachable) {
error(n, Diagnostics.Unreachable_code_detected);
currentState = ControlFlowState.ReportedUnreachable;
}
}

// label name -> index in 'labelStack'
var labels: Map<number> = {};
// CF state at all seen labels
var labelStack: ControlFlowState[] = [];
// indices of implicit labels in 'labelStack'
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what this means.

var implicitLabels: number[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

Explain what an implicit label is; right now I'm assuming it's the entry to point to something like a basic block.


function pushNamedLabel(name: Identifier): boolean {
if (hasProperty(labels, name.text)) {
return false;
}
var newLen = labelStack.push(ControlFlowState.Uninitialized);
labels[name.text] = newLen - 1;
return true;
}

function pushImplicitLabel(): number {
var newLen = labelStack.push(ControlFlowState.Uninitialized);
implicitLabels.push(newLen - 1);
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.

if (mergedStates === ControlFlowState.Uninitialized) {
if (name) {
error(name, Diagnostics.Unused_label);
}
setState(outerState);
}
else {
setState(or(mergedStates, outerState));
Copy link
Contributor

Choose a reason for hiding this comment

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

can you help explain why this is right?

}
}

function popNamedLabel(name: Identifier, outerState: ControlFlowState): void {
Debug.assert(hasProperty(labels, name.text));
var index = labels[name.text];
Debug.assert(labelStack.length === index + 1);
labels[name.text] = undefined;
var mergedStates = labelStack.pop();
setFinalStateAtLabel(mergedStates, outerState, name);
}

function popImplicitLabel(index: number, outerState: ControlFlowState): void {
Debug.assert(labelStack.length === index + 1);
var i = implicitLabels.pop();
Debug.assert(index === i);
var mergedStates = labelStack.pop();
setFinalStateAtLabel(mergedStates, outerState, /*name*/ undefined);
}

function gotoLabel(label: Identifier, outerState: ControlFlowState): void {
var stateIndex: number;
if (label) {
if (!hasProperty(labels, label.text)) {
// reference to non-existing label
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Either add a newline, or remove the one after the if block in the next else block.

stateIndex = labels[label.text];
}
else {
if (implicitLabels.length === 0) {
// non-labeled break\continue being used outside loops
return;
}

stateIndex = implicitLabels[implicitLabels.length - 1];
}
var stateAtLabel = labelStack[stateIndex];
labelStack[stateIndex] = stateAtLabel === ControlFlowState.Uninitialized ? outerState : or(outerState, stateAtLabel);
}

function checkWhileStatement(n: WhileStatement): void {
verifyReachable(n);

var preWhileState: ControlFlowState = n.expression.kind === SyntaxKind.FalseKeyword ? ControlFlowState.Unreachable : currentState;
var postWhileState: ControlFlowState = n.expression.kind === SyntaxKind.TrueKeyword ? ControlFlowState.Unreachable : currentState;

setState(preWhileState);

var index = pushImplicitLabel();
check(n.statement);
popImplicitLabel(index, postWhileState);
}

function checkDoStatement(n: DoStatement): void {
verifyReachable(n);
var preDoState = currentState;

var index = pushImplicitLabel();
check(n.statement);

var postDoState = n.expression.kind === SyntaxKind.TrueKeyword ? ControlFlowState.Unreachable : preDoState;
popImplicitLabel(index, postDoState);
}

function checkForStatement(n: ForStatement): void {
verifyReachable(n);

var preForState = currentState;
var index = pushImplicitLabel();
check(n.statement);
var postForState = n.declarations || n.initializer || n.condition || n.iterator ? preForState : ControlFlowState.Unreachable;
Copy link
Member

Choose a reason for hiding this comment

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

... || (n.condition && n.condition !== SyntaxKind.TrueKeyword) || ...

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, I can add it

popImplicitLabel(index, postForState);
}

function checkForInStatement(n: ForInStatement): void {
verifyReachable(n);
var preForInState = currentState;
var index = pushImplicitLabel();
check(n.statement);
popImplicitLabel(index, preForInState);
}

function checkBlock(n: Block): void {
forEach(n.statements, check);
}

function checkIfStatement(n: IfStatement): void {
var ifTrueState: ControlFlowState = n.expression.kind === SyntaxKind.FalseKeyword ? ControlFlowState.Unreachable : currentState;
var ifFalseState: ControlFlowState = n.expression.kind === SyntaxKind.TrueKeyword ? ControlFlowState.Unreachable : currentState;

setState(ifTrueState);
check(n.thenStatement);
ifTrueState = currentState;

setState(ifFalseState);
check(n.elseStatement);

currentState = or(currentState, ifTrueState);
}

function checkReturnOrThrow(n: Node): void {
verifyReachable(n);
setState(ControlFlowState.Unreachable);
}

function checkBreakOrContinueStatement(n: BreakOrContinueStatement): void {
verifyReachable(n);
if (n.kind === SyntaxKind.BreakStatement) {
gotoLabel(n.label, currentState);
}
else {
gotoLabel(n.label, ControlFlowState.Unreachable); // touch label so it will be marked a used
}
setState(ControlFlowState.Unreachable);
}

function checkTryStatement(n: TryStatement): void {
verifyReachable(n);

// catch\finally blocks has the same reachability as try block
var startState = currentState;
check(n.tryBlock);
var postTryState = currentState;

setState(startState);
Copy link
Member

Choose a reason for hiding this comment

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

I think there's some potential to be more accurate here, but I think we'd get diminishing returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory - yes, we might consider catch block unreachable if we can prove that try block either never throws or region that might throw is unreachable. In practice this will add a lot to the overall complexity and yield little value since it will capture a very small amount of very special cases. The same reasoning is applied to the initial reachability of finally blocks

check(n.catchBlock);
var postCatchState = currentState;

setState(startState);
check(n.finallyBlock);
setState(or(postTryState, postCatchState));
}

function checkSwitchStatement(n: SwitchStatement): void {
verifyReachable(n);
var startState = currentState;
var hasDefault = false;

var index = pushImplicitLabel();

forEach(n.clauses, (c: CaseOrDefaultClause) => {
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

error(c.expression, Diagnostics.Fallthrough_case_in_switch);
}
});

// post switch state is unreachable if switch is exaustive (has a default case ) and does not have fallthrough from the last case
var postSwitchState = hasDefault && currentState !== ControlFlowState.Reachable ? ControlFlowState.Unreachable : startState;
Copy link
Member

Choose a reason for hiding this comment

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

What about

function f(x) {
    switch (x) {
        case 1:
        case 2:
        case 3:
            break;
        default:
            return x;
    }

    return -x;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will work correctly since break to a implicit label will make return statement reachable


popImplicitLabel(index, postSwitchState);
}

function checkLabelledStatement(n: LabeledStatement): void {
verifyReachable(n);
var ok = pushNamedLabel(n.label);
check(n.statement);
if (ok) {
popNamedLabel(n.label, currentState);
}
}

function checkWithStatement(n: WithStatement): void {
verifyReachable(n);
check(n.statement);
}

// current assumption: only statements affect CF
function check(n: Node): void {
if (!n || currentState === ControlFlowState.ReportedUnreachable) {
return;
}
switch (n.kind) {
case SyntaxKind.WhileStatement:
checkWhileStatement(<WhileStatement>n);
break;
case SyntaxKind.SourceFile:
checkBlock(<SourceFile>n);
break;
case SyntaxKind.Block:
case SyntaxKind.TryBlock:
case SyntaxKind.CatchBlock:
case SyntaxKind.FinallyBlock:
case SyntaxKind.ModuleBlock:
case SyntaxKind.FunctionBlock:
checkBlock(<Block>n);
break;
case SyntaxKind.IfStatement:
checkIfStatement(<IfStatement>n);
break;
case SyntaxKind.ReturnStatement:
case SyntaxKind.ThrowStatement:
checkReturnOrThrow(n);
break;
case SyntaxKind.BreakStatement:
case SyntaxKind.ContinueStatement:
checkBreakOrContinueStatement(<BreakOrContinueStatement>n);
break;
case SyntaxKind.VariableStatement:
case SyntaxKind.EmptyStatement:
case SyntaxKind.ExpressionStatement:
case SyntaxKind.DebuggerStatement:
verifyReachable(n);
break;
case SyntaxKind.DoStatement:
checkDoStatement(<DoStatement>n);
break;
case SyntaxKind.ForInStatement:
checkForInStatement(<ForInStatement>n);
break;
case SyntaxKind.ForStatement:
checkForStatement(<ForStatement>n);
break;
case SyntaxKind.LabeledStatement:
checkLabelledStatement(<LabeledStatement>n);
break;
case SyntaxKind.SwitchStatement:
checkSwitchStatement(<SwitchStatement>n);
break;
case SyntaxKind.TryStatement:
checkTryStatement(<TryStatement>n);
break;
case SyntaxKind.WithStatement:
checkWithStatement(<WithStatement>n);
break;
}
}

check(decl);
return currentState;
}

const enum ControlFlowState {
Uninitialized = 0,
Reachable = 1,
Unreachable = 2,
ReportedUnreachable = 3,
}
}
4 changes: 4 additions & 0 deletions src/compiler/diagnosticInformationMap.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,10 @@ module ts {
Type_alias_0_circularly_references_itself: { code: 2456, category: DiagnosticCategory.Error, key: "Type alias '{0}' circularly references itself." },
Type_alias_name_cannot_be_0: { code: 2457, category: DiagnosticCategory.Error, key: "Type alias name cannot be '{0}'" },
An_AMD_module_cannot_have_multiple_name_assignments: { code: 2458, category: DiagnosticCategory.Error, key: "An AMD module cannot have multiple name assignments." },
Not_all_code_paths_return_a_value: { code: 2459, category: DiagnosticCategory.Error, key: "Not all code paths return a value" },
Unreachable_code_detected: { code: 2460, category: DiagnosticCategory.Error, key: "Unreachable code detected" },
Unused_label: { code: 2461, category: DiagnosticCategory.Error, key: "Unused label" },
Fallthrough_case_in_switch: { code: 2462, category: DiagnosticCategory.Error, key: "Fallthrough case in switch" },
Import_declaration_0_is_using_private_name_1: { code: 4000, category: DiagnosticCategory.Error, key: "Import declaration '{0}' is using private name '{1}'." },
Type_parameter_0_of_exported_class_has_or_is_using_private_name_1: { code: 4002, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported class has or is using private name '{1}'." },
Type_parameter_0_of_exported_interface_has_or_is_using_private_name_1: { code: 4004, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported interface has or is using private name '{1}'." },
Expand Down
17 changes: 16 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1095,8 +1095,23 @@
"An AMD module cannot have multiple name assignments.": {
"category": "Error",
"code": 2458
},
"Not all code paths return a value": {
"category": "Error",
"code": 2459
},
"Unreachable code detected": {
"category": "Error",
"code": 2460
},
"Unused label": {
"category": "Error",
"code": 2461
},
"Fallthrough case in switch": {
"category": "Error",
"code": 2462
},

"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
"code": 4000
Expand Down