NLL: ui/E0501.rs refers to "first borrow" but not "second borrow"

[pnkfelix]

This diagnostic output

rust/src/test/ui/E0501.ast.nll.stderr

Lines 1 to 13 in f99911a

	error[E0501]: cannot borrow `*a` as mutable because previous closure requires unique access
	--> $DIR/E0501.rs:28:23
	|
	LL | let bar = || {
	| -- closure construction occurs here
	LL | inside_closure(a)
	| - first borrow occurs due to use of `a` in closure
	LL | };
	LL | outside_closure_1(a); //[ast]~ ERROR cannot borrow `*a` as mutable because previous closure requires unique access
	| ^ borrow occurs here
	...
	LL | drop(bar);
	| --- first borrow later used here

is not a regression, per se. But we put in effort to identify something as the "first borrow" (rather than "previous borrow"); that means we should similarly identify the other borrow as the "second borrow".

[pnkfelix]

triage: P-medium

[davidtwco]

Here are some mentoring instructions to get started on this issue. If you're reading this and want to give this issue a go, drop a comment here and feel free to ask for any help or clarification on Zulip.

If this is your first contribution then there are instructions on getting a local build of the compiler available in the rustc guide.

---

These pointers are up-to-date as of master being 451987d86c89b38ddd8c4c124f1b9b6d4ded6983, though I wouldn't expect it to go out-of-date too quickly.

So, the function that handles reporting errors like this is report_conflicting_borrow:

rust/src/librustc_mir/borrow_check/error_reporting.rs

Lines 323 to 329 in abe19a7

	pub(super) fn report_conflicting_borrow(
	&mut self,
	context: Context,
	(place, span): (&Place<'tcx>, Span),
	gen_borrow_kind: BorrowKind,
	issued_borrow: &BorrowData<'tcx>,
	) {

Normally you can find functions like this by grepping for the messages you see in the error. If we do that, we can grep for E0501 which leads us to the cannot_reborrow_already_uniquely_borrowed function:

rust/src/librustc_mir/util/borrowck_errors.rs

Lines 266 to 285 in 451987d

	let mut err = struct_span_err!(
	self,
	new_loan_span,
	E0501,
	"cannot borrow `{}`{} as {} because previous closure \
	requires unique access{OGN}",
	desc_new,
	opt_via,
	kind_new,
	OGN = o
	);
	err.span_label(new_loan_span, format!("borrow occurs here{}", opt_via));
	err.span_label(
	old_loan_span,
	format!("{} construction occurs here{}", container_name, old_opt_via),
	);
	if let Some(previous_end_span) = previous_end_span {
	err.span_label(previous_end_span, "borrow from closure ends here");
	}
	self.cancel_if_wrong_origin(err, o)

In this function, we can see a error being created and a label being added saying "borrow occurs here" - that must be where we want to prefix "second".

We can see that function is being called from the first function I linked:

rust/src/librustc_mir/borrow_check/error_reporting.rs

Lines 448 to 458 in 451987d

	tcx.cannot_reborrow_already_uniquely_borrowed(
	span,
	container_name,
	&desc_place,
	"",
	lft,
	issued_span,
	"",
	None,
	Origin::Mir,
	)

We want to be careful though and make sure we only add "second" when there's something labelled "first".

Just above the call site of that function, we see a first_borrow_desc variable being set to "first ". If we follow that variable, we see it being passed to the add_explanation_to_diagnostic function:

rust/src/librustc_mir/borrow_check/error_reporting.rs

Lines 516 to 517 in 451987d

	self.explain_why_borrow_contains_point(context, issued_borrow, None)
	.add_explanation_to_diagnostic(self.infcx.tcx, self.mir, &mut err, first_borrow_desc);

If we look at the source for that function, then it looks like it adds more labels to our error and uses that prefix - however, it doesn't add any labels like that when it doesn't have an explanation (that is, the return value of explain_why_borrow_contains_point is BorrowExplanation::Unexplained).

So, what we'll want to do to fix this issue is move the computation of the borrow explanation (the call to explain_why_borrow_contains_point) above where we match to work out what type of error to throw (we can probably keep adding the explanation to diagnostic where it is). Then, we can add a new argument to cannot_reborrow_already_uniquely_borrowed with a prefix. That prefix should be "second " or an empty &str depending on whether we have a borrow explanation - we should add a function to BorrowExplanation called is_explained that works this out.

---

If you've got this working then you should run the tests and update any that change correctly and then open a PR with a line saying r? @davidtwco in the description so I'll be assigned to review it.

[wildarch]

I'll have a go at this 😄