Skip to content

Commit

Permalink
initial rev of controlflow analysis
Browse files Browse the repository at this point in the history
  • Loading branch information
vladima committed Nov 8, 2014
1 parent d99023e commit 3ec5eb1
Show file tree
Hide file tree
Showing 5 changed files with 342 additions and 4 deletions.
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"/>

module ts {

Expand Down Expand Up @@ -5896,7 +5897,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));
}
if (node.body.kind === SyntaxKind.FunctionBlock) {
checkSourceElement(node.body);
Expand Down Expand Up @@ -7015,7 +7017,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) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

Why &&?

return ControlFlowState.ReportedUnreachable;
}
return ControlFlowState.Unreachable;
}

function verifyReachable(n: Node): void {
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'
var implicitLabels: number[] = [];

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 {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

I don't love the name mergedStates

if (mergedStates === ControlFlowState.Uninitialized) {
if (name) {
error(name, Diagnostics.Unused_label);

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

I was going to ask why you don't just initially mark the label with its outer state. I suppose this error is the reason why, correct? If you just initialized the label with its outer state, you wouldn't be able to detect that there are no explicit jumps to a label?

This comment has been minimized.

Copy link
@vladima

vladima Dec 1, 2014

Author Contributor

that is right

}
setState(outerState);
}
else {
setState(or(mergedStates, outerState));
}
}

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);

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

If this ends up Unreachable, where do you report the error? Do you only report the error if there are additional statements underneath?

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

In other words, do you get an error for this:

function f() {
   while (true) { }
}

This comment has been minimized.

Copy link
@vladima

vladima Dec 1, 2014

Author Contributor

error is reported on the subsequent statement so in this case there will be no errors. If you add return type annotation then we'll report an error on a function 'not all paths return something'

}

function gotoLabel(label: Identifier, outerState: ControlFlowState): void {
var stateIndex: number;
if (label) {
if (!hasProperty(labels, label.text)) {
// reference to non-existing label
return;
}
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;
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);

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

Maybe add two cases, one if we have an else, and one if we do not

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);
check(n.catchBlock);
var postCatchState = currentState;

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

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

Hmm, seems like even if the post of the try and catch are unreachable, the finally may still be reachable, right?

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

Oh wait, this is the state after the finally block, nvm

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

Actually, don't you also have to "or" the currentState here? If you return or throw from the finally, then currentState should be unreachable.

This comment has been minimized.

Copy link
@vladima

vladima via email Dec 1, 2014

Author Contributor
}

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) {
error(c.expression, Diagnostics.Fallthrough_case_in_switch);

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

Why do we want to give an error for falling through? Does this also give an error if the first case has no statements (like batching cases together)?

This comment has been minimized.

Copy link
@vladima

vladima Dec 1, 2014

Author Contributor

it will give an error only if case has at least one statement so batching will work fine

}
});

// 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;

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:

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

For variableStatement, you probably only want to check it if it's initialized.

case SyntaxKind.EmptyStatement:
case SyntaxKind.ExpressionStatement:
case SyntaxKind.DebuggerStatement:
verifyReachable(n);

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Dec 1, 2014

Contributor

Should verifyReachable just be before this switch? Seems like you ultimately do it for every statement

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 @@ -270,6 +270,10 @@ module ts {
Type_argument_candidate_1_is_not_a_valid_type_argument_because_it_is_not_a_supertype_of_candidate_0: { code: 2455, category: DiagnosticCategory.Error, key: "Type argument candidate '{1}' is not a valid type argument because it is not a supertype of candidate '{0}'." },
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}'" },
Not_all_code_paths_return_a_value: { code: 2458, category: DiagnosticCategory.Error, key: "Not all code paths return a value" },
Unreachable_code_detected: { code: 2459, category: DiagnosticCategory.Error, key: "Unreachable code detected" },
Unused_label: { code: 2460, category: DiagnosticCategory.Error, key: "Unused label" },
Fallthrough_case_in_switch: { code: 2461, 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_name_1_from_private_module_2: { code: 4001, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported class has or is using name '{1}' from private module '{2}'." },
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}'." },
Expand Down
17 changes: 16 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1076,7 +1076,22 @@
"category": "Error",
"code": 2457
},

"Not all code paths return a value": {
"category": "Error",
"code": 2458
},
"Unreachable code detected": {
"category": "Error",
"code": 2459
},
"Unused label": {
"category": "Error",
"code": 2460
},
"Fallthrough case in switch": {
"category": "Error",
"code": 2461
},
"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
"code": 4000
Expand Down

0 comments on commit 3ec5eb1

Please sign in to comment.