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] Improve "borrow later used here" messages #54666

Merged
merged 2 commits into from
Oct 4, 2018

Conversation

matthewjasper
Copy link
Contributor

  • In the case of two conflicting borrows, the later used message says which borrow it's referring to
  • If the later use is a function call (from the users point of view) say that the later use is for the call. Point just to the function.

r? @pnkfelix
Closes #48643

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2018
@memoryruins memoryruins added the A-NLL Area: Non-lexical lifetimes (NLL) label Oct 1, 2018
@estebank
Copy link
Contributor

estebank commented Oct 2, 2018

The "borrow later used by call" messages look much better now!

@@ -298,9 +300,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
"immutable",
"mutable",
) {
(BorrowKind::Shared, lft, _, BorrowKind::Mut { .. }, _, rgt)
| (BorrowKind::Mut { .. }, _, lft, BorrowKind::Shared, rgt, _) => tcx
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. If my memory is right, the reason this was structured as a match on a tuple was to allow collapsing multiple cases like these and selecting the right string to use by setting lft and rgt to the appropriate string literal from the match input.

Reading over the code now, I am thinking that there is potentially no longer any instance of two arms being collapsed, which means we really should consider cleaning up the code by getting rid of those string literals in the tuple (and instead just pass the appropriate strings below), I think...

That cleanup doesn't have to be part of this PR, though.

@@ -68,7 +84,7 @@ impl<'tcx> BorrowExplanation<'tcx> {
BorrowExplanation::MustBeValidFor(region) => {
tcx.note_and_explain_free_region(
err,
"borrowed value must be valid for ",
&(borrow_desc + "borrowed value must be valid for "),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoa, I barely ever see string concatenation via + in Rust code I read...

@pnkfelix
Copy link
Member

pnkfelix commented Oct 2, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Oct 2, 2018

📌 Commit d18177c34b048feb00814bee5bdbf8d53e665aab has been approved by pnkfelix

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2018
@bors
Copy link
Contributor

bors commented Oct 2, 2018

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

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 2, 2018
@estebank
Copy link
Contributor

estebank commented Oct 2, 2018

Once this gets rebased it should be given a high priority to avoid more merge conflicts.

Give a special message when the later use is from a call. Use the span
of the callee instead of the whole expression. For conflicting borrow
messages say that the later use is of the first borrow.
@matthewjasper
Copy link
Contributor Author

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Oct 3, 2018

📌 Commit bc4f9b8 has been approved by pnkfelix

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2018
@bors
Copy link
Contributor

bors commented Oct 4, 2018

⌛ Testing commit bc4f9b8 with merge a57f1c9...

bors added a commit that referenced this pull request Oct 4, 2018
[NLL] Improve "borrow later used here" messages

* In the case of two conflicting borrows, the later used message says which borrow it's referring to
* If the later use is a function call (from the users point of view) say that the later use is for the call. Point just to the function.

r? @pnkfelix
Closes #48643
@bors
Copy link
Contributor

bors commented Oct 4, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing a57f1c9 to master...

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) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants