Report when borrow could cause &mut aliasing during Drop
#54310
Conversation
…at needs exclusive access.
In particular:
1. Extend `WriteKind::StorageDeadOrDrop` with state to track whether
we are running a destructor or just freeing backing storage. (As
part of this, when we drop a Box<..<Box<T>..> where `T` does not
need drop, we now signal that the drop of `T` is a kind of storage
dead rather than a drop.)
2. When reporting that a value does not live long enough, check if
we're doing an "interesting" drop, i.e. we aren't just trivally
freeing the borrowed state, but rather a user-defined dtor will
run and potentially require exclusive aces to the borrowed state.
3. Added a new diagnosic to describe the scenario here.
It is worth pointing out that the reason that so few diagnostics are effected is because of the filter I put in where it only goes down the new path if the borrowed place is *not* a prefix of the dropped place. (Without that filter, a *lot* of the tests would need this change, and it would probably be a net loss for the UX, since you'd see it even in cases like borrows of generic types where there is no explicit mention of `Drop`.)
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| ... | ||
| LL | } | ||
| | - temporary value only lives until here | ||
| | - drop of temporary value occurs here |
There was a problem hiding this comment.
hmm I have to admit that this delta (namely losing the printout of the match's input expression) seems unfortunate. I should look into incorporating that into the diagnostic output in cases like this.
There was a problem hiding this comment.
Musing on this, I was thinking that it might be a good idea for us to explain why the value is being dropped here. One way I could imagine doing that is:
- First, find the thing that was borrowed. If it is a user-local, then add a note with that span and say something like:
= note: local variables are dropped when they go out of scope
let x;
- `x` declared here
}
- `x` goes out of scope here
note sure if we can do notes with multispans like that (cc @estebank) but it seems ideal. Alternatively, we could merge this into the main error.
There was a problem hiding this comment.
For temporaries, we would probably give a slightly different error, along the lines of what @mikhail-m1 did in #54164
There was a problem hiding this comment.
@nikomatsakis not supported yet, but something I've had in my backlog for a while. Should I prioritize it?
There was a problem hiding this comment.
@estebank I personally don't think you need to re-prioritize. We can work around its absence, as @nikomatsakis notes...
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
okay this time i'm going to actually test my example before pushing to the branch |
This comment has been minimized.
This comment has been minimized.
|
ok well I think this is a solid step forward. r=me but I would like to do a kind of overview of all of our error messages at some point to think carefully about how to structure them. |
|
And now I'm going so far as to run the error index tests locally too Sharing how to do it for reference with my future self and anyone else who might care: |
error[E0106]: missing lifetime specifier
--> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:11424:23
|
9 | fn demo(s: &mut S) -> &mut String { let p = &mut *(*s).data; p }
| ^ expected lifetime parameter
|
= help: this function's return type contains a borrowed value, but the signature does not say which one of `s`'s 2 lifetimes it is borrowed from
pnkfelix
force-pushed
the
issue-52059-report-borrow-drop-conflict
branch
from
12ac520 to
c9cf499
Compare
|
@bors r=nikomatsakis |
|
📌 Commit c9cf499 has been approved by |
bors
added
S-waiting-on-bors
|
(If this and some of the other NLL diagnostics PRs are still in the queue on Monday, I might go roll them up manually because I know certain ones are going to collide...) |
…ct, r=nikomatsakis Report when borrow could cause `&mut` aliasing during Drop We were already issuing an error for the cases where this cropped up, so this is not fixing any soundness holes. The previous diagnostic just wasn't accurately describing the problem in the user's code. Fix #52059
|
☀️ Test successful - status-appveyor, status-travis |
We were already issuing an error for the cases where this cropped up, so this is not fixing any soundness holes. The previous diagnostic just wasn't accurately describing the problem in the user's code.
Fix #52059