Auto merge of #12843 - mdm:fix-unnecessary-to-owned-println-interaction, r=y21
Fix `unnecessary_to_owned` interaction with macro expansion fixes #12821 In the case of an unnecessary `.iter().cloned()`, the lint `unnecessary_to_owned` might suggest to remove the `&` from references without checking if such references are inside a macro expansion. This can lead to unexpected behavior or even broken code if the lint suggestion is applied blindly. See issue #12821 for an example. This PR checks if such references are inside macro expansions and skips this part of the lint suggestion in these cases. changelog: [`unnecessary_to_owned`]: Don't suggest to remove `&` inside macro expansion
This commit is contained in:
bors
commit
7e4c1ae0b6
@ -38,7 +38,7 @@ pub fn check_for_loop_iter(
|
||||
) -> bool {
|
||||
if let Some(grandparent) = get_parent_expr(cx, expr).and_then(|parent| get_parent_expr(cx, parent))
|
||||
&& let Some(ForLoop { pat, body, .. }) = ForLoop::hir(grandparent)
|
||||
&& let (clone_or_copy_needed, addr_of_exprs) = clone_or_copy_needed(cx, pat, body)
|
||||
&& let (clone_or_copy_needed, references_to_binding) = clone_or_copy_needed(cx, pat, body)
|
||||
&& !clone_or_copy_needed
|
||||
&& let Some(receiver_snippet) = snippet_opt(cx, receiver.span)
|
||||
{
|
||||
@ -123,14 +123,12 @@ fn is_caller_or_fields_change(cx: &LateContext<'_>, body: &Expr<'_>, caller: &Ex
|
||||
Applicability::MachineApplicable
|
||||
};
|
||||
diag.span_suggestion(expr.span, "use", snippet, applicability);
|
||||
for addr_of_expr in addr_of_exprs {
|
||||
match addr_of_expr.kind {
|
||||
ExprKind::AddrOf(_, _, referent) => {
|
||||
let span = addr_of_expr.span.with_hi(referent.span.lo());
|
||||
diag.span_suggestion(span, "remove this `&`", "", applicability);
|
||||
},
|
||||
_ => unreachable!(),
|
||||
}
|
||||
if !references_to_binding.is_empty() {
|
||||
diag.multipart_suggestion(
|
||||
"remove any references to the binding",
|
||||
references_to_binding,
|
||||
applicability,
|
||||
);
|
||||
}
|
||||
},
|
||||
);
|
||||
|
||||
@ -9,6 +9,7 @@
|
||||
use rustc_middle::hir::nested_filter;
|
||||
use rustc_middle::ty::{self, Ty};
|
||||
use rustc_span::symbol::sym;
|
||||
use rustc_span::Span;
|
||||
|
||||
pub(super) fn derefs_to_slice<'tcx>(
|
||||
cx: &LateContext<'tcx>,
|
||||
@ -96,15 +97,15 @@ pub(super) fn clone_or_copy_needed<'tcx>(
|
||||
cx: &LateContext<'tcx>,
|
||||
pat: &Pat<'tcx>,
|
||||
body: &'tcx Expr<'tcx>,
|
||||
) -> (bool, Vec<&'tcx Expr<'tcx>>) {
|
||||
) -> (bool, Vec<(Span, String)>) {
|
||||
let mut visitor = CloneOrCopyVisitor {
|
||||
cx,
|
||||
binding_hir_ids: pat_bindings(pat),
|
||||
clone_or_copy_needed: false,
|
||||
addr_of_exprs: Vec::new(),
|
||||
references_to_binding: Vec::new(),
|
||||
};
|
||||
visitor.visit_expr(body);
|
||||
(visitor.clone_or_copy_needed, visitor.addr_of_exprs)
|
||||
(visitor.clone_or_copy_needed, visitor.references_to_binding)
|
||||
}
|
||||
|
||||
/// Returns a vector of all `HirId`s bound by the pattern.
|
||||
@ -127,7 +128,7 @@ struct CloneOrCopyVisitor<'cx, 'tcx> {
|
||||
cx: &'cx LateContext<'tcx>,
|
||||
binding_hir_ids: Vec<HirId>,
|
||||
clone_or_copy_needed: bool,
|
||||
addr_of_exprs: Vec<&'tcx Expr<'tcx>>,
|
||||
references_to_binding: Vec<(Span, String)>,
|
||||
}
|
||||
|
||||
impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> {
|
||||
@ -142,8 +143,11 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
|
||||
if self.is_binding(expr) {
|
||||
if let Some(parent) = get_parent_expr(self.cx, expr) {
|
||||
match parent.kind {
|
||||
ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => {
|
||||
self.addr_of_exprs.push(parent);
|
||||
ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, referent) => {
|
||||
if !parent.span.from_expansion() {
|
||||
self.references_to_binding
|
||||
.push((parent.span.until(referent.span), String::new()));
|
||||
}
|
||||
return;
|
||||
},
|
||||
ExprKind::MethodCall(.., args, _) => {
|
||||
|
||||
@ -170,3 +170,32 @@ fn check_mut_iteratee_and_modify_inner_variable() {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
mod issue_12821 {
|
||||
fn foo() {
|
||||
let v: Vec<_> = "hello".chars().collect();
|
||||
for c in v.iter() {
|
||||
//~^ ERROR: unnecessary use of `cloned`
|
||||
println!("{c}"); // should not suggest to remove `&`
|
||||
}
|
||||
}
|
||||
|
||||
fn bar() {
|
||||
let v: Vec<_> = "hello".chars().collect();
|
||||
for c in v.iter() {
|
||||
//~^ ERROR: unnecessary use of `cloned`
|
||||
let ref_c = c; //~ HELP: remove any references to the binding
|
||||
println!("{ref_c}");
|
||||
}
|
||||
}
|
||||
|
||||
fn baz() {
|
||||
let v: Vec<_> = "hello".chars().enumerate().collect();
|
||||
for (i, c) in v.iter() {
|
||||
//~^ ERROR: unnecessary use of `cloned`
|
||||
let ref_c = c; //~ HELP: remove any references to the binding
|
||||
let ref_i = i;
|
||||
println!("{i} {ref_c}"); // should not suggest to remove `&` from `i`
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@ -170,3 +170,32 @@ fn list(&self) -> &[String] {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
mod issue_12821 {
|
||||
fn foo() {
|
||||
let v: Vec<_> = "hello".chars().collect();
|
||||
for c in v.iter().cloned() {
|
||||
//~^ ERROR: unnecessary use of `cloned`
|
||||
println!("{c}"); // should not suggest to remove `&`
|
||||
}
|
||||
}
|
||||
|
||||
fn bar() {
|
||||
let v: Vec<_> = "hello".chars().collect();
|
||||
for c in v.iter().cloned() {
|
||||
//~^ ERROR: unnecessary use of `cloned`
|
||||
let ref_c = &c; //~ HELP: remove any references to the binding
|
||||
println!("{ref_c}");
|
||||
}
|
||||
}
|
||||
|
||||
fn baz() {
|
||||
let v: Vec<_> = "hello".chars().enumerate().collect();
|
||||
for (i, c) in v.iter().cloned() {
|
||||
//~^ ERROR: unnecessary use of `cloned`
|
||||
let ref_c = &c; //~ HELP: remove any references to the binding
|
||||
let ref_i = &i;
|
||||
println!("{i} {ref_c}"); // should not suggest to remove `&` from `i`
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@ -10,7 +10,7 @@ help: use
|
||||
|
|
||||
LL | for (t, path) in files {
|
||||
| ~~~~~
|
||||
help: remove this `&`
|
||||
help: remove any references to the binding
|
||||
|
|
||||
LL - let other = match get_file_path(&t) {
|
||||
LL + let other = match get_file_path(t) {
|
||||
@ -26,11 +26,49 @@ help: use
|
||||
|
|
||||
LL | for (t, path) in files.iter() {
|
||||
| ~~~~~~~~~~~~
|
||||
help: remove this `&`
|
||||
help: remove any references to the binding
|
||||
|
|
||||
LL - let other = match get_file_path(&t) {
|
||||
LL + let other = match get_file_path(t) {
|
||||
|
|
||||
|
||||
error: aborting due to 2 previous errors
|
||||
error: unnecessary use of `cloned`
|
||||
--> tests/ui/unnecessary_iter_cloned.rs:177:18
|
||||
|
|
||||
LL | for c in v.iter().cloned() {
|
||||
| ^^^^^^^^^^^^^^^^^ help: use: `v.iter()`
|
||||
|
||||
error: unnecessary use of `cloned`
|
||||
--> tests/ui/unnecessary_iter_cloned.rs:185:18
|
||||
|
|
||||
LL | for c in v.iter().cloned() {
|
||||
| ^^^^^^^^^^^^^^^^^
|
||||
|
|
||||
help: use
|
||||
|
|
||||
LL | for c in v.iter() {
|
||||
| ~~~~~~~~
|
||||
help: remove any references to the binding
|
||||
|
|
||||
LL - let ref_c = &c;
|
||||
LL + let ref_c = c;
|
||||
|
|
||||
|
||||
error: unnecessary use of `cloned`
|
||||
--> tests/ui/unnecessary_iter_cloned.rs:194:23
|
||||
|
|
||||
LL | for (i, c) in v.iter().cloned() {
|
||||
| ^^^^^^^^^^^^^^^^^
|
||||
|
|
||||
help: use
|
||||
|
|
||||
LL | for (i, c) in v.iter() {
|
||||
| ~~~~~~~~
|
||||
help: remove any references to the binding
|
||||
|
|
||||
LL ~ let ref_c = c;
|
||||
LL ~ let ref_i = i;
|
||||
|
|
||||
|
||||
error: aborting due to 5 previous errors
|
||||
|
||||
|
||||
@ -487,7 +487,7 @@ help: use
|
||||
|
|
||||
LL | for t in file_types {
|
||||
| ~~~~~~~~~~
|
||||
help: remove this `&`
|
||||
help: remove any references to the binding
|
||||
|
|
||||
LL - let path = match get_file_path(&t) {
|
||||
LL + let path = match get_file_path(t) {
|
||||
|
||||
Loading…
Reference in New Issue
Block a user