From 4bd2c882d3455526400266ffb3b13f320757012e Mon Sep 17 00:00:00 2001 From: rzvxa <3788964+rzvxa@users.noreply.github.com> Date: Thu, 20 Jun 2024 01:27:07 +0000 Subject: [PATCH] fix(linter): fix and promote `getter-return` to correctness. (#3777) I had to rewrite some parts of this rule so now it uses a set dfs. It is arguably a little bit slower than before but it is less prone to false negatives. closes #2312 [oxlint-ecosystem-ci](https://github.com/oxc-project/oxlint-ecosystem-ci/actions/runs/9586832129/job/26435575457?pr=16) --- .../src/rules/eslint/getter_return.rs | 215 +++++++++--------- .../src/snapshots/getter_return.snap | 14 ++ 2 files changed, 118 insertions(+), 111 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/getter_return.rs b/crates/oxc_linter/src/rules/eslint/getter_return.rs index 47fefec5c..d0d07ef3c 100644 --- a/crates/oxc_linter/src/rules/eslint/getter_return.rs +++ b/crates/oxc_linter/src/rules/eslint/getter_return.rs @@ -6,8 +6,11 @@ use oxc_ast::{ AstKind, }; use oxc_cfg::{ - graph::visit::neighbors_filtered_by_edge_weight, EdgeType, InstructionKind, - ReturnInstructionKind, + graph::{ + visit::{set_depth_first_search, Control, DfsEvent}, + Direction, + }, + EdgeType, ErrorEdgeKind, InstructionKind, ReturnInstructionKind, }; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; @@ -49,7 +52,7 @@ declare_oxc_lint!( /// } /// ``` GetterReturn, - nursery + correctness ); impl Rule for GetterReturn { @@ -187,135 +190,102 @@ impl GetterReturn { let cfg = ctx.cfg(); - let output = neighbors_filtered_by_edge_weight( - cfg.graph(), - node.cfg_id(), - &|edge| match edge { - EdgeType::Jump | EdgeType::Normal => None, - // We don't need to handle backedges because we would have already visited - // them on the forward pass - | EdgeType::Backedge - // We don't need to visit NewFunction edges because it's not going to be evaluated - // immediately, and we are only doing a pass over things that will be immediately evaluated - | EdgeType::NewFunction - // Unreachable nodes aren't reachable so we don't follow them. - | EdgeType::Unreachable - // TODO: For now we ignore the error path to simplify this rule, We can also - // analyze the error path as a nice to have addition. - | EdgeType::Error(_) - | EdgeType::Finalize - | EdgeType::Join - // By returning Some(X), - // we signal that we don't walk to this path any farther. - // - // We stop this path on a ::Yes because if it was a ::No, - // we would have already returned early before exploring more edges - => Some(DefinitelyReturnsOrThrowsOrUnreachable::Yes), - }, - // We ignore the state going into this rule because we only care whether - // or not this path definitely returns or throws. + let graph = cfg.graph(); + let definitely_returns_in_all_codepaths = 'returns: { + // The expression is the equivalent of return. + // Therefore, if a function is an expression, it always returns its value. // - // Whether or not the path definitely returns is only has two states, Yes (the default) - // or No (when we see this, we immediately stop walking). Other rules that require knowing - // previous such as [`no_this_before_super`] we would want to observe this value. - &mut |basic_block_id, _state_going_into_this_rule| { - // The expression is the equivalent of return. - // Therefore, if a function is an expression, it always returns its value. - // - // Example expression: - // ```js - // const fn = () => 1; - // ``` - if let AstKind::ArrowFunctionExpression(arrow_expr) = node.kind() { - if arrow_expr.expression { - return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false); - } + // Example expression: + // ```js + // const fn = () => 1; + // ``` + if let AstKind::ArrowFunctionExpression(arrow_expr) = node.kind() { + if arrow_expr.expression { + break 'returns true; } - // If the signature of function supports the return of the `undefined` value, // you do not need to check this rule if let AstKind::Function(func) = node.kind() { if let Some(ref ret) = func.return_type { if ret.type_annotation.is_maybe_undefined() { - return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false); + break 'returns true; } } } - - // Scan through the values in this basic block. - for entry in cfg.basic_block(*basic_block_id).instructions() { - match entry.kind { - // If the element is a return. - // `allow_implicit` allows returning without a value to not - // fail the rule. - // Return false as the second argument to signify we should - // not continue walking this branch, as we know a return - // is the end of this path. - InstructionKind::Return(ReturnInstructionKind::ImplicitUndefined) if !self.allow_implicit => { - return (DefinitelyReturnsOrThrowsOrUnreachable::No, false); + } + let output = set_depth_first_search(graph, Some(node.cfg_id()), |event| { + match event { + // We only need to check paths that are normal or jump. + DfsEvent::TreeEdge(a, b) => { + let edges = graph.edges_connecting(a, b).collect::>(); + if edges.iter().any(|e| { + matches!( + e.weight(), + EdgeType::Normal + | EdgeType::Jump + | EdgeType::Error(ErrorEdgeKind::Explicit) + ) + }) { + Control::Continue + } else { + Control::Prune } - // Otherwise, we definitely returned since we assigned - // to the return register. - // - // Return false as the second argument to signify we should - // not continue walking this branch, as we know a return - // is the end of this path. - | InstructionKind::Return(_) - // Throws are classified as returning. - // - // todo: test with catching... - | InstructionKind::Throw - // Although the unreachable code is not returned, it will never be executed. - // There is no point in checking it for return. - // - // An example in such cases: - // ```js - // switch (val) { - // default: return 1; - // } - // return -1; - // ``` - // Make return useless. - | InstructionKind::Unreachable =>{ - return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false); - } - // Ignore irrelevant elements. - | InstructionKind::Break(_) - | InstructionKind::Continue(_) - | InstructionKind::Iteration(_) - | InstructionKind::Condition - | InstructionKind::Statement => {} } + DfsEvent::Discover(basic_block_id, _) => { + let return_instruction = + cfg.basic_block(basic_block_id).instructions().iter().find(|it| { + match it.kind { + // Throws are classified as returning. + InstructionKind::Return(_) | InstructionKind::Throw => true, + // Ignore irrelevant elements. + InstructionKind::Break(_) + | InstructionKind::Continue(_) + | InstructionKind::Iteration(_) + | InstructionKind::Unreachable + | InstructionKind::Condition + | InstructionKind::Statement => false, + } + }); + + let does_return = return_instruction.is_some_and(|ret| { + !matches! { ret.kind, + InstructionKind::Return(ReturnInstructionKind::ImplicitUndefined) + if !self.allow_implicit + } + }); + + // Return true as the second argument to signify we should + // continue walking this branch, as we haven't seen anything + // that will signify to us that this path of the program will + // definitely return or throw. + if graph.edges_directed(basic_block_id, Direction::Outgoing).any(|e| { + matches!( + e.weight(), + EdgeType::Jump + | EdgeType::Normal + | EdgeType::Error(ErrorEdgeKind::Explicit) + ) + }) { + Control::Continue + } else if does_return { + Control::Prune + } else { + Control::Break(()) + } + } + _ => Control::Continue, } + }); - // Return true as the second argument to signify we should - // continue walking this branch, as we haven't seen anything - // that will signify to us that this path of the program will - // definitely return or throw. - (DefinitelyReturnsOrThrowsOrUnreachable::No, true) - }, - ); + output.break_value().is_none() + }; - // Deciding whether we definitely return or throw in all - // codepaths is as simple as seeing if each individual codepath - // definitely returns or throws. - let definitely_returns_in_all_codepaths = - output.into_iter().all(|y| matches!(y, DefinitelyReturnsOrThrowsOrUnreachable::Yes)); - - // If not, flag it as a diagnostic. if !definitely_returns_in_all_codepaths { ctx.diagnostic(getter_return_diagnostic(span)); } } } -#[derive(Default, Copy, Clone, Debug)] -enum DefinitelyReturnsOrThrowsOrUnreachable { - #[default] - No, - Yes, -} - #[test] fn test() { use crate::tester::Tester; @@ -366,6 +336,27 @@ fn test() { ", None, ), + (r" + var foo = { + get bar() { + let name = ([] || [])[1]; + return name; + }, + }; + ", None), + ("var foo = { get bar() { try { return a(); } finally { } } };", None), + (" + var foo = { + get bar() { + switch (baz) { + case VS_LIGHT_THEME: return a; + case VS_HC_THEME: return b; + case VS_HC_LIGHT_THEME: return c; + default: return d; + } + } + }; + ", None), ]; let fail = vec![ @@ -439,6 +430,8 @@ fn test() { "(Object?.create)(foo, { bar: { get: function (){} } });", Some(serde_json::json!([{ "allowImplicit": true }])), ), + ("var foo = { get bar() { try { return a(); } catch {} } };", None), + ("var foo = { get bar() { try { return a(); } catch { } finally { } } };", None), ]; Tester::new(GetterReturn::NAME, pass, fail) diff --git a/crates/oxc_linter/src/snapshots/getter_return.snap b/crates/oxc_linter/src/snapshots/getter_return.snap index 7bc9cd5d4..512321bde 100644 --- a/crates/oxc_linter/src/snapshots/getter_return.snap +++ b/crates/oxc_linter/src/snapshots/getter_return.snap @@ -240,3 +240,17 @@ source: crates/oxc_linter/src/tester.rs · ───────────── ╰──── help: Return a value from all code paths in getter. + + ⚠ eslint(getter-return): Expected to always return a value in getter. + ╭─[getter_return.js:1:20] + 1 │ var foo = { get bar() { try { return a(); } catch {} } }; + · ─────────────────────────────────── + ╰──── + help: Return a value from all code paths in getter. + + ⚠ eslint(getter-return): Expected to always return a value in getter. + ╭─[getter_return.js:1:20] + 1 │ var foo = { get bar() { try { return a(); } catch { } finally { } } }; + · ────────────────────────────────────────────────── + ╰──── + help: Return a value from all code paths in getter.