Skip to content

Fix control flow analysis in try-catch-finally #34880

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 5, 2019
Merged

Fix control flow analysis in try-catch-finally #34880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc15671
Revise creation of control flow graph for try-catch-finally statements
ahejlsberg Nov 2, 2019
03a5a27
Add tests
ahejlsberg Nov 2, 2019
f2b3e06
Accept new baselines
ahejlsberg Nov 2, 2019
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
150 changes: 56 additions & 94 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,6 @@ namespace ts {
return node;
}

let flowNodeCreated: <T extends FlowNode>(node: T) => T = initFlowNode;

const binder = createBinder();

export function bindSourceFile(file: SourceFile, options: CompilerOptions) {
Expand Down Expand Up @@ -199,6 +197,7 @@ namespace ts {
let currentReturnTarget: FlowLabel | undefined;
let currentTrueTarget: FlowLabel | undefined;
let currentFalseTarget: FlowLabel | undefined;
let currentExceptionTarget: FlowLabel | undefined;
let preSwitchCaseFlow: FlowNode | undefined;
let activeLabels: ActiveLabel[] | undefined;
let hasExplicitReturn: boolean;
Expand Down Expand Up @@ -271,6 +270,7 @@ namespace ts {
currentReturnTarget = undefined;
currentTrueTarget = undefined;
currentFalseTarget = undefined;
currentExceptionTarget = undefined;
activeLabels = undefined!;
hasExplicitReturn = false;
emitFlags = NodeFlags.None;
Expand Down Expand Up @@ -624,11 +624,11 @@ namespace ts {
blockScopeContainer.locals = undefined;
}
if (containerFlags & ContainerFlags.IsControlFlowContainer) {
const saveFlowNodeCreated = flowNodeCreated;
const saveCurrentFlow = currentFlow;
const saveBreakTarget = currentBreakTarget;
const saveContinueTarget = currentContinueTarget;
const saveReturnTarget = currentReturnTarget;
const saveExceptionTarget = currentExceptionTarget;
const saveActiveLabels = activeLabels;
const saveHasExplicitReturn = hasExplicitReturn;
const isIIFE = containerFlags & ContainerFlags.IsFunctionExpression && !hasModifier(node, ModifierFlags.Async) &&
Expand All @@ -644,11 +644,11 @@ namespace ts {
// We create a return control flow graph for IIFEs and constructors. For constructors
// we use the return control flow graph in strict property initialization checks.
currentReturnTarget = isIIFE || node.kind === SyntaxKind.Constructor ? createBranchLabel() : undefined;
currentExceptionTarget = undefined;
currentBreakTarget = undefined;
currentContinueTarget = undefined;
activeLabels = undefined;
hasExplicitReturn = false;
flowNodeCreated = initFlowNode;
bindChildren(node);
// Reset all reachability check related flags on node (for incremental scenarios)
node.flags &= ~NodeFlags.ReachabilityAndEmitFlags;
Expand All @@ -674,9 +674,9 @@ namespace ts {
currentBreakTarget = saveBreakTarget;
currentContinueTarget = saveContinueTarget;
currentReturnTarget = saveReturnTarget;
currentExceptionTarget = saveExceptionTarget;
activeLabels = saveActiveLabels;
hasExplicitReturn = saveHasExplicitReturn;
flowNodeCreated = saveFlowNodeCreated;
}
else if (containerFlags & ContainerFlags.IsInterface) {
seenThisKeyword = false;
Expand Down Expand Up @@ -961,30 +961,29 @@ namespace ts {
return antecedent;
}
setFlowNodeReferenced(antecedent);
return flowNodeCreated({ flags, antecedent, node: expression });
return initFlowNode({ flags, antecedent, node: expression });
}

function createFlowSwitchClause(antecedent: FlowNode, switchStatement: SwitchStatement, clauseStart: number, clauseEnd: number): FlowNode {
if (!isNarrowingExpression(switchStatement.expression)) {
return antecedent;
}
setFlowNodeReferenced(antecedent);
return flowNodeCreated({ flags: FlowFlags.SwitchClause, antecedent, switchStatement, clauseStart, clauseEnd });
return initFlowNode({ flags: FlowFlags.SwitchClause, antecedent, switchStatement, clauseStart, clauseEnd });
}

function createFlowAssignment(antecedent: FlowNode, node: Expression | VariableDeclaration | BindingElement): FlowNode {
function createFlowMutation(flags: FlowFlags, antecedent: FlowNode, node: Node): FlowNode {
setFlowNodeReferenced(antecedent);
return flowNodeCreated({ flags: FlowFlags.Assignment, antecedent, node });
const result = initFlowNode({ flags, antecedent, node });
if (currentExceptionTarget) {
addAntecedent(currentExceptionTarget, result);
}
return result;
}

function createFlowCall(antecedent: FlowNode, node: CallExpression): FlowNode {
setFlowNodeReferenced(antecedent);
return flowNodeCreated({ flags: FlowFlags.Call, antecedent, node });
}

function createFlowArrayMutation(antecedent: FlowNode, node: CallExpression | BinaryExpression): FlowNode {
setFlowNodeReferenced(antecedent);
return flowNodeCreated({ flags: FlowFlags.ArrayMutation, antecedent, node });
return initFlowNode({ flags: FlowFlags.Call, antecedent, node });
}

function finishFlowLabel(flow: FlowLabel): FlowNode {
Expand Down Expand Up @@ -1192,93 +1191,56 @@ namespace ts {

function bindTryStatement(node: TryStatement): void {
const preFinallyLabel = createBranchLabel();
const preTryFlow = currentFlow;
const tryPriors: FlowNode[] = [];
const oldFlowNodeCreated = flowNodeCreated;
// We hook the creation of all flow nodes within the `try` scope and store them so we can add _all_ of them
// as possible antecedents of the start of the `catch` or `finally` blocks.
// Don't bother intercepting the call if there's no finally or catch block that needs the information
if (node.catchClause || node.finallyBlock) {
flowNodeCreated = node => (tryPriors.push(node), initFlowNode(node));
}
// We conservatively assume that *any* code in the try block can cause an exception, but we only need
// to track code that causes mutations (because only mutations widen the possible control flow type of
// a variable). The currentExceptionTarget is the target label for control flows that result from
// exceptions. We add all mutation flow nodes as antecedents of this label such that we can analyze them
// as possible antecedents of the start of catch or finally blocks. Furthermore, we add the current
// control flow to represent exceptions that occur before any mutations.
const saveExceptionTarget = currentExceptionTarget;
currentExceptionTarget = createBranchLabel();
addAntecedent(currentExceptionTarget, currentFlow);
bind(node.tryBlock);
flowNodeCreated = oldFlowNodeCreated;
addAntecedent(preFinallyLabel, currentFlow);

const flowAfterTry = currentFlow;
let flowAfterCatch = unreachableFlow;

if (node.catchClause) {
currentFlow = preTryFlow;
if (tryPriors.length) {
const preCatchFlow = createBranchLabel();
addAntecedent(preCatchFlow, currentFlow);
for (const p of tryPriors) {
addAntecedent(preCatchFlow, p);
}
currentFlow = finishFlowLabel(preCatchFlow);
}

// Start of catch clause is the target of exceptions from try block.
currentFlow = finishFlowLabel(currentExceptionTarget);
// The currentExceptionTarget now represents control flows from exceptions in the catch clause.
// Effectively, in a try-catch-finally, if an exception occurs in the try block, the catch block
// acts like a second try block.
currentExceptionTarget = createBranchLabel();
addAntecedent(currentExceptionTarget, currentFlow);
bind(node.catchClause);
addAntecedent(preFinallyLabel, currentFlow);

flowAfterCatch = currentFlow;
}
const exceptionTarget = finishFlowLabel(currentExceptionTarget);
currentExceptionTarget = saveExceptionTarget;
if (node.finallyBlock) {
// We add the nodes within the `try` block to the `finally`'s antecedents if there's no catch block
// (If there is a `catch` block, it will have all these antecedents instead, and the `finally` will
// have the end of the `try` block and the end of the `catch` block)
let preFinallyPrior = preTryFlow;
if (!node.catchClause) {
if (tryPriors.length) {
const preFinallyFlow = createBranchLabel();
addAntecedent(preFinallyFlow, preTryFlow);
for (const p of tryPriors) {
addAntecedent(preFinallyFlow, p);
}
preFinallyPrior = finishFlowLabel(preFinallyFlow);
}
}

// in finally flow is combined from pre-try/flow from try/flow from catch
// pre-flow is necessary to make sure that finally is reachable even if finally flows in both try and finally blocks are unreachable

// also for finally blocks we inject two extra edges into the flow graph.
// first -> edge that connects pre-try flow with the label at the beginning of the finally block, it has lock associated with it
// second -> edge that represents post-finally flow.
// these edges are used in following scenario:
// let a; (1)
// try { a = someOperation(); (2)}
// finally { (3) console.log(a) } (4)
// (5) a

// flow graph for this case looks roughly like this (arrows show ):
// (1-pre-try-flow) <--.. <-- (2-post-try-flow)
// ^ ^
// |*****(3-pre-finally-label) -----|
// ^
// |-- ... <-- (4-post-finally-label) <--- (5)
// In case when we walk the flow starting from inside the finally block we want to take edge '*****' into account
// since it ensures that finally is always reachable. However when we start outside the finally block and go through label (5)
// then edge '*****' should be discarded because label 4 is only reachable if post-finally label-4 is reachable
// Simply speaking code inside finally block is treated as reachable as pre-try-flow
// since we conservatively assume that any line in try block can throw or return in which case we'll enter finally.
// However code after finally is reachable only if control flow was not abrupted in try/catch or finally blocks - it should be composed from
// final flows of these blocks without taking pre-try flow into account.
//
// extra edges that we inject allows to control this behavior
// if when walking the flow we step on post-finally edge - we can mark matching pre-finally edge as locked so it will be skipped.
const preFinallyFlow: PreFinallyFlow = initFlowNode({ flags: FlowFlags.PreFinally, antecedent: preFinallyPrior, lock: {} });
// Possible ways control can reach the finally block:
// 1) Normal completion of try block of a try-finally or try-catch-finally
// 2) Normal completion of catch block (following exception in try block) of a try-catch-finally
// 3) Exception in try block of a try-finally
// 4) Exception in catch block of a try-catch-finally
// When analyzing a control flow graph that starts inside a finally block we want to consider all
// four possibilities above. However, when analyzing a control flow graph that starts outside (past)
// the finally block, we only want to consider the first two (if we're past a finally block then it
// must have completed normally). To make this possible, we inject two extra nodes into the control
// flow graph: An after-finally with an antecedent of the control flow at the end of the finally
// block, and a pre-finally with an antecedent that represents all exceptional control flows. The
// 'lock' property of the pre-finally references the after-finally, and the after-finally has a
// boolean 'locked' property that we set to true when analyzing a control flow that contained the
// the after-finally node. When the lock associated with a pre-finally is locked, the antecedent of
// the pre-finally (i.e. the exceptional control flows) are skipped.
const preFinallyFlow: PreFinallyFlow = initFlowNode({ flags: FlowFlags.PreFinally, antecedent: exceptionTarget, lock: {} });
addAntecedent(preFinallyLabel, preFinallyFlow);

currentFlow = finishFlowLabel(preFinallyLabel);
bind(node.finallyBlock);
// if flow after finally is unreachable - keep it
// otherwise check if flows after try and after catch are unreachable
// if yes - convert current flow to unreachable
// i.e.
// try { return "1" } finally { console.log(1); }
// console.log(2); // this line should be unreachable even if flow falls out of finally block
// If the end of the finally block is reachable, but the end of the try and catch blocks are not,
// convert the current flow to unreachable. For example, 'try { return 1; } finally { ... }' should
// result in an unreachable current control flow.
if (!(currentFlow.flags & FlowFlags.Unreachable)) {
if ((flowAfterTry.flags & FlowFlags.Unreachable) && (flowAfterCatch.flags & FlowFlags.Unreachable)) {
currentFlow = flowAfterTry === reportedUnreachableFlow || flowAfterCatch === reportedUnreachableFlow
Expand All @@ -1287,7 +1249,7 @@ namespace ts {
}
}
if (!(currentFlow.flags & FlowFlags.Unreachable)) {
const afterFinallyFlow: AfterFinallyFlow = flowNodeCreated({ flags: FlowFlags.AfterFinally, antecedent: currentFlow });
const afterFinallyFlow: AfterFinallyFlow = initFlowNode({ flags: FlowFlags.AfterFinally, antecedent: currentFlow });
preFinallyFlow.lock = afterFinallyFlow;
currentFlow = afterFinallyFlow;
}
Expand Down Expand Up @@ -1409,7 +1371,7 @@ namespace ts {

function bindAssignmentTargetFlow(node: Expression) {
if (isNarrowableReference(node)) {
currentFlow = createFlowAssignment(currentFlow, node);
currentFlow = createFlowMutation(FlowFlags.Assignment, currentFlow, node);
}
else if (node.kind === SyntaxKind.ArrayLiteralExpression) {
for (const e of (<ArrayLiteralExpression>node).elements) {
Expand Down Expand Up @@ -1492,7 +1454,7 @@ namespace ts {
if (operator === SyntaxKind.EqualsToken && node.left.kind === SyntaxKind.ElementAccessExpression) {
const elementAccess = <ElementAccessExpression>node.left;
if (isNarrowableOperand(elementAccess.expression)) {
currentFlow = createFlowArrayMutation(currentFlow, node);
currentFlow = createFlowMutation(FlowFlags.ArrayMutation, currentFlow, node);
}
}
}
Expand Down Expand Up @@ -1530,7 +1492,7 @@ namespace ts {
}
}
else {
currentFlow = createFlowAssignment(currentFlow, node);
currentFlow = createFlowMutation(FlowFlags.Assignment, currentFlow, node);
}
}

Expand Down Expand Up @@ -1649,7 +1611,7 @@ namespace ts {
if (node.expression.kind === SyntaxKind.PropertyAccessExpression) {
const propertyAccess = <PropertyAccessExpression>node.expression;
if (isNarrowableOperand(propertyAccess.expression) && isPushOrUnshiftIdentifier(propertyAccess.name)) {
currentFlow = createFlowArrayMutation(currentFlow, node);
currentFlow = createFlowMutation(FlowFlags.ArrayMutation, currentFlow, node);
}
}
}
Expand Down
121 changes: 121 additions & 0 deletions tests/baselines/reference/tryCatchFinallyControlFlow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
//// [tryCatchFinallyControlFlow.ts]
// Repro from #34797

function f1() {
let a: number | null = null;
try {
a = 123;
return a;
}
catch (e) {
throw e;
}
finally {
if (a != null && a.toFixed(0) == "123") {
}
}
}

function f2() {
let x: 0 | 1 | 2 | 3 = 0;
try {
x = 1;
}
catch (e) {
x = 2;
throw e;
}
finally {
x; // 0 | 1 | 2
}
x; // 1
}

function f3() {
let x: 0 | 1 | 2 | 3 = 0;
try {
x = 1;
}
catch (e) {
x = 2;
return;
}
finally {
x; // 0 | 1 | 2
}
x; // 1
}

function f4() {
let x: 0 | 1 | 2 | 3 = 0;
try {
x = 1;
}
catch (e) {
x = 2;
}
finally {
x; // 0 | 1 | 2
}
x; // 1 | 2
}


//// [tryCatchFinallyControlFlow.js]
"use strict";
// Repro from #34797
function f1() {
var a = null;
try {
a = 123;
return a;
}
catch (e) {
throw e;
}
finally {
if (a != null && a.toFixed(0) == "123") {
}
}
}
function f2() {
var x = 0;
try {
x = 1;
}
catch (e) {
x = 2;
throw e;
}
finally {
x; // 0 | 1 | 2
}
x; // 1
}
function f3() {
var x = 0;
try {
x = 1;
}
catch (e) {
x = 2;
return;
}
finally {
x; // 0 | 1 | 2
}
x; // 1
}
function f4() {
var x = 0;
try {
x = 1;
}
catch (e) {
x = 2;
}
finally {
x; // 0 | 1 | 2
}
x; // 1 | 2
}
Loading