[NLL] Rework checking for borrows conflicting with drops #54509
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
matthewjasper
force-pushed
the
better-drop-access
branch
from
2c40191 to
8226f41
Compare
This comment has been minimized.
This comment has been minimized.
When dropping a self-borrowing struct we shouldn't add a "values in a scope are dropped in the opposite order they are defined" message, since there is only one value being dropped.
Previously, we would split the drop access into multiple checks for each field of a struct/tuple/closure and through `Box` dereferences. This changes this to check if the borrow is accessed by the drop in places_conflict. This also allows us to handle enums in a simpler way, since we don't have to construct any new places.
matthewjasper
force-pushed
the
better-drop-access
branch
from
8226f41 to
cfbd1a9
Compare
| bug!("Tracking borrow behind shared reference."); | ||
| } | ||
| (ProjectionElem::Deref, ty::Ref(_, _, hir::MutMutable), AccessDepth::Drop) => { | ||
| // Values behind a mutatble reference are not access either by Dropping a |
There was a problem hiding this comment.
nit: typo in "mutatble" here. (And also, "are not accessed" would be more grammatically correct, I think)
| // Drop can read/write arbitrary projections, so places | ||
| // conflict regardless of further projections. | ||
| if def.has_dtor(tcx) { | ||
| return true; |
There was a problem hiding this comment.
I am a little bit wary of returning true early here, but I have failed to put my finger on exactly why that is so.
If we were tracking even the projections that go through &-references, then returning true here would flag false conflicts.
But since you have already changed that case above to fire off a bug! in that scenario, and have also added the new case above it to eagerly return false for a Drop of a &mut-reference, ... I guess its fine; I just wish the coupling of the different pieces of logic here was more explicit.
| fn drop(&mut self) {} | ||
| } | ||
|
|
||
| // Dropping opt could access the value behind the reference, |
There was a problem hiding this comment.
(GIven that the type T has no trait bounds and the type carries no closures of type Fn(T), I think this comment about whether the drop could access the value is only true if the code were makes use of specialization. Or at least, we used to have some complicated arguments about what Drop on a parametric type was actually capable of.)
There was a problem hiding this comment.
But that's just me being pedantic. I have no problem with you leaving this code and comment as it stands.
|
@bors r+ |
|
📌 Commit cfbd1a9 has been approved by |
bors
added
S-waiting-on-bors
|
(I left some nits, but none of my critiques are worth blocking this PR. If there's a bors conflict and you happen to get a chance to fix them, then that would be great. Or if I follow through on doing a rollup of the NLL PR's, maybe I'll adjust the typos there...) |
[NLL] Rework checking for borrows conflicting with drops Previously, we would split the drop access into multiple checks for each field of a struct/tuple/closure and through `Box` dereferences. This changes this to check if the borrow is accessed by the drop in `places_conflict`. We also now handle enums containing `Drop` types. Closes #53569 r? @nikomatsakis cc @pnkfelix
|
☀️ Test successful - status-appveyor, status-travis |
|
This was a small perf win for NLL builds. |
Previously, we would split the drop access into multiple checks for each
field of a struct/tuple/closure and through
Boxdereferences. Thischanges this to check if the borrow is accessed by the drop in
places_conflict.We also now handle enums containing
Droptypes.Closes #53569
r? @nikomatsakis
cc @pnkfelix