better lvalue errors for things implementing DerefMut

compiler-errors · compiler-errors · commit d50d3fccdd30 · 2022-05-17T21:20:24.000-07:00
diff --git a/compiler/rustc_typeck/src/check/coercion.rs b/compiler/rustc_typeck/src/check/coercion.rs
@@ -58,7 +58,8 @@ use rustc_session::parse::feature_err;
 use rustc_span::symbol::sym;
 use rustc_span::{self, BytePos, DesugaringKind, Span};
 use rustc_target::spec::abi::Abi;
-use rustc_trait_selection::traits::error_reporting::InferCtxtExt;
+use rustc_trait_selection::infer::InferCtxtExt as _;
+use rustc_trait_selection::traits::error_reporting::InferCtxtExt as _;
 use rustc_trait_selection::traits::{self, ObligationCause, ObligationCauseCode};
 
 use smallvec::{smallvec, SmallVec};
@@ -962,6 +963,26 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             .find_map(|(ty, steps)| self.probe(|_| coerce.unify(ty, target)).ok().map(|_| steps))
     }
 
+    /// Given a type, this function will calculate and return the type given
+    /// for `<Ty as Deref>::Target` only if `Ty` also implements `DerefMut`.
+    ///
+    /// This function is for diagnostics only, since it does not register
+    /// trait or region sub-obligations. (presumably we could, but it's not
+    /// particularly important for diagnostics...)
+    pub fn deref_once_mutably_for_diagnostic(&self, expr_ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
+        self.autoderef(rustc_span::DUMMY_SP, expr_ty).nth(1).and_then(|(deref_ty, _)| {
+            self.infcx
+                .type_implements_trait(
+                    self.infcx.tcx.lang_items().deref_mut_trait()?,
+                    expr_ty,
+                    ty::List::empty(),
+                    self.param_env,
+                )
+                .may_apply()
+                .then(|| deref_ty)
+        })
+    }
+
     /// Given some expressions, their known unified type and another expression,
     /// tries to unify the types, potentially inserting coercions on any of the
     /// provided expressions and returns their LUB (aka "common supertype").
diff --git a/compiler/rustc_typeck/src/check/demand.rs b/compiler/rustc_typeck/src/check/demand.rs
@@ -696,28 +696,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         };
 
                         if let Some(hir::Node::Expr(hir::Expr {
-                            kind: hir::ExprKind::Assign(left_expr, ..),
+                            kind: hir::ExprKind::Assign(..),
                             ..
                         })) = self.tcx.hir().find(self.tcx.hir().get_parent_node(expr.hir_id))
                         {
                             if mutability == hir::Mutability::Mut {
-                                // Found the following case:
-                                // fn foo(opt: &mut Option<String>){ opt = None }
-                                //                                   ---   ^^^^
-                                //                                   |     |
-                                //    consider dereferencing here: `*opt`  |
-                                // expected mutable reference, found enum `Option`
-                                if sm.span_to_snippet(left_expr.span).is_ok() {
-                                    return Some((
-                                        left_expr.span.shrink_to_lo(),
-                                        "consider dereferencing here to assign to the mutable \
-                                         borrowed piece of memory"
-                                            .to_string(),
-                                        "*".to_string(),
-                                        Applicability::MachineApplicable,
-                                        true,
-                                    ));
-                                }
+                                // Suppressing this diagnostic, we'll properly print it in `check_expr_assign`
+                                return None;
                             }
                         }
 
diff --git a/compiler/rustc_typeck/src/check/expr.rs b/compiler/rustc_typeck/src/check/expr.rs
@@ -51,6 +51,7 @@ use rustc_span::lev_distance::find_best_match_for_name;
 use rustc_span::source_map::Span;
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
 use rustc_span::{BytePos, Pos};
+use rustc_trait_selection::infer::InferCtxtExt;
 use rustc_trait_selection::traits::{self, ObligationCauseCode};
 
 impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
@@ -1055,25 +1056,45 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
         let lhs_ty = self.check_expr_with_needs(&lhs, Needs::MutPlace);
 
-        self.check_lhs_assignable(lhs, "E0070", span, |err| {
-            let rhs_ty = self.check_expr(&rhs);
-
-            // FIXME: This could be done any time lhs_ty is DerefMut into something that
-            // is compatible with rhs_ty, and not _just_ `&mut`
-            if let ty::Ref(_, lhs_inner_ty, hir::Mutability::Mut) = lhs_ty.kind() {
-                if self.can_coerce(rhs_ty, *lhs_inner_ty) {
+        let suggest_deref_binop = |err: &mut DiagnosticBuilder<'tcx, ErrorGuaranteed>,
+                                   rhs_ty: Ty<'tcx>| {
+            if let Some(lhs_deref_ty) = self.deref_once_mutably_for_diagnostic(lhs_ty) {
+                // Can only assign if the type is sized, so if `DerefMut` yields a type that is
+                // unsized, do not suggest dereferencing it.
+                let lhs_deref_ty_is_sized = self
+                    .infcx
+                    .type_implements_trait(
+                        self.tcx.lang_items().sized_trait().unwrap(),
+                        lhs_deref_ty,
+                        ty::List::empty(),
+                        self.param_env,
+                    )
+                    .may_apply();
+                if lhs_deref_ty_is_sized && self.can_coerce(rhs_ty, lhs_deref_ty) {
                     err.span_suggestion_verbose(
                         lhs.span.shrink_to_lo(),
-                        "consider dereferencing here to assign to the mutable \
-                    borrowed piece of memory",
+                        "consider dereferencing here to assign to the mutably borrowed value",
                         "*".to_string(),
                         Applicability::MachineApplicable,
                     );
                 }
             }
+        };
+
+        self.check_lhs_assignable(lhs, "E0070", span, |err| {
+            let rhs_ty = self.check_expr(&rhs);
+            suggest_deref_binop(err, rhs_ty);
         });
 
-        let rhs_ty = self.check_expr_coercable_to_type(&rhs, lhs_ty, Some(lhs));
+        // This is (basically) inlined `check_expr_coercable_to_type`, but we want
+        // to suggest an additional fixup here in `suggest_deref_binop`.
+        let rhs_ty = self.check_expr_with_hint(&rhs, lhs_ty);
+        if let (_, Some(mut diag)) =
+            self.demand_coerce_diag(rhs, rhs_ty, lhs_ty, Some(lhs), AllowTwoPhase::No)
+        {
+            suggest_deref_binop(&mut diag, rhs_ty);
+            diag.emit();
+        }
 
         self.require_type_is_sized(lhs_ty, lhs.span, traits::AssignmentLhsSized);
 
diff --git a/compiler/rustc_typeck/src/check/op.rs b/compiler/rustc_typeck/src/check/op.rs
@@ -42,9 +42,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             };
 
         self.check_lhs_assignable(lhs, "E0067", op.span, |err| {
-            if let Ref(_, rty, hir::Mutability::Mut) = lhs_ty.kind() {
+            if let Some(lhs_deref_ty) = self.deref_once_mutably_for_diagnostic(lhs_ty) {
                 if self
-                    .lookup_op_method(*rty, Some(rhs_ty), Some(rhs), Op::Binary(op, IsAssign::Yes))
+                    .lookup_op_method(
+                        lhs_deref_ty,
+                        Some(rhs_ty),
+                        Some(rhs),
+                        Op::Binary(op, IsAssign::Yes),
+                    )
                     .is_ok()
                 {
                     // Suppress this error, since we already emitted
@@ -415,23 +420,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         (err, missing_trait, use_output)
                     }
                 };
-                if let Ref(_, rty, mutability) = lhs_ty.kind() {
-                    let is_copy =
-                        self.infcx.type_is_copy_modulo_regions(self.param_env, *rty, lhs_expr.span);
-                    // We should suggest `a + b` => `*a + b` if `a` is copy, and suggest
-                    // `a += b` => `*a += b` if a is a mut ref.
-                    // FIXME: This could be done any time lhs_ty is DerefMut into something that
-                    // is compatible with rhs_ty, and not _just_ `&mut` (for IsAssign::Yes).
-                    if ((is_assign == IsAssign::No && is_copy)
-                        || (is_assign == IsAssign::Yes && *mutability == hir::Mutability::Mut))
-                        && self
-                            .lookup_op_method(
-                                *rty,
-                                Some(rhs_ty),
-                                Some(rhs_expr),
-                                Op::Binary(op, is_assign),
-                            )
-                            .is_ok()
+
+                let mut suggest_deref_binop = |lhs_deref_ty: Ty<'tcx>| {
+                    if self
+                        .lookup_op_method(
+                            lhs_deref_ty,
+                            Some(rhs_ty),
+                            Some(rhs_expr),
+                            Op::Binary(op, is_assign),
+                        )
+                        .is_ok()
                     {
                         if let Ok(lstring) = source_map.span_to_snippet(lhs_expr.span) {
                             let msg = &format!(
@@ -441,7 +439,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                                     IsAssign::Yes => "=",
                                     IsAssign::No => "",
                                 },
-                                rty.peel_refs(),
+                                lhs_deref_ty.peel_refs(),
                                 lstring,
                             );
                             err.span_suggestion_verbose(
@@ -452,6 +450,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                             );
                         }
                     }
+                };
+
+                // We should suggest `a + b` => `*a + b` if `a` is copy, and suggest
+                // `a += b` => `*a += b` if a is a mut ref.
+                if is_assign == IsAssign::Yes
+                    && let Some(lhs_deref_ty) = self.deref_once_mutably_for_diagnostic(lhs_ty) {
+                        suggest_deref_binop(lhs_deref_ty);
+                } else if is_assign == IsAssign::No
+                    && let Ref(_, lhs_deref_ty, _) = lhs_ty.kind() {
+                    if self.infcx.type_is_copy_modulo_regions(self.param_env, *lhs_deref_ty, lhs_expr.span) {
+                        suggest_deref_binop(*lhs_deref_ty);
+                    }
                 }
                 if let Some(missing_trait) = missing_trait {
                     let mut visitor = TypeParamVisitor(vec![]);
diff --git a/src/test/ui/suggestions/mut-ref-reassignment.stderr b/src/test/ui/suggestions/mut-ref-reassignment.stderr
@@ -8,7 +8,7 @@ LL |     opt = None;
    |
    = note: expected mutable reference `&mut Option<String>`
                            found enum `Option<_>`
-help: consider dereferencing here to assign to the mutable borrowed piece of memory
+help: consider dereferencing here to assign to the mutably borrowed value
    |
 LL |     *opt = None;
    |     +
@@ -34,7 +34,7 @@ LL |     opt = Some(String::new())
    |
    = note: expected mutable reference `&mut Option<String>`
                            found enum `Option<String>`
-help: consider dereferencing here to assign to the mutable borrowed piece of memory
+help: consider dereferencing here to assign to the mutably borrowed value
    |
 LL |     *opt = Some(String::new())
    |     +
diff --git a/src/test/ui/typeck/assign-non-lval-derefmut.fixed b/src/test/ui/typeck/assign-non-lval-derefmut.fixed
@@ -0,0 +1,15 @@
+// run-rustfix
+
+fn main() {
+    let x = std::sync::Mutex::new(1usize);
+    *x.lock().unwrap() = 2;
+    //~^ ERROR invalid left-hand side of assignment
+    *x.lock().unwrap() += 1;
+    //~^ ERROR binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>`
+
+    let mut y = x.lock().unwrap();
+    *y = 2;
+    //~^ ERROR mismatched types
+    *y += 1;
+    //~^ ERROR binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>`
+}
diff --git a/src/test/ui/typeck/assign-non-lval-derefmut.rs b/src/test/ui/typeck/assign-non-lval-derefmut.rs
@@ -0,0 +1,15 @@
+// run-rustfix
+
+fn main() {
+    let x = std::sync::Mutex::new(1usize);
+    x.lock().unwrap() = 2;
+    //~^ ERROR invalid left-hand side of assignment
+    x.lock().unwrap() += 1;
+    //~^ ERROR binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>`
+
+    let mut y = x.lock().unwrap();
+    y = 2;
+    //~^ ERROR mismatched types
+    y += 1;
+    //~^ ERROR binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>`
+}
diff --git a/src/test/ui/typeck/assign-non-lval-derefmut.stderr b/src/test/ui/typeck/assign-non-lval-derefmut.stderr
@@ -0,0 +1,58 @@
+error[E0070]: invalid left-hand side of assignment
+  --> $DIR/assign-non-lval-derefmut.rs:5:23
+   |
+LL |     x.lock().unwrap() = 2;
+   |     ----------------- ^
+   |     |
+   |     cannot assign to this expression
+   |
+help: consider dereferencing here to assign to the mutably borrowed value
+   |
+LL |     *x.lock().unwrap() = 2;
+   |     +
+
+error[E0368]: binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>`
+  --> $DIR/assign-non-lval-derefmut.rs:7:5
+   |
+LL |     x.lock().unwrap() += 1;
+   |     -----------------^^^^^
+   |     |
+   |     cannot use `+=` on type `MutexGuard<'_, usize>`
+   |
+help: `+=` can be used on `usize`, you can dereference `x.lock().unwrap()`
+   |
+LL |     *x.lock().unwrap() += 1;
+   |     +
+
+error[E0308]: mismatched types
+  --> $DIR/assign-non-lval-derefmut.rs:11:9
+   |
+LL |     let mut y = x.lock().unwrap();
+   |                 ----------------- expected due to this value
+LL |     y = 2;
+   |         ^ expected struct `MutexGuard`, found integer
+   |
+   = note: expected struct `MutexGuard<'_, usize>`
+                found type `{integer}`
+help: consider dereferencing here to assign to the mutably borrowed value
+   |
+LL |     *y = 2;
+   |     +
+
+error[E0368]: binary assignment operation `+=` cannot be applied to type `MutexGuard<'_, usize>`
+  --> $DIR/assign-non-lval-derefmut.rs:13:5
+   |
+LL |     y += 1;
+   |     -^^^^^
+   |     |
+   |     cannot use `+=` on type `MutexGuard<'_, usize>`
+   |
+help: `+=` can be used on `usize`, you can dereference `y`
+   |
+LL |     *y += 1;
+   |     +
+
+error: aborting due to 4 previous errors
+
+Some errors have detailed explanations: E0070, E0308, E0368.
+For more information about an error, try `rustc --explain E0070`.
diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.fixed b/src/test/ui/typeck/assign-non-lval-mut-ref.fixed
@@ -6,4 +6,10 @@ fn main() {
     //~^ ERROR invalid left-hand side of assignment
     *x.last_mut().unwrap() += 1;
     //~^ ERROR binary assignment operation `+=` cannot be applied to type `&mut usize`
+
+    let y = x.last_mut().unwrap();
+    *y = 2;
+    //~^ ERROR mismatched types
+    *y += 1;
+    //~^ ERROR binary assignment operation `+=` cannot be applied to type `&mut usize`
 }
diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.rs b/src/test/ui/typeck/assign-non-lval-mut-ref.rs
@@ -6,4 +6,10 @@ fn main() {
     //~^ ERROR invalid left-hand side of assignment
     x.last_mut().unwrap() += 1;
     //~^ ERROR binary assignment operation `+=` cannot be applied to type `&mut usize`
+
+    let y = x.last_mut().unwrap();
+    y = 2;
+    //~^ ERROR mismatched types
+    y += 1;
+    //~^ ERROR binary assignment operation `+=` cannot be applied to type `&mut usize`
 }
diff --git a/src/test/ui/typeck/assign-non-lval-mut-ref.stderr b/src/test/ui/typeck/assign-non-lval-mut-ref.stderr
@@ -6,7 +6,7 @@ LL |     x.last_mut().unwrap() = 2;
    |     |
    |     cannot assign to this expression
    |
-help: consider dereferencing here to assign to the mutable borrowed piece of memory
+help: consider dereferencing here to assign to the mutably borrowed value
    |
 LL |     *x.last_mut().unwrap() = 2;
    |     +
@@ -24,7 +24,33 @@ help: `+=` can be used on `usize`, you can dereference `x.last_mut().unwrap()`
 LL |     *x.last_mut().unwrap() += 1;
    |     +
 
-error: aborting due to 2 previous errors
+error[E0308]: mismatched types
+  --> $DIR/assign-non-lval-mut-ref.rs:11:9
+   |
+LL |     let y = x.last_mut().unwrap();
+   |             --------------------- expected due to this value
+LL |     y = 2;
+   |         ^ expected `&mut usize`, found integer
+   |
+help: consider dereferencing here to assign to the mutably borrowed value
+   |
+LL |     *y = 2;
+   |     +
+
+error[E0368]: binary assignment operation `+=` cannot be applied to type `&mut usize`
+  --> $DIR/assign-non-lval-mut-ref.rs:13:5
+   |
+LL |     y += 1;
+   |     -^^^^^
+   |     |
+   |     cannot use `+=` on type `&mut usize`
+   |
+help: `+=` can be used on `usize`, you can dereference `y`
+   |
+LL |     *y += 1;
+   |     +
+
+error: aborting due to 4 previous errors
 
-Some errors have detailed explanations: E0070, E0368.
+Some errors have detailed explanations: E0070, E0308, E0368.
 For more information about an error, try `rustc --explain E0070`.
diff --git a/src/test/ui/typeck/issue-93486.stderr b/src/test/ui/typeck/issue-93486.stderr
@@ -6,7 +6,7 @@ LL |         vec![].last_mut().unwrap() = 3_u8;
    |         |
    |         cannot assign to this expression
    |
-help: consider dereferencing here to assign to the mutable borrowed piece of memory
+help: consider dereferencing here to assign to the mutably borrowed value
    |
 LL |         *vec![].last_mut().unwrap() = 3_u8;
    |         +

Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@ LL \| opt = None;`
`8`	`8`	`\|`
`9`	`9`	= note: expected mutable reference `&mut Option<String>`
`10`	`10`	found enum `Option<_>`
`11`		`-help: consider dereferencing here to assign to the mutable borrowed piece of memory`
	`11`	`+help: consider dereferencing here to assign to the mutably borrowed value`
`12`	`12`	`\|`
`13`	`13`	`LL \| *opt = None;`
`14`	`14`	`\| +`
`@@ -34,7 +34,7 @@ LL \| opt = Some(String::new())`
`34`	`34`	`\|`
`35`	`35`	= note: expected mutable reference `&mut Option<String>`
`36`	`36`	found enum `Option<String>`
`37`		`-help: consider dereferencing here to assign to the mutable borrowed piece of memory`
	`37`	`+help: consider dereferencing here to assign to the mutably borrowed value`
`38`	`38`	`\|`
`39`	`39`	`LL \| *opt = Some(String::new())`
`40`	`40`	`\| +`
Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,7 @@ LL \| vec![].last_mut().unwrap() = 3_u8;`
`6`	`6`	`\| \|`
`7`	`7`	`\| cannot assign to this expression`
`8`	`8`	`\|`
`9`		`-help: consider dereferencing here to assign to the mutable borrowed piece of memory`
	`9`	`+help: consider dereferencing here to assign to the mutably borrowed value`
`10`	`10`	`\|`
`11`	`11`	`LL \| *vec![].last_mut().unwrap() = 3_u8;`
`12`	`12`	`\| +`