Skip to content

Preserve place context through projections #73732

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

Closed

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jun 25, 2020

Resolves #72931. Implements rust-lang/compiler-team#300.

This removes PlaceContext::MutatingUse(Projection), which was used for x.y = 42, *x = 42 and &mut x.y, as well as PlaceContext::NonMutatingUse(Projection), which was used for let _ = x.y, let _ = *x, and &x.y. Now, these places receive their original PlaceContext unless there is a Deref projection.

Unfortunately, there's a lot of code that relies on a context only appearing in visit_local when the entire local is being assigned. Originally, I was going to switch these visitors to use visit_place. However, visit_place is not called for every use of a Local in the MIR, notably Index projections and Storage{Live,Dead} will result in a call to visit_local without one to visit_place. Instead I added an extra parameter to visit_local that is set when the Local was in a Place with projections.

This is not very clean, and the other solutions all have problems as well. I'm starting to wonder if we would be better off with a MIR Visitor 2.0 that was designed specifically for traversing the CFG instead of the current, one-size-fits-all trait. That way, we could make small quality-of-life improvements without worrying about subtly breaking old code that relies on a certain visitation order.

r? @oli-obk, although I'm not very proud of the current state of this.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2020
@ecstatic-morse ecstatic-morse changed the title Implements rust-lang/compiler-team#300 Preserve place context through projections Jun 25, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 26, 2020

So... back when @spastorino refactored places to be non-recursive datastructures, I think we discussed that we should just eliminate visit_local and visit_projection and just have a visit_place. Non-place visit_locals are problematic for mutation visitors (as for purely reading visitors we can easily just pull dummy values from the ether). Maybe we should just do this and make mutable visitors assert that the visitor did not try to replace an index projection's local or a StorageLive's local with a projection. Not ideal, but probably better than the status quo.

I don't think we should merge this PR as it is, it seems very fragile with all the workarounds "for when we had a projection context". If you want we can do a brainstorming/design sync session to hash out a plan.

@ecstatic-morse
Copy link
Contributor Author

Sounds good. I'll close this and we can take it to Zulip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor non_ssa_locals to remove LocalAnalyzer::process_place
3 participants