Account for trait/impl difference when suggesting changing argument from ref to mut ref

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

@@ -21,7 +21,7 @@ use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt;
 
 use crate::diagnostics::BorrowedContentSource;
 use crate::util::FindAssignments;
-use crate::MirBorrowckCtxt;
+use crate::{session_diagnostics, MirBorrowckCtxt};
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 pub(crate) enum AccessKind {
@@ -234,7 +234,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                         Some(mir::BorrowKind::Mut { kind: mir::MutBorrowKind::Default }),
                         |_kind, var_span| {
                             let place = self.describe_any_place(access_place.as_ref());
-                            crate::session_diagnostics::CaptureVarCause::MutableBorrowUsePlaceClosure {
+                            session_diagnostics::CaptureVarCause::MutableBorrowUsePlaceClosure {
                                 place,
                                 var_span,
                             }
@@ -667,19 +667,26 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
     /// User cannot make signature of a trait mutable without changing the
     /// trait. So we find if this error belongs to a trait and if so we move
     /// suggestion to the trait or disable it if it is out of scope of this crate
-    fn is_error_in_trait(&self, local: Local) -> (bool, Option<Span>) {
+    ///
+    /// The returned values are:
+    ///  - is the current item an assoc `fn` of an impl that corresponds to a trait def? if so, we
+    ///    have to suggest changing both the impl `fn` arg and the trait `fn` arg
+    ///  - is the trait from the local crate? If not, we can't suggest changing signatures
+    ///  - `Span` of the argument in the trait definition
+    fn is_error_in_trait(&self, local: Local) -> (bool, bool, Option<Span>) {
         if self.body.local_kind(local) != LocalKind::Arg {
-            return (false, None);
+            return (false, false, None);
         }
         let my_def = self.body.source.def_id();
         let my_hir = self.infcx.tcx.local_def_id_to_hir_id(my_def.as_local().unwrap());
         let Some(td) =
             self.infcx.tcx.impl_of_method(my_def).and_then(|x| self.infcx.tcx.trait_id_of_impl(x))
         else {
-            return (false, None);
+            return (false, false, None);
         };
         (
             true,
+            td.is_local(),
             td.as_local().and_then(|tld| match self.infcx.tcx.hir_node_by_def_id(tld) {
                 Node::Item(hir::Item { kind: hir::ItemKind::Trait(_, _, _, _, items), .. }) => {
                     let mut f_in_trait_opt = None;
@@ -695,19 +702,16 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                         break;
                     }
                     f_in_trait_opt.and_then(|f_in_trait| {
-                        match self.infcx.tcx.hir_node(f_in_trait) {
-                            Node::TraitItem(hir::TraitItem {
-                                kind:
-                                    hir::TraitItemKind::Fn(
-                                        hir::FnSig { decl: hir::FnDecl { inputs, .. }, .. },
-                                        _,
-                                    ),
-                                ..
-                            }) => {
-                                let hir::Ty { span, .. } = *inputs.get(local.index() - 1)?;
-                                Some(span)
-                            }
-                            _ => None,
+                        if let Node::TraitItem(ti) = self.infcx.tcx.hir_node(f_in_trait)
+                            && let hir::TraitItemKind::Fn(sig, _) = ti.kind
+                            && let Some(ty) = sig.decl.inputs.get(local.index() - 1)
+                            && let hir::TyKind::Ref(_, mut_ty) = ty.kind
+                            && let hir::Mutability::Not = mut_ty.mutbl
+                            && sig.decl.implicit_self.has_implicit_self()
+                        {
+                            Some(ty.span)
+                        } else {
+                            None
                         }
                     })
                 }
@@ -1061,20 +1065,24 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
         let (pointer_sigil, pointer_desc) =
             if local_decl.ty.is_ref() { ("&", "reference") } else { ("*const", "pointer") };
 
-        let (is_trait_sig, local_trait) = self.is_error_in_trait(local);
-        if is_trait_sig && local_trait.is_none() {
+        let (is_trait_sig, is_local, local_trait) = self.is_error_in_trait(local);
+
+        if is_trait_sig && !is_local {
+            // Do not suggest to change the signature when the trait comes from another crate.
+            err.span_label(
+                local_decl.source_info.span,
+                format!("this is an immutable {pointer_desc}"),
+            );
             return;
         }
-
-        let decl_span = match local_trait {
-            Some(span) => span,
-            None => local_decl.source_info.span,
-        };
+        let decl_span = local_decl.source_info.span;
 
         let label = match *local_decl.local_info() {
             LocalInfo::User(mir::BindingForm::ImplicitSelf(_)) => {
                 let suggestion = suggest_ampmut_self(self.infcx.tcx, decl_span);
-                Some((true, decl_span, suggestion))
+                let additional =
+                    local_trait.map(|span| (span, suggest_ampmut_self(self.infcx.tcx, span)));
+                Some((true, decl_span, suggestion, additional))
             }
 
             LocalInfo::User(mir::BindingForm::Var(mir::VarBindingForm {
@@ -1113,7 +1121,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     // don't create labels for compiler-generated spans
                     Some(_) => None,
                     None => {
-                        let label = if name != kw::SelfLower {
+                        let (has_sugg, decl_span, sugg) = if name != kw::SelfLower {
                             suggest_ampmut(
                                 self.infcx.tcx,
                                 local_decl.ty,
@@ -1140,7 +1148,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                                 ),
                             }
                         };
-                        Some(label)
+                        Some((has_sugg, decl_span, sugg, None))
                     }
                 }
             }
@@ -1151,22 +1159,33 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
             })) => {
                 let pattern_span: Span = local_decl.source_info.span;
                 suggest_ref_mut(self.infcx.tcx, pattern_span)
-                    .map(|span| (true, span, "mut ".to_owned()))
+                    .map(|span| (true, span, "mut ".to_owned(), None))
             }
 
             _ => unreachable!(),
         };
 
         match label {
-            Some((true, err_help_span, suggested_code)) => {
-                err.span_suggestion_verbose(
-                    err_help_span,
-                    format!("consider changing this to be a mutable {pointer_desc}"),
-                    suggested_code,
+            Some((true, err_help_span, suggested_code, additional)) => {
+                let mut sugg = vec![(err_help_span, suggested_code)];
+                if let Some(s) = additional {
+                    sugg.push(s);
+                }
+
+                err.multipart_suggestion_verbose(
+                    format!(
+                        "consider changing this to be a mutable {pointer_desc}{}",
+                        if is_trait_sig {
+                            " in the `impl` method and the `trait` definition"
+                        } else {
+                            ""
+                        }
+                    ),
+                    sugg,
                     Applicability::MachineApplicable,
                 );
             }
-            Some((false, err_label_span, message)) => {
+            Some((false, err_label_span, message, _)) => {
                 let def_id = self.body.source.def_id();
                 let hir_id = if let Some(local_def_id) = def_id.as_local()
                     && let Some(body_id) = self.infcx.tcx.hir().maybe_body_owned_by(local_def_id)

tests/ui/borrowck/argument_number_mismatch_ice.stderr

@@ -12,6 +12,11 @@ error[E0594]: cannot assign to `*input`, which is behind a `&` reference
    |
 LL |         *input = self.0;
    |         ^^^^^^^^^^^^^^^ `input` is a `&` reference, so the data it refers to cannot be written
+   |
+help: consider changing this to be a mutable reference in the `impl` method and the `trait` definition
+   |
+LL |     fn example(&self, input: &mut i32) {
+   |                               +++
 
 error: aborting due to 2 previous errors
 

tests/ui/borrowck/trait-impl-argument-difference-ice.rs

@@ -0,0 +1,25 @@
+// Issue https://github.com/rust-lang/rust/issues/123414
+trait MemoryUnit {
+    extern "C" fn read_word(&mut self) -> u8;
+    extern "C" fn read_dword(Self::Assoc<'_>) -> u16;
+    //~^ WARNING anonymous parameters are deprecated and will be removed in the next edition
+    //~| WARNING this is accepted in the current edition (Rust 2015) but is a hard error in Rust 2018!
+    //~| ERROR associated type `Assoc` not found for `Self`
+}
+
+struct ROM {}
+
+impl MemoryUnit for ROM {
+//~^ ERROR not all trait items implemented, missing: `read_word`
+    extern "C" fn read_dword(&'_ self) -> u16 {
+        //~^ ERROR method `read_dword` has a `&self` declaration in the impl, but not in the trait
+        let a16 = self.read_word() as u16;
+        //~^ ERROR cannot borrow `*self` as mutable, as it is behind a `&` reference
+        let b16 = self.read_word() as u16;
+        //~^ ERROR cannot borrow `*self` as mutable, as it is behind a `&` reference
+
+        (b16 << 8) | a16
+    }
+}
+
+pub fn main() {}

tests/ui/borrowck/trait-impl-argument-difference-ice.stderr

@@ -0,0 +1,60 @@
+warning: anonymous parameters are deprecated and will be removed in the next edition
+  --> $DIR/trait-impl-argument-difference-ice.rs:4:30
+   |
+LL |     extern "C" fn read_dword(Self::Assoc<'_>) -> u16;
+   |                              ^^^^^^^^^^^^^^^ help: try naming the parameter or explicitly ignoring it: `_: Self::Assoc<'_>`
+   |
+   = warning: this is accepted in the current edition (Rust 2015) but is a hard error in Rust 2018!
+   = note: for more information, see issue #41686 <https://github.com/rust-lang/rust/issues/41686>
+   = note: `#[warn(anonymous_parameters)]` on by default
+
+error[E0220]: associated type `Assoc` not found for `Self`
+  --> $DIR/trait-impl-argument-difference-ice.rs:4:36
+   |
+LL |     extern "C" fn read_dword(Self::Assoc<'_>) -> u16;
+   |                                    ^^^^^ associated type `Assoc` not found
+
+error[E0185]: method `read_dword` has a `&self` declaration in the impl, but not in the trait
+  --> $DIR/trait-impl-argument-difference-ice.rs:14:5
+   |
+LL |     extern "C" fn read_dword(Self::Assoc<'_>) -> u16;
+   |     ------------------------------------------------- trait method declared without `&self`
+...
+LL |     extern "C" fn read_dword(&'_ self) -> u16 {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `&self` used in impl
+
+error[E0046]: not all trait items implemented, missing: `read_word`
+  --> $DIR/trait-impl-argument-difference-ice.rs:12:1
+   |
+LL |     extern "C" fn read_word(&mut self) -> u8;
+   |     ----------------------------------------- `read_word` from trait
+...
+LL | impl MemoryUnit for ROM {
+   | ^^^^^^^^^^^^^^^^^^^^^^^ missing `read_word` in implementation
+
+error[E0596]: cannot borrow `*self` as mutable, as it is behind a `&` reference
+  --> $DIR/trait-impl-argument-difference-ice.rs:16:19
+   |
+LL |         let a16 = self.read_word() as u16;
+   |                   ^^^^ `self` is a `&` reference, so the data it refers to cannot be borrowed as mutable
+   |
+help: consider changing this to be a mutable reference in the `impl` method and the `trait` definition
+   |
+LL |     extern "C" fn read_dword(&'_ mut self) -> u16 {
+   |                              ~~~~~~~~~~~~
+
+error[E0596]: cannot borrow `*self` as mutable, as it is behind a `&` reference
+  --> $DIR/trait-impl-argument-difference-ice.rs:18:19
+   |
+LL |         let b16 = self.read_word() as u16;
+   |                   ^^^^ `self` is a `&` reference, so the data it refers to cannot be borrowed as mutable
+   |
+help: consider changing this to be a mutable reference in the `impl` method and the `trait` definition
+   |
+LL |     extern "C" fn read_dword(&'_ mut self) -> u16 {
+   |                              ~~~~~~~~~~~~
+
+error: aborting due to 5 previous errors; 1 warning emitted
+
+Some errors have detailed explanations: E0046, E0185, E0220, E0596.
+For more information about an error, try `rustc --explain E0046`.

tests/ui/suggestions/issue-68049-1.stderr

@@ -1,6 +1,8 @@
 error[E0594]: cannot assign to `self.0`, which is behind a `&` reference
   --> $DIR/issue-68049-1.rs:7:9
    |
+LL |     unsafe fn alloc(&self, _layout: Layout) -> *mut u8 {
+   |                     ----- this is an immutable reference
 LL |         self.0 += 1;
    |         ^^^^^^^^^^^ `self` is a `&` reference, so the data it refers to cannot be written
 

tests/ui/suggestions/issue-68049-2.rs

@@ -1,19 +1,19 @@
 trait Hello {
-  fn example(&self, input: &i32); // should suggest here
+  fn example(&self, input: &i32);
 }
 
 struct Test1(i32);
 
 impl Hello for Test1 {
-  fn example(&self, input: &i32) { // should not suggest here
+  fn example(&self, input: &i32) {
       *input = self.0; //~ ERROR cannot assign
   }
 }
 
 struct Test2(i32);
 
 impl Hello for Test2 {
-  fn example(&self, input: &i32) { // should not suggest here
+  fn example(&self, input: &i32) {
     self.0 += *input; //~ ERROR cannot assign
   }
 }

tests/ui/suggestions/issue-68049-2.stderr

@@ -4,9 +4,9 @@ error[E0594]: cannot assign to `*input`, which is behind a `&` reference
 LL |       *input = self.0;
    |       ^^^^^^^^^^^^^^^ `input` is a `&` reference, so the data it refers to cannot be written
    |
-help: consider changing this to be a mutable reference
+help: consider changing this to be a mutable reference in the `impl` method and the `trait` definition
    |
-LL |   fn example(&self, input: &mut i32) { // should not suggest here
+LL |   fn example(&self, input: &mut i32) {
    |                             +++
 
 error[E0594]: cannot assign to `self.0`, which is behind a `&` reference
@@ -15,10 +15,14 @@ error[E0594]: cannot assign to `self.0`, which is behind a `&` reference
 LL |     self.0 += *input;
    |     ^^^^^^^^^^^^^^^^ `self` is a `&` reference, so the data it refers to cannot be written
    |
-help: consider changing this to be a mutable reference
+help: consider changing this to be a mutable reference in the `impl` method and the `trait` definition
+   |
+LL ~   fn example(&mut self, input: &i32);
+LL | }
+ ...
+LL | impl Hello for Test2 {
+LL ~   fn example(&mut self, input: &i32) {
    |
-LL |   fn example(&mut self, input: &i32); // should suggest here
-   |              ~~~~~~~~~
 
 error: aborting due to 2 previous errors