Skip to content

Preserve PlaceContext through projections #300

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
3 of 4 tasks
ecstatic-morse opened this issue Jun 2, 2020 · 1 comment
Closed
3 of 4 tasks

Preserve PlaceContext through projections #300

ecstatic-morse opened this issue Jun 2, 2020 · 1 comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jun 2, 2020

TL;DR

Remove the Projection variants of PlaceContext, instead propagating the original context of the use (e.g., Borrow, Store, etc.) or a newly added Deref variant if that place is dereferenced.

Links and Details

The visit_place, visit_local and similar methods on MIR visitors have access to a PlaceContext so the visitor knows how that Place or Local was being used without having to match on StatementKind or TerminatorKind themselves. However, PlaceContext basically becomes useless as soon as projections are involved. For example, the following uses of x.y are all given MutatingUseContext::Projection, despite being very different in meaning. I want to instead preserve the original use context or a newly added Deref variant.

let b = &mut x.y; // `MutatingUseContext::Borrow`
x.y = 42;         // `MutatingUseContext::Store`
*x.y = 42;        // `MutatingUseContext::Store` for the place `*x.y`, `MutatingUseContext::Deref` for the place `x.y` and the local `x`.

This will allow more code to make use of PlaceContext, which is easier (and thus less prone to breakage) than manually inspecting statement or terminator kinds and/or place projections. Because Projection is used for all projections, the dataflow liveness analysis for locals (which uses PlaceContext) is not as precise as it could be.

Mentors or Reviewers

???

(Not sure who "owns" PlaceContext)

What is this issue?

This is a major change proposal, which means a proposal to make a notable change to the compiler -- one that either alters the architecture of some component, affects a lot of people, or makes a small but noticeable public change (e.g., adding a compiler flag). You can read more about the MCP process on https://forge.rust-lang.org/.

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

MCP Checklist

  • MCP filed. Automatically, as a result of filing this issue:
    • The @rust-lang/wg-prioritization group will add this to the triage meeting agenda so folks see it.
    • A Zulip topic in the stream #t-compiler/major changes will be created for this issue.
  • MCP seconded. The MCP is "seconded" when a compiler team member or contributor issues the @rustbot second command. This should only be done by someone knowledgable with the area -- before seconding, it may be a good idea to cc other stakeholders as well and get their opinion.
  • Final comment period (FCP). Once the MCP is approved, the FCP begins and lasts for 10 days. This is a time for other members to review and raise concerns -- concerns that should block acceptance should be noted as comments on the thread, ideally with a link to Zulip for further discussion.
  • MCP Accepted. At the end of the FCP, a compiler team lead will review the comments and discussion and decide whether to accept the MCP.
    • At this point, the major-change-accepted label is added and the issue is closed. You can link to it for future reference.

A note on stability. If your change is proposing a new stable feature, such as a -C flag, then a full team checkoff will be required before the feature can be landed. Often it is better to start with an unstable flag, like a -Z flag, and then move to stabilize as a secondary step.

@ecstatic-morse ecstatic-morse added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Jun 2, 2020
@rustbot rustbot added the to-announce Announce this issue on triage meeting label Jun 2, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2020

@rustbot seconded

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Jun 3, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Jun 3, 2020
@spastorino spastorino added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants