Simpler diagnostic when passing arg to closure and missing borrow

[Akida31]

fixes #64915

I followed roughly the instructions and the older PR #76362.
The number of references for the expected and the found types will be compared and depending on which has more the diagnostic will be emitted.

I'm not quite sure if my approach with the many span_bug!s is good, it could lead to some ICEs. Would it be better if those errors are ignored?

As far as I know the following code works similarly but in a different context. Is this probably reusable since it looks like it would emit better diagnostics?

rust/compiler/rustc_hir_analysis/src/check/demand.rs

Lines 713 to 1061 in a688a03

	pub fn check_ref(
	&self,
	expr: &hir::Expr<'tcx>,
	checked_ty: Ty<'tcx>,
	expected: Ty<'tcx>,
	) -> Option<(Span, String, String, Applicability, bool /* verbose */)> {
	let sess = self.sess();
	let sp = expr.span;
	// If the span is from an external macro, there's no suggestion we can make.
	if in_external_macro(sess, sp) {
	return None;
	}
	let sm = sess.source_map();
	let replace_prefix = |s: &str, old: &str, new: &str| {
	s.strip_prefix(old).map(|stripped| new.to_string() + stripped)
	};
	// `ExprKind::DropTemps` is semantically irrelevant for these suggestions.
	let expr = expr.peel_drop_temps();
	match (&expr.kind, expected.kind(), checked_ty.kind()) {
	(_, &ty::Ref(_, exp, _), &ty::Ref(_, check, _)) => match (exp.kind(), check.kind()) {
	(&ty::Str, &ty::Array(arr, _) | &ty::Slice(arr)) if arr == self.tcx.types.u8 => {
	if let hir::ExprKind::Lit(_) = expr.kind
	&& let Ok(src) = sm.span_to_snippet(sp)
	&& replace_prefix(&src, "b\"", "\"").is_some()
	{
	let pos = sp.lo() + BytePos(1);
	return Some((
	sp.with_hi(pos),
	"consider removing the leading `b`".to_string(),
	String::new(),
	Applicability::MachineApplicable,
	true,
	));
	}
	}
	(&ty::Array(arr, _) | &ty::Slice(arr), &ty::Str) if arr == self.tcx.types.u8 => {
	if let hir::ExprKind::Lit(_) = expr.kind
	&& let Ok(src) = sm.span_to_snippet(sp)
	&& replace_prefix(&src, "\"", "b\"").is_some()
	{
	return Some((
	sp.shrink_to_lo(),
	"consider adding a leading `b`".to_string(),
	"b".to_string(),
	Applicability::MachineApplicable,
	true,
	));
	}
	}
	_ => {}
	},
	(_, &ty::Ref(_, _, mutability), _) => {
	// Check if it can work when put into a ref. For example:
	//
	// ```
	// fn bar(x: &mut i32) {}
	//
	// let x = 0u32;
	// bar(&x); // error, expected &mut
	// ```
	let ref_ty = match mutability {
	hir::Mutability::Mut => {
	self.tcx.mk_mut_ref(self.tcx.mk_region(ty::ReStatic), checked_ty)
	}
	hir::Mutability::Not => {
	self.tcx.mk_imm_ref(self.tcx.mk_region(ty::ReStatic), checked_ty)
	}
	};
	if self.can_coerce(ref_ty, expected) {
	let mut sugg_sp = sp;
	if let hir::ExprKind::MethodCall(ref segment, receiver, args, _) = expr.kind {
	let clone_trait =
	self.tcx.require_lang_item(LangItem::Clone, Some(segment.ident.span));
	if args.is_empty()
	&& self.typeck_results.borrow().type_dependent_def_id(expr.hir_id).map(
	|did| {
	let ai = self.tcx.associated_item(did);
	ai.trait_container(self.tcx) == Some(clone_trait)
	},
	) == Some(true)
	&& segment.ident.name == sym::clone
	{
	// If this expression had a clone call when suggesting borrowing
	// we want to suggest removing it because it'd now be unnecessary.
	sugg_sp = receiver.span;
	}
	}
	if let Ok(src) = sm.span_to_snippet(sugg_sp) {
	let needs_parens = match expr.kind {
	// parenthesize if needed (Issue #46756)
	hir::ExprKind::Cast(_, _) | hir::ExprKind::Binary(_, _, _) => true,
	// parenthesize borrows of range literals (Issue #54505)
	_ if is_range_literal(expr) => true,
	_ => false,
	};
	if let Some(sugg) = self.can_use_as_ref(expr) {
	return Some((
	sugg.0,
	sugg.1.to_string(),
	sugg.2,
	Applicability::MachineApplicable,
	false,
	));
	}
	let prefix = match self.maybe_get_struct_pattern_shorthand_field(expr) {
	Some(ident) => format!("{ident}: "),
	None => String::new(),
	};
	if let Some(hir::Node::Expr(hir::Expr {
	kind: hir::ExprKind::Assign(..),
	..
	})) = self.tcx.hir().find(self.tcx.hir().get_parent_node(expr.hir_id))
	{
	if mutability == hir::Mutability::Mut {
	// Suppressing this diagnostic, we'll properly print it in `check_expr_assign`
	return None;
	}
	}
	let sugg_expr = if needs_parens { format!("({src})") } else { src };
	return Some(match mutability {
	hir::Mutability::Mut => (
	sp,
	"consider mutably borrowing here".to_string(),
	format!("{prefix}&mut {sugg_expr}"),
	Applicability::MachineApplicable,
	false,
	),
	hir::Mutability::Not => (
	sp,
	"consider borrowing here".to_string(),
	format!("{prefix}&{sugg_expr}"),
	Applicability::MachineApplicable,
	false,
	),
	});
	}
	}
	}
	(
	hir::ExprKind::AddrOf(hir::BorrowKind::Ref, _, ref expr),
	_,
	&ty::Ref(_, checked, _),
	) if self.can_sub(self.param_env, checked, expected).is_ok() => {
	// We have `&T`, check if what was expected was `T`. If so,
	// we may want to suggest removing a `&`.
	if sm.is_imported(expr.span) {
	// Go through the spans from which this span was expanded,
	// and find the one that's pointing inside `sp`.
	//
	// E.g. for `&format!("")`, where we want the span to the
	// `format!()` invocation instead of its expansion.
	if let Some(call_span) =
	iter::successors(Some(expr.span), |s| s.parent_callsite())
	.find(|&s| sp.contains(s))
	&& sm.is_span_accessible(call_span)
	{
	return Some((
	sp.with_hi(call_span.lo()),
	"consider removing the borrow".to_string(),
	String::new(),
	Applicability::MachineApplicable,
	true,
	));
	}
	return None;
	}
	if sp.contains(expr.span)
	&& sm.is_span_accessible(expr.span)
	{
	return Some((
	sp.with_hi(expr.span.lo()),
	"consider removing the borrow".to_string(),
	String::new(),
	Applicability::MachineApplicable,
	true,
	));
	}
	}
	(
	_,
	&ty::RawPtr(TypeAndMut { ty: ty_b, mutbl: mutbl_b }),
	&ty::Ref(_, ty_a, mutbl_a),
	) => {
	if let Some(steps) = self.deref_steps(ty_a, ty_b)
	// Only suggest valid if dereferencing needed.
	&& steps > 0
	// The pointer type implements `Copy` trait so the suggestion is always valid.
	&& let Ok(src) = sm.span_to_snippet(sp)
	{
	let derefs = "*".repeat(steps);
	if let Some((span, src, applicability)) = match mutbl_b {
	hir::Mutability::Mut => {
	let new_prefix = "&mut ".to_owned() + &derefs;
	match mutbl_a {
	hir::Mutability::Mut => {
	replace_prefix(&src, "&mut ", &new_prefix).map(|_| {
	let pos = sp.lo() + BytePos(5);
	let sp = sp.with_lo(pos).with_hi(pos);
	(sp, derefs, Applicability::MachineApplicable)
	})
	}
	hir::Mutability::Not => {
	replace_prefix(&src, "&", &new_prefix).map(|_| {
	let pos = sp.lo() + BytePos(1);
	let sp = sp.with_lo(pos).with_hi(pos);
	(
	sp,
	format!("mut {derefs}"),
	Applicability::Unspecified,
	)
	})
	}
	}
	}
	hir::Mutability::Not => {
	let new_prefix = "&".to_owned() + &derefs;
	match mutbl_a {
	hir::Mutability::Mut => {
	replace_prefix(&src, "&mut ", &new_prefix).map(|_| {
	let lo = sp.lo() + BytePos(1);
	let hi = sp.lo() + BytePos(5);
	let sp = sp.with_lo(lo).with_hi(hi);
	(sp, derefs, Applicability::MachineApplicable)
	})
	}
	hir::Mutability::Not => {
	replace_prefix(&src, "&", &new_prefix).map(|_| {
	let pos = sp.lo() + BytePos(1);
	let sp = sp.with_lo(pos).with_hi(pos);
	(sp, derefs, Applicability::MachineApplicable)
	})
	}
	}
	}
	} {
	return Some((
	span,
	"consider dereferencing".to_string(),
	src,
	applicability,
	true,
	));
	}
	}
	}
	_ if sp == expr.span => {
	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();
	// 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
	prefix_span = prefix_span.with_hi(inner.span.lo());
	expr = inner;
	remove += match mutbl {
	hir::Mutability::Not => "&",
	hir::Mutability::Mut => "&mut ",
	};
	steps -= 1;
	} else {
	break;
	}
	}
	// 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 {
	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<T>`
	if self.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,
	));
	}
	}
	}
	_ => {}
	}
	None
	}

When running the tests locally, a codegen test failed. Is there something I can/ should do about that?

If you have some improvements/ corrections please say so and I will happily include them.

r? @estebank (as you added the mentoring instructions to the issue)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

Please see the contribution instructions for more information.

[estebank]

I'll review in full soon, but I notice that the only case that is unacounted for are generators, otherwise the bug calls seem like reasonable invariant holding.

From a cursory look, I'd like it if we could move this logic to its own function to make skimming the code easier.

[estebank]

Suggested change

	"consider borrowing here:"
	"consider borrowing the argument"

[estebank]

Suggested change

	"consider removing the borrow:"
	"do not borrow the argument"

[bors]

☔ The latest upstream changes (presumably #102896) made this pull request unmergeable. Please resolve the merge conflicts.

[Akida31]

Two questions:

• There is the method hir::Node::fn_decl, should I use that to cover generators? It would allow some other cases which are unexpected and currently bug!s in my implementation.
• The diagnostics of the test in src/test/ui/closures/multiple-fn-bounds.rs regresses with this PR, but this seems to be a constructed case which should not appear in user code. Is it fine (for now or in general) to ignore this regression?

[estebank]

There is the method hir::Node::fn_decl, should I use that to cover generators? It would allow some other cases which are unexpected and currently bug!s in my implementation.

Yeah that 'd be better, because even if that method has a bug for generators, it'll be eventually fixed for all of its callers.

The diagnostics of the test in src/test/ui/closures/multiple-fn-bounds.rs regresses with this PR, but this seems to be a constructed case which should not appear in user code. Is it fine (for now or in general) to ignore this regression?

That suggestion regression is acceptable.

[estebank]

The only thing I'd like changed is to make this a single suggestion for all arguments, but we can land as is.

[estebank]

@bors r+

[bors]

📌 Commit 0f5409b has been approved by estebank

It is now in the queue for this repository.

[Manishearth]

@bors r-

fails CI

#104270 (comment)

[estebank]

Please rebase.

[Akida31]

I added a tidy-ignore for the file-length. I don't know what the best idea would be to split this long file. Additionally I tried to combine all similar suggestions, but I'm not sure if this is the preferred way to do that with the diagnostics. Happy to make improvements :)

[bors]

☔ The latest upstream changes (presumably #104902) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #105644) made this pull request unmergeable. Please resolve the merge conflicts.

[estebank]

Apologies, I'd missed that you'd rebased and re-pushed. r=me after rebasing :)

[estebank]

@bors r+

[bors]

📌 Commit 05bc251 has been approved by estebank

It is now in the queue for this repository.

[bors]

⌛ Testing commit 05bc251 with merge 0f529f0...

[bors]

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 0f529f0 to master...

[rust-timer]

Finished benchmarking commit (0f529f0): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.9%, 1.1%]	2
Regressions ❌ (secondary)	2.0%	[0.9%, 2.6%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[0.9%, 1.1%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	7.5%	[7.5%, 7.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

[lqd]

These benchmarks are currently noisy.
@rustbot label: +perf-regression-triaged