Drop Tracking: Implement fake_read callback#96923
Conversation
|
Some changes occurred in src/tools/clippy. cc @rust-lang/clippy |
rustbot
added
the
T-compiler
rust-highfive
added
the
S-waiting-on-review
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
Tweaked wording of one comment, but I'm happy
|
|
||
| // fake reads happen in places like the scrutinee of a match expression, so we can treat | ||
| // these as a borrow. | ||
| self.borrow_place(place_with_id); |
There was a problem hiding this comment.
hmm, I'm surprised this would borrow the place and not just "read" it. I guess I have to take a look at where this is invoked.
There was a problem hiding this comment.
I think a borrow is actually ok, just like with copy -- i.e., we are kind of "transiently borrowing" this to read its value -- the only thing is that we don't necessarily want to consider it a borrowed temporary -- but I am don't see it does any harm if we do so (we might over-approximate a bit).
There was a problem hiding this comment.
I like your updated comment, and thanks for the review!
We don't really have a concept of read here, we just have borrows, so I think treating a read as a borrow makes sense. You borrow it so that you can read from it.
compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs
Outdated
Show resolved
Hide resolved
…/record_consumed_borrow.rs
|
@bors r+ |
|
📌 Commit bf21a81 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (3b64fe9): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
Drop Tracking: Implement `fake_read` callback This PR updates drop tracking's use of `ExprUseVisitor` so that we treat `fake_read` events as borrows. Without doing this, we were not handling match expressions correctly, which showed up as a breakage in the `addassign-yield.rs` test. We did not previously notice this because we still had rather large temporary scopes that we held borrows for, which changed in rust-lang#94309. This PR also includes a variant of the `addassign-yield.rs` test case to make sure we continue to have correct behavior here with drop tracking. r? `@nikomatsakis`
6 participants
This PR updates drop tracking's use of
ExprUseVisitorso that we treatfake_readevents as borrows. Without doing this, we were not handling match expressions correctly, which showed up as a breakage in theaddassign-yield.rstest. We did not previously notice this because we still had rather large temporary scopes that we held borrows for, which changed in #94309.This PR also includes a variant of the
addassign-yield.rstest case to make sure we continue to have correct behavior here with drop tracking.r? @nikomatsakis