2229: Fix issues with move closures and mutability #80092
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
arora-aman
commented
Dec 16, 2020
compiler/rustc_mir/src/borrow_check/diagnostics/mutability_errors.rs
Outdated
Show resolved
Hide resolved
| @@ -14,7 +14,6 @@ fn main() { | |||
| let mut c = || { | |||
| //~^ ERROR: cannot borrow `z.0.0.0` as mutable, as it is behind a `&` reference | |||
| z.0.0.0 = format!("X1"); | |||
There was a problem hiding this comment.
I would have expected us to not try read the place z on its own. When the feature isn't enabled we only read z.0.0.0.
Debug logs from the report_mutability_error when the feature is enabled.
DEBUG rustc_mir::borrow_check::diagnostics::mutability_errors report_mutability_error: access_place_desc=Some("z")
....
DEBUG rustc_mir::borrow_check::diagnostics::mutability_errors report_mutability_error: access_place_desc=Some("z.0.0.0")
There was a problem hiding this comment.
This is definitely not the error message we want-- we'd like the error to be reported on z.0.0.0, right? I think this may be related to my comment about the used_mut_upvars but I'm not 100% sure.
arora-aman
commented
Dec 16, 2020
arora-aman
commented
Dec 16, 2020
| let mut c = || { | ||
| //~^ ERROR: cannot borrow `z.0.0.0` as mutable, as it is behind a `&` reference | ||
| z.0.0.0 = format!("X1"); | ||
| //~^ ERROR: cannot assign to `z`, as it is not declared as mutable |
There was a problem hiding this comment.
I hack around this error in a later commit
arora-aman
commented
Dec 16, 2020
| // b. Is some path starting from a captured place in our parent closure | ||
| // c. Is a path starting from a local variable in our parent closure. | ||
|
|
||
| // Handle (a) |
There was a problem hiding this comment.
I think this is maybe missing a case -- if we capture a deref of an &mut. Also, I don't think that (a) is distinct from (b).
| } | ||
| } else { | ||
| this.used_mut.insert(place.local); | ||
| } |
There was a problem hiding this comment.
To handle the deref of &mut` case, we could check here if
- the projection is a deref
- the type of the
place_refis an&mut
| // The place isn't mutable once we dereference a immutable reference. | ||
| ty::Ref(.., hir::Mutability::Not) => return hir::Mutability::Not, | ||
|
|
||
| _ => (), |
There was a problem hiding this comment.
I would add ty::Adt with an assertion that this is a bug, and then make _ => bug!("deref of unexpected pointer type {:?}", pointer_ty.kind()) or something like that
There was a problem hiding this comment.
I'm assuming you meant Adt(boxed)
| @@ -14,7 +14,6 @@ fn main() { | |||
| let mut c = || { | |||
| //~^ ERROR: cannot borrow `z.0.0.0` as mutable, as it is behind a `&` reference | |||
| z.0.0.0 = format!("X1"); | |||
There was a problem hiding this comment.
This is definitely not the error message we want-- we'd like the error to be reported on z.0.0.0, right? I think this may be related to my comment about the used_mut_upvars but I'm not 100% sure.
src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm.rs
Show resolved
Hide resolved
arora-aman
force-pushed
the
restrict_precision
branch
2 times, most recently
from
dc7edd1
to
e6efc89
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
arora-aman
force-pushed
the
restrict_precision
branch
from
e6efc89
to
fcff889
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
arora-aman
force-pushed
the
restrict_precision
branch
from
fcff889
to
d07335a
Compare
arora-aman
commented
Dec 26, 2020
src/test/ui/closures/2229_closure_analysis/diagnostics/cant-mutate-imm.rs
Show resolved
Hide resolved
| // The place isn't mutable once we dereference a immutable reference. | ||
| ty::Ref(.., hir::Mutability::Not) => return hir::Mutability::Not, | ||
|
|
||
| _ => (), |
There was a problem hiding this comment.
I'm assuming you meant Adt(boxed)
| for (i, proj) in place.projections.iter().enumerate() { | ||
| if proj.ty.is_unsafe_ptr() { | ||
| // Don't apply any projections on top of an unsafe ptr | ||
| truncated_length = truncated_length.min(i + 1); |
There was a problem hiding this comment.
I went with this approach here because either we need to do a checked_add or init unsafe_ptr_idx = usize::MAX - 1. compared to the two, this seemed clearer.
arora-aman
commented
Dec 27, 2020
| let c = || { | ||
| //~^ ERROR: cannot borrow `**ref_mref_x` as mutable, as it is behind a `&` reference | ||
| **ref_mref_x = y; | ||
| //~^ERROR: cannot assign to `ref_mref_x`, as it is not declared as mutable |
There was a problem hiding this comment.
This gets hacked away as well in a later commit.
| let c = || { | ||
| //~^ ERROR: cannot borrow `**mref_ref_x` as mutable, as it is behind a `&` reference | ||
| **mref_ref_x = y; | ||
| //~^ERROR: cannot assign to `mref_ref_x`, as it is not declared as mutable |
There was a problem hiding this comment.
This gets hacked away as well in a later commit.
|
☔ The latest upstream changes (presumably #80503) made this pull request unmergeable. Please resolve the merge conflicts. |
arora-aman
force-pushed
the
restrict_precision
branch
from
d07335a
to
69ea484
Compare
|
Conflicts should be resolved now |
arora-aman
force-pushed
the
restrict_precision
branch
from
69ea484
to
967c2b2
Compare
|
☔ The latest upstream changes (presumably #81113) made this pull request unmergeable. Please resolve the merge conflicts. |
nikomatsakis
approved these changes
Jan 28, 2021
| #[derive(PartialEq, Clone, Debug, TyEncodable, TyDecodable, TypeFoldable, HashStable)] | ||
| pub struct CapturedPlace<'tcx> { | ||
| /// The `Place` that is captured. |
There was a problem hiding this comment.
I love how you're adding comments here btw 😍
| } | ||
|
|
||
| impl CapturedPlace<'tcx> { | ||
| pub fn get_root_variable(&self) -> hir::HirId { |
There was a problem hiding this comment.
I'd like to see a comment here, something like
/// Returns the hir-id of the root variable for the captured place. e.g., if a.b.c was captured, would return the hir-id for a.
| #[derive(PartialEq, Clone, Debug, TyEncodable, TyDecodable, TypeFoldable, HashStable)] | ||
| pub struct CapturedPlace<'tcx> { | ||
| /// The `Place` that is captured. |
There was a problem hiding this comment.
So happy to see added comments 😍
| @@ -1374,13 +1358,38 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { | |||
|
|
|||
| fn propagate_closure_used_mut_upvar(&mut self, operand: &Operand<'tcx>) { | |||
| let propagate_closure_used_mut_place = |this: &mut Self, place: Place<'tcx>| { | |||
| if !place.projection.is_empty() { | |||
| if let Some(field) = this.is_upvar_field_projection(place.as_ref()) { | |||
| // We have three possiblities here: | |||
There was a problem hiding this comment.
Suggested change
| // We have three possiblities here: | |
| // We have three possibilities here: |
| // We have three possiblities here: | ||
| // a. We are modifying something through a mut-ref | ||
| // b. We are modifying something that is local to our parent | ||
| // c. Current body is a nested clsoure, and we are modifying path starting from |
There was a problem hiding this comment.
Suggested change
| // c. Current body is a nested clsoure, and we are modifying path starting from | |
| // c. Current body is a nested closure, and we are modifying a path starting from |
|
@bors delegate+ Left a few nits but this looks good. @arora-aman, you wanted to make a final change as well, right? Feel free to r=nikomatsakis afterwards. |
|
✌️ @arora-aman can now approve this pull request |
- No Derefs in move closure, this will result in value behind a reference getting moved. - No projections are applied to raw pointers, since these require unsafe blocks. We capture them completely. Motivations for these are recorded here: https://hackmd.io/71qq-IOpTNqzMkPpAI1dVg?view
When `capture_disjoint_fields` is not enabled, checking if the root variable binding is mutable would suffice. However with the feature enabled, the captured place might be mutable because it dereferences a mutable reference. This PR computes the mutability of each capture after capture analysis in rustc_typeck. We store this in `ty::CapturedPlace` and then use `ty::CapturedPlace::mutability` in mir_build and borrow_check.
arora-aman
force-pushed
the
restrict_precision
branch
from
967c2b2
to
0f4bab2
Compare
|
@bors r=nikomatsakis |
|
📌 Commit 0f4bab2 has been approved by |
bors
added
S-waiting-on-bors
m-ou-se
added a commit
to m-ou-se/rust
that referenced
this pull request
Jan 30, 2021
…ikomatsakis
2229: Fix issues with move closures and mutability
This PR fixes two issues when feature `capture_disjoint_fields` is used.
1. Can't mutate using a mutable reference
2. Move closures try to move value out through a reference.
To do so, we
1. Compute the mutability of the capture and store it as part of the `CapturedPlace` that is written in TypeckResults
2. Restrict capture precision. Note this is temporary for now, to allow the feature to be used with move closures and ByValue captures and might change depending on discussions with the lang team.
- No Derefs are captured for ByValue captures, since that will result in value behind a reference getting moved.
- No projections are applied to raw pointers since these require unsafe blocks. We capture
them completely.
r? `@nikomatsakis`
This was referenced Jan 31, 2021
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 31, 2021
…as-schievink Rollup of 11 pull requests Successful merges: - rust-lang#80092 (2229: Fix issues with move closures and mutability) - rust-lang#80404 (Remove const_in_array_repeat) - rust-lang#81255 (Don't link with --export-dynamic on wasm32-wasi) - rust-lang#81480 (Add suggestion for nested fields) - rust-lang#81549 (Misc ip documentation fixes) - rust-lang#81566 (Add a test for rust-lang#71202) - rust-lang#81568 (Fix an old FIXME in redundant paren lint) - rust-lang#81571 (Fix typo in E0759) - rust-lang#81572 (Edit multiple error code Markdown files) - rust-lang#81589 (Fix small typo in string.rs) - rust-lang#81590 (Stabilize int_bits_const) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This was referenced Feb 1, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes two issues when feature
capture_disjoint_fieldsis used.To do so, we
CapturedPlacethat is written in TypeckResultsthem completely.
r? @nikomatsakis