From 5ce3f5664130eaf24d187d04dcd51c4577336ab5 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Sun, 5 Dec 2021 04:51:36 +0000 Subject: [PATCH] Make deref suggestion better --- compiler/rustc_typeck/src/check/demand.rs | 150 +++++++++++------- .../src/check/fn_ctxt/suggestions.rs | 4 +- src/test/ui/parser/expr-as-stmt-2.stderr | 5 + src/test/ui/parser/expr-as-stmt.stderr | 5 + .../disallowed-positions.stderr | 12 ++ src/test/ui/typeck/deref-multi.rs | 26 +++ src/test/ui/typeck/deref-multi.stderr | 72 +++++++++ 7 files changed, 214 insertions(+), 60 deletions(-) create mode 100644 src/test/ui/typeck/deref-multi.rs create mode 100644 src/test/ui/typeck/deref-multi.stderr diff --git a/compiler/rustc_typeck/src/check/demand.rs b/compiler/rustc_typeck/src/check/demand.rs index 80096b90f9530..47fc4b6683368 100644 --- a/compiler/rustc_typeck/src/check/demand.rs +++ b/compiler/rustc_typeck/src/check/demand.rs @@ -566,7 +566,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { expr: &hir::Expr<'tcx>, checked_ty: Ty<'tcx>, expected: Ty<'tcx>, - ) -> Option<(Span, &'static str, String, Applicability, bool /* verbose */)> { + ) -> Option<(Span, String, String, Applicability, bool /* verbose */)> { let sess = self.sess(); let sp = expr.span; @@ -593,7 +593,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let pos = sp.lo() + BytePos(1); return Some(( sp.with_hi(pos), - "consider removing the leading `b`", + "consider removing the leading `b`".to_string(), String::new(), Applicability::MachineApplicable, true, @@ -608,7 +608,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if replace_prefix(&src, "\"", "b\"").is_some() { return Some(( sp.shrink_to_lo(), - "consider adding a leading `b`", + "consider adding a leading `b`".to_string(), "b".to_string(), Applicability::MachineApplicable, true, @@ -669,7 +669,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if let Some(sugg) = self.can_use_as_ref(expr) { return Some(( sugg.0, - sugg.1, + sugg.1.to_string(), sugg.2, Applicability::MachineApplicable, false, @@ -697,7 +697,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { return Some(( left_expr.span.shrink_to_lo(), "consider dereferencing here to assign to the mutable \ - borrowed piece of memory", + borrowed piece of memory" + .to_string(), "*".to_string(), Applicability::MachineApplicable, true, @@ -709,14 +710,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { return Some(match mutability { hir::Mutability::Mut => ( sp, - "consider mutably borrowing here", + "consider mutably borrowing here".to_string(), format!("{}&mut {}", prefix, sugg_expr), Applicability::MachineApplicable, false, ), hir::Mutability::Not => ( sp, - "consider borrowing here", + "consider borrowing here".to_string(), format!("{}&{}", prefix, sugg_expr), Applicability::MachineApplicable, false, @@ -745,7 +746,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if sm.span_to_snippet(call_span).is_ok() { return Some(( sp.with_hi(call_span.lo()), - "consider removing the borrow", + "consider removing the borrow".to_string(), String::new(), Applicability::MachineApplicable, true, @@ -758,7 +759,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { if sm.span_to_snippet(expr.span).is_ok() { return Some(( sp.with_hi(expr.span.lo()), - "consider removing the borrow", + "consider removing the borrow".to_string(), String::new(), Applicability::MachineApplicable, true, @@ -824,7 +825,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } { return Some(( span, - "consider dereferencing", + "consider dereferencing".to_string(), src, applicability, true, @@ -835,60 +836,93 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } } _ if sp == expr.span => { - if let Some(steps) = self.deref_steps(checked_ty, expected) { - let expr = expr.peel_blocks(); + if let Some(mut steps) = self.deref_steps(checked_ty, expected) { + let mut expr = expr.peel_blocks(); + let mut prefix_span = expr.span.shrink_to_lo(); + let mut remove = String::new(); - if steps == 1 { + // Try peeling off any existing `&` and `&mut` to reach our target type + while steps > 0 { if let hir::ExprKind::AddrOf(_, mutbl, inner) = expr.kind { // If the expression has `&`, removing it would fix the error - let prefix_span = expr.span.with_hi(inner.span.lo()); - let message = match mutbl { - hir::Mutability::Not => "consider removing the `&`", - hir::Mutability::Mut => "consider removing the `&mut`", + prefix_span = prefix_span.with_hi(inner.span.lo()); + expr = inner; + remove += match mutbl { + hir::Mutability::Not => "&", + hir::Mutability::Mut => "&mut ", }; - let suggestion = String::new(); - return Some(( - prefix_span, - message, - suggestion, - Applicability::MachineApplicable, - false, - )); + steps -= 1; + } else { + break; } - - // For this suggestion to make sense, the type would need to be `Copy`, - // or we have to be moving out of a `Box` - if self.infcx.type_is_copy_modulo_regions(self.param_env, expected, sp) - || checked_ty.is_box() - { - let message = if checked_ty.is_box() { - "consider unboxing the value" - } else if checked_ty.is_region_ptr() { - "consider dereferencing the borrow" - } else { - "consider dereferencing the type" - }; - let prefix = match self.maybe_get_struct_pattern_shorthand_field(expr) { - Some(ident) => format!("{}: ", ident), - None => String::new(), - }; - let (span, suggestion) = if self.is_else_if_block(expr) { - // Don't suggest nonsense like `else *if` - return None; - } else if let Some(expr) = self.maybe_get_block_expr(expr) { - // prefix should be empty here.. - (expr.span.shrink_to_lo(), "*".to_string()) + } + // If we've reached our target type with just removing `&`, then just print now. + if steps == 0 { + return Some(( + prefix_span, + format!("consider removing the `{}`", remove.trim()), + String::new(), + // Do not remove `&&` to get to bool, because it might be something like + // { a } && b, which we have a separate fixup suggestion that is more + // likely correct... + if remove.trim() == "&&" && expected == self.tcx.types.bool { + Applicability::MaybeIncorrect } else { - (expr.span.shrink_to_lo(), format!("{}*", prefix)) - }; - return Some(( - span, - message, - suggestion, - Applicability::MachineApplicable, - true, - )); - } + Applicability::MachineApplicable + }, + true, + )); + } + + // For this suggestion to make sense, the type would need to be `Copy`, + // or we have to be moving out of a `Box` + if self.infcx.type_is_copy_modulo_regions(self.param_env, expected, sp) + // FIXME(compiler-errors): We can actually do this if the checked_ty is + // `steps` layers of boxes, not just one, but this is easier and most likely. + || (checked_ty.is_box() && steps == 1) + { + let deref_kind = if checked_ty.is_box() { + "unboxing the value" + } else if checked_ty.is_region_ptr() { + "dereferencing the borrow" + } else { + "dereferencing the type" + }; + + // Suggest removing `&` if we have removed any, otherwise suggest just + // dereferencing the remaining number of steps. + let message = if remove.is_empty() { + format!("consider {}", deref_kind) + } else { + format!( + "consider removing the `{}` and {} instead", + remove.trim(), + deref_kind + ) + }; + + let prefix = match self.maybe_get_struct_pattern_shorthand_field(expr) { + Some(ident) => format!("{}: ", ident), + None => String::new(), + }; + + let (span, suggestion) = if self.is_else_if_block(expr) { + // Don't suggest nonsense like `else *if` + return None; + } else if let Some(expr) = self.maybe_get_block_expr(expr) { + // prefix should be empty here.. + (expr.span.shrink_to_lo(), "*".to_string()) + } else { + (prefix_span, format!("{}{}", prefix, "*".repeat(steps))) + }; + + return Some(( + span, + message, + suggestion, + Applicability::MachineApplicable, + true, + )); } } } diff --git a/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs b/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs index 8cad4fc707ea3..523602d5b1888 100644 --- a/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs +++ b/compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs @@ -218,9 +218,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.check_ref(expr, found, expected) { if verbose { - err.span_suggestion_verbose(sp, msg, suggestion, applicability); + err.span_suggestion_verbose(sp, &msg, suggestion, applicability); } else { - err.span_suggestion(sp, msg, suggestion, applicability); + err.span_suggestion(sp, &msg, suggestion, applicability); } } else if let (ty::FnDef(def_id, ..), true) = (&found.kind(), self.suggest_fn_call(err, expr, expected, found)) diff --git a/src/test/ui/parser/expr-as-stmt-2.stderr b/src/test/ui/parser/expr-as-stmt-2.stderr index 2b6314c38ceb6..b7516babc1330 100644 --- a/src/test/ui/parser/expr-as-stmt-2.stderr +++ b/src/test/ui/parser/expr-as-stmt-2.stderr @@ -36,6 +36,11 @@ LL | / && LL | | if let Some(y) = a { true } else { false } | |______________________________________________^ expected `bool`, found `&&bool` | +help: consider removing the `&&` + | +LL - && +LL + if let Some(y) = a { true } else { false } + | help: parentheses are required to parse this as an expression | LL | (if let Some(x) = a { true } else { false }) diff --git a/src/test/ui/parser/expr-as-stmt.stderr b/src/test/ui/parser/expr-as-stmt.stderr index df0e4dcb16e2c..e63da52c8fe1d 100644 --- a/src/test/ui/parser/expr-as-stmt.stderr +++ b/src/test/ui/parser/expr-as-stmt.stderr @@ -170,6 +170,11 @@ LL | fn revenge_from_mars() -> bool { LL | { true } && { true } | ^^^^^^^^^^^ expected `bool`, found `&&bool` | +help: consider removing the `&&` + | +LL - { true } && { true } +LL + { true } { true } + | help: parentheses are required to parse this as an expression | LL | ({ true }) && { true } diff --git a/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.stderr b/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.stderr index 4c830554d435c..897de54a7bf8c 100644 --- a/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.stderr +++ b/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.stderr @@ -676,6 +676,12 @@ error[E0308]: mismatched types | LL | if let Range { start: true, end } = t..&&false {} | ^^^^^^^ expected `bool`, found `&&bool` + | +help: consider removing the `&&` + | +LL - if let Range { start: true, end } = t..&&false {} +LL + if let Range { start: true, end } = t..false {} + | error[E0308]: mismatched types --> $DIR/disallowed-positions.rs:83:8 @@ -866,6 +872,12 @@ error[E0308]: mismatched types | LL | while let Range { start: true, end } = t..&&false {} | ^^^^^^^ expected `bool`, found `&&bool` + | +help: consider removing the `&&` + | +LL - while let Range { start: true, end } = t..&&false {} +LL + while let Range { start: true, end } = t..false {} + | error[E0308]: mismatched types --> $DIR/disallowed-positions.rs:147:11 diff --git a/src/test/ui/typeck/deref-multi.rs b/src/test/ui/typeck/deref-multi.rs new file mode 100644 index 0000000000000..3dc4771fefb07 --- /dev/null +++ b/src/test/ui/typeck/deref-multi.rs @@ -0,0 +1,26 @@ +fn a(x: &&i32) -> i32 { + x + //~^ ERROR mismatched types +} + +fn a2(x: &&&&&i32) -> i32 { + x + //~^ ERROR mismatched types +} + +fn b(x: &i32) -> i32 { + &x + //~^ ERROR mismatched types +} + +fn c(x: Box) -> i32 { + &x + //~^ ERROR mismatched types +} + +fn d(x: std::sync::Mutex<&i32>) -> i32 { + x.lock().unwrap() + //~^ ERROR mismatched types +} + +fn main() {} diff --git a/src/test/ui/typeck/deref-multi.stderr b/src/test/ui/typeck/deref-multi.stderr new file mode 100644 index 0000000000000..bd6575c73d244 --- /dev/null +++ b/src/test/ui/typeck/deref-multi.stderr @@ -0,0 +1,72 @@ +error[E0308]: mismatched types + --> $DIR/deref-multi.rs:2:5 + | +LL | fn a(x: &&i32) -> i32 { + | --- expected `i32` because of return type +LL | x + | ^ expected `i32`, found `&&i32` + | +help: consider dereferencing the borrow + | +LL | **x + | ++ + +error[E0308]: mismatched types + --> $DIR/deref-multi.rs:7:5 + | +LL | fn a2(x: &&&&&i32) -> i32 { + | --- expected `i32` because of return type +LL | x + | ^ expected `i32`, found `&&&&&i32` + | +help: consider dereferencing the borrow + | +LL | *****x + | +++++ + +error[E0308]: mismatched types + --> $DIR/deref-multi.rs:12:5 + | +LL | fn b(x: &i32) -> i32 { + | --- expected `i32` because of return type +LL | &x + | ^^ expected `i32`, found `&&i32` + | +help: consider removing the `&` and dereferencing the borrow instead + | +LL | *x + | ~ + +error[E0308]: mismatched types + --> $DIR/deref-multi.rs:17:5 + | +LL | fn c(x: Box) -> i32 { + | --- expected `i32` because of return type +LL | &x + | ^^ expected `i32`, found `&Box` + | + = note: expected type `i32` + found reference `&Box` +help: consider removing the `&` and dereferencing the borrow instead + | +LL | *x + | ~ + +error[E0308]: mismatched types + --> $DIR/deref-multi.rs:22:5 + | +LL | fn d(x: std::sync::Mutex<&i32>) -> i32 { + | --- expected `i32` because of return type +LL | x.lock().unwrap() + | ^^^^^^^^^^^^^^^^^ expected `i32`, found struct `MutexGuard` + | + = note: expected type `i32` + found struct `MutexGuard<'_, &i32>` +help: consider dereferencing the type + | +LL | **x.lock().unwrap() + | ++ + +error: aborting due to 5 previous errors + +For more information about this error, try `rustc --explain E0308`.