diff --git a/.changeset/fix-no-unreachable-try-finally.md b/.changeset/fix-no-unreachable-try-finally.md new file mode 100644 index 000000000000..0649c926d241 --- /dev/null +++ b/.changeset/fix-no-unreachable-try-finally.md @@ -0,0 +1,5 @@ +--- +"@biomejs/biome": patch +--- + +Fixed [#4946](https://github.com/biomejs/biome/issues/4946): `noUnreachable` no longer reports code inside `finally` blocks as unreachable when there is a `break`, `continue`, or `return` in the corresponding `try` body. diff --git a/crates/biome_js_analyze/src/lint/correctness/no_unreachable.rs b/crates/biome_js_analyze/src/lint/correctness/no_unreachable.rs index 2e43c75670ca..afefcbe56b61 100644 --- a/crates/biome_js_analyze/src/lint/correctness/no_unreachable.rs +++ b/crates/biome_js_analyze/src/lint/correctness/no_unreachable.rs @@ -422,10 +422,20 @@ struct PathState<'cfg> { next_block: BlockId, /// Set of all blocks already visited on this path visited: RoaringBitmap, - /// Current terminating instruction for the path, if one was - /// encountered - terminator: Option>, + /// Current terminating instruction for the path, if one was encountered. + /// See [`Terminator`] for the semantics of each `Option` level. + terminator: Option, exception_handlers: Option<&'cfg [ExceptionHandler]>, + /// The terminator that was active before entering a `finally` block via a + /// dead implicit jump (i.e. when `break`/`continue`/`return`/`throw` + /// precedes the implicit try-body-to-finally fall-through). + /// + /// `None` means we are not currently inside such a finally block. + /// `Some(t)` stores the `Option` that was in `self.terminator` + /// when the finally block was entered; it is restored by `finally_fallthrough` + /// so that code *after* the `try/finally` construct is still correctly seen + /// as unreachable. + pre_finally_terminator: Option>, } /// Perform a simple reachability analysis on the control flow graph by @@ -433,7 +443,7 @@ struct PathState<'cfg> { fn traverse_cfg( cfg: &JsControlFlowGraph, signals: &mut UnreachableRanges, -) -> FxHashMap>>> { +) -> FxHashMap>> { let mut queue = VecDeque::new(); queue.push_back(PathState { @@ -441,6 +451,7 @@ fn traverse_cfg( visited: RoaringBitmap::new(), terminator: None, exception_handlers: None, + pre_finally_terminator: None, }); // This maps holds a list of "path state", the active terminator @@ -480,6 +491,7 @@ fn traverse_cfg( visited: path.visited.clone(), terminator: path.terminator, exception_handlers: find_catch_handlers(handlers), + pre_finally_terminator: None, }); } } @@ -495,10 +507,34 @@ fn traverse_cfg( InstructionKind::Statement => {} InstructionKind::Jump { conditional, - block, + block: jump_target, finally_fallthrough, } => { - handle_jump(&mut queue, &path, block, finally_fallthrough); + // If we are in dead code (has_direct_terminator) but the jump + // target is a cleanup (finally) handler for this block, the finally + // block is still reachable — it always runs before any early exit + // (break/continue/return/throw). Clear the terminator so the finally + // block is not incorrectly reported as unreachable. Store the + // original terminator in `pre_finally_terminator` so it can be + // restored when the finally block exits via `finally_fallthrough`. + let (effective_terminator, pre_finally_terminator) = if has_direct_terminator + && block + .cleanup_handlers + .iter() + .any(|h| h.target == jump_target) + { + (None, Some(path.terminator)) + } else { + (path.terminator, None) + }; + handle_jump( + &mut queue, + &path, + jump_target, + finally_fallthrough, + effective_terminator, + pre_finally_terminator, + ); // Jump is a terminator instruction if it's unconditional if path.terminator.is_none() && !conditional { @@ -565,12 +601,20 @@ fn find_catch_handlers(handlers: &[ExceptionHandler]) -> Option<&[ExceptionHandl } } -/// Create an additional visitor path from a jump instruction and push it to the queue +/// Create an additional visitor path from a jump instruction and push it to the queue. +/// `effective_terminator` is the terminator to propagate to the target block; callers may +/// pass `None` when the target is known to be reachable regardless of any prior terminator +/// (e.g. an implicit jump from the end of a try body to its finally block). +/// `pre_finally_terminator` should be set when `effective_terminator` was cleared to allow +/// a finally block to appear reachable; it stores the original terminator so it can be +/// restored when the finally block exits via `finally_fallthrough`. fn handle_jump<'cfg>( queue: &mut VecDeque>, path: &PathState<'cfg>, block: BlockId, finally_fallthrough: bool, + effective_terminator: Option, + pre_finally_terminator: Option>, ) { // If this jump is exiting a finally clause and this path is visiting // an exception handlers chain @@ -587,16 +631,45 @@ fn handle_jump<'cfg>( visited: path.visited.clone(), terminator: path.terminator, exception_handlers: Some(handlers), + pre_finally_terminator: None, + }); + } + } else if finally_fallthrough { + // Exiting a finally block in the normal (non-exception) context. + // Determine the terminator to propagate past the try/finally: + // - If the finally block itself terminates (e.g. contains a `return` + // or `throw`), prefer that as the most proximate cause. + // - Otherwise, if we entered finally via a dead implicit jump (the + // try body had `break`/`continue`/`return`), restore the saved + // pre-finally terminator so code after the try/finally is still + // correctly seen as unreachable. + let continuation_terminator = path + .terminator + .or(path.pre_finally_terminator.unwrap_or(None)); + + if !path.visited.contains(block.index()) { + queue.push_back(PathState { + next_block: block, + visited: path.visited.clone(), + terminator: continuation_terminator, + exception_handlers: path.exception_handlers, + pre_finally_terminator: None, }); } } else if !path.visited.contains(block.index()) { // Push the jump target block to the queue if it hasn't - // been visited yet in this path + // been visited yet in this path. + // Propagate `pre_finally_terminator` from the current path so it + // survives across internal jumps inside a finally block (e.g. an + // if/else inside finally) and is available when the block exits via + // `finally_fallthrough`. + let effective_pre_finally = pre_finally_terminator.or(path.pre_finally_terminator); queue.push_back(PathState { next_block: block, visited: path.visited.clone(), - terminator: path.terminator, + terminator: effective_terminator, exception_handlers: path.exception_handlers, + pre_finally_terminator: effective_pre_finally, }); } } @@ -616,6 +689,7 @@ fn handle_return<'cfg>( visited: path.visited.clone(), terminator: path.terminator, exception_handlers: Some(handlers), + pre_finally_terminator: None, }); } } @@ -967,6 +1041,16 @@ pub struct UnreachableRange { terminators: Vec, } +/// The terminator state of a path through the control flow graph. +/// +/// Each level of `Option` carries distinct semantics: +/// - Outer `Option`: `None` = the path has no terminator yet (code is still live); +/// `Some(_)` = a terminating instruction has been encountered on this path. +/// - Inner `Option`: `None` = the terminator has no associated +/// syntax node (e.g. an implicit jump); `Some(pt)` = the terminator corresponds +/// to a concrete syntax node that can be pointed to in a diagnostic. +type Terminator = Option; + #[derive(Debug, Clone, Copy)] struct PathTerminator { kind: JsSyntaxKind, diff --git a/crates/biome_js_analyze/tests/specs/correctness/noUnreachable/JsTryFinallyStatement.js b/crates/biome_js_analyze/tests/specs/correctness/noUnreachable/JsTryFinallyStatement.js index 2c4a3b706308..2eeaf8580512 100644 --- a/crates/biome_js_analyze/tests/specs/correctness/noUnreachable/JsTryFinallyStatement.js +++ b/crates/biome_js_analyze/tests/specs/correctness/noUnreachable/JsTryFinallyStatement.js @@ -1,3 +1,5 @@ +// should generate diagnostics + function JsTryFinallyStatement1() { try { tryBlock(); @@ -58,3 +60,63 @@ function JsTryFinallyStatement4() { afterTryStatement(); } + +// https://github.com/biomejs/biome/issues/4946 +// finally block should be reachable when there is a jump (break/continue/return) before it +function JsTryFinallyStatement5() { + while (true) { + try { + break; + } finally { + console.log("reachable"); + } + console.log("unreachable"); + } +} + +function JsTryFinallyStatement6() { + while (true) { + try { + continue; + } finally { + console.log("reachable"); + } + console.log("unreachable"); + } +} + +function JsTryFinallyStatement7() { + try { + return; + } finally { + console.log("reachable"); + } + console.log("unreachable"); +} + +// finally itself contains a return: code after the try/finally is unreachable +// due to the finally's return (not the try's return) +function JsTryFinallyStatement8() { + try { + return 1; + } finally { + return 2; + } + console.log("unreachable"); +} + +// nested try/finally: both inner and outer finally blocks should be reachable +function JsTryFinallyStatement9() { + while (true) { + try { + try { + break; + } finally { + console.log("inner finally reachable"); + } + } finally { + console.log("outer finally reachable"); + } + console.log("unreachable"); + } +} diff --git a/crates/biome_js_analyze/tests/specs/correctness/noUnreachable/JsTryFinallyStatement.js.snap b/crates/biome_js_analyze/tests/specs/correctness/noUnreachable/JsTryFinallyStatement.js.snap index d494b5985e9b..81c9db50a9bb 100644 --- a/crates/biome_js_analyze/tests/specs/correctness/noUnreachable/JsTryFinallyStatement.js.snap +++ b/crates/biome_js_analyze/tests/specs/correctness/noUnreachable/JsTryFinallyStatement.js.snap @@ -4,6 +4,8 @@ expression: JsTryFinallyStatement.js --- # Input ```js +// should generate diagnostics + function JsTryFinallyStatement1() { try { tryBlock(); @@ -65,142 +67,298 @@ function JsTryFinallyStatement4() { afterTryStatement(); } +// https://github.com/biomejs/biome/issues/4946 +// finally block should be reachable when there is a jump (break/continue/return) before it +function JsTryFinallyStatement5() { + while (true) { + try { + break; + } finally { + console.log("reachable"); + } + console.log("unreachable"); + } +} + +function JsTryFinallyStatement6() { + while (true) { + try { + continue; + } finally { + console.log("reachable"); + } + console.log("unreachable"); + } +} + +function JsTryFinallyStatement7() { + try { + return; + } finally { + console.log("reachable"); + } + console.log("unreachable"); +} + +// finally itself contains a return: code after the try/finally is unreachable +// due to the finally's return (not the try's return) +function JsTryFinallyStatement8() { + try { + return 1; + } finally { + return 2; + } + console.log("unreachable"); +} + +// nested try/finally: both inner and outer finally blocks should be reachable +function JsTryFinallyStatement9() { + while (true) { + try { + try { + break; + } finally { + console.log("inner finally reachable"); + } + } finally { + console.log("outer finally reachable"); + } + console.log("unreachable"); + } +} + ``` # Diagnostics ``` -JsTryFinallyStatement.js:11:5 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +JsTryFinallyStatement.js:13:5 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ × This code will never be reached ... - 9 │ } - 10 │ - > 11 │ afterFinallyReturn(); + 11 │ } + 12 │ + > 13 │ afterFinallyReturn(); │ ^^^^^^^^^^^^^^^^^^^^^ - 12 │ } - 13 │ + 14 │ } + 15 │ i ... because this statement will return from the function beforehand - 6 │ } finally { - 7 │ finallyClause(); - > 8 │ return; + 8 │ } finally { + 9 │ finallyClause(); + > 10 │ return; │ ^^^^^^^ - 9 │ } - 10 │ + 11 │ } + 12 │ ``` ``` -JsTryFinallyStatement.js:17:5 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +JsTryFinallyStatement.js:19:5 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ × This code will never be reached ... - 15 │ return; - 16 │ - > 17 │ try { + 17 │ return; + 18 │ + > 19 │ try { │ ^^^^^ - > 18 │ tryBlock(); + > 20 │ tryBlock(); ... - > 22 │ finallyClause(); - > 23 │ } + > 24 │ finallyClause(); + > 25 │ } │ ^ - 24 │ } - 25 │ + 26 │ } + 27 │ i ... because this statement will return from the function beforehand - 14 │ function JsTryFinallyStatement2() { - > 15 │ return; + 16 │ function JsTryFinallyStatement2() { + > 17 │ return; │ ^^^^^^^ - 16 │ - 17 │ try { + 18 │ + 19 │ try { ``` ``` -JsTryFinallyStatement.js:35:9 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +JsTryFinallyStatement.js:37:9 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ × This code will never be reached ... - 33 │ } - 34 │ - > 35 │ afterTryStatement1(); + 35 │ } + 36 │ + > 37 │ afterTryStatement1(); │ ^^^^^^^^^^^^^^^^^^^^^ - 36 │ } catch (err) { - 37 │ catchClause2(); + 38 │ } catch (err) { + 39 │ catchClause2(); i ... because this statement will return from the function beforehand - 30 │ } catch { - 31 │ } finally { - > 32 │ return; + 32 │ } catch { + 33 │ } finally { + > 34 │ return; │ ^^^^^^^ - 33 │ } - 34 │ + 35 │ } + 36 │ ``` ``` -JsTryFinallyStatement.js:36:19 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +JsTryFinallyStatement.js:38:19 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ × This code will never be reached ... - 35 │ afterTryStatement1(); - > 36 │ } catch (err) { + 37 │ afterTryStatement1(); + > 38 │ } catch (err) { │ ^ - > 37 │ catchClause2(); - > 38 │ } - > 39 │ - > 40 │ afterTryStatement2(); + > 39 │ catchClause2(); + > 40 │ } + > 41 │ + > 42 │ afterTryStatement2(); │ ^^^^^^^^^^^^^^^^^^^^^ - 41 │ } - 42 │ + 43 │ } + 44 │ i ... because this statement will return from the function beforehand - 30 │ } catch { - 31 │ } finally { - > 32 │ return; + 32 │ } catch { + 33 │ } finally { + > 34 │ return; │ ^^^^^^^ - 33 │ } - 34 │ + 35 │ } + 36 │ ``` ``` -JsTryFinallyStatement.js:59:5 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +JsTryFinallyStatement.js:61:5 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ × This code will never be reached ... - 57 │ } - 58 │ - > 59 │ afterTryStatement(); + 59 │ } + 60 │ + > 61 │ afterTryStatement(); │ ^^^^^^^^^^^^^^^^^^^^ - 60 │ } - 61 │ + 62 │ } + 63 │ i ... because either this statement ... - 44 │ try { - 45 │ tryBlock1(); - > 46 │ return; + 46 │ try { + 47 │ tryBlock1(); + > 48 │ return; │ ^^^^^^^ - 47 │ } catch { - 48 │ return; + 49 │ } catch { + 50 │ return; i ... or this statement will return from the function beforehand - 46 │ return; - 47 │ } catch { - > 48 │ return; + 48 │ return; + 49 │ } catch { + > 50 │ return; │ ^^^^^^^ - 49 │ } finally { - 50 │ if (value) { + 51 │ } finally { + 52 │ if (value) { + + +``` + +``` +JsTryFinallyStatement.js:73:9 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × This code will never be reached ... + + 71 │ console.log("reachable"); + 72 │ } + > 73 │ console.log("unreachable"); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 74 │ } + 75 │ } + + i ... because this statement will break the flow of the code beforehand + + 67 │ while (true) { + 68 │ try { + > 69 │ break; + │ ^^^^^^ + 70 │ } finally { + 71 │ console.log("reachable"); + + +``` + +``` +JsTryFinallyStatement.js:84:9 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × This code will never be reached ... + + 82 │ console.log("reachable"); + 83 │ } + > 84 │ console.log("unreachable"); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 85 │ } + 86 │ } + + i ... because this statement will continue the loop beforehand + + 78 │ while (true) { + 79 │ try { + > 80 │ continue; + │ ^^^^^^^^^ + 81 │ } finally { + 82 │ console.log("reachable"); + + +``` + +``` +JsTryFinallyStatement.js:94:5 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × This code will never be reached ... + + 92 │ console.log("reachable"); + 93 │ } + > 94 │ console.log("unreachable"); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 95 │ } + 96 │ + + i ... because this statement will return from the function beforehand + + 88 │ function JsTryFinallyStatement7() { + 89 │ try { + > 90 │ return; + │ ^^^^^^^ + 91 │ } finally { + 92 │ console.log("reachable"); + + +``` + +``` +JsTryFinallyStatement.js:105:5 lint/correctness/noUnreachable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × This code will never be reached ... + + 103 │ return 2; + 104 │ } + > 105 │ console.log("unreachable"); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 106 │ } + 107 │ + + i ... because this statement will return from the function beforehand + + 101 │ return 1; + 102 │ } finally { + > 103 │ return 2; + │ ^^^^^^^^^ + 104 │ } + 105 │ console.log("unreachable"); ```