diff options
| -rw-r--r-- | clippy_lints/src/copies.rs | 51 | ||||
| -rw-r--r-- | tests/ui/branches_sharing_code/shared_at_bottom.rs | 37 | ||||
| -rw-r--r-- | tests/ui/branches_sharing_code/shared_at_bottom.stderr | 17 | ||||
| -rw-r--r-- | tests/ui/branches_sharing_code/shared_at_top.rs | 31 | ||||
| -rw-r--r-- | tests/ui/branches_sharing_code/shared_at_top.stderr | 17 | ||||
| -rw-r--r-- | tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs | 39 | ||||
| -rw-r--r-- | tests/ui/branches_sharing_code/shared_at_top_and_bottom.stderr | 28 |
7 files changed, 215 insertions, 5 deletions
diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 2467fc95fd0..5ef726638a5 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -425,7 +425,9 @@ fn scan_block_for_eq<'tcx>( modifies_any_local(cx, stmt, &cond_locals) || !eq_stmts(stmt, blocks, |b| b.stmts.get(i), &mut eq, &mut moved_locals) }) - .map_or(block.stmts.len(), |(i, _)| i); + .map_or(block.stmts.len(), |(i, stmt)| { + adjust_by_closest_callsite(i, stmt, block.stmts[..i].iter().enumerate().rev()) + }); if local_needs_ordered_drop { return BlockEq { @@ -467,7 +469,9 @@ fn scan_block_for_eq<'tcx>( .is_none_or(|s| hash != hash_stmt(cx, s)) }) }) - .map_or(block.stmts.len() - start_end_eq, |(i, _)| i); + .map_or(block.stmts.len() - start_end_eq, |(i, stmt)| { + adjust_by_closest_callsite(i, stmt, (0..i).rev().zip(block.stmts[(block.stmts.len() - i)..].iter())) + }); let moved_locals_at_start = moved_locals.len(); let mut i = end_search_start; @@ -522,6 +526,49 @@ fn scan_block_for_eq<'tcx>( } } +/// Adjusts the index for which the statements begin to differ to the closest macro callsite. This +/// avoids giving suggestions that requires splitting a macro call in half, when only a part of the +/// macro expansion is equal. +/// +/// For example, for the following macro: +/// ```rust,ignore +/// macro_rules! foo { +/// ($x:expr) => { +/// let y = 42; +/// $x; +/// }; +/// } +/// ``` +/// If the macro is called like this: +/// ```rust,ignore +/// if false { +/// let z = 42; +/// foo!(println!("Hello")); +/// } else { +/// let z = 42; +/// foo!(println!("World")); +/// } +/// ``` +/// Although the expanded `let y = 42;` is equal, the macro call should not be included in the +/// suggestion. +fn adjust_by_closest_callsite<'tcx>( + i: usize, + stmt: &'tcx Stmt<'tcx>, + mut iter: impl Iterator<Item = (usize, &'tcx Stmt<'tcx>)>, +) -> usize { + let Some((_, first)) = iter.next() else { + return 0; + }; + + // If it is already at the boundary of a macro call, then just return. + if first.span.source_callsite() != stmt.span.source_callsite() { + return i; + } + + iter.find(|(_, stmt)| stmt.span.source_callsite() != first.span.source_callsite()) + .map_or(0, |(i, _)| i + 1) +} + fn check_for_warn_of_moved_symbol(cx: &LateContext<'_>, symbols: &[(HirId, Symbol)], if_expr: &Expr<'_>) -> bool { get_enclosing_block(cx, if_expr.hir_id).is_some_and(|block| { let ignore_span = block.span.shrink_to_lo().to(if_expr.span); diff --git a/tests/ui/branches_sharing_code/shared_at_bottom.rs b/tests/ui/branches_sharing_code/shared_at_bottom.rs index 06472a4f5d5..922d30443fc 100644 --- a/tests/ui/branches_sharing_code/shared_at_bottom.rs +++ b/tests/ui/branches_sharing_code/shared_at_bottom.rs @@ -239,3 +239,40 @@ fn fp_if_let_issue7054() { } fn main() {} + +mod issue14873 { + fn foo() -> i32 { + todo!() + } + + macro_rules! qux { + ($a:ident, $b:ident, $condition:expr) => { + if $condition { + "." + } else { + "" + }; + $a = foo(); + $b = foo(); + }; + } + + fn share_on_bottom() { + let mut a = 0; + let mut b = 0; + if false { + qux!(a, b, a == b); + } else { + qux!(a, b, a != b); + }; + + if false { + qux!(a, b, a == b); + let y = 1; + } else { + qux!(a, b, a != b); + let y = 1; + //~^ branches_sharing_code + } + } +} diff --git a/tests/ui/branches_sharing_code/shared_at_bottom.stderr b/tests/ui/branches_sharing_code/shared_at_bottom.stderr index 648a99c65ed..f437db8b733 100644 --- a/tests/ui/branches_sharing_code/shared_at_bottom.stderr +++ b/tests/ui/branches_sharing_code/shared_at_bottom.stderr @@ -157,5 +157,20 @@ LL ~ if x == 17 { b = 1; a = 0x99; } else { } LL + a = 0x99; | -error: aborting due to 9 previous errors +error: all if blocks contain the same code at the end + --> tests/ui/branches_sharing_code/shared_at_bottom.rs:274:9 + | +LL | / let y = 1; +LL | | +LL | | } + | |_________^ + | + = warning: some moved values might need to be renamed to avoid wrong references +help: consider moving these statements after the if + | +LL ~ } +LL + let y = 1; + | + +error: aborting due to 10 previous errors diff --git a/tests/ui/branches_sharing_code/shared_at_top.rs b/tests/ui/branches_sharing_code/shared_at_top.rs index 694c67d4c85..dcd77679fc6 100644 --- a/tests/ui/branches_sharing_code/shared_at_top.rs +++ b/tests/ui/branches_sharing_code/shared_at_top.rs @@ -124,3 +124,34 @@ fn pf_local_with_inferred_type_issue7053() { } fn main() {} + +mod issue14873 { + fn foo() -> i32 { + todo!() + } + + macro_rules! qux { + ($a:ident, $b:ident, $condition:expr) => { + let $a: i32 = foo(); + let $b: i32 = foo(); + if $condition { "." } else { "" } + }; + } + + fn share_on_top() { + if false { + qux!(a, b, a == b); + } else { + qux!(a, b, a != b); + }; + + if false { + //~^ branches_sharing_code + let x = 1; + qux!(a, b, a == b); + } else { + let x = 1; + qux!(a, b, a != b); + } + } +} diff --git a/tests/ui/branches_sharing_code/shared_at_top.stderr b/tests/ui/branches_sharing_code/shared_at_top.stderr index d28e9c7af29..30efb98b3f5 100644 --- a/tests/ui/branches_sharing_code/shared_at_top.stderr +++ b/tests/ui/branches_sharing_code/shared_at_top.stderr @@ -125,5 +125,20 @@ note: the lint level is defined here LL | #![deny(clippy::branches_sharing_code, clippy::if_same_then_else)] | ^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 7 previous errors +error: all if blocks contain the same code at the start + --> tests/ui/branches_sharing_code/shared_at_top.rs:148:9 + | +LL | / if false { +LL | | +LL | | let x = 1; + | |______________________^ + | + = warning: some moved values might need to be renamed to avoid wrong references +help: consider moving these statements before the if + | +LL ~ let x = 1; +LL + if false { + | + +error: aborting due to 8 previous errors diff --git a/tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs index 75334f70f1f..e848f0601e3 100644 --- a/tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs +++ b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs @@ -128,3 +128,42 @@ fn added_note_for_expression_use() -> u32 { } fn main() {} + +mod issue14873 { + fn foo() -> i32 { + todo!() + } + + macro_rules! qux { + ($a:ident, $b:ident, $condition:expr) => { + let mut $a: i32 = foo(); + let mut $b: i32 = foo(); + if $condition { + "." + } else { + "" + }; + $a = foo(); + $b = foo(); + }; + } + + fn share_on_top_and_bottom() { + if false { + qux!(a, b, a == b); + } else { + qux!(a, b, a != b); + }; + + if false { + //~^ branches_sharing_code + let x = 1; + qux!(a, b, a == b); + let y = 1; + } else { + let x = 1; + qux!(a, b, a != b); + let y = 1; + } + } +} diff --git a/tests/ui/branches_sharing_code/shared_at_top_and_bottom.stderr b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.stderr index 2200ab45089..40f3453edb9 100644 --- a/tests/ui/branches_sharing_code/shared_at_top_and_bottom.stderr +++ b/tests/ui/branches_sharing_code/shared_at_top_and_bottom.stderr @@ -159,5 +159,31 @@ LL ~ } LL + x * 4 | -error: aborting due to 5 previous errors +error: all if blocks contain the same code at both the start and the end + --> tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs:158:9 + | +LL | / if false { +LL | | +LL | | let x = 1; + | |______________________^ + | +note: this code is shared at the end + --> tests/ui/branches_sharing_code/shared_at_top_and_bottom.rs:166:9 + | +LL | / let y = 1; +LL | | } + | |_________^ + = warning: some moved values might need to be renamed to avoid wrong references +help: consider moving these statements before the if + | +LL ~ let x = 1; +LL + if false { + | +help: consider moving these statements after the if + | +LL ~ } +LL + let y = 1; + | + +error: aborting due to 6 previous errors |