Skip to content

Commit 046ff3f

Browse files
committed
feat(linter/eslint): add no_unreachable rule. (#3238)
closes #621 [no-unreachable](https://github.com/eslint/eslint/blob/069aa680c78b8516b9a1b568519f1d01e74fb2a2/lib/rules/no-unreachable.js#L196) [oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9406195143/job/25909079029) This rule is done but since it is running for every possible statement and does quite a bit of work on them to determine whether it is 100% reachable or not; The performance in my opinion is kind of abysmal. I'll try to work it out, I know Biome does 2 types of checks to simplify the rule for some nodes, However, they have a lot more false negatives than our implementation. ##### Here is one example of those [false negatives](https://biomejs.dev/playground/?code=ZgB1AG4AYwB0AGkAbwBuACAAeAAoACkAIAB7ACAAZABvACAAewAgAGEAKAApADsAIAB9ACAAdwBoAGkAbABlACgAdAByAHUAZQApADsAIABiACgAKQA7ACAAfQA%3D) ------------- ### Update 1: I've benchmarked this rule using only the simplified reachability checks and it was around 5% faster, To be honest, it isn't much improvement especially considering that we can only use this check for a small portion of nodes and even that is accompanied by newly introduced checks which would lessen the amount of performance gain further. Most of the performance regression is because of allocations during our depth first search since we have to store both the visited and finished nodes which results in a bunch of rapid-fire allocations back to back. Currently, At the moment I don't have a great idea of how to improve it, We may have to implement our own graph to use arenas underneath. Given that this rule is the most extensive use case of control flow (It doesn't come with a limited scope similar to property and constructor rules already implemented) this performance drop might be reasonable to some extent. ------------ ### Update 2: I reworked my approach in 2 senses, First I used @Boshen's suggestion inspired by TypeScript and kept some of the reachability information in the basic block structure instead of calculating it on the fly. It is done by propagating the `Unreachable` edge and `Unreachable` instruction throughout subgraphs. This for sure helped with the performance but the next part is what never failed to amaze me, Going from something near `O(n!)` in the worst-case scenario to `O(n^2)` (in the worst-case scenario). By changing the approach instead of checking the reachability of each statement we do it in 3 paths; First, we do a path on the entire CFG and query all reachable but suspicious cases, and then we do another path on each of these suspicions subgraphs to determine the reachability with higher confidence. Finally, we iterate all of the appropriate nodes and check their reachability status according to the information collected in 2 previous paths. With these 2 this rule went from `-24%` to `~-2%`. This performance gain doesn't come for free though; It increases the likelihood of false positives/negatives, But as long as we are passing our `ecosystem-ci` it should be fine. We can always sacrifice some performance to check for edge cases if there are any. [new oxlint-echosystem-ci result](https://github.com/rzvxa/oxlint-ecosystem-ci/actions/runs/9490791181)
1 parent 9c31ed9 commit 046ff3f

File tree

6 files changed

+548
-5
lines changed

6 files changed

+548
-5
lines changed

crates/oxc_ast/src/ast/js.rs

+4
Original file line numberDiff line numberDiff line change
@@ -1690,6 +1690,10 @@ impl<'a> VariableDeclaration<'a> {
16901690
pub fn is_typescript_syntax(&self) -> bool {
16911691
self.modifiers.contains(ModifierKind::Declare)
16921692
}
1693+
1694+
pub fn has_init(&self) -> bool {
1695+
self.declarations.iter().any(|decl| decl.init.is_some())
1696+
}
16931697
}
16941698

16951699
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]

crates/oxc_linter/src/rules.rs

+2
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ mod eslint {
9595
pub mod no_ternary;
9696
pub mod no_this_before_super;
9797
pub mod no_undef;
98+
pub mod no_unreachable;
9899
pub mod no_unsafe_finally;
99100
pub mod no_unsafe_negation;
100101
pub mod no_unsafe_optional_chaining;
@@ -483,6 +484,7 @@ oxc_macros::declare_all_lint_rules! {
483484
eslint::no_shadow_restricted_names,
484485
eslint::no_sparse_arrays,
485486
eslint::no_undef,
487+
eslint::no_unreachable,
486488
eslint::no_unsafe_finally,
487489
eslint::no_unsafe_negation,
488490
eslint::no_unsafe_optional_chaining,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,307 @@
1+
use oxc_ast::{ast::VariableDeclarationKind, AstKind};
2+
use oxc_diagnostics::OxcDiagnostic;
3+
use oxc_macros::declare_oxc_lint;
4+
use oxc_semantic::{
5+
petgraph::{
6+
visit::{depth_first_search, Control, DfsEvent, EdgeRef},
7+
Direction,
8+
},
9+
EdgeType, InstructionKind,
10+
};
11+
use oxc_span::{GetSpan, Span};
12+
13+
use crate::{context::LintContext, rule::Rule};
14+
15+
fn no_unreachable_diagnostic(span: Span) -> OxcDiagnostic {
16+
OxcDiagnostic::error("eslint(no-unreachable): Unreachable code.").with_labels([span.into()])
17+
}
18+
19+
/// <https://github.com/eslint/eslint/blob/069aa680c78b8516b9a1b568519f1d01e74fb2a2/lib/rules/no-unreachable.js#L196>
20+
#[derive(Debug, Default, Clone)]
21+
pub struct NoUnreachable;
22+
23+
declare_oxc_lint!(
24+
/// ### What it does
25+
///
26+
/// Disallow unreachable code after `return`, `throw`, `continue`, and `break` statements
27+
///
28+
NoUnreachable,
29+
correctness
30+
);
31+
32+
impl Rule for NoUnreachable {
33+
fn run_once(&self, ctx: &LintContext) {
34+
let nodes = ctx.nodes();
35+
let Some(root) = nodes.root_node() else { return };
36+
let cfg = ctx.semantic().cfg();
37+
let graph = &cfg.graph;
38+
39+
// A pre-allocated vector containing the reachability status of all the basic blocks.
40+
// We initialize this vector with all nodes set to `unreachable` since if we don't visit a
41+
// node in our paths then it should be unreachable by definition.
42+
let mut unreachables = vec![true; cfg.basic_blocks.len()];
43+
44+
// All of the end points of infinite loops we encountered.
45+
let mut infinite_loops = Vec::new();
46+
47+
// Set the root as reachable.
48+
unreachables[root.cfg_id().index()] = false;
49+
50+
// In our first path we first check if each block is definitely unreachable, If it is then
51+
// we set it as such, If we encounter an infinite loop we keep its end block since it can
52+
// prevent other reachable blocks from ever getting executed.
53+
let _: Control<()> = depth_first_search(graph, Some(root.cfg_id()), |event| match event {
54+
DfsEvent::Finish(node, _) => {
55+
let bb = cfg.basic_block(node);
56+
let unreachable = if bb.unreachable {
57+
true
58+
} else {
59+
graph
60+
.edges_directed(node, Direction::Incoming)
61+
.any(|edge| matches!(edge.weight(), EdgeType::Join))
62+
};
63+
unreachables[node.index()] = unreachable;
64+
65+
if let Some(it) = cfg.is_infinite_loop_start(node, nodes) {
66+
infinite_loops.push(it);
67+
}
68+
69+
Control::Continue
70+
}
71+
_ => Control::Continue,
72+
});
73+
74+
// In the second path we go for each infinite loop end block and follow it marking all
75+
// edges as unreachable unless they have a reachable jump (eg. break).
76+
for loop_ in infinite_loops {
77+
// A loop end block usually is also its condition and start point but what is common
78+
// in all cases is that it may have `Jump` or `Backedge` edges so we only want to
79+
// follow the `Normal` edges as these are the exiting edges.
80+
let starts: Vec<_> = graph
81+
.edges_directed(loop_.1, Direction::Outgoing)
82+
.filter(|it| matches!(it.weight(), EdgeType::Normal))
83+
.map(|it| it.target())
84+
.collect();
85+
86+
// Search with all `Normal` edges as starting point(s).
87+
let _: Control<()> = depth_first_search(graph, starts, |event| match event {
88+
DfsEvent::Discover(node, _) => {
89+
let mut incoming = graph.edges_directed(node, Direction::Incoming);
90+
if incoming.any(|e| match e.weight() {
91+
// `NewFunction` is always reachable
92+
| EdgeType::NewFunction
93+
// `Finalize` can be reachable if we encounter an error in the loop.
94+
| EdgeType::Finalize => true,
95+
96+
// If we have an incoming `Jump` and it is from a `Break` instruction,
97+
// We know with high confidence that we are visiting a reachable block.
98+
// NOTE: May cause false negatives but I couldn't think of one.
99+
EdgeType::Jump
100+
if cfg
101+
.basic_block(e.source())
102+
.instructions()
103+
.iter()
104+
.any(|it| matches!(it.kind, InstructionKind::Break(_))) =>
105+
{
106+
true
107+
}
108+
_ => false,
109+
}) {
110+
// We prune this branch if it is reachable from this point forward.
111+
Control::Prune
112+
} else {
113+
// Otherwise we set it to unreachable and continue.
114+
unreachables[node.index()] = true;
115+
Control::Continue
116+
}
117+
}
118+
_ => Control::Continue,
119+
});
120+
}
121+
for node in ctx.nodes().iter() {
122+
// exit early if we are not visiting a statement.
123+
if !node.kind().is_statement() {
124+
continue;
125+
}
126+
127+
// exit early if it is an empty statement.
128+
if matches!(node.kind(), AstKind::EmptyStatement(_)) {
129+
continue;
130+
}
131+
132+
if matches! {
133+
node.kind(),
134+
AstKind::VariableDeclaration(decl)
135+
if matches!(decl.kind, VariableDeclarationKind::Var) && !decl.has_init()
136+
} {
137+
// Skip `var` declarations without any initialization,
138+
// These work because of the JavaScript hoisting rules.
139+
continue;
140+
}
141+
142+
if unreachables[node.cfg_id().index()] {
143+
ctx.diagnostic(no_unreachable_diagnostic(node.kind().span()));
144+
}
145+
}
146+
}
147+
}
148+
149+
#[test]
150+
fn test() {
151+
use crate::tester::Tester;
152+
153+
let pass = vec![
154+
"function foo() { function bar() { return 1; } return bar(); }",
155+
"function foo() { return bar(); function bar() { return 1; } }",
156+
"function foo() { return x; var x; }",
157+
"function foo() { var x = 1; var y = 2; }",
158+
"function foo() { var x = 1; var y = 2; return; }",
159+
"while (true) { switch (foo) { case 1: x = 1; x = 2;} }",
160+
"while (true) { break; var x; }",
161+
"while (true) { continue; var x, y; }",
162+
"while (true) { throw 'message'; var x; }",
163+
"while (true) { if (true) break; var x = 1; }",
164+
"while (true) continue;",
165+
"switch (foo) { case 1: break; var x; }",
166+
"switch (foo) { case 1: break; var x; default: throw true; };",
167+
"const arrow_direction = arrow => { switch (arrow) { default: throw new Error(); };}",
168+
"var x = 1; y = 2; throw 'uh oh'; var y;",
169+
"function foo() { var x = 1; if (x) { return; } x = 2; }",
170+
"function foo() { var x = 1; if (x) { } else { return; } x = 2; }",
171+
"function foo() { var x = 1; switch (x) { case 0: break; default: return; } x = 2; }",
172+
"function foo() { var x = 1; while (x) { return; } x = 2; }",
173+
"function foo() { var x = 1; for (x in {}) { return; } x = 2; }",
174+
"function foo() { var x = 1; try { return; } finally { x = 2; } }",
175+
"function foo() { var x = 1; for (;;) { if (x) break; } x = 2; }",
176+
"A: { break A; } foo()",
177+
"function* foo() { try { yield 1; return; } catch (err) { return err; } }",
178+
"function foo() { try { bar(); return; } catch (err) { return err; } }",
179+
"function foo() { try { a.b.c = 1; return; } catch (err) { return err; } }",
180+
"class C { foo = reachable; }",
181+
"class C { foo = reachable; constructor() {} }",
182+
"class C extends B { foo = reachable; }",
183+
"class C extends B { foo = reachable; constructor() { super(); } }",
184+
"class C extends B { static foo = reachable; constructor() {} }",
185+
"function foo() { var x = 1; for (;x == 1;) { if (x) continue; } x = 2; }",
186+
"
187+
if (a) {
188+
a();
189+
} else {
190+
for (let i = 1; i <= 10; i++) {
191+
b();
192+
}
193+
194+
for (let i = 1; i <= 10; i++) {
195+
c();
196+
}
197+
}
198+
",
199+
"
200+
try {
201+
throw 'error';
202+
} catch (err) {
203+
b();
204+
}
205+
c();
206+
",
207+
"
208+
export const getPagePreviewText = (page) => {
209+
if (!a) {
210+
return '';
211+
}
212+
while (a && b > c && d-- > 0) {
213+
}
214+
};
215+
",
216+
"
217+
try {
218+
for (const a of b) {
219+
c();
220+
}
221+
222+
while (true) {
223+
d();
224+
}
225+
} finally {
226+
}
227+
",
228+
"
229+
switch (authType) {
230+
case 1:
231+
return a();
232+
case 2:
233+
return b();
234+
case 3:
235+
return c();
236+
}
237+
d();
238+
",
239+
"
240+
try {
241+
a();
242+
} catch (e) {
243+
b();
244+
} finally {
245+
c();
246+
}
247+
d();
248+
",
249+
"
250+
try {
251+
while (true) {
252+
a();
253+
}
254+
} finally {
255+
b();
256+
}
257+
",
258+
];
259+
260+
let fail = vec![
261+
//[{ messageId: "unreachableCode", type: "VariableDeclaration" }]
262+
"function foo() { return x; var x = 1; }",
263+
//[{ messageId: "unreachableCode", type: "VariableDeclaration" }]
264+
"function foo() { return x; var x, y = 1; }",
265+
"while (true) { break; var x = 1; }",
266+
//[{ messageId: "unreachableCode", type: "VariableDeclaration" }]
267+
"while (true) { continue; var x = 1; }",
268+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
269+
"function foo() { return; x = 1; }",
270+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
271+
"function foo() { throw error; x = 1; }",
272+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
273+
"while (true) { break; x = 1; }",
274+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
275+
"while (true) { continue; x = 1; }",
276+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
277+
"function foo() { switch (foo) { case 1: return; x = 1; } }",
278+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
279+
"function foo() { switch (foo) { case 1: throw e; x = 1; } }",
280+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
281+
"while (true) { switch (foo) { case 1: break; x = 1; } }",
282+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
283+
"while (true) { switch (foo) { case 1: continue; x = 1; } }",
284+
//[{ messageId: "unreachableCode", type: "VariableDeclaration" }]
285+
"var x = 1; throw 'uh oh'; var y = 2;",
286+
// [{ messageId: "unreachableCode", type: "ExpressionStatement" }]
287+
"function foo() { var x = 1; if (x) { return; } else { throw e; } x = 2; }",
288+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
289+
"function foo() { var x = 1; if (x) return; else throw -1; x = 2; }",
290+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
291+
"function foo() { var x = 1; try { return; } finally {} x = 2; }",
292+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
293+
"function foo() { var x = 1; try { } finally { return; } x = 2; }",
294+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
295+
"function foo() { var x = 1; do { return; } while (x); x = 2; }",
296+
// [{ messageId: "unreachableCode", type: "ExpressionStatement" }]
297+
"function foo() { var x = 1; while (x) { if (x) break; else continue; x = 2; } }",
298+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
299+
"function foo() { var x = 1; for (;;) { if (x) continue; } x = 2; }",
300+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
301+
"function foo() { var x = 1; while (true) { } x = 2; }",
302+
//[{ messageId: "unreachableCode", type: "ExpressionStatement" }]
303+
"function foo() { var x = 1; do { } while (true); x = 2; }",
304+
];
305+
306+
Tester::new(NoUnreachable::NAME, pass, fail).test_and_snapshot();
307+
}

0 commit comments

Comments
 (0)