-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Erroneous loop diagnostic in nll #56113
Erroneous loop diagnostic in nll #56113
Conversation
@nikomatsakis we need to discuss the changed output of the 2 existing tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I think is why the tests fail
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
97f0188
to
d8f9775
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
d8f9775
to
15b775e
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ping fromt triage @spastorino: It looks like your PR failed on travis. |
@TimNN yeah, we are currently discussing this on Zulip. Consider this PR a WIP. Adding WIP to the title to state this clearly. |
15b775e
to
1062697
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1062697
to
065cced
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
065cced
to
08f6d0b
Compare
block: *bb, | ||
}) | ||
.filter(|s| visited_locations.insert(*s)) | ||
.filter(|s| !self.is_back_edge(location, *s) || to == *s), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this miss this case:
loop {
loop {
if cond { break };
use(v);
}
borrow(&mut v);
}
It is only possible to reach the borrow
from the use
by going through the back-edge in the inner loop
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that a usable definition of "a cfg node N is inside the loop whose header is the cfg node L" is:
- L dominates N
- there is a path from N to L that goes only through nodes that L dominates.
Because:
L
dominates all the nodes within the loop, so that ifN
is within the loop and has a path within the loop that goes back toL
, the condition will be true.- If
N
is outside of the loop but dominated byL
, then going back toL
will require going through an outer loop header, which will dominateN
and therefore not be dominated by it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another class of "confusible" cases:
loop {
loop {
if cond { break; }
borrow(&v);
}
use(&mut v);
}
is the same MIR as
loop {
if cond {
use(&mut v);
} else {
borrow(&v);
}
}
but the error message should be different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the goal was to "first do no harm", right? i.e., stop printing "previous iteration of loop" at wrong spots, but not necessarily always print in all the "right spots"? In that case:
- First example does not print "prior iteration of loop", and indeed it might be nice if it did, but that seems ok.
- Same for the second example
If we do merge the PR, these are good examples to put on the "follow-up" issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, given the second example, I think that any CFG-based model will be fairly inaccurate, so I'm ok with this
☔ The latest upstream changes (presumably #56460) made this pull request unmergeable. Please resolve the merge conflicts. |
7931e12
to
461ec7a
Compare
8a1ae3c
to
6b6a0c5
Compare
@Dylan-DPC I don't think so. Niko even did an r+. The problem with this issue is that tests are failing on |
fe2f1f8
to
06ec1a4
Compare
@nikomatsakis let me know if with the last commit added the rest of the cases also look good to you. Left the changes in a different commit so you can more easily review the non approved changes. When you think it's ok I can squash this last commit in the right place and get rid of this noisy last one :). |
@@ -5,7 +5,7 @@ LL | for &x in &vector { | |||
| ------- | |||
| | | |||
| immutable borrow occurs here | |||
| immutable borrow used here, in later iteration of loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikomatsakis this change is for the worse, I don't remember you saying that with this approach some things were going to be worse but I couldn't find the discussion about it and if this is one of those cases or it's just an error in the code :).
@@ -17,7 +17,7 @@ LL | for &x in &vector { | |||
| ------- | |||
| | | |||
| immutable borrow occurs here | |||
| immutable borrow used here, in later iteration of loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here.
@@ -13,7 +13,7 @@ error[E0499]: cannot borrow `x` as mutable more than once at a time | |||
--> $DIR/borrowck-mut-borrow-linear-errors.rs:15:30 | |||
| | |||
LL | 1 => { addr.push(&mut x); } //[ast]~ ERROR [E0499] | |||
| ---- first borrow used here, in later iteration of loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two changes in this files also happened in AST and we already approved that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see an analogous message in
https://github.com/rust-lang/rust/blob/06ec1a422a5ff147c3a09af7eb80db77459e797b/src/test/ui/borrowck/borrowck-mut-borrow-linear-errors.ast.stderr#L4-L20
Were you referring to a different file when you said "also happened in AST" ? Or do you mean that the above diagnostic is analogous to what NLL is being changed to produced above?
(To be clear, I think AST is currently producing a third totally different diagnostic message right now for this case...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(note in particular that the AST-borrowck error message is highlighting a use of x
in the second match arm, while NLL is highlighting a use of addr
in the first match arm.)
I'm not saying I object to the change. I'm just trying to understand the comment you have written here on github about it.
@@ -5,7 +5,7 @@ LL | let v: Vec<&str> = line.split_whitespace().collect(); | |||
| ^^^^ borrowed value does not live long enough | |||
... | |||
LL | acc += cnt2; | |||
| --- borrow used here, in later iteration of loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway the error is still ungreat :)
☔ The latest upstream changes (presumably #58316) made this pull request unmergeable. Please resolve the merge conflicts. |
I think the blessed changes from 06ec1a4 are fine. Of course we now need to rebase the PR. r=me under assumption that the rebase is trivial. |
Hey @spastorino do you think you'll have a chance to rebase this in the near future? |
@pnkfelix yes, I can rebase it now. I was wondering if the changes are considered good enough or better than what we currently have because there are things that are "improvable" or work for future PRs. |
06ec1a4
to
02117c3
Compare
@pnkfelix rebased, let's see if CI is still ok :) |
This commit fixes the logic of detecting when a use happen in a later iteration of where a borrow was defined Fixes rust-lang#53773
02117c3
to
a12982c
Compare
@bors r+ |
📌 Commit a12982c has been approved by |
…=pnkfelix Erroneous loop diagnostic in nll Closes #53773 r? @nikomatsakis
☀️ Test successful - checks-travis, status-appveyor |
☀️ Test successful - checks-travis, status-appveyor |
Closes #53773
r? @nikomatsakis