From ebba1c3d1a3c7d0a52a0531c2ec5edb9b2f94597 Mon Sep 17 00:00:00 2001 From: Vladimir Matveev Date: Sun, 16 Oct 2016 20:57:51 -0700 Subject: [PATCH 1/2] fix flow in finally blocks --- src/compiler/binder.ts | 36 ++++++++++++++----- tests/baselines/reference/flowInFinally1.js | 33 +++++++++++++++++ .../reference/flowInFinally1.symbols | 29 +++++++++++++++ .../baselines/reference/flowInFinally1.types | 34 ++++++++++++++++++ tests/cases/compiler/flowInFinally1.ts | 16 +++++++++ 5 files changed, 140 insertions(+), 8 deletions(-) create mode 100644 tests/baselines/reference/flowInFinally1.js create mode 100644 tests/baselines/reference/flowInFinally1.symbols create mode 100644 tests/baselines/reference/flowInFinally1.types create mode 100644 tests/cases/compiler/flowInFinally1.ts diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 4711d54e0b2..31732bc833f 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -984,24 +984,44 @@ namespace ts { } function bindTryStatement(node: TryStatement): void { - const postFinallyLabel = createBranchLabel(); + const preFinallyLabel = createBranchLabel(); const preTryFlow = currentFlow; // TODO: Every statement in try block is potentially an exit point! bind(node.tryBlock); - addAntecedent(postFinallyLabel, currentFlow); + addAntecedent(preFinallyLabel, currentFlow); + + const flowAfterTry = currentFlow; + let flowAfterCatch = unreachableFlow; + if (node.catchClause) { currentFlow = preTryFlow; bind(node.catchClause); - addAntecedent(postFinallyLabel, currentFlow); + addAntecedent(preFinallyLabel, currentFlow); + + flowAfterCatch = currentFlow; } if (node.finallyBlock) { - currentFlow = preTryFlow; + // 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 finaly flows in both try and finally blocks are unreachable + addAntecedent(preFinallyLabel, preTryFlow); + 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 (!(currentFlow.flags & FlowFlags.Unreachable)) { + if ((flowAfterTry.flags & FlowFlags.Unreachable) && (flowAfterCatch.flags & FlowFlags.Unreachable)) { + currentFlow = flowAfterTry == reportedUnreachableFlow || flowAfterCatch === reportedUnreachableFlow + ? reportedUnreachableFlow + : unreachableFlow; + } + } } - // if try statement has finally block and flow after finally block is unreachable - keep it - // otherwise use whatever flow was accumulated at postFinallyLabel - if (!node.finallyBlock || !(currentFlow.flags & FlowFlags.Unreachable)) { - currentFlow = finishFlowLabel(postFinallyLabel); + else { + currentFlow = finishFlowLabel(preFinallyLabel); } } diff --git a/tests/baselines/reference/flowInFinally1.js b/tests/baselines/reference/flowInFinally1.js new file mode 100644 index 00000000000..b6bf494e832 --- /dev/null +++ b/tests/baselines/reference/flowInFinally1.js @@ -0,0 +1,33 @@ +//// [flowInFinally1.ts] + +class A { + constructor() { } + method() { } +} + +let a: A | null = null; + +try { + a = new A(); +} finally { + if (a) { + a.method(); + } +} + +//// [flowInFinally1.js] +var A = (function () { + function A() { + } + A.prototype.method = function () { }; + return A; +}()); +var a = null; +try { + a = new A(); +} +finally { + if (a) { + a.method(); + } +} diff --git a/tests/baselines/reference/flowInFinally1.symbols b/tests/baselines/reference/flowInFinally1.symbols new file mode 100644 index 00000000000..2ecedaee025 --- /dev/null +++ b/tests/baselines/reference/flowInFinally1.symbols @@ -0,0 +1,29 @@ +=== tests/cases/compiler/flowInFinally1.ts === + +class A { +>A : Symbol(A, Decl(flowInFinally1.ts, 0, 0)) + + constructor() { } + method() { } +>method : Symbol(A.method, Decl(flowInFinally1.ts, 2, 19)) +} + +let a: A | null = null; +>a : Symbol(a, Decl(flowInFinally1.ts, 6, 3)) +>A : Symbol(A, Decl(flowInFinally1.ts, 0, 0)) + +try { + a = new A(); +>a : Symbol(a, Decl(flowInFinally1.ts, 6, 3)) +>A : Symbol(A, Decl(flowInFinally1.ts, 0, 0)) + +} finally { + if (a) { +>a : Symbol(a, Decl(flowInFinally1.ts, 6, 3)) + + a.method(); +>a.method : Symbol(A.method, Decl(flowInFinally1.ts, 2, 19)) +>a : Symbol(a, Decl(flowInFinally1.ts, 6, 3)) +>method : Symbol(A.method, Decl(flowInFinally1.ts, 2, 19)) + } +} diff --git a/tests/baselines/reference/flowInFinally1.types b/tests/baselines/reference/flowInFinally1.types new file mode 100644 index 00000000000..c9cbab6dbd1 --- /dev/null +++ b/tests/baselines/reference/flowInFinally1.types @@ -0,0 +1,34 @@ +=== tests/cases/compiler/flowInFinally1.ts === + +class A { +>A : A + + constructor() { } + method() { } +>method : () => void +} + +let a: A | null = null; +>a : A | null +>A : A +>null : null +>null : null + +try { + a = new A(); +>a = new A() : A +>a : A | null +>new A() : A +>A : typeof A + +} finally { + if (a) { +>a : A | null + + a.method(); +>a.method() : void +>a.method : () => void +>a : A +>method : () => void + } +} diff --git a/tests/cases/compiler/flowInFinally1.ts b/tests/cases/compiler/flowInFinally1.ts new file mode 100644 index 00000000000..4ab2977e7df --- /dev/null +++ b/tests/cases/compiler/flowInFinally1.ts @@ -0,0 +1,16 @@ +// @strictNullChecks: true + +class A { + constructor() { } + method() { } +} + +let a: A | null = null; + +try { + a = new A(); +} finally { + if (a) { + a.method(); + } +} \ No newline at end of file From 09b9ea5507b8b70d5a7c9e325e2d7126b17404d9 Mon Sep 17 00:00:00 2001 From: Vladimir Matveev Date: Mon, 17 Oct 2016 14:29:24 -0700 Subject: [PATCH 2/2] address PR feedback --- src/compiler/binder.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 31732bc833f..086ec9dd927 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -1002,7 +1002,7 @@ 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 finaly flows in both try and finally blocks are unreachable + // 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); currentFlow = finishFlowLabel(preFinallyLabel); bind(node.finallyBlock); @@ -1014,7 +1014,7 @@ namespace ts { // console.log(2); // this line should be unreachable even if flow falls out of finally block if (!(currentFlow.flags & FlowFlags.Unreachable)) { if ((flowAfterTry.flags & FlowFlags.Unreachable) && (flowAfterCatch.flags & FlowFlags.Unreachable)) { - currentFlow = flowAfterTry == reportedUnreachableFlow || flowAfterCatch === reportedUnreachableFlow + currentFlow = flowAfterTry === reportedUnreachableFlow || flowAfterCatch === reportedUnreachableFlow ? reportedUnreachableFlow : unreachableFlow; }