mirror of
https://github.com/danbulant/oxc
synced 2026-05-19 12:19:15 +00:00
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)
This commit is contained in:
rzvxa
parent
3e78f9852f
commit
4bd2c882d3
2 changed files with 118 additions and 111 deletions
|
|
@ -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::<Vec<_>>();
|
||||
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)
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in a new issue