From c5f0ed3182b31100ab64f43036c29ec2adaac8eb Mon Sep 17 00:00:00 2001 From: camc314 <18101008+camc314@users.noreply.github.com> Date: Thu, 19 Mar 2026 22:32:49 +0000 Subject: [PATCH] feat(linter/array-callback-return): use CFG for analysis (#20498) --- .../rules/eslint/array_callback_return/mod.rs | 3 +- .../array_callback_return/return_checker.rs | 529 ++++-------------- .../eslint_array_callback_return.snap | 8 + 3 files changed, 119 insertions(+), 421 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/array_callback_return/mod.rs b/crates/oxc_linter/src/rules/eslint/array_callback_return/mod.rs index b625f35ded070..f0caf1f4a96bb 100644 --- a/crates/oxc_linter/src/rules/eslint/array_callback_return/mod.rs +++ b/crates/oxc_linter/src/rules/eslint/array_callback_return/mod.rs @@ -198,7 +198,7 @@ impl Rule for ArrayCallbackReturn { let return_status = if always_explicit_return { StatementReturnStatus::AlwaysExplicit } else { - check_function_body(function_body) + check_function_body(node.id(), ctx.semantic()) }; match (array_method, self.check_for_each, self.allow_void, self.allow_implicit) { @@ -665,6 +665,7 @@ const _test = fruits.map((fruit) => { ("foo.every(() => {})", None), ("foo.every(function() { if (a) return true; })", None), ("foo.every(function cb() { if (a) return true; })", None), + ("foo.map(() => { while (true) { if (a) break; return 1; } })", None), ("foo.every(function() { switch (a) { case 0: break; default: return true; } })", None), ("foo.every(function foo() { switch (a) { case 0: break; default: return true; } })", None), ("foo.every(function() { try { bar(); } catch (err) { return true; } })", None), diff --git a/crates/oxc_linter/src/rules/eslint/array_callback_return/return_checker.rs b/crates/oxc_linter/src/rules/eslint/array_callback_return/return_checker.rs index 0f255cbe6da93..e345e84ca7eb3 100644 --- a/crates/oxc_linter/src/rules/eslint/array_callback_return/return_checker.rs +++ b/crates/oxc_linter/src/rules/eslint/array_callback_return/return_checker.rs @@ -1,12 +1,17 @@ use oxc_allocator::Vec as AllocatorVec; use oxc_ast::ast::{ - ArrowFunctionExpression, BlockStatement, Expression, Function, FunctionBody, ReturnStatement, - Statement, SwitchCase, UnaryOperator, + ArrowFunctionExpression, Expression, Function, FunctionBody, ReturnStatement, Statement, + UnaryOperator, }; use oxc_ast_visit::Visit; -use oxc_ecmascript::{ToBoolean, WithoutGlobalReferenceInformation}; -use oxc_semantic::ScopeFlags; +use oxc_cfg::{ + EdgeType, InstructionKind, ReturnInstructionKind, + graph::{Direction, visit::EdgeRef}, +}; + +use oxc_semantic::{NodeId, ScopeFlags, Semantic}; use oxc_span::{GetSpan, Span}; +use rustc_hash::FxHashSet; /// `StatementReturnStatus` describes whether the CFG corresponding to /// the statement is termitated by return statement in all/some/nome of @@ -37,44 +42,6 @@ pub enum StatementReturnStatus { } impl StatementReturnStatus { - /// Join the status of two branches. Similar to a logical *and* operation. - /// - /// E.g., - /// if (test) { - /// return a // `AlwaysExplicit` - /// } else { - /// var a = 0; // `NotReturn` - /// } - /// - /// will produce `SomeExplicit` for the whole if statement - pub fn join(self, rhs: Self) -> Self { - let must_return = self.must_return() && rhs.must_return(); - let explicit = self.may_return_explicit() || rhs.may_return_explicit(); - let implicit = self.may_return_implicit() || rhs.may_return_implicit(); - - Self::create(must_return, explicit, implicit) - } - - /// Union the status of two sequential statements. Similar to a logical *or* operation. - /// - /// E.g., - /// { - /// if (test) { - /// return a; - /// } // `SomeExplicit` - /// - /// return // `AlwaysImplicit` - /// } - /// - /// will produce `AlwaysMixed` for the block statement. - pub fn union(self, rhs: Self) -> Self { - let must_return = self.must_return() || rhs.must_return(); - let explicit = self.may_return_explicit() || rhs.may_return_explicit(); - let implicit = self.may_return_implicit() || rhs.may_return_implicit(); - - Self::create(must_return, explicit, implicit) - } - fn create(must_return: bool, maybe_explicit: bool, maybe_implicit: bool) -> Self { match (must_return, maybe_explicit, maybe_implicit) { (true, true, true) => Self::AlwaysMixed, @@ -107,20 +74,105 @@ impl StatementReturnStatus { } } -pub fn check_function_body(function: &FunctionBody) -> StatementReturnStatus { - // function body can be viewed as a block statement, but we don't - // short-circuit to catch all the possible returns. - // E.g. - // foo.map(() => { - // return 1; - // return; // Error - // }) - let mut status = StatementReturnStatus::NotReturn; - for stmt in &function.statements { - status = status.union(check_statement(stmt)); +pub fn check_function_body(function_node_id: NodeId, semantic: &Semantic) -> StatementReturnStatus { + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default)] + enum PendingExit { + #[default] + None, + Explicit, + Implicit, } - status + fn pending_exit_after_block( + block: &oxc_cfg::BasicBlock, + incoming_exit: PendingExit, + ) -> PendingExit { + let mut pending_exit = incoming_exit; + + for instruction in block.instructions() { + pending_exit = match instruction.kind { + InstructionKind::Return(ReturnInstructionKind::NotImplicitUndefined) + | InstructionKind::Throw => PendingExit::Explicit, + InstructionKind::Return(ReturnInstructionKind::ImplicitUndefined) => { + PendingExit::Implicit + } + InstructionKind::Unreachable => break, + _ => pending_exit, + }; + + if matches!( + instruction.kind, + InstructionKind::Return(_) | InstructionKind::Throw | InstructionKind::Unreachable + ) { + break; + } + } + + pending_exit + } + + fn is_terminal_edge(edge: &EdgeType) -> bool { + matches!( + edge, + EdgeType::Jump + | EdgeType::Normal + | EdgeType::Backedge + | EdgeType::Finalize + | EdgeType::Join + | EdgeType::Error(oxc_cfg::ErrorEdgeKind::Explicit) + ) + } + + let cfg = semantic.cfg().expect("CFG should be available"); + + let mut worklist = vec![(semantic.nodes().cfg_id(function_node_id), PendingExit::None)]; + let mut seen = FxHashSet::default(); + let mut terminal_states = vec![]; + + while let Some((block_id, incoming_exit)) = worklist.pop() { + if !seen.insert((block_id, incoming_exit)) { + continue; + } + + let block = cfg.basic_block(block_id); + if block.is_unreachable() { + continue; + } + + let state_after_block = pending_exit_after_block(block, incoming_exit); + let mut has_successor = false; + + for edge in cfg.graph().edges_directed(block_id, Direction::Outgoing) { + let edge_kind = edge.weight(); + if !is_terminal_edge(edge_kind) { + continue; + } + + has_successor = true; + let propagated_exit = + if matches!(edge_kind, EdgeType::Error(oxc_cfg::ErrorEdgeKind::Explicit)) { + incoming_exit + } else { + state_after_block + }; + + worklist.push((edge.target(), propagated_exit)); + } + + if !has_successor { + terminal_states.push(state_after_block); + } + } + + if terminal_states.is_empty() { + return StatementReturnStatus::NotReturn; + } + + StatementReturnStatus::create( + terminal_states.iter().all(|exit| !matches!(exit, PendingExit::None)), + terminal_states.iter().any(|exit| matches!(exit, PendingExit::Explicit)), + terminal_states.iter().any(|exit| matches!(exit, PendingExit::Implicit)), + ) } /// Collect spans of **explicit** return values (`return `) in the given function body. @@ -160,12 +212,11 @@ impl Visit<'_> for ReturnStatementFinder { return; }; - if !self.allow_void || !is_expression_void(argument) { - self.spans.push(argument.span()); - } - if is_expression_void(argument) { self.has_void_expression = true; + if !self.allow_void { + self.spans.push(argument.span()); + } } else { self.spans.push(argument.span()); } @@ -204,365 +255,3 @@ fn is_expression_void(statement_expression: &Expression<'_>) -> bool { _ => false, } } - -/// Return checkers runs a Control Flow-like Analysis on a statement to see if it -/// always returns on all paths of execution. -pub fn check_statement(statement: &Statement) -> StatementReturnStatus { - match statement { - Statement::ReturnStatement(ret) => { - if ret.argument.is_some() { - StatementReturnStatus::AlwaysExplicit - } else { - StatementReturnStatus::AlwaysImplicit - } - } - - Statement::IfStatement(stmt) => { - let test = &stmt.test; - let left = check_statement(&stmt.consequent); - let right = - stmt.alternate.as_ref().map_or(StatementReturnStatus::NotReturn, check_statement); - - test.to_boolean(&WithoutGlobalReferenceInformation {}) - .map_or_else(|| left.join(right), |val| if val { left } else { right }) - } - - Statement::WhileStatement(stmt) => { - let test = &stmt.test; - let inner_return = check_statement(&stmt.body); - if test.to_boolean(&WithoutGlobalReferenceInformation {}) == Some(true) { - inner_return - } else { - inner_return.join(StatementReturnStatus::NotReturn) - } - } - - // do while loop always executes at least once - Statement::DoWhileStatement(stmt) => check_statement(&stmt.body), - - // A switch statement always return if: - // 1. Every branch that eventually breaks out of the switch breaks via return - // 2. There is a default case that returns - Statement::SwitchStatement(stmt) => { - let mut case_statuses = vec![]; - // The default case maybe is not the last case and fallthrough - let mut default_case_fallthrough_continue = false; - let mut default_case_status = StatementReturnStatus::NotReturn; - - let mut current_case_status = StatementReturnStatus::NotReturn; - for case in &stmt.cases { - let branch_terminated = check_switch_case(case, &mut current_case_status); - if case.is_default_case() { - if branch_terminated { - default_case_status = current_case_status; - // Cases below the default case are not considered. - break; - } - default_case_fallthrough_continue = true; - } else if branch_terminated { - if default_case_fallthrough_continue { - default_case_status = current_case_status; - break; - } - case_statuses.push(current_case_status); - current_case_status = StatementReturnStatus::NotReturn; - } // Falls through to next case, accumulating lattice - } - - case_statuses.iter().fold(default_case_status, |accum, &lattice| accum.join(lattice)) - } - - Statement::BlockStatement(stmt) => check_block_statement(stmt), - - Statement::LabeledStatement(stmt) => check_statement(&stmt.body), - - Statement::WithStatement(stmt) => check_statement(&stmt.body), - - Statement::TryStatement(stmt) => { - let mut status = check_block_statement(&stmt.block); - if let Some(catch) = &stmt.handler { - status = status.join(check_block_statement(&catch.body)); - } - if let Some(finally) = &stmt.finalizer { - status = status.union(check_block_statement(finally)); - } - status - } - - Statement::ThrowStatement(_) => StatementReturnStatus::AlwaysExplicit, - - _ => StatementReturnStatus::NotReturn, - } -} - -/// Checks whether this switch case falls in: -/// 1. always return explicitly -/// 2. always return at least implicitly -/// 3. might not return and break out of the switch -/// 4. might not return and fall through -pub fn check_switch_case( - case: &SwitchCase, - accum: &mut StatementReturnStatus, /* Lattice accumulated from previous branches */ -) -> bool { - for s in &case.consequent { - // This case is over - if let Statement::BreakStatement(_) = s { - return true; - } - - let status = check_statement(s); - *accum = accum.union(status); - - if accum.must_return() { - return true; - } - } - - // This branch does not either return or break. Fall through - false -} - -pub fn check_block_statement(block: &BlockStatement) -> StatementReturnStatus { - let mut all_statements_status = StatementReturnStatus::NotReturn; - - for s in &block.body { - // The only case where we can see break is if the block is inside a loop, - // which means the loop does not return - if let Statement::BreakStatement(_) = s { - break; - } - - let current_stmt_status = check_statement(s); - all_statements_status = all_statements_status.union(current_stmt_status); - if all_statements_status.must_return() { - break; - } - } - - all_statements_status -} - -#[cfg(test)] -mod tests { - use oxc_allocator::Allocator; - use oxc_ast::ast::Program; - use oxc_parser::Parser; - use oxc_span::SourceType; - - use super::*; - - fn parse_statement_and_test(source: &'static str, expected: StatementReturnStatus) { - let source_type = SourceType::default(); - let alloc = Allocator::default(); - let parser = Parser::new(&alloc, source, source_type); - let ret = parser.parse(); - assert!(ret.errors.is_empty()); - - // The program is a function declaration with a single statement - let program = ret.program; - let Program { body, .. } = program; - let stmt = body.first().unwrap(); - let Statement::FunctionDeclaration(func) = stmt else { unreachable!() }; - - let first_statement = &func.body.as_ref().unwrap().statements[0]; - - test_match_expected(first_statement, expected); - } - - fn test_match_expected(statement: &Statement, expected: StatementReturnStatus) { - let actual = check_statement(statement); - - assert_eq!(expected, actual); - } - - #[test] - fn test_switch_always_explicit() { - // Return Explicit - let always_explicit = r#" - function d() { - switch (a) { - case "C": - switch (b) { - case "A": - var a = 1; - default: - return 123; - } - default: - return 1; - } - } - "#; - parse_statement_and_test(always_explicit, StatementReturnStatus::AlwaysExplicit); - } - - #[test] - fn test_switch_always_implicit() { - let always_implicit = r#" - function d() { - switch (a) { - case "C": - switch (b) { - case "A": - var a = 1; - default: - return; - } - default: - return; - } - } - "#; - parse_statement_and_test(always_implicit, StatementReturnStatus::AlwaysImplicit); - } - - #[test] - fn test_switch_always_mixed() { - let always_mixed = r#" - function d() { - switch (a) { - case "C": - switch (b) { - case "A": - var a = 1; - default: - return 123; - } - default: - return; - } - } - "#; - parse_statement_and_test(always_mixed, StatementReturnStatus::AlwaysMixed); - } - - #[test] - fn test_switch_some_mixed() { - let source = r#" - function foo() { - switch (a) { - case "C": - return 1; - case "B": - return; - } - } - "#; - - parse_statement_and_test(source, StatementReturnStatus::SomeMixed); - } - - #[test] - fn test_switch_some_explicit() { - let source = r#" - function foo() { - switch (a) { - case "C": - return 1; - } - } - "#; - - parse_statement_and_test(source, StatementReturnStatus::SomeExplicit); - } - - #[test] - fn test_if_always_true() { - let always_true = r" - function foo() { - if (true) return 1; - else { - var a = 123; - console.log(a); - } - } - "; - parse_statement_and_test(always_true, StatementReturnStatus::AlwaysExplicit); - } - - #[test] - fn test_if_always_false() { - let always_false = r" - function foo() { - if (false) { - var a = 123; - } else { - return 123; - } - } - "; - parse_statement_and_test(always_false, StatementReturnStatus::AlwaysExplicit); - } - - #[test] - fn test_if_non_static() { - let non_static = r" - function foo() { - if (a) { - return 123; - } else { - var c = 0; - } - } - "; - parse_statement_and_test(non_static, StatementReturnStatus::SomeExplicit); - } - - #[test] - fn test_block() { - // The block statement could: return a, return, or does not return in the end - let source = r" - function foo() { - { - if (a) { - return a; - } - - if (b) { - return; - } - } - } - "; - - parse_statement_and_test(source, StatementReturnStatus::SomeMixed); - } - - #[test] - fn test_while_true() { - let source = " - function foo() { - while (true) { - return; - } - } - "; - parse_statement_and_test(source, StatementReturnStatus::AlwaysImplicit); - } - - #[test] - fn test_throw_statement() { - let source = " - function foo() { - throw new Error('test'); - } - "; - parse_statement_and_test(source, StatementReturnStatus::AlwaysExplicit); - } - - #[test] - fn test_if_with_throw() { - let source = " - function foo() { - if (a) { - return 1; - } else if (b) { - return 2; - } else { - throw new Error('test'); - } - } - "; - parse_statement_and_test(source, StatementReturnStatus::AlwaysExplicit); - } -} diff --git a/crates/oxc_linter/src/snapshots/eslint_array_callback_return.snap b/crates/oxc_linter/src/snapshots/eslint_array_callback_return.snap index 4ed581c1d6896..33e65ac9b21b0 100644 --- a/crates/oxc_linter/src/snapshots/eslint_array_callback_return.snap +++ b/crates/oxc_linter/src/snapshots/eslint_array_callback_return.snap @@ -320,6 +320,14 @@ source: crates/oxc_linter/src/tester.rs help: This `if` has no `else` branch, so the callback may reach the end without a `return`. Add an `else`, or add a final `return`/`throw` after the `if`. Return a value on each path (or enable `allowImplicit` to allow `return;`). + ⚠ eslint(array-callback-return): Callback for array method "Array.prototype.map" does not return on all code paths + ╭─[array_callback_return.tsx:1:15] + 1 │ foo.map(() => { while (true) { if (a) break; return 1; } }) + · ──────────────────────────────────────────── + ╰──── + help: "Array.prototype.map" uses the callback's return value. Add a `return` on every possible code path. + Return a value on each path (or enable `allowImplicit` to allow `return;`). + ⚠ eslint(array-callback-return): Callback for array method "Array.prototype.every" does not return on all code paths ╭─[array_callback_return.tsx:1:22] 1 │ foo.every(function() { switch (a) { case 0: break; default: return true; } })