From d50d3fccdd3038b2621245e9591ea6e20eebde2a Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sat, 5 Mar 2022 01:57:43 -0800 Subject: [PATCH] better lvalue errors for things implementing DerefMut --- compiler/rustc_typeck/src/check/coercion.rs | 23 +++++++- compiler/rustc_typeck/src/check/demand.rs | 21 +------ compiler/rustc_typeck/src/check/expr.rs | 41 +++++++++---- compiler/rustc_typeck/src/check/op.rs | 50 +++++++++------- .../suggestions/mut-ref-reassignment.stderr | 4 +- .../ui/typeck/assign-non-lval-derefmut.fixed | 15 +++++ .../ui/typeck/assign-non-lval-derefmut.rs | 15 +++++ .../ui/typeck/assign-non-lval-derefmut.stderr | 58 +++++++++++++++++++ .../ui/typeck/assign-non-lval-mut-ref.fixed | 6 ++ src/test/ui/typeck/assign-non-lval-mut-ref.rs | 6 ++ .../ui/typeck/assign-non-lval-mut-ref.stderr | 32 +++++++++- src/test/ui/typeck/issue-93486.stderr | 2 +- 12 files changed, 218 insertions(+), 55 deletions(-) create mode 100644 src/test/ui/typeck/assign-non-lval-derefmut.fixed create mode 100644 src/test/ui/typeck/assign-non-lval-derefmut.rs create mode 100644 src/test/ui/typeck/assign-non-lval-derefmut.stderr diff --git a/compiler/rustc_typeck/src/check/coercion.rs b/compiler/rustc_typeck/src/check/coercion.rs index 57d30799354..22683b1de75 100644 --- 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 `::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> { + 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 index ae4cebe866b..ede2180a8e9 100644 --- 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){ 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 index bf438b44989..6d56445771a 100644 --- 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 index d250f38fffa..c99d9d8f923 100644 --- 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 index 3bd98c76307..b3cb6dd0614 100644 --- 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` 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` 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 = 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 new file mode 100644 index 00000000000..0c23199af22 --- /dev/null +++ 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 new file mode 100644 index 00000000000..ec1882f5271 --- /dev/null +++ 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 new file mode 100644 index 00000000000..a6fcdfe21f4 --- /dev/null +++ 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 index c4dadfbdfce..10c7b9dbfb3 100644 --- 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 index 39573ddb6d0..bceff0ef09d 100644 --- 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 index b0ca089b709..be2e9fe95e8 100644 --- 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; + | + -Some errors have detailed explanations: E0070, E0368. +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, 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 index 95eb021965f..167edc8942a 100644 --- 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; | +