Fix branches_sharing_code suggests wrongly when dealing with macros (#14907)

Closes rust-lang/rust-clippy#14873

changelog: [`branches_sharing_code`] fix wrong suggestions when dealing
with macros
This commit is contained in:
dswij 2025-06-07 13:52:02 +00:00 committed by GitHub
commit 33052be5a6
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 215 additions and 5 deletions

View file

@ -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);

View file

@ -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
}
}
}

View file

@ -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

View file

@ -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);
}
}
}

View file

@ -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

View file

@ -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;
}
}
}

View file

@ -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