Skip to content

Commit c06d296

Browse files
CedricCondayCedricConday
committed
Fix reachability of code after try with a branching finally (#63583)
The reachability analysis caches results for shared flow nodes in a process-global cache (flowNodeReachable). When a ReduceLabel is active (try/finally), it temporarily rewrites the target label's antecedents, making the reachability of nodes reached during that walk dependent on the reduced context. The ReduceLabel handler cleared only the single lastFlowNode cache, not the shared cache, so a branch inside a finally block (a shared merge node) had its reachability cached under one reduce context and reused under another, corrupting end-of-function and unreachable-code analysis. Suppress the shared reachability cache while a label reduction is active, depth-counted for nested try/finally.
1 parent 637d574 commit c06d296

5 files changed

Lines changed: 464 additions & 1 deletion

‎src/compiler/checker.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2364,6 +2364,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
23642364
var sharedFlowTypes: FlowType[] = [];
23652365
var flowNodeReachable: (boolean | undefined)[] = [];
23662366
var flowNodePostSuper: (boolean | undefined)[] = [];
2367+
var reduceLabelDepth = 0;
23672368
var potentialThisCollisions: Node[] = [];
23682369
var potentialNewTargetCollisions: Node[] = [];
23692370
var potentialWeakMapSetCollisions: Node[] = [];
@@ -28867,7 +28868,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
2886728868
}
2886828869
const flags = flow.flags;
2886928870
if (flags & FlowFlags.Shared) {
28870-
if (!noCacheCheck) {
28871+
// While a reduce label is active (try/finally), a shared node's reachability is
28872+
// context-dependent on the temporarily reduced label antecedents, so it must not
28873+
// be served from or written to the process-global cache (see #63583).
28874+
if (!noCacheCheck && reduceLabelDepth === 0) {
2887128875
const id = getFlowNodeId(flow);
2887228876
const reachable = flowNodeReachable[id];
2887328877
return reachable !== undefined ? reachable : (flowNodeReachable[id] = isReachableFlowNodeWorker(flow, /*noCacheCheck*/ true));
@@ -28920,7 +28924,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
2892028924
const target = (flow as FlowReduceLabel).node.target;
2892128925
const saveAntecedents = target.antecedent;
2892228926
target.antecedent = (flow as FlowReduceLabel).node.antecedents;
28927+
// Suppress the shared reachability cache for the duration of the reduced walk:
28928+
// reachability computed under the reduced antecedents is not valid outside this
28929+
// context (and vice versa). Depth-counted to handle nested try/finally (see #63583).
28930+
reduceLabelDepth++;
2892328931
const result = isReachableFlowNodeWorker((flow as FlowReduceLabel).antecedent, /*noCacheCheck*/ false);
28932+
reduceLabelDepth--;
2892428933
target.antecedent = saveAntecedents;
2892528934
return result;
2892628935
}

‎tests/baselines/reference/controlFlowSwitchReturnWithBranchingFinally.errors.txt‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
controlFlowSwitchReturnWithBranchingFinally.ts(32,9): error TS7027: Unreachable code detected.
2+
3+
4+
==== controlFlowSwitchReturnWithBranchingFinally.ts (1 errors) ====
5+
// https://github.com/microsoft/TypeScript/issues/63583
6+
// A branch inside a `finally` block must not corrupt the reachability of the
7+
// flow after the protected `try`. Here every `switch` case returns, so the
8+
// function fully returns and the trailing code is genuinely unreachable —
9+
// regardless of what the `finally` block does.
10+
11+
let y = false;
12+
13+
// Was wrongly: "Function lacks ending return statement..." because the branch
14+
// in `finally` (the short-circuit of `||=`) polluted the shared reachability cache.
15+
function test1(x: boolean): number {
16+
try {
17+
switch (x) {
18+
case true: return 1;
19+
case false: return 0;
20+
}
21+
}
22+
finally { y ||= true; }
23+
}
24+
25+
// test2 documents the *correct* counterpart: with an explicit trailing return,
26+
// that return is genuinely unreachable (the switch is exhaustive on `boolean`),
27+
// so reporting it is correct and finally-independent. Before the fix, test1 and
28+
// test2 contradicted each other (test1 demanded this return; test2 then rejected
29+
// it). After the fix they are consistent: test1 needs no return, test2's is dead.
30+
function test2(x: boolean): number {
31+
try {
32+
switch (x) {
33+
case true: return 1;
34+
case false: return 0;
35+
}
36+
return 0; // correctly flagged unreachable
37+
~~~~~~~~~
38+
!!! error TS7027: Unreachable code detected.
39+
}
40+
finally { y ||= true; }
41+
}
42+
43+
// A plain `if` (read with a branch) in `finally` triggered the same bug.
44+
function test3(x: boolean): number {
45+
try {
46+
switch (x) {
47+
case true: return 1;
48+
case false: return 0;
49+
}
50+
}
51+
finally { if (y) { } }
52+
}
53+
54+
// Control: linear (non-branching) finally always worked.
55+
function test4(x: boolean): number {
56+
try {
57+
switch (x) {
58+
case true: return 1;
59+
case false: return 0;
60+
}
61+
}
62+
finally { y = true; }
63+
}
64+
65+
// Nested try/finally with branching finally blocks (exercises depth counting).
66+
function test5(x: boolean): number {
67+
try {
68+
try {
69+
switch (x) {
70+
case true: return 1;
71+
case false: return 0;
72+
}
73+
}
74+
finally { y ||= true; }
75+
}
76+
finally { if (y) { } }
77+
}
78+

‎tests/baselines/reference/controlFlowSwitchReturnWithBranchingFinally.symbols‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
//// [tests/cases/compiler/controlFlowSwitchReturnWithBranchingFinally.ts] ////
2+
3+
=== controlFlowSwitchReturnWithBranchingFinally.ts ===
4+
// https://github.com/microsoft/TypeScript/issues/63583
5+
// A branch inside a `finally` block must not corrupt the reachability of the
6+
// flow after the protected `try`. Here every `switch` case returns, so the
7+
// function fully returns and the trailing code is genuinely unreachable —
8+
// regardless of what the `finally` block does.
9+
10+
let y = false;
11+
>y : Symbol(y, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 6, 3))
12+
13+
// Was wrongly: "Function lacks ending return statement..." because the branch
14+
// in `finally` (the short-circuit of `||=`) polluted the shared reachability cache.
15+
function test1(x: boolean): number {
16+
>test1 : Symbol(test1, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 6, 14))
17+
>x : Symbol(x, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 10, 15))
18+
19+
try {
20+
switch (x) {
21+
>x : Symbol(x, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 10, 15))
22+
23+
case true: return 1;
24+
case false: return 0;
25+
}
26+
}
27+
finally { y ||= true; }
28+
>y : Symbol(y, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 6, 3))
29+
}
30+
31+
// test2 documents the *correct* counterpart: with an explicit trailing return,
32+
// that return is genuinely unreachable (the switch is exhaustive on `boolean`),
33+
// so reporting it is correct and finally-independent. Before the fix, test1 and
34+
// test2 contradicted each other (test1 demanded this return; test2 then rejected
35+
// it). After the fix they are consistent: test1 needs no return, test2's is dead.
36+
function test2(x: boolean): number {
37+
>test2 : Symbol(test2, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 18, 1))
38+
>x : Symbol(x, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 25, 15))
39+
40+
try {
41+
switch (x) {
42+
>x : Symbol(x, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 25, 15))
43+
44+
case true: return 1;
45+
case false: return 0;
46+
}
47+
return 0; // correctly flagged unreachable
48+
}
49+
finally { y ||= true; }
50+
>y : Symbol(y, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 6, 3))
51+
}
52+
53+
// A plain `if` (read with a branch) in `finally` triggered the same bug.
54+
function test3(x: boolean): number {
55+
>test3 : Symbol(test3, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 34, 1))
56+
>x : Symbol(x, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 37, 15))
57+
58+
try {
59+
switch (x) {
60+
>x : Symbol(x, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 37, 15))
61+
62+
case true: return 1;
63+
case false: return 0;
64+
}
65+
}
66+
finally { if (y) { } }
67+
>y : Symbol(y, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 6, 3))
68+
}
69+
70+
// Control: linear (non-branching) finally always worked.
71+
function test4(x: boolean): number {
72+
>test4 : Symbol(test4, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 45, 1))
73+
>x : Symbol(x, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 48, 15))
74+
75+
try {
76+
switch (x) {
77+
>x : Symbol(x, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 48, 15))
78+
79+
case true: return 1;
80+
case false: return 0;
81+
}
82+
}
83+
finally { y = true; }
84+
>y : Symbol(y, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 6, 3))
85+
}
86+
87+
// Nested try/finally with branching finally blocks (exercises depth counting).
88+
function test5(x: boolean): number {
89+
>test5 : Symbol(test5, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 56, 1))
90+
>x : Symbol(x, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 59, 15))
91+
92+
try {
93+
try {
94+
switch (x) {
95+
>x : Symbol(x, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 59, 15))
96+
97+
case true: return 1;
98+
case false: return 0;
99+
}
100+
}
101+
finally { y ||= true; }
102+
>y : Symbol(y, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 6, 3))
103+
}
104+
finally { if (y) { } }
105+
>y : Symbol(y, Decl(controlFlowSwitchReturnWithBranchingFinally.ts, 6, 3))
106+
}
107+

0 commit comments

Comments
 (0)