inject pre-finally and after-finally edges into flow graph to possible ignore pre-finally during flow walk (#13845)

vladima · web-flow · commit f673f48fad7e · 2017-02-13T14:36:12.000-08:00
diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts
@@ -1045,7 +1045,35 @@ namespace ts {
             if (node.finallyBlock) {
                 // 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
-                addAntecedent(preFinallyLabel, preTryFlow);
+
+                // 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 = { flags: FlowFlags.PreFinally, antecedent: preTryFlow, lock: {} };
+                addAntecedent(preFinallyLabel, preFinallyFlow);
+
                 currentFlow = finishFlowLabel(preFinallyLabel);
                 bind(node.finallyBlock);
                 // if flow after finally is unreachable - keep it
@@ -1061,6 +1089,11 @@ namespace ts {
                             : unreachableFlow;
                     }
                 }
+                if (!(currentFlow.flags & FlowFlags.Unreachable)) {
+                    const afterFinallyFlow: AfterFinallyFlow = { flags: FlowFlags.AfterFinally, antecedent: currentFlow };
+                    preFinallyFlow.lock = afterFinallyFlow;
+                    currentFlow = afterFinallyFlow;
+                }
             }
             else {
                 currentFlow = finishFlowLabel(preFinallyLabel);
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -9916,7 +9916,19 @@ namespace ts {
                         }
                     }
                     let type: FlowType;
-                    if (flow.flags & FlowFlags.Assignment) {
+                    if (flow.flags & FlowFlags.AfterFinally) {
+                        // block flow edge: finally -> pre-try (for larger explanation check comment in binder.ts - bindTryStatement 
+                        (<AfterFinallyFlow>flow).locked = true;
+                        type = getTypeAtFlowNode((<AfterFinallyFlow>flow).antecedent);
+                        (<AfterFinallyFlow>flow).locked = false;
+                    }
+                    else if (flow.flags & FlowFlags.PreFinally) {
+                        // locked pre-finally flows are filtered out in getTypeAtFlowBranchLabel
+                        // so here just redirect to antecedent
+                        flow = (<PreFinallyFlow>flow).antecedent;
+                        continue;
+                    }
+                    else if (flow.flags & FlowFlags.Assignment) {
                         type = getTypeAtFlowAssignment(<FlowAssignment>flow);
                         if (!type) {
                             flow = (<FlowAssignment>flow).antecedent;
@@ -10072,6 +10084,12 @@ namespace ts {
                 let subtypeReduction = false;
                 let seenIncomplete = false;
                 for (const antecedent of flow.antecedents) {
+                    if (antecedent.flags & FlowFlags.PreFinally && (<PreFinallyFlow>antecedent).lock.locked) {
+                        // if flow correspond to branch from pre-try to finally and this branch is locked - this means that 
+                        // we initially have started following the flow outside the finally block.
+                        // in this case we should ignore this branch.
+                        continue;
+                    }
                     const flowType = getTypeAtFlowNode(antecedent);
                     const type = getTypeFromFlowType(flowType);
                     // If the type at a particular antecedent path is the declared type and the
diff --git a/src/compiler/types.ts b/src/compiler/types.ts
@@ -2071,10 +2071,25 @@
         ArrayMutation  = 1 << 8,  // Potential array mutation
         Referenced     = 1 << 9,  // Referenced as antecedent once
         Shared         = 1 << 10, // Referenced as antecedent more than once
+        PreFinally     = 1 << 11, // Injected edge that links pre-finally label and pre-try flow
+        AfterFinally   = 1 << 12, // Injected edge that links post-finally flow with the rest of the graph
         Label = BranchLabel | LoopLabel,
         Condition = TrueCondition | FalseCondition
     }
 
+    export interface FlowLock {
+        locked?: boolean;
+    }
+
+    export interface AfterFinallyFlow extends FlowNode, FlowLock {
+        antecedent: FlowNode;
+    }
+
+    export interface PreFinallyFlow extends FlowNode {
+        antecedent: FlowNode;
+        lock: FlowLock;
+    }
+
     export interface FlowNode {
         flags: FlowFlags;
         id?: number;     // Node id used by flow type cache in checker
diff --git a/tests/baselines/reference/flowAfterFinally1.js b/tests/baselines/reference/flowAfterFinally1.js
@@ -0,0 +1,25 @@
+//// [flowAfterFinally1.ts]
+declare function openFile(): void
+declare function closeFile(): void
+declare function someOperation(): {}
+
+var result: {}
+openFile()
+try {
+  result = someOperation()
+} finally {
+  closeFile()
+}
+
+result // should not error here
+
+//// [flowAfterFinally1.js]
+var result;
+openFile();
+try {
+    result = someOperation();
+}
+finally {
+    closeFile();
+}
+result; // should not error here
diff --git a/tests/baselines/reference/flowAfterFinally1.symbols b/tests/baselines/reference/flowAfterFinally1.symbols
@@ -0,0 +1,29 @@
+=== tests/cases/compiler/flowAfterFinally1.ts ===
+declare function openFile(): void
+>openFile : Symbol(openFile, Decl(flowAfterFinally1.ts, 0, 0))
+
+declare function closeFile(): void
+>closeFile : Symbol(closeFile, Decl(flowAfterFinally1.ts, 0, 33))
+
+declare function someOperation(): {}
+>someOperation : Symbol(someOperation, Decl(flowAfterFinally1.ts, 1, 34))
+
+var result: {}
+>result : Symbol(result, Decl(flowAfterFinally1.ts, 4, 3))
+
+openFile()
+>openFile : Symbol(openFile, Decl(flowAfterFinally1.ts, 0, 0))
+
+try {
+  result = someOperation()
+>result : Symbol(result, Decl(flowAfterFinally1.ts, 4, 3))
+>someOperation : Symbol(someOperation, Decl(flowAfterFinally1.ts, 1, 34))
+
+} finally {
+  closeFile()
+>closeFile : Symbol(closeFile, Decl(flowAfterFinally1.ts, 0, 33))
+}
+
+result // should not error here
+>result : Symbol(result, Decl(flowAfterFinally1.ts, 4, 3))
+
diff --git a/tests/baselines/reference/flowAfterFinally1.types b/tests/baselines/reference/flowAfterFinally1.types
@@ -0,0 +1,33 @@
+=== tests/cases/compiler/flowAfterFinally1.ts ===
+declare function openFile(): void
+>openFile : () => void
+
+declare function closeFile(): void
+>closeFile : () => void
+
+declare function someOperation(): {}
+>someOperation : () => {}
+
+var result: {}
+>result : {}
+
+openFile()
+>openFile() : void
+>openFile : () => void
+
+try {
+  result = someOperation()
+>result = someOperation() : {}
+>result : {}
+>someOperation() : {}
+>someOperation : () => {}
+
+} finally {
+  closeFile()
+>closeFile() : void
+>closeFile : () => void
+}
+
+result // should not error here
+>result : {}
+
diff --git a/tests/cases/compiler/flowAfterFinally1.ts b/tests/cases/compiler/flowAfterFinally1.ts
@@ -0,0 +1,14 @@
+// @strictNullChecks: true
+declare function openFile(): void
+declare function closeFile(): void
+declare function someOperation(): {}
+
+var result: {}
+openFile()
+try {
+  result = someOperation()
+} finally {
+  closeFile()
+}
+
+result // should not error here
