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

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

Closed
pnkfelix opened this issue Oct 24, 2018 · 3 comments
Closed

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

pnkfelix opened this issue Oct 24, 2018 · 3 comments
Labels
A-NLL Area: Non-lexical lifetimes (NLL) E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. NLL-diagnostics Working towards the "diagnostic parity" goal P-medium Medium priority

Comments

@pnkfelix
Copy link
Member

This diagnostic output

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 pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal labels Oct 24, 2018
@pnkfelix
Copy link
Member Author

pnkfelix commented Nov 6, 2018

triage: P-medium

@pnkfelix pnkfelix added P-medium Medium priority E-needs-mentor labels Nov 6, 2018
@davidtwco
Copy link
Member

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:

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:

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:

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:

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.

@davidtwco davidtwco added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed E-needs-mentor labels Nov 25, 2018
@wildarch
Copy link
Contributor

I'll have a go at this 😄

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Dec 1, 2018
…ref, r=davidtwco

Refer to the second borrow as the "second borrow" in E0501.rs

Fixes rust-lang#55314.

r? @davidtwco
Centril added a commit to Centril/rust that referenced this issue Dec 1, 2018
…ref, r=davidtwco

Refer to the second borrow as the "second borrow" in E0501.rs

Fixes rust-lang#55314.

r? @davidtwco
Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018
…ref, r=davidtwco

Refer to the second borrow as the "second borrow" in E0501.rs

Fixes rust-lang#55314.

r? @davidtwco
kennytm added a commit to kennytm/rust that referenced this issue Dec 4, 2018
…ref, r=davidtwco

Refer to the second borrow as the "second borrow" in E0501.rs

Fixes rust-lang#55314.

r? @davidtwco
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Dec 4, 2018
…ref, r=davidtwco

Refer to the second borrow as the "second borrow" in E0501.rs

Fixes rust-lang#55314.

r? @davidtwco
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Dec 5, 2018
…ref, r=davidtwco

Refer to the second borrow as the "second borrow" in E0501.rs

Fixes rust-lang#55314.

r? @davidtwco
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 5, 2018
…ref, r=davidtwco

Refer to the second borrow as the "second borrow" in E0501.rs

Fixes rust-lang#55314.

r? @davidtwco
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 5, 2018
…ref, r=davidtwco

Refer to the second borrow as the "second borrow" in E0501.rs

Fixes rust-lang#55314.

r? @davidtwco
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. NLL-diagnostics Working towards the "diagnostic parity" goal P-medium Medium priority
Projects
None yet
Development

No branches or pull requests

3 participants