ergonomic_clones_dotuse_capture_by_ref: Capture upvar by ref for .use in non-move closures#157270
ergonomic_clones_dotuse_capture_by_ref: Capture upvar by ref for .use in non-move closures#157270Dnreikronos wants to merge 2 commits into
.use in non-move closures#157270Conversation
rustbot
added
S-waiting-on-review
|
r? @folkertdev rustbot has assigned @folkertdev. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? types |
rustbot
added
the
T-types
|
hi, @jackh726! |
|
The first commit looks completely unrelated? Can you split that into a separate PR? The |
A plain `|| { x.use }` closure marked the upvar capture as `ByUse`,
which clones the value into the closure at construction time. The
`.use` in the body then clones again on every call, so the value was
cloned once more than the number of invocations.
A `ByUse` capture in a non-`move`/`use` closure can only originate from
a `.use` expression in the body; a `use ||` capture clause is handled by
`adjust_for_use_closure` instead. Capture such places by immutable
borrow so the `.use` clones once per evaluation and nothing is cloned
into the closure at construction.
1b52868 to
0806c0a
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
thanks, yeah fair. i rebased on current main and the unrelated method-suggestion commit dropped out. idk how i missed that in the stack before. imo this looks cleaner now: the pr should only have the .use fix and the ui test. lgtm from my side. |
|
hi @jackh726! |
|
@rustbot reroll |
|
@nikomatsakis I'm reading rust-lang/rfcs#3968 but I'm not clear on whether the Anyway, I'm not qualified to review this, so |
6 participants
Fixes #157141
A plain
|| { x.use }closure was cloning the captured value one time too many: once when the closure is built, then once per call. So two invocations produced three clones.The capture analysis marked the upvar as
ByUse, which clones the value into the closure at construction time, and the.usein the body still clones on every call. But for a closure without amoveorusekeyword, aByUsecapture can only come from a.useexpression in the body. Ause ||capture clause is handled separately inadjust_for_use_closure. Theast::CaptureBy::Usedocs already spell this out: a regular|| x.useshould not produce aUsecapture, it should look at the type and treatx.useas a copy/clone/move as appropriate.So in
adjust_for_non_move_closure, capture such places by immutable borrow instead. The.usethen clones once per evaluation and nothing is cloned into the closure up front, matching the count you get from a directx.useor amove ||closure.use ||andmove ||are unaffected.