Unhelpful compiler suggestion when trying to dereference a pointer that is not Copy

[cyrgani]

Code

fn main() {
    let val = String::from("test");
    let p = std::ptr::addr_of!(val);
    unsafe {
        let inner: String = *p;
        dbg!(inner);
        std::mem::forget(val);
    }
}

Current output

error[E0507]: cannot move out of `*p` which is behind a raw pointer
 --> src/main.rs:5:29
  |
5 |         let inner: String = *p;
  |                             ^^ move occurs because `*p` has type `String`, which does not implement the `Copy` trait
  |
help: consider removing the dereference here
  |
5 -         let inner: String = *p;
5 +         let inner: String = p;
  |
help: consider cloning the value if the performance cost is acceptable
  |
5 -         let inner: String = *p;
5 +         let inner: String = p.clone();
  |

For more information about this error, try `rustc --explain E0507`.

Desired output

error[E0507]: cannot move out of `*p` which is behind a raw pointer
 --> src/main.rs:5:29
  |
5 |         let inner: String = *p;
  |                             ^^ move occurs because `*p` has type `String`, which does not implement the `Copy` trait
  |
help: consider using `ptr::read` to get the value the pointer refers to
  |
5 -         let inner: String = *p;
5 +         let inner: String = p.read();  
  |

For more information about this error, try `rustc --explain E0507`.

Rationale and extra context

Cloning the pointer doesn't make sense in that situation and would cause a type mismatch error. I'm not sure whether any of the two current suggestions should be kept or removed in this case.

Other cases

No response

Rust Version

rustc 1.79.0 (129f3b996 2024-06-10)
binary: rustc
commit-hash: 129f3b9964af4d4a709d1383930ade12dfe7c081
commit-date: 2024-06-10
host: x86_64-unknown-linux-gnu
release: 1.79.0
LLVM version: 18.1.7

Anything else?

No response

[workingjubilee]

Note that this works:

        let inner: String = (*p).clone();

It works because (*p) is a place expression and then we attach .clone() to that, which then reborrows the String.

I don't think we should suggest .read(), which may have undesirable or unexpected effects in the case of things like types that handle backing buffers, instead of issuing a safe .clone() which will recreate the backing allocation correctly.

[cyrgani]

I didn't know that was possible, thanks for letting me know. I guess that the best approach would then be to change the .clone() suggestion accordingly as below?

help: consider cloning the value if the performance cost is acceptable
  |
5 -         let inner: String = *p;
5 +         let inner: String = (*p).clone();
  |

[workingjubilee]

Correct.

Also, we could mention read in the error for pointers, but we shouldn't recommend it. It's unsafe code, we should strive to preserve the "you break it, you buy it" / "do your own research" experience for that.

[workingjubilee]

The relevant section of diagnostic code is here

rust/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

Lines 1299 to 1327 in fc555cd

	// Remove uses of `&` and `*` when suggesting `.clone()`.
	while let hir::ExprKind::AddrOf(.., inner) | hir::ExprKind::Unary(hir::UnOp::Deref, inner) =
	&inner_expr.kind
	{
	if let hir::ExprKind::AddrOf(_, hir::Mutability::Mut, _) = inner_expr.kind {
	// We assume that `&mut` refs are desired for their side-effects, so cloning the
	// value wouldn't do what the user wanted.
	return false;
	}
	inner_expr = inner;
	}
	if inner_expr.span.lo() != expr.span.lo() {
	sugg.push((expr.span.with_hi(inner_expr.span.lo()), String::new()));
	}
	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())
	} else {
	expr.span.shrink_to_hi()
	};
	sugg.push((span, suggestion));
	let msg = if let ty::Adt(def, _) = ty.kind()
	&& [tcx.get_diagnostic_item(sym::Arc), tcx.get_diagnostic_item(sym::Rc)]
	.contains(&Some(def.did()))
	{
	"clone the value to increment its reference count"
	} else {
	"consider cloning the value if the performance cost is acceptable"
	};

but it will require some restructuring to account for the fact that the ty::RawPtr suggestion will not be doing things like stripping parentheses, but suggesting adding them if they don't exist.

[linyihai]

@rustbot claim
It looks good for me to make a try.