Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prefer (*p).clone to p.clone if the p is a raw pointer #127114

Merged
merged 1 commit into from
Jun 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1288,14 +1288,16 @@ 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 {
".clone()".to_owned()
};
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
Expand All @@ -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));
Expand Down
27 changes: 21 additions & 6 deletions compiler/rustc_borrowck/src/diagnostics/move_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
10 changes: 2 additions & 8 deletions tests/ui/borrowck/borrowck-move-from-unsafe-ptr.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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<isize>`, 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

Expand Down
10 changes: 0 additions & 10 deletions tests/ui/borrowck/issue-20801.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down
10 changes: 4 additions & 6 deletions tests/ui/borrowck/move-from-union-field-issue-66500.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down
Loading