Skip to content
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

Implement reborrow for closure captures #82007

Merged
merged 4 commits into from
Feb 18, 2021
Merged

Conversation

arora-aman
Copy link
Member

@arora-aman arora-aman commented Feb 11, 2021

The strategy for captures is detailed here with examples: https://hackmd.io/PzxYMPY4RF-B9iH9uj9GTA

Key points:

  • We only need to reborrow a capture in case of move closures.

    • If we mutate something via a &mut we store it as a MutBorrow/UniqueMuBorrow of the path containing the &mut,
    • Similarly, if it's read via & ref we just store it as a ImmBorrow of the path containing the & ref.
    • If a path doesn't deref a &mut, &, then that path is captured by Move.
    • If the use of a path results in a move when the closure is called, then that path is truncated before any deref and the truncated path is moved into the closure.
  • In the case of non-move closure if a use of a path results in a move, then the path is truncated before any deref and the truncated path is moved into the closure.

Note that the implementation differs a bit from the document to allow for truncated path to be used in the ClosureKind analysis that happens as part of the first capture analysis pass.

Closes: rust-lang/project-rfc-2229#31

r? @nikomatsakis

@rust-highfive
Copy link
Collaborator

Updates src/tools/cargo.

cc @ehuss

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 11, 2021
@arora-aman
Copy link
Member Author

Hmm fixing the cargo.lock and submodule issues 🙃

@rust-log-analyzer

This comment has been minimized.

Comment on lines 1209 to 1470
for (i, proj) in place.projections.iter().enumerate() {
match proj.kind {
ProjectionKind::Deref => {
// We only drop Derefs in case of move closures
// There might be an index projection or raw ptr ahead, so we don't stop here.
place.projections.truncate(i);
return place;
}
_ => {}
}
}

place
Copy link
Contributor

@LingMan LingMan Feb 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit shorter with Iterator::position:

Suggested change
for (i, proj) in place.projections.iter().enumerate() {
match proj.kind {
ProjectionKind::Deref => {
// We only drop Derefs in case of move closures
// There might be an index projection or raw ptr ahead, so we don't stop here.
place.projections.truncate(i);
return place;
}
_ => {}
}
}
place
if let Some(i) = place.projections.iter().position(|proj| proj.kind == ProjectionKind::Deref) {
// We only drop Derefs in case of move closures
// There might be an index projection or raw ptr ahead, so we don't stop here.
place.projections.truncate(i);
}
place

Edit: Removed unhelpful temporary.

Copy link
Member Author

@arora-aman arora-aman Feb 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, didn't know this was a thing. I'll update the PR at once, after the PR's been completely reviewed

@nikomatsakis
Copy link
Contributor

This looks great. Go ahead and change to position() if you want.

@bors delegate+

@bors
Copy link
Contributor

bors commented Feb 12, 2021

✌️ @arora-aman can now approve this pull request

@nikomatsakis
Copy link
Contributor

After that you can tell bors r=nikomatsakis

@arora-aman
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 13, 2021

📌 Commit 20788d666da8881ff3207e18967402c8e70bb7ed has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2021
@bors
Copy link
Contributor

bors commented Feb 15, 2021

☔ The latest upstream changes (presumably #82103) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 15, 2021
@arora-aman
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 16, 2021

📌 Commit f99e152 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 16, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 16, 2021
Implement reborrow for closure captures

The strategy for captures is detailed here with examples: https://hackmd.io/PzxYMPY4RF-B9iH9uj9GTA

Key points:
- We only need to reborrow a capture in case of move closures.
  - If we mutate something via a `&mut` we store it as a `MutBorrow`/`UniqueMuBorrow` of the path containing the `&mut`,
  - Similarly, if it's read via `&` ref we just store it as a `ImmBorrow` of the path containing the `&` ref.
  - If a path doesn't deref a `&mut`, `&`, then that path is captured by Move.
  - If the use of a path results in a move when the closure is called, then that path is truncated before any deref and the truncated path is moved into the closure.

- In the case of non-move closure if a use of a path results in a move, then the path is truncated before any deref and the truncated path is moved into the closure.

Note that the implementation differs a bit from the document to allow for truncated path to be used in the ClosureKind analysis that happens as part of the first capture analysis pass.

Closes: rust-lang/project-rfc-2229#31

r? `@nikomatsakis`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 16, 2021
Implement reborrow for closure captures

The strategy for captures is detailed here with examples: https://hackmd.io/PzxYMPY4RF-B9iH9uj9GTA

Key points:
- We only need to reborrow a capture in case of move closures.
  - If we mutate something via a `&mut` we store it as a `MutBorrow`/`UniqueMuBorrow` of the path containing the `&mut`,
  - Similarly, if it's read via `&` ref we just store it as a `ImmBorrow` of the path containing the `&` ref.
  - If a path doesn't deref a `&mut`, `&`, then that path is captured by Move.
  - If the use of a path results in a move when the closure is called, then that path is truncated before any deref and the truncated path is moved into the closure.

- In the case of non-move closure if a use of a path results in a move, then the path is truncated before any deref and the truncated path is moved into the closure.

Note that the implementation differs a bit from the document to allow for truncated path to be used in the ClosureKind analysis that happens as part of the first capture analysis pass.

Closes: rust-lang/project-rfc-2229#31

r? ``@nikomatsakis``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 17, 2021
Implement reborrow for closure captures

The strategy for captures is detailed here with examples: https://hackmd.io/PzxYMPY4RF-B9iH9uj9GTA

Key points:
- We only need to reborrow a capture in case of move closures.
  - If we mutate something via a `&mut` we store it as a `MutBorrow`/`UniqueMuBorrow` of the path containing the `&mut`,
  - Similarly, if it's read via `&` ref we just store it as a `ImmBorrow` of the path containing the `&` ref.
  - If a path doesn't deref a `&mut`, `&`, then that path is captured by Move.
  - If the use of a path results in a move when the closure is called, then that path is truncated before any deref and the truncated path is moved into the closure.

- In the case of non-move closure if a use of a path results in a move, then the path is truncated before any deref and the truncated path is moved into the closure.

Note that the implementation differs a bit from the document to allow for truncated path to be used in the ClosureKind analysis that happens as part of the first capture analysis pass.

Closes: rust-lang/project-rfc-2229#31

r? ```@nikomatsakis```
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#77728 (Expose force_quotes on Windows.)
 - rust-lang#80572 (Add a `Result::into_ok_or_err` method to extract a `T` from `Result<T, T>`)
 - rust-lang#81860 (Fix SourceMap::start_point)
 - rust-lang#81869 (Simplify pattern grammar, improve or-pattern diagnostics)
 - rust-lang#81898 (Fix debug information for function arguments of type &str or slice.)
 - rust-lang#81972 (Placeholder lifetime error cleanup)
 - rust-lang#82007 (Implement reborrow for closure captures)
 - rust-lang#82021 (Spell out nested Self type in lint message)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0b2f2b9 into rust-lang:master Feb 18, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 18, 2021
@pthariensflame
Copy link
Contributor

Is this visible in the stable language? If so, does it need relnotes?

@arora-aman
Copy link
Member Author

This is behind the capture_disjoint_feilds feature gate, so not part of stable yet. That said I'm unsure what all goes in relnotes, so here is some context:

If I recall correcly this increases the precision of the capture (for non-unsafe code) which was restricted while trying to support move of a path that uses accesses reference in #80092. That PR was 1.51 based on the associated milestone.

@nikomatsakis
Copy link
Contributor

relnotes usually sticks to stable stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move out of reference in move closures
8 participants