Rollup merge of #123523 - estebank:issue-123414, r=BoxyUwU

Account for trait/impl difference when suggesting changing argument from ref to mut ref Do not ICE when encountering a lifetime error involving an argument with an immutable reference of a method that differs from the trait definition. Fix #123414.
2024-10-30 14:01:51 +00:00 · 2024-04-11 22:38:54 +02:00 · 2024-04-11 22:38:54 +02:00 · ec91d71a38
commit ec91d71a38
parent 1e99af514b 731c0e59a4
7 changed files with 158 additions and 43 deletions
--- a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs
+++ b/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)
--- a/tests/ui/borrowck/argument_number_mismatch_ice.stderr
+++ b/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

--- a/tests/ui/borrowck/trait-impl-argument-difference-ice.rs
+++ b/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() {}
--- a/tests/ui/borrowck/trait-impl-argument-difference-ice.stderr
+++ b/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`.
--- a/tests/ui/suggestions/issue-68049-1.stderr
+++ b/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

--- a/tests/ui/suggestions/issue-68049-2.rs
+++ b/tests/ui/suggestions/issue-68049-2.rs
@ -1,11 +1,11 @@
 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
  }
 }
@ -13,7 +13,7 @@ impl Hello for Test1 {
 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
  }
 }
--- a/tests/ui/suggestions/issue-68049-2.stderr
+++ b/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