Skip to content

Properly reflect CFA effects of return in try or catch blocks #35730

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

Merged
merged 3 commits into from
Dec 18, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
14 changes: 9 additions & 5 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1198,8 +1198,10 @@ namespace ts {
// 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 saveReturnTarget = currentReturnTarget;
const saveExceptionTarget = currentExceptionTarget;
currentExceptionTarget = createBranchLabel();
currentReturnTarget = createBranchLabel();
currentExceptionTarget = node.catchClause ? createBranchLabel() : currentReturnTarget;
addAntecedent(currentExceptionTarget, currentFlow);
bind(node.tryBlock);
addAntecedent(preFinallyLabel, currentFlow);
Expand All @@ -1211,22 +1213,24 @@ namespace ts {
// 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();
currentExceptionTarget = currentReturnTarget;
addAntecedent(currentExceptionTarget, currentFlow);
bind(node.catchClause);
addAntecedent(preFinallyLabel, currentFlow);
flowAfterCatch = currentFlow;
}
const exceptionTarget = finishFlowLabel(currentExceptionTarget);
currentReturnTarget = saveReturnTarget;
currentExceptionTarget = saveExceptionTarget;
if (node.finallyBlock) {
// 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
// 3) Return in try or catch block of a try-finally or try-catch-finally
// 4) Exception in try block of a try-finally
// 5) 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)
// five 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
Expand Down
132 changes: 132 additions & 0 deletions tests/baselines/reference/tryCatchFinallyControlFlow.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
tests/cases/compiler/tryCatchFinallyControlFlow.ts(105,5): error TS7027: Unreachable code detected.


==== tests/cases/compiler/tryCatchFinallyControlFlow.ts (1 errors) ====
// 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
}

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

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

function f7() {
let x: 0 | 1 | 2 | 3 = 0;
try {
x = 1;
return;
}
catch (e) {
x = 2;
return;
}
finally {
x; // 0 | 1 | 2
}
x; // Unreachable
~~
!!! error TS7027: Unreachable code detected.
}

// Repro from #35644

const main = () => {
let hoge: string | undefined = undefined;
try {
hoge = 'hoge!';
return;
}
catch {
return;
}
finally {
if (hoge) {
hoge.length;
}
return;
}
}

125 changes: 125 additions & 0 deletions tests/baselines/reference/tryCatchFinallyControlFlow.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,71 @@ function f4() {
}
x; // 1 | 2
}

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

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

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

// Repro from #35644

const main = () => {
let hoge: string | undefined = undefined;
try {
hoge = 'hoge!';
return;
}
catch {
return;
}
finally {
if (hoge) {
hoge.length;
}
return;
}
}


//// [tryCatchFinallyControlFlow.js]
Expand Down Expand Up @@ -119,3 +184,63 @@ function f4() {
}
x; // 1 | 2
}
function f5() {
var x = 0;
try {
x = 1;
return;
}
catch (e) {
x = 2;
}
finally {
x; // 0 | 1 | 2
}
x; // 2
}
function f6() {
var x = 0;
try {
x = 1;
}
catch (e) {
x = 2;
return;
}
finally {
x; // 0 | 1 | 2
}
x; // 1
}
function f7() {
var x = 0;
try {
x = 1;
return;
}
catch (e) {
x = 2;
return;
}
finally {
x; // 0 | 1 | 2
}
x; // Unreachable
}
// Repro from #35644
var main = function () {
var hoge = undefined;
try {
hoge = 'hoge!';
return;
}
catch (_a) {
return;
}
finally {
if (hoge) {
hoge.length;
}
return;
}
};
Loading