nll: strange suggestion to "consider removing the *

[pnkfelix]

Spawned off of #54825 (comment)

For this code:

rust/src/test/ui/borrowck/borrowck-move-from-unsafe-ptr.rs

Lines 12 to 15 in fe8ace8

	unsafe fn foo(x: *const Box<isize>) -> Box<isize> {
	let y = *x; //~ ERROR cannot move out of dereference of raw pointer
	return y;
	}

which produces this diagnostic output:

rust/src/test/ui/borrowck/borrowck-move-from-unsafe-ptr.nll.stderr

Lines 4 to 8 in fe8ace8

	LL | let y = *x; //~ ERROR cannot move out of dereference of raw pointer
	| ^^
	| |
	| cannot move out of dereference of raw pointer
	| help: consider removing the `*`: `x`

@nikomatsakis made this comment about the HELP on line 8:

Pre-existing but: it is...strange that we make this suggestion. It seems almost certainly wrong. It doesn't preserve the type or "intent" of the code in any particular way...? It's also distinct from what the AST checker says, which is to suggest &*x (though I would argue that is also wrong, for similar reasons). It feels like the best suggestion might be ptr::read but I'm sort of inclined to just not suggest anything at all.

[estebank]

rust/src/librustc_mir/borrow_check/move_errors.rs

Lines 353 to 406 in b8b4150

	fn add_move_hints(
	&self,
	error: GroupedMoveError<'tcx>,
	err: &mut DiagnosticBuilder<'a>,
	span: Span,
	) {
	let snippet = self.infcx.tcx.sess.source_map().span_to_snippet(span).unwrap();
	match error {
	GroupedMoveError::MovesFromPlace {
	mut binds_to,
	move_from,
	..
	} => {
	let try_remove_deref = match move_from {
	Place::Projection(box PlaceProjection {
	elem: ProjectionElem::Deref,
	..
	}) => true,
	_ => false,
	};
	if try_remove_deref && snippet.starts_with('*') {
	// The snippet doesn't start with `*` in (e.g.) index
	// expressions `a[b]`, which roughly desugar to
	// `*Index::index(&a, b)` or
	// `*IndexMut::index_mut(&mut a, b)`.
	err.span_suggestion_with_applicability(
	span,
	"consider removing the `*`",
	snippet[1..].to_owned(),
	Applicability::Unspecified,
	);
	} else {
	err.span_suggestion_with_applicability(
	span,
	"consider borrowing here",
	format!("&{}", snippet),
	Applicability::Unspecified,
	);
	}
	binds_to.sort();
	binds_to.dedup();
	self.add_move_error_details(err, &binds_to);
	}
	GroupedMoveError::MovesFromValue { mut binds_to, .. } => {
	binds_to.sort();
	binds_to.dedup();
	self.add_move_error_suggestions(err, &binds_to);
	self.add_move_error_details(err, &binds_to);
	}
	// No binding. Nothing to suggest.
	GroupedMoveError::OtherIllegalMove { .. } => (),
	}
	}

[pnkfelix]

How bad might it be to just remove this suggestion entirely (vs attempting to fine tune when we emit it)? Can we easily weigh the good it brings vs the harm it causes?

[nikomatsakis]

I'd prefer to just remove it entirely. I think it adds pretty little value.

[nikomatsakis]

I could imagine using an extended error code to offer more detailed hints

[nikomatsakis]

or else a reference to something online?

[pnkfelix]

marking as NLL-deferred as this should not block RC2 (or even the Release, IMO).

[pnkfelix]

NLL triage. Tagging as P-medium priority.

[oli-obk]

Diagnostics triage: marking as E-easy since it's just removing the suggestion

[awaitlink]

Shouldn't this be closed because #61332 is now merged?