Disallow accesses through an Index projection when a sibling ConstantIndex projection has been moved out of - #160780
Disallow accesses through an Index projection when a sibling ConstantIndex projection has been moved out of#160780beepster4096 wants to merge 2 commits into
Conversation
…ons to be overlapping in some cases
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
r? @oli-obk rustbot has assigned @oli-obk. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| mpi: MovePathIndex, | ||
|
|
||
| /// The PlaceElem in the place immediately projecting from the parent move path. | ||
| next_elem: PlaceElem<'tcx>, |
There was a problem hiding this comment.
Should this be a ProjectionKind?
There was a problem hiding this comment.
Yeah, makes more sense considering the variant is the only info used right now
| // Subslice and ConstantIndex projections of slices also overlap siblings, | ||
| // but the parent slice will never have a move path | ||
| // Subslice projections of arrays are specifically checked in `check_if_subslice_element_is_moved` | ||
| LookupResult::Parent { mpi, next_elem: PlaceElem::Index(..) } => self |
There was a problem hiding this comment.
Could we match exhaustively on next_elem?
|
While this is a soundness fix where we're def landing it, let's still crater it and inform any potentially broken crates. @bors try |
This comment has been minimized.
This comment has been minimized.
Disallow accesses through an Index projection when a sibling ConstantIndex projection has been moved out of
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot cancel See #162233 |
|
🗑️ Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
Note: In the crater rollup, I added this small change to make the code compile. Hopefully that's fine. |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
There was a problem hiding this comment.
sad that it needed another special casing of the index projection. But after digging around in your branch for a bit I think this is indeed the right solution
@bors r+
|
📋 This PR cannot be approved because it currently has the following label: |
|
Ah darn it. Could have started the FCP while crater was running. oh well. @rfcbot fcp merge types This PR fixes an unsoundness in borrowck where we didn't realize index operations can overlap with slice patterns under certain circumstances (they both need to be within a single expression, otherwise we notice the local is uninit). The other projections are mutually exclusive (e.g. can't have a field projection and a deref projection on the same type/local), so they don't run into this situation. |
|
@oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
View all comments
Fixes #160525
LookupResultenum returned byMovePathLookup::find:LookupResult::Parent(None)case is now a separate variant,LookupResult::NoneLookupResult::Parenthas an additional field containing the variant immediately following the parent move path.check_if_full_path_is_movednow checks that new field and if it isPlaceElem::Index, it makes sure there are no uninit descendants of the indexed array's move path (which can only beConstantIndexand descendants of those.)move_path_closest_tois removed and inlined intocheck_if_full_path_is_moved.