Rollup merge of rust-lang#106360 - estebank:remove-borrow-suggestion, r=compiler-errors

matthiaskrgr · web-flow · commit e4dd99fae919 · 2023-01-12T06:52:28.000+01:00
Tweak E0277 `&`-removal suggestions Fix rust-lang#64068, fix rust-lang#84837.
diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
@@ -1416,57 +1416,117 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
         err: &mut Diagnostic,
         trait_pred: ty::PolyTraitPredicate<'tcx>,
     ) -> bool {
-        let span = obligation.cause.span;
+        let mut span = obligation.cause.span;
+        let mut trait_pred = trait_pred;
+        let mut code = obligation.cause.code();
+        while let Some((c, Some(parent_trait_pred))) = code.parent() {
+            // We want the root obligation, in order to detect properly handle
+            // `for _ in &mut &mut vec![] {}`.
+            code = c;
+            trait_pred = parent_trait_pred;
+        }
+        while span.desugaring_kind().is_some() {
+            // Remove all the hir desugaring contexts while maintaining the macro contexts.
+            span.remove_mark();
+        }
+        let mut expr_finder = super::FindExprBySpan::new(span);
+        let Some(hir::Node::Expr(body)) = self.tcx.hir().find(obligation.cause.body_id) else {
+            return false;
+        };
+        expr_finder.visit_expr(&body);
+        let mut maybe_suggest = |suggested_ty, count, suggestions| {
+            // Remapping bound vars here
+            let trait_pred_and_suggested_ty =
+                trait_pred.map_bound(|trait_pred| (trait_pred, suggested_ty));
+
+            let new_obligation = self.mk_trait_obligation_with_new_self_ty(
+                obligation.param_env,
+                trait_pred_and_suggested_ty,
+            );
 
-        let mut suggested = false;
-        if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
-            let refs_number =
-                snippet.chars().filter(|c| !c.is_whitespace()).take_while(|c| *c == '&').count();
-            if let Some('\'') = snippet.chars().filter(|c| !c.is_whitespace()).nth(refs_number) {
-                // Do not suggest removal of borrow from type arguments.
-                return false;
+            if self.predicate_may_hold(&new_obligation) {
+                let msg = if count == 1 {
+                    "consider removing the leading `&`-reference".to_string()
+                } else {
+                    format!("consider removing {count} leading `&`-references")
+                };
+
+                err.multipart_suggestion_verbose(
+                    &msg,
+                    suggestions,
+                    Applicability::MachineApplicable,
+                );
+                true
+            } else {
+                false
             }
+        };
 
-            // Skipping binder here, remapping below
-            let mut suggested_ty = trait_pred.self_ty().skip_binder();
+        // Maybe suggest removal of borrows from types in type parameters, like in
+        // `src/test/ui/not-panic/not-panic-safe.rs`.
+        let mut count = 0;
+        let mut suggestions = vec![];
+        // Skipping binder here, remapping below
+        let mut suggested_ty = trait_pred.self_ty().skip_binder();
+        if let Some(mut hir_ty) = expr_finder.ty_result {
+            while let hir::TyKind::Ref(_, mut_ty) = &hir_ty.kind {
+                count += 1;
+                let span = hir_ty.span.until(mut_ty.ty.span);
+                suggestions.push((span, String::new()));
 
-            for refs_remaining in 0..refs_number {
                 let ty::Ref(_, inner_ty, _) = suggested_ty.kind() else {
                     break;
                 };
                 suggested_ty = *inner_ty;
 
-                // Remapping bound vars here
-                let trait_pred_and_suggested_ty =
-                    trait_pred.map_bound(|trait_pred| (trait_pred, suggested_ty));
+                hir_ty = mut_ty.ty;
 
-                let new_obligation = self.mk_trait_obligation_with_new_self_ty(
-                    obligation.param_env,
-                    trait_pred_and_suggested_ty,
-                );
+                if maybe_suggest(suggested_ty, count, suggestions.clone()) {
+                    return true;
+                }
+            }
+        }
 
-                if self.predicate_may_hold(&new_obligation) {
-                    let sp = self
-                        .tcx
-                        .sess
-                        .source_map()
-                        .span_take_while(span, |c| c.is_whitespace() || *c == '&');
+        // Maybe suggest removal of borrows from expressions, like in `for i in &&&foo {}`.
+        let Some(mut expr) = expr_finder.result else { return false; };
+        let mut count = 0;
+        let mut suggestions = vec![];
+        // Skipping binder here, remapping below
+        let mut suggested_ty = trait_pred.self_ty().skip_binder();
+        'outer: loop {
+            while let hir::ExprKind::AddrOf(_, _, borrowed) = expr.kind {
+                count += 1;
+                let span = if expr.span.eq_ctxt(borrowed.span) {
+                    expr.span.until(borrowed.span)
+                } else {
+                    expr.span.with_hi(expr.span.lo() + BytePos(1))
+                };
+                suggestions.push((span, String::new()));
 
-                    let remove_refs = refs_remaining + 1;
+                let ty::Ref(_, inner_ty, _) = suggested_ty.kind() else {
+                    break 'outer;
+                };
+                suggested_ty = *inner_ty;
 
-                    let msg = if remove_refs == 1 {
-                        "consider removing the leading `&`-reference".to_string()
-                    } else {
-                        format!("consider removing {} leading `&`-references", remove_refs)
-                    };
+                expr = borrowed;
 
-                    err.span_suggestion_short(sp, &msg, "", Applicability::MachineApplicable);
-                    suggested = true;
-                    break;
+                if maybe_suggest(suggested_ty, count, suggestions.clone()) {
+                    return true;
                 }
             }
+            if let hir::ExprKind::Path(hir::QPath::Resolved(None, path)) = expr.kind
+                && let hir::def::Res::Local(hir_id) = path.res
+                && let Some(hir::Node::Pat(binding)) = self.tcx.hir().find(hir_id)
+                && let Some(hir::Node::Local(local)) = self.tcx.hir().find_parent(binding.hir_id)
+                && let None = local.ty
+                && let Some(binding_expr) = local.init
+            {
+                expr = binding_expr;
+            } else {
+                break 'outer;
+            }
         }
-        suggested
+        false
     }
 
     fn suggest_remove_await(&self, obligation: &PredicateObligation<'tcx>, err: &mut Diagnostic) {
diff --git a/tests/ui/auto-traits/typeck-default-trait-impl-precedence.stderr b/tests/ui/auto-traits/typeck-default-trait-impl-precedence.stderr
@@ -4,7 +4,6 @@ error[E0277]: the trait bound `u32: Signed` is not satisfied
 LL |     is_defaulted::<&'static u32>();
    |                    ^^^^^^^^^^^^ the trait `Signed` is not implemented for `u32`
    |
-   = help: the trait `Signed` is implemented for `i32`
 note: required for `&'static u32` to implement `Defaulted`
   --> $DIR/typeck-default-trait-impl-precedence.rs:10:19
    |
@@ -17,6 +16,11 @@ note: required by a bound in `is_defaulted`
    |
 LL | fn is_defaulted<T:Defaulted>() { }
    |                   ^^^^^^^^^ required by this bound in `is_defaulted`
+help: consider removing the leading `&`-reference
+   |
+LL -     is_defaulted::<&'static u32>();
+LL +     is_defaulted::<u32>();
+   |
 
 error: aborting due to previous error
 
diff --git a/tests/ui/impl-trait/in-trait/issue-102140.stderr b/tests/ui/impl-trait/in-trait/issue-102140.stderr
@@ -2,11 +2,15 @@ error[E0277]: the trait bound `&dyn MyTrait: MyTrait` is not satisfied
   --> $DIR/issue-102140.rs:23:22
    |
 LL |         MyTrait::foo(&self)
-   |         ------------ -^^^^
-   |         |            |
-   |         |            the trait `MyTrait` is not implemented for `&dyn MyTrait`
-   |         |            help: consider removing the leading `&`-reference
+   |         ------------ ^^^^^ the trait `MyTrait` is not implemented for `&dyn MyTrait`
+   |         |
    |         required by a bound introduced by this call
+   |
+help: consider removing the leading `&`-reference
+   |
+LL -         MyTrait::foo(&self)
+LL +         MyTrait::foo(self)
+   |
 
 error[E0277]: the trait bound `&dyn MyTrait: MyTrait` is not satisfied
   --> $DIR/issue-102140.rs:23:9
diff --git a/tests/ui/kindck/kindck-copy.stderr b/tests/ui/kindck/kindck-copy.stderr
@@ -4,25 +4,33 @@ error[E0277]: the trait bound `&'static mut isize: Copy` is not satisfied
 LL |     assert_copy::<&'static mut isize>();
    |                   ^^^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `&'static mut isize`
    |
-   = help: the trait `Copy` is implemented for `isize`
 note: required by a bound in `assert_copy`
   --> $DIR/kindck-copy.rs:5:18
    |
 LL | fn assert_copy<T:Copy>() { }
    |                  ^^^^ required by this bound in `assert_copy`
+help: consider removing the leading `&`-reference
+   |
+LL -     assert_copy::<&'static mut isize>();
+LL +     assert_copy::<isize>();
+   |
 
 error[E0277]: the trait bound `&'a mut isize: Copy` is not satisfied
   --> $DIR/kindck-copy.rs:28:19
    |
 LL |     assert_copy::<&'a mut isize>();
    |                   ^^^^^^^^^^^^^ the trait `Copy` is not implemented for `&'a mut isize`
    |
-   = help: the trait `Copy` is implemented for `isize`
 note: required by a bound in `assert_copy`
   --> $DIR/kindck-copy.rs:5:18
    |
 LL | fn assert_copy<T:Copy>() { }
    |                  ^^^^ required by this bound in `assert_copy`
+help: consider removing the leading `&`-reference
+   |
+LL -     assert_copy::<&'a mut isize>();
+LL +     assert_copy::<isize>();
+   |
 
 error[E0277]: the trait bound `Box<isize>: Copy` is not satisfied
   --> $DIR/kindck-copy.rs:31:19
diff --git a/tests/ui/not-panic/not-panic-safe-4.stderr b/tests/ui/not-panic/not-panic-safe-4.stderr
@@ -12,6 +12,11 @@ note: required by a bound in `assert`
    |
 LL | fn assert<T: UnwindSafe + ?Sized>() {}
    |              ^^^^^^^^^^ required by this bound in `assert`
+help: consider removing the leading `&`-reference
+   |
+LL -     assert::<&RefCell<i32>>();
+LL +     assert::<RefCell<i32>>();
+   |
 
 error[E0277]: the type `UnsafeCell<isize>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   --> $DIR/not-panic-safe-4.rs:9:14
@@ -28,6 +33,11 @@ note: required by a bound in `assert`
    |
 LL | fn assert<T: UnwindSafe + ?Sized>() {}
    |              ^^^^^^^^^^ required by this bound in `assert`
+help: consider removing the leading `&`-reference
+   |
+LL -     assert::<&RefCell<i32>>();
+LL +     assert::<RefCell<i32>>();
+   |
 
 error: aborting due to 2 previous errors
 
diff --git a/tests/ui/not-panic/not-panic-safe.rs b/tests/ui/not-panic/not-panic-safe.rs
@@ -5,6 +5,6 @@ use std::panic::UnwindSafe;
 fn assert<T: UnwindSafe + ?Sized>() {}
 
 fn main() {
-    assert::<&mut i32>();
-    //~^ ERROR the type `&mut i32` may not be safely transferred across an unwind boundary
+    assert::<&mut &mut &i32>();
+    //~^ ERROR the type `&mut &mut &i32` may not be safely transferred across an unwind boundary
 }
diff --git a/tests/ui/not-panic/not-panic-safe.stderr b/tests/ui/not-panic/not-panic-safe.stderr
@@ -1,19 +1,21 @@
-error[E0277]: the type `&mut i32` may not be safely transferred across an unwind boundary
+error[E0277]: the type `&mut &mut &i32` may not be safely transferred across an unwind boundary
   --> $DIR/not-panic-safe.rs:8:14
    |
-LL |     assert::<&mut i32>();
-   |              -^^^^^^^
-   |              |
-   |              `&mut i32` may not be safely transferred across an unwind boundary
-   |              help: consider removing the leading `&`-reference
+LL |     assert::<&mut &mut &i32>();
+   |              ^^^^^^^^^^^^^^ `&mut &mut &i32` may not be safely transferred across an unwind boundary
    |
-   = help: the trait `UnwindSafe` is not implemented for `&mut i32`
-   = note: `UnwindSafe` is implemented for `&i32`, but not for `&mut i32`
+   = help: the trait `UnwindSafe` is not implemented for `&mut &mut &i32`
+   = note: `UnwindSafe` is implemented for `&&mut &i32`, but not for `&mut &mut &i32`
 note: required by a bound in `assert`
   --> $DIR/not-panic-safe.rs:5:14
    |
 LL | fn assert<T: UnwindSafe + ?Sized>() {}
    |              ^^^^^^^^^^ required by this bound in `assert`
+help: consider removing 2 leading `&`-references
+   |
+LL -     assert::<&mut &mut &i32>();
+LL +     assert::<&i32>();
+   |
 
 error: aborting due to previous error
 
diff --git a/tests/ui/suggestions/suggest-remove-refs-1.stderr b/tests/ui/suggestions/suggest-remove-refs-1.stderr
@@ -2,13 +2,15 @@ error[E0277]: `&Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator
   --> $DIR/suggest-remove-refs-1.rs:6:19
    |
 LL |     for (i, _) in &v.iter().enumerate() {
-   |                   -^^^^^^^^^^^^^^^^^^^^
-   |                   |
-   |                   `&Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator
-   |                   help: consider removing the leading `&`-reference
+   |                   ^^^^^^^^^^^^^^^^^^^^^ `&Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator
    |
    = help: the trait `Iterator` is not implemented for `&Enumerate<std::slice::Iter<'_, {integer}>>`
    = note: required for `&Enumerate<std::slice::Iter<'_, {integer}>>` to implement `IntoIterator`
+help: consider removing the leading `&`-reference
+   |
+LL -     for (i, _) in &v.iter().enumerate() {
+LL +     for (i, _) in v.iter().enumerate() {
+   |
 
 error: aborting due to previous error
 
diff --git a/tests/ui/suggestions/suggest-remove-refs-2.stderr b/tests/ui/suggestions/suggest-remove-refs-2.stderr
@@ -2,13 +2,15 @@ error[E0277]: `&&&&&Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterat
   --> $DIR/suggest-remove-refs-2.rs:6:19
    |
 LL |     for (i, _) in & & & & &v.iter().enumerate() {
-   |                   ---------^^^^^^^^^^^^^^^^^^^^
-   |                   |
-   |                   `&&&&&Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator
-   |                   help: consider removing 5 leading `&`-references
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `&&&&&Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator
    |
    = help: the trait `Iterator` is not implemented for `&&&&&Enumerate<std::slice::Iter<'_, {integer}>>`
    = note: required for `&&&&&Enumerate<std::slice::Iter<'_, {integer}>>` to implement `IntoIterator`
+help: consider removing 5 leading `&`-references
+   |
+LL -     for (i, _) in & & & & &v.iter().enumerate() {
+LL +     for (i, _) in v.iter().enumerate() {
+   |
 
 error: aborting due to previous error
 
diff --git a/tests/ui/suggestions/suggest-remove-refs-3.stderr b/tests/ui/suggestions/suggest-remove-refs-3.stderr
@@ -1,18 +1,20 @@
 error[E0277]: `&&&&&Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator
   --> $DIR/suggest-remove-refs-3.rs:6:19
    |
-LL |        for (i, _) in & & &
-   |  ____________________^
-   | | ___________________|
-   | ||
-LL | ||         & &v
-   | ||___________- help: consider removing 5 leading `&`-references
-LL | |          .iter()
-LL | |          .enumerate() {
-   | |_____________________^ `&&&&&Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator
+LL |       for (i, _) in & & &
+   |  ___________________^
+LL | |         & &v
+LL | |         .iter()
+LL | |         .enumerate() {
+   | |____________________^ `&&&&&Enumerate<std::slice::Iter<'_, {integer}>>` is not an iterator
    |
    = help: the trait `Iterator` is not implemented for `&&&&&Enumerate<std::slice::Iter<'_, {integer}>>`
    = note: required for `&&&&&Enumerate<std::slice::Iter<'_, {integer}>>` to implement `IntoIterator`
+help: consider removing 5 leading `&`-references
+   |
+LL -     for (i, _) in & & &
+LL +     for (i, _) in v
+   |
 
 error: aborting due to previous error
 
diff --git a/tests/ui/suggestions/suggest-remove-refs-4.fixed b/tests/ui/suggestions/suggest-remove-refs-4.fixed
@@ -0,0 +1,5 @@
+// run-rustfix
+fn main() {
+    let foo = [1,2,3].iter();
+    for _i in foo {} //~ ERROR E0277
+}
diff --git a/tests/ui/suggestions/suggest-remove-refs-4.rs b/tests/ui/suggestions/suggest-remove-refs-4.rs
@@ -0,0 +1,5 @@
+// run-rustfix
+fn main() {
+    let foo = &[1,2,3].iter();
+    for _i in &foo {} //~ ERROR E0277
+}
diff --git a/tests/ui/suggestions/suggest-remove-refs-4.stderr b/tests/ui/suggestions/suggest-remove-refs-4.stderr
@@ -0,0 +1,17 @@
+error[E0277]: `&&std::slice::Iter<'_, {integer}>` is not an iterator
+  --> $DIR/suggest-remove-refs-4.rs:4:15
+   |
+LL |     for _i in &foo {}
+   |               ^^^^ `&&std::slice::Iter<'_, {integer}>` is not an iterator
+   |
+   = help: the trait `Iterator` is not implemented for `&&std::slice::Iter<'_, {integer}>`
+   = note: required for `&&std::slice::Iter<'_, {integer}>` to implement `IntoIterator`
+help: consider removing 2 leading `&`-references
+   |
+LL ~     let foo = [1,2,3].iter();
+LL ~     for _i in foo {}
+   |
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0277`.
diff --git a/tests/ui/suggestions/suggest-remove-refs-5.fixed b/tests/ui/suggestions/suggest-remove-refs-5.fixed
@@ -0,0 +1,8 @@
+// run-rustfix
+fn main() {
+    let v = &mut Vec::<i32>::new();
+    for _ in v {} //~ ERROR E0277
+
+    let v = &mut [1u8];
+    for _ in v {} //~ ERROR E0277
+}
diff --git a/tests/ui/suggestions/suggest-remove-refs-5.rs b/tests/ui/suggestions/suggest-remove-refs-5.rs
diff --git a/tests/ui/suggestions/suggest-remove-refs-5.stderr b/tests/ui/suggestions/suggest-remove-refs-5.stderr
diff --git a/tests/ui/typeck/issue-57404.stderr b/tests/ui/typeck/issue-57404.stderr

Original file line number	Diff line number	Diff line change
@@ -4,7 +4,6 @@ error[E0277]: the trait bound `u32: Signed` is not satisfied
`4`	`4`	`LL \| is_defaulted::<&'static u32>();`
`5`	`5`	\| ^^^^^^^^^^^^ the trait `Signed` is not implemented for `u32`
`6`	`6`	`\|`
`7`		- = help: the trait `Signed` is implemented for `i32`
`8`	`7`	note: required for `&'static u32` to implement `Defaulted`
`9`	`8`	`--> $DIR/typeck-default-trait-impl-precedence.rs:10:19`
`10`	`9`	`\|`
@@ -17,6 +16,11 @@ note: required by a bound in `is_defaulted`
`17`	`16`	`\|`
`18`	`17`	`LL \| fn is_defaulted<T:Defaulted>() { }`
`19`	`18`	\| ^^^^^^^^^ required by this bound in `is_defaulted`
	`19`	+help: consider removing the leading `&`-reference
	`20`	`+ \|`
	`21`	`+LL - is_defaulted::<&'static u32>();`
	`22`	`+LL + is_defaulted::<u32>();`
	`23`	`+ \|`
`20`	`24`
`21`	`25`	`error: aborting due to previous error`
`22`	`26`
Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,6 @@ use std::panic::UnwindSafe;`
`5`	`5`	`fn assert<T: UnwindSafe + ?Sized>() {}`
`6`	`6`
`7`	`7`	`fn main() {`
`8`		`- assert::<&mut i32>();`
`9`		- //~^ ERROR the type `&mut i32` may not be safely transferred across an unwind boundary
	`8`	`+ assert::<&mut &mut &i32>();`
	`9`	+ //~^ ERROR the type `&mut &mut &i32` may not be safely transferred across an unwind boundary
`10`	`10`	`}`