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: Deduplicate errors for incorrect move in loop #53995

Merged
merged 3 commits into from
Sep 19, 2018

Conversation

davidtwco
Copy link
Member

Fixes #53807.

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2018
Copy link
Member Author

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

I'm not sure if this approach to deduplicating errors is the best approach, but it seems to have been relatively effective with only one case I'd consider a regression.

I'm happy to take another approach if this isn't deemed optimal.

@memoryruins memoryruins added the A-NLL Area: Non-lexical lifetimes (NLL) label Sep 6, 2018
@nikomatsakis

This comment has been minimized.

@bors

This comment has been minimized.

@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 Sep 6, 2018
@@ -1,23 +1,3 @@
error[E0382]: use of moved value: `foo`
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Do you have an intuition as to which of the errors are being preferred for presentation by this PR? I am a little surprised to see it favoring the error on line 29 here over the ones on line 28.

I guess I'd want to make sure we don't run into a scenario where a beginner erroneously infers that some statement is not performing a move (or does not have a use of the local, whatever), when in fact there is a move/use at that statement, but the diagnostic system is filtering the error mentioning it out from the error report.

Copy link
Member Author

@davidtwco davidtwco Sep 6, 2018

Choose a reason for hiding this comment

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

This PR groups errors by the MoveOutIndex(s) that are relevant (however this error reporting function was coming to that conclusion previously) and then emitting, for that grouping, the error that corresponds to the place that is most "specific".

In this case, a more "specific" means a Place that is not a prefix of a less "specific" Place. So I just decline to create and buffer an error if the Place for that error is a prefix of the Place that I already have a buffered error for.

There's a log statement that indicates an error is being suppressed and you still get the log statement showing which arguments the reporting function was called with before it suppresses it. There's also a comment attempting to explain the above logic on the field that stores the buffered errors.

Copy link
Contributor

@nikomatsakis nikomatsakis Sep 7, 2018

Choose a reason for hiding this comment

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

Hmm, @pnkfelix has an interesting point — or, at least, I that citing some point in a match arm as a "use" is more confusing than citing the match foo { itself.

Did you experiment with preferring the most general move? e.g., if the common prefix of the grouping is foo, perhaps preferring moves of foo?

That said, I don't think we should dump a ton of errors relating to a single move — I think that is more likely to confuse users. I could imagine trying to include more than one "use after move" point within the error itself.

e.g., we might display

 --> $DIR/issue-17385.rs:28:11
   |
LL |     drop(foo);
   |          --- value moved here
LL |     match foo { //~ ERROR use of moved value
   |           ^^^ value used here after move
LL | X(1) => (),
   |   - value also used here after move
   = note: move occurs because `foo` has type `X`, which does not implement the `Copy` trait

sorry the indentation doesn't line up, hopefully you get the idea

Copy link
Contributor

Choose a reason for hiding this comment

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

or perhaps with a kind of "note" like

 --> $DIR/issue-17385.rs:28:11
   |
LL |     drop(foo);
   |          --- value moved here
LL |     match foo { //~ ERROR use of moved value
   |           ^^^ value used here after move
   = note: move occurs because `foo` has type `X`, which does not implement the `Copy` trait
   = note: value is also used in other places after the move
LL | X(1) => (),
   |   -

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't try the the most general move. When I was digging into the example case from the issue, I thought the best error there was the one with the "used in previous iteration of loop" message, and I noticed it was on the most specific place, which is why I used that as my heuristic.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 8, 2018
Too many errors for incorrect move in loop with NLL enabled

Fixes rust-lang#53807.

r? @nikomatsakis
@bors

This comment has been minimized.

@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 Sep 9, 2018
@kennytm

This comment has been minimized.

@davidtwco

This comment has been minimized.

@kennytm

This comment has been minimized.

@davidtwco

This comment has been minimized.

@davidtwco

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me with comment

src/librustc_mir/borrow_check/mod.rs Outdated Show resolved Hide resolved
@nikomatsakis

This comment has been minimized.

@bors

This comment has been minimized.

@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 Sep 11, 2018
@bors
Copy link
Contributor

bors commented Sep 18, 2018

📌 Commit 88ca341 has been approved by nikomatsakis

@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 Sep 18, 2018
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

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

This comment has been minimized.

@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 Sep 18, 2018
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 18, 2018
@rust-highfive

This comment has been minimized.

@davidtwco
Copy link
Member Author

@bors retry #40474

@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 Sep 18, 2018
@pnkfelix pnkfelix changed the title Too many errors for incorrect move in loop with NLL enabled Deduplicate errors for incorrect move in loop with NLL enabled Sep 18, 2018
@pnkfelix pnkfelix changed the title Deduplicate errors for incorrect move in loop with NLL enabled NLL: Deduplicate errors for incorrect move in loop Sep 18, 2018
@bors
Copy link
Contributor

bors commented Sep 19, 2018

⌛ Testing commit 88ca341 with merge 8f37677...

bors added a commit that referenced this pull request Sep 19, 2018
NLL: Deduplicate errors for incorrect move in loop

Fixes #53807.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Sep 19, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 8f37677 to master...

@bors bors merged commit 88ca341 into rust-lang:master Sep 19, 2018
@davidtwco davidtwco deleted the issue-53807 branch September 19, 2018 09:22
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.

7 participants