[ruff] Implement unnecessary-nested-literal (RUF039)
#14323
Conversation
sbrugman
force-pushed
the
unnecessary-nested-literal
branch
2 times, most recently
from
15e08d3
to
ae4a677
Compare
sbrugman
force-pushed
the
unnecessary-nested-literal
branch
from
ae4a677
to
ebbd9f1
Compare
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF039 | 2 | 2 | 0 | 0 | 0 |
MichaReiser
added
rule
There was a problem hiding this comment.
@AlexWaygood would you mind doing a quick glance at the rule definition? I already reviewed the code
|
|
||
| let mut check_for_duplicate_members = |expr: &'a Expr, parent: &'a Expr| { | ||
| // If the parent is not equal to the `literal_expr` then we know we are traversing recursively. | ||
| if parent != literal_expr { |
There was a problem hiding this comment.
You could consider using AnyNodeRef::ptr_eq here because all you want to test is if the expressions are the same (Expr::eq does a deep comparison by default)
| if !is_nested { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can you expand on why we have to check inside the rule whether it is a nested literal, considering that the rule is only called when semantic.in_nested_literal is true?
There was a problem hiding this comment.
We only consider the top-level union, so when semantic.in_nested_literal is false. I've considered to run this run only when semantic.in_nested_literal is true, but then the top-level union is unavailable for the autofix.
There was a problem hiding this comment.
Oh, we actually only run the rule when not in a nested literal...
crates/ruff_linter/src/rules/ruff/rules/unnecessary_nested_literal.rs
Outdated
Show resolved
Hide resolved
…eral.rs Co-authored-by: Micha Reiser <[email protected]>
|
|
||
| /// RUF039 | ||
| pub(crate) fn unnecessary_nested_literal<'a>(checker: &mut Checker, literal_expr: &'a Expr) { | ||
| let mut nodes: Vec<&Expr> = Vec::new(); |
There was a problem hiding this comment.
It might also be worth to use a SmallVec here with a size of 1 to avoid allocating if this is a not-nested literal.
There was a problem hiding this comment.
nodes will be populated with all entries in Literal, so even for not-nested literals it can exceed 1. Shall I do a first pass to check if the Literal is nested, followed by another to collect the nodes? This will reduce the allocation on the common path.
There was a problem hiding this comment.
Similar to #14319 (review), I feel like I'm not sure how much this antipattern really comes up in practice. But, I can see the value if this is a pattern that could be introduced by the fix for other rules we implement!
| /// ## What it does | ||
| /// Checks for unnecessary nested `Literal`. | ||
| /// | ||
| /// ## Why is this bad? |
There was a problem hiding this comment.
The direct answer to the question of "Why is this bad?" is "it's less readable than the alternative". I think something to that effect should be the first sentence of this section, as it makes it clear that this rule is about readability and style rather than correctness.
crates/ruff_linter/src/rules/ruff/rules/unnecessary_nested_literal.rs
Outdated
Show resolved
Hide resolved
…eral.rs Co-authored-by: Alex Waygood <[email protected]>
Summary
Implementing
unnecessary-nested-literal.This rule could help simplify other rules' fixes by handling the flattening of
Literals here.See also https://github.com/astral-sh/ruff/pull/14270/files#r1837810594 (unions in a follow-up PR)
Test Plan
cargo testThe ecosystem results are correct.
Some of the nesting emits multiple violations.
I've got a fix for this, but that depends on #14280.
We can go on with merging this PR after review regardless (the violations are not wrong).