Rollup merge of #114288 - Urgau:fix-issue-109352, r=b-naber

Improve diagnostic for wrong borrow on binary operations

This PR improves the diagnostic for wrong borrow on binary operations by suggesting to reborrow on appropriate expressions.

```diff
+    = note: an implementation for `&Foo * &Foo` exist
+ help: consider reborrowing both sides
+    |
+ LL |     let _ = &*ref_mut_foo * &*ref_mut_foo;
+    |             ++              ++
```

Fixes #109352

Author: matthiaskrgr

compiler/rustc_hir_typeck/src/op.rs

@@ -4,7 +4,7 @@ use super::method::MethodCallee;
 use super::{has_expected_num_generic_args, FnCtxt};
 use crate::Expectation;
 use rustc_ast as ast;
-use rustc_errors::{self, struct_span_err, Applicability, Diagnostic};
+use rustc_errors::{self, struct_span_err, Applicability, Diagnostic, DiagnosticBuilder};
 use rustc_hir as hir;
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
 use rustc_infer::traits::ObligationCauseCode;
@@ -380,33 +380,93 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                     }
                 };
 
-                let mut suggest_deref_binop = |lhs_deref_ty: Ty<'tcx>| {
-                    if self
-                        .lookup_op_method(
-                            lhs_deref_ty,
-                            Some((rhs_expr, rhs_ty)),
-                            Op::Binary(op, is_assign),
-                            expected,
-                        )
-                        .is_ok()
-                    {
-                        let msg = format!(
-                            "`{}{}` can be used on `{}` if you dereference the left-hand side",
-                            op.node.as_str(),
-                            match is_assign {
-                                IsAssign::Yes => "=",
-                                IsAssign::No => "",
-                            },
-                            lhs_deref_ty,
-                        );
-                        err.span_suggestion_verbose(
-                            lhs_expr.span.shrink_to_lo(),
-                            msg,
-                            "*",
-                            rustc_errors::Applicability::MachineApplicable,
-                        );
-                    }
-                };
+                let suggest_deref_binop =
+                    |err: &mut DiagnosticBuilder<'_, _>, lhs_deref_ty: Ty<'tcx>| {
+                        if self
+                            .lookup_op_method(
+                                lhs_deref_ty,
+                                Some((rhs_expr, rhs_ty)),
+                                Op::Binary(op, is_assign),
+                                expected,
+                            )
+                            .is_ok()
+                        {
+                            let msg = format!(
+                                "`{}{}` can be used on `{}` if you dereference the left-hand side",
+                                op.node.as_str(),
+                                match is_assign {
+                                    IsAssign::Yes => "=",
+                                    IsAssign::No => "",
+                                },
+                                lhs_deref_ty,
+                            );
+                            err.span_suggestion_verbose(
+                                lhs_expr.span.shrink_to_lo(),
+                                msg,
+                                "*",
+                                rustc_errors::Applicability::MachineApplicable,
+                            );
+                        }
+                    };
+
+                let suggest_different_borrow =
+                    |err: &mut DiagnosticBuilder<'_, _>,
+                     lhs_adjusted_ty,
+                     lhs_new_mutbl: Option<ast::Mutability>,
+                     rhs_adjusted_ty,
+                     rhs_new_mutbl: Option<ast::Mutability>| {
+                        if self
+                            .lookup_op_method(
+                                lhs_adjusted_ty,
+                                Some((rhs_expr, rhs_adjusted_ty)),
+                                Op::Binary(op, is_assign),
+                                expected,
+                            )
+                            .is_ok()
+                        {
+                            let op_str = op.node.as_str();
+                            err.note(format!("an implementation for `{lhs_adjusted_ty} {op_str} {rhs_adjusted_ty}` exists"));
+
+                            if let Some(lhs_new_mutbl) = lhs_new_mutbl
+                                && let Some(rhs_new_mutbl) = rhs_new_mutbl
+                                && lhs_new_mutbl.is_not()
+                                && rhs_new_mutbl.is_not() {
+                                err.multipart_suggestion_verbose(
+                                    "consider reborrowing both sides",
+                                    vec![
+                                        (lhs_expr.span.shrink_to_lo(), "&*".to_string()),
+                                        (rhs_expr.span.shrink_to_lo(), "&*".to_string())
+                                    ],
+                                    rustc_errors::Applicability::MachineApplicable,
+                                );
+                            } else {
+                                let mut suggest_new_borrow = |new_mutbl: ast::Mutability, sp: Span| {
+                                    // Can reborrow (&mut -> &)
+                                    if new_mutbl.is_not() {
+                                        err.span_suggestion_verbose(
+                                            sp.shrink_to_lo(),
+                                            "consider reborrowing this side",
+                                            "&*",
+                                            rustc_errors::Applicability::MachineApplicable,
+                                        );
+                                    // Works on &mut but have &
+                                    } else {
+                                        err.span_help(
+                                            sp,
+                                            "consider making this expression a mutable borrow",
+                                        );
+                                    }
+                                };
+
+                                if let Some(lhs_new_mutbl) = lhs_new_mutbl {
+                                    suggest_new_borrow(lhs_new_mutbl, lhs_expr.span);
+                                }
+                                if let Some(rhs_new_mutbl) = rhs_new_mutbl {
+                                    suggest_new_borrow(rhs_new_mutbl, rhs_expr.span);
+                                }
+                            }
+                        }
+                    };
 
                 let is_compatible_after_call = |lhs_ty, rhs_ty| {
                     self.lookup_op_method(
@@ -429,15 +489,60 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 } else if is_assign == IsAssign::Yes
                     && let Some(lhs_deref_ty) = self.deref_once_mutably_for_diagnostic(lhs_ty)
                 {
-                    suggest_deref_binop(lhs_deref_ty);
+                    suggest_deref_binop(&mut err, lhs_deref_ty);
                 } else if is_assign == IsAssign::No
-                    && let Ref(_, lhs_deref_ty, _) = lhs_ty.kind()
+                    && let Ref(region, lhs_deref_ty, mutbl) = lhs_ty.kind()
                 {
                     if self.type_is_copy_modulo_regions(
                         self.param_env,
                         *lhs_deref_ty,
                     ) {
-                        suggest_deref_binop(*lhs_deref_ty);
+                        suggest_deref_binop(&mut err, *lhs_deref_ty);
+                    } else {
+                        let lhs_inv_mutbl = mutbl.invert();
+                        let lhs_inv_mutbl_ty = Ty::new_ref(
+                            self.tcx,
+                            *region,
+                            ty::TypeAndMut {
+                                ty: *lhs_deref_ty,
+                                mutbl: lhs_inv_mutbl,
+                            },
+                        );
+
+                        suggest_different_borrow(
+                            &mut err,
+                            lhs_inv_mutbl_ty,
+                            Some(lhs_inv_mutbl),
+                            rhs_ty,
+                            None,
+                        );
+
+                        if let Ref(region, rhs_deref_ty, mutbl) = rhs_ty.kind() {
+                            let rhs_inv_mutbl = mutbl.invert();
+                            let rhs_inv_mutbl_ty = Ty::new_ref(
+                                self.tcx,
+                                *region,
+                                ty::TypeAndMut {
+                                    ty: *rhs_deref_ty,
+                                    mutbl: rhs_inv_mutbl,
+                                },
+                            );
+
+                            suggest_different_borrow(
+                                &mut err,
+                                lhs_ty,
+                                None,
+                                rhs_inv_mutbl_ty,
+                                Some(rhs_inv_mutbl),
+                            );
+                            suggest_different_borrow(
+                                &mut err,
+                                lhs_inv_mutbl_ty,
+                                Some(lhs_inv_mutbl),
+                                rhs_inv_mutbl_ty,
+                                Some(rhs_inv_mutbl),
+                            );
+                        }
                     }
                 } else if self.suggest_fn_call(&mut err, lhs_expr, lhs_ty, |lhs_ty| {
                     is_compatible_after_call(lhs_ty, rhs_ty)

tests/ui/binop/borrow-suggestion-109352-2.rs

@@ -0,0 +1,27 @@
+struct Bar;
+
+impl std::ops::Mul for &mut Bar {
+    type Output = Bar;
+
+    fn mul(self, _rhs: Self) -> Self::Output {
+        unimplemented!()
+    }
+}
+
+fn main() {
+    let ref_mut_bar: &mut Bar = &mut Bar;
+    let ref_bar: &Bar = &Bar;
+    let owned_bar: Bar = Bar;
+
+    let _ = ref_mut_bar * ref_mut_bar;
+
+    // FIXME: we should be able to suggest borrowing both side
+    let _ = owned_bar * owned_bar;
+    //~^ ERROR cannot multiply
+    let _ = ref_bar * ref_bar;
+    //~^ ERROR cannot multiply
+    let _ = ref_bar * ref_mut_bar;
+    //~^ ERROR cannot multiply
+    let _ = ref_mut_bar * ref_bar;
+    //~^ ERROR mismatched types
+}

tests/ui/binop/borrow-suggestion-109352-2.stderr

@@ -0,0 +1,64 @@
+error[E0369]: cannot multiply `Bar` by `Bar`
+  --> $DIR/borrow-suggestion-109352-2.rs:19:23
+   |
+LL |     let _ = owned_bar * owned_bar;
+   |             --------- ^ --------- Bar
+   |             |
+   |             Bar
+   |
+note: an implementation of `Mul` might be missing for `Bar`
+  --> $DIR/borrow-suggestion-109352-2.rs:1:1
+   |
+LL | struct Bar;
+   | ^^^^^^^^^^ must implement `Mul`
+note: the trait `Mul` must be implemented
+  --> $SRC_DIR/core/src/ops/arith.rs:LL:COL
+
+error[E0369]: cannot multiply `&Bar` by `&Bar`
+  --> $DIR/borrow-suggestion-109352-2.rs:21:21
+   |
+LL |     let _ = ref_bar * ref_bar;
+   |             ------- ^ ------- &Bar
+   |             |
+   |             &Bar
+   |
+   = note: an implementation for `&mut Bar * &mut Bar` exists
+help: consider making this expression a mutable borrow
+  --> $DIR/borrow-suggestion-109352-2.rs:21:13
+   |
+LL |     let _ = ref_bar * ref_bar;
+   |             ^^^^^^^
+help: consider making this expression a mutable borrow
+  --> $DIR/borrow-suggestion-109352-2.rs:21:23
+   |
+LL |     let _ = ref_bar * ref_bar;
+   |                       ^^^^^^^
+
+error[E0369]: cannot multiply `&Bar` by `&mut Bar`
+  --> $DIR/borrow-suggestion-109352-2.rs:23:21
+   |
+LL |     let _ = ref_bar * ref_mut_bar;
+   |             ------- ^ ----------- &mut Bar
+   |             |
+   |             &Bar
+   |
+   = note: an implementation for `&mut Bar * &mut Bar` exists
+help: consider making this expression a mutable borrow
+  --> $DIR/borrow-suggestion-109352-2.rs:23:13
+   |
+LL |     let _ = ref_bar * ref_mut_bar;
+   |             ^^^^^^^
+
+error[E0308]: mismatched types
+  --> $DIR/borrow-suggestion-109352-2.rs:25:27
+   |
+LL |     let _ = ref_mut_bar * ref_bar;
+   |                           ^^^^^^^ types differ in mutability
+   |
+   = note: expected mutable reference `&mut Bar`
+                      found reference `&Bar`
+
+error: aborting due to 4 previous errors
+
+Some errors have detailed explanations: E0308, E0369.
+For more information about an error, try `rustc --explain E0308`.

tests/ui/binop/borrow-suggestion-109352.fixed

@@ -0,0 +1,27 @@
+// run-rustfix
+
+struct Foo;
+
+impl std::ops::Mul for &Foo {
+    type Output = Foo;
+
+    fn mul(self, _rhs: Self) -> Self::Output {
+        unimplemented!()
+    }
+}
+
+fn main() {
+    let ref_mut_foo: &mut Foo = &mut Foo;
+    let ref_foo: &Foo = &Foo;
+    let owned_foo: Foo = Foo;
+
+    let _ = ref_foo * ref_foo;
+    let _ = ref_foo * ref_mut_foo;
+
+    let _ = &*ref_mut_foo * ref_foo;
+    //~^ ERROR cannot multiply
+    let _ = &*ref_mut_foo * &*ref_mut_foo;
+    //~^ ERROR cannot multiply
+    let _ = &*ref_mut_foo * &owned_foo;
+    //~^ ERROR cannot multiply
+}

tests/ui/binop/borrow-suggestion-109352.rs

@@ -0,0 +1,27 @@
+// run-rustfix
+
+struct Foo;
+
+impl std::ops::Mul for &Foo {
+    type Output = Foo;
+
+    fn mul(self, _rhs: Self) -> Self::Output {
+        unimplemented!()
+    }
+}
+
+fn main() {
+    let ref_mut_foo: &mut Foo = &mut Foo;
+    let ref_foo: &Foo = &Foo;
+    let owned_foo: Foo = Foo;
+
+    let _ = ref_foo * ref_foo;
+    let _ = ref_foo * ref_mut_foo;
+
+    let _ = ref_mut_foo * ref_foo;
+    //~^ ERROR cannot multiply
+    let _ = ref_mut_foo * ref_mut_foo;
+    //~^ ERROR cannot multiply
+    let _ = ref_mut_foo * &owned_foo;
+    //~^ ERROR cannot multiply
+}

tests/ui/binop/borrow-suggestion-109352.stderr

@@ -0,0 +1,45 @@
+error[E0369]: cannot multiply `&mut Foo` by `&Foo`
+  --> $DIR/borrow-suggestion-109352.rs:21:25
+   |
+LL |     let _ = ref_mut_foo * ref_foo;
+   |             ----------- ^ ------- &Foo
+   |             |
+   |             &mut Foo
+   |
+   = note: an implementation for `&Foo * &Foo` exists
+help: consider reborrowing this side
+   |
+LL |     let _ = &*ref_mut_foo * ref_foo;
+   |             ++
+
+error[E0369]: cannot multiply `&mut Foo` by `&mut Foo`
+  --> $DIR/borrow-suggestion-109352.rs:23:25
+   |
+LL |     let _ = ref_mut_foo * ref_mut_foo;
+   |             ----------- ^ ----------- &mut Foo
+   |             |
+   |             &mut Foo
+   |
+   = note: an implementation for `&Foo * &Foo` exists
+help: consider reborrowing both sides
+   |
+LL |     let _ = &*ref_mut_foo * &*ref_mut_foo;
+   |             ++              ++
+
+error[E0369]: cannot multiply `&mut Foo` by `&Foo`
+  --> $DIR/borrow-suggestion-109352.rs:25:25
+   |
+LL |     let _ = ref_mut_foo * &owned_foo;
+   |             ----------- ^ ---------- &Foo
+   |             |
+   |             &mut Foo
+   |
+   = note: an implementation for `&Foo * &Foo` exists
+help: consider reborrowing this side
+   |
+LL |     let _ = &*ref_mut_foo * &owned_foo;
+   |             ++
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0369`.