From 8dc36c16470d60fa1bd7bc370c77ac63ef25bfe4 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Sat, 29 Jun 2024 16:49:35 +0800 Subject: [PATCH] fix: prefer `(*p).clone` to `p.clone` if the `p` is a raw pointer --- .../src/diagnostics/conflict_errors.rs | 26 +++++++++++++++--- .../src/diagnostics/move_errors.rs | 27 ++++++++++++++----- .../borrowck-move-from-unsafe-ptr.stderr | 10 ++----- tests/ui/borrowck/issue-20801.stderr | 10 ------- .../move-from-union-field-issue-66500.stderr | 10 +++---- 5 files changed, 50 insertions(+), 33 deletions(-) diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index 1cc7fee718e87..c26bbb926eaa1 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs @@ -1288,7 +1288,7 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, '_, 'tcx> { return false; } // Try to find predicates on *generic params* that would allow copying `ty` - let suggestion = + let mut suggestion = if let Some(symbol) = tcx.hir().maybe_get_struct_pattern_shorthand_field(expr) { format!(": {symbol}.clone()") } else { @@ -1296,6 +1296,8 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, '_, 'tcx> { }; let mut sugg = Vec::with_capacity(2); let mut inner_expr = expr; + let mut is_raw_ptr = false; + let typeck_result = self.infcx.tcx.typeck(self.mir_def_id()); // Remove uses of `&` and `*` when suggesting `.clone()`. while let hir::ExprKind::AddrOf(.., inner) | hir::ExprKind::Unary(hir::UnOp::Deref, inner) = &inner_expr.kind @@ -1306,14 +1308,32 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, '_, 'tcx> { return false; } inner_expr = inner; + if let Some(inner_type) = typeck_result.node_type_opt(inner.hir_id) { + if matches!(inner_type.kind(), ty::RawPtr(..)) { + is_raw_ptr = true; + break; + } + } } - if inner_expr.span.lo() != expr.span.lo() { + // Cloning the raw pointer doesn't make sense in some cases and would cause a type mismatch error. (see #126863) + if inner_expr.span.lo() != expr.span.lo() && !is_raw_ptr { + // Remove "(*" or "(&" sugg.push((expr.span.with_hi(inner_expr.span.lo()), String::new())); } + // Check whether `expr` is surrounded by parentheses or not. let span = if inner_expr.span.hi() != expr.span.hi() { // Account for `(*x)` to suggest `x.clone()`. - expr.span.with_lo(inner_expr.span.hi()) + if is_raw_ptr { + expr.span.shrink_to_hi() + } else { + // Remove the close parenthesis ")" + expr.span.with_lo(inner_expr.span.hi()) + } } else { + if is_raw_ptr { + sugg.push((expr.span.shrink_to_lo(), "(".to_string())); + suggestion = ").clone()".to_string(); + } expr.span.shrink_to_hi() }; sugg.push((span, suggestion)); diff --git a/compiler/rustc_borrowck/src/diagnostics/move_errors.rs b/compiler/rustc_borrowck/src/diagnostics/move_errors.rs index 12fa4c4f5ee05..407b83d497793 100644 --- a/compiler/rustc_borrowck/src/diagnostics/move_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/move_errors.rs @@ -639,12 +639,27 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, '_, 'tcx> { fn add_borrow_suggestions(&self, err: &mut Diag<'_>, span: Span) { match self.infcx.tcx.sess.source_map().span_to_snippet(span) { Ok(snippet) if snippet.starts_with('*') => { - err.span_suggestion_verbose( - span.with_hi(span.lo() + BytePos(1)), - "consider removing the dereference here", - String::new(), - Applicability::MaybeIncorrect, - ); + let sp = span.with_lo(span.lo() + BytePos(1)); + let inner = self.find_expr(sp); + let mut is_raw_ptr = false; + if let Some(inner) = inner { + let typck_result = self.infcx.tcx.typeck(self.mir_def_id()); + if let Some(inner_type) = typck_result.node_type_opt(inner.hir_id) { + if matches!(inner_type.kind(), ty::RawPtr(..)) { + is_raw_ptr = true; + } + } + } + // If the `inner` is a raw pointer, do not suggest removing the "*", see #126863 + // FIXME: need to check whether the assigned object can be a raw pointer, see `tests/ui/borrowck/issue-20801.rs`. + if !is_raw_ptr { + err.span_suggestion_verbose( + span.with_hi(span.lo() + BytePos(1)), + "consider removing the dereference here", + String::new(), + Applicability::MaybeIncorrect, + ); + } } _ => { err.span_suggestion_verbose( diff --git a/tests/ui/borrowck/borrowck-move-from-unsafe-ptr.stderr b/tests/ui/borrowck/borrowck-move-from-unsafe-ptr.stderr index ebc3b6ebcacda..79f18624c618b 100644 --- a/tests/ui/borrowck/borrowck-move-from-unsafe-ptr.stderr +++ b/tests/ui/borrowck/borrowck-move-from-unsafe-ptr.stderr @@ -4,16 +4,10 @@ error[E0507]: cannot move out of `*x` which is behind a raw pointer LL | let y = *x; | ^^ move occurs because `*x` has type `Box`, which does not implement the `Copy` trait | -help: consider removing the dereference here - | -LL - let y = *x; -LL + let y = x; - | help: consider cloning the value if the performance cost is acceptable | -LL - let y = *x; -LL + let y = x.clone(); - | +LL | let y = (*x).clone(); + | + +++++++++ error: aborting due to 1 previous error diff --git a/tests/ui/borrowck/issue-20801.stderr b/tests/ui/borrowck/issue-20801.stderr index 20a4bd4e42378..c1d06ac3e2196 100644 --- a/tests/ui/borrowck/issue-20801.stderr +++ b/tests/ui/borrowck/issue-20801.stderr @@ -67,11 +67,6 @@ LL | struct T(u8); ... LL | let c = unsafe { *mut_ptr() }; | ---------- you could clone this value -help: consider removing the dereference here - | -LL - let c = unsafe { *mut_ptr() }; -LL + let c = unsafe { mut_ptr() }; - | error[E0507]: cannot move out of a raw pointer --> $DIR/issue-20801.rs:36:22 @@ -87,11 +82,6 @@ LL | struct T(u8); ... LL | let d = unsafe { *const_ptr() }; | ------------ you could clone this value -help: consider removing the dereference here - | -LL - let d = unsafe { *const_ptr() }; -LL + let d = unsafe { const_ptr() }; - | error: aborting due to 4 previous errors; 1 warning emitted diff --git a/tests/ui/borrowck/move-from-union-field-issue-66500.stderr b/tests/ui/borrowck/move-from-union-field-issue-66500.stderr index c951ce8e3cd8f..7f4593eefcaf6 100644 --- a/tests/ui/borrowck/move-from-union-field-issue-66500.stderr +++ b/tests/ui/borrowck/move-from-union-field-issue-66500.stderr @@ -30,9 +30,8 @@ LL | *u.c | help: consider cloning the value if the performance cost is acceptable | -LL - *u.c -LL + u.c.clone() - | +LL | (*u.c).clone() + | + +++++++++ error[E0507]: cannot move out of `*u.d` which is behind a raw pointer --> $DIR/move-from-union-field-issue-66500.rs:24:5 @@ -42,9 +41,8 @@ LL | *u.d | help: consider cloning the value if the performance cost is acceptable | -LL - *u.d -LL + u.d.clone() - | +LL | (*u.d).clone() + | + +++++++++ error: aborting due to 4 previous errors