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.

Sign up for GitHub

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

Jump to bottom

use PlaceRef abstraction more consistently #80647

Open
RalfJung opened this issue Jan 3, 2021 · 11 comments
Open

use PlaceRef abstraction more consistently #80647

RalfJung opened this issue Jan 3, 2021 · 11 comments
Assignees
@ericmarkmartin
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jan 3, 2021 •
edited by rustbot
Loading

We have quite a bit of code that needs to work with the projections that make up a Place or PlaceRef. Much of it works with the projections array directly, which is rather verbose. We now have better APIs that avoid having to "break the abstraction" of Place/PlaceRef: we have Place::iter_projections and PlaceRef::last_projection.

It would be good to clean things up by using these higher-level APIs consistently. To do that, you'll need to find places where the lower-level API is used. One good way to find these places is to grep for Place::ty_from -- this helper function is only useful when a PlaceRef has been broken apart into its constituents, so it is a sign that some higher-level APIs can be used. In almost all cases, the code will either "do something with the last projection (if any)", in which case it should use PlaceRef::last_projection, or it will iterate all the projections, in which case it should use Place::iter_projections (usually iteration starts with the outermost projection, i.e., you should use place.iter_projections().rev()).

You can see #80624 for some examples for the kinds of changes that are needed to perform this cleanup.
@RalfJung RalfJung added C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Jan 3, 2021
@RalfJung RalfJung changed the title use PlaceRef more consistently use PlaceRef abstraction more consistently Jan 3, 2021
@jonas-schievink jonas-schievink added the A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Jan 3, 2021
@RalfJung RalfJung mentioned this issue Jan 5, 2021
Merged
@RalfJung

This comment has been minimized.

Sign in to view
@oliviacrain oliviacrain mentioned this issue Jan 10, 2021
Merged
@oliviacrain
Copy link
Contributor

@RalfJung I went ahead and did some of this work for the rustc_mir crate in #80865. If I'm not completely off-base with these changes, I can go ahead and take a deeper look at this.

m-ou-se added a commit to m-ou-se/rust that referenced this issue Jan 16, 2021
@m-ou-se
Rollup merge of rust-lang#80865 - oliviacrain:proj_based, r=RalfJung
42801b4 
Use PlaceRef projection abstractions more consistently in rustc_mir

PlaceRef contains abstractions for dealing with the `projections` array. This PR uses these abstractions more consistently within the `rustc_mir` crate.

See associated issue: rust-lang#80647.

r? `@RalfJung`
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 18, 2021
@bors
Auto merge of rust-lang#80865 - oliviacrain:proj_based, r=RalfJung
0677d97 
Use PlaceRef projection abstractions more consistently in rustc_mir

PlaceRef contains abstractions for dealing with the `projections` array. This PR uses these abstractions more consistently within the `rustc_mir` crate.

See associated issue: rust-lang#80647.

r? `@RalfJung`
@henryboisdequin henryboisdequin mentioned this issue Feb 14, 2021
Merged
@RalfJung
Copy link
Member Author

@oliviacrain

@RalfJung I went ahead and did some of this work for the rustc_mir crate in #80865. If I'm not completely off-base with these changes, I can go ahead and take a deeper look at this.

That MR has been merged; do you still plan to push the same kind of change through the rest of the compiler?

@RalfJung
Copy link
Member Author

Another thing I just noticed: visit_projection of the MIR visitor takes a Local and a projection list separately; that can become a single PlaceRef I think (but I didn't check all use sites).

@henryboisdequin
Copy link
Contributor

Another thing I just noticed: visit_projection of the MIR visitor takes a Local and a projection list separately; that can become a single PlaceRef I think (but I didn't check all use sites).

Working on it in #82091 :)

@RalfJung
Copy link
Member Author

RalfJung commented Feb 16, 2021 •
edited
Loading

@henryboisdequin wrote

on Zulip I and a couple of others discussed that it would be impossible to replace &mut Place with &mut PlaceRef in a mutable visitor

If you could leave a summary of the problem here, that'd be good. Note that I was specifically talking about visit_projection; I am not suggesting to change visit_place in any way. So I am confused about why you want to replace Place by PlaceRef anywhere. The mutable vistitor does not have visit_projection; it has process_projection instead which however doesn't even take a Local... I am not entirely sure what is going on there.

@henryboisdequin
Copy link
Contributor

@henryboisdequin wrote

on Zulip I and a couple of others discussed that it would be impossible to replace &mut Place with &mut PlaceRef in a mutable visitor

If you could leave a summary of the problem here, that'd be good. Note that I was specifically talking about visit_projection; I am not suggesting to change visit_place in any way. So I am confused about why you want to replace Place by PlaceRef anywhere. The mutable vistitor does not have visit_projection; it has process_projection instead which however doesn't even take a Local... I am not entirely sure what is going on there.

@RalfJung, oops sorry, I think I messed up visit_projection and visit_place. In #82091, I'll change visit_projection to take a PlaceRef and leave visit_place alone.

@RalfJung
Copy link
Member Author

Also see this for another opportunity: super_projection should probably use iter_projections, and visit_projection_elem should take PlaceRef.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 20, 2021
@Dylan-DPC
Rollup merge of rust-lang#82091 - henryboisdequin:use-place-ref-more,…
06e04fd 
… r=RalfJung

use PlaceRef abstractions more consistently

Addresses this [comment](https://github.com/rust-lang/rust/pull/80865/files#r558978715)
Associated issue: rust-lang#80647

r? `@RalfJung`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 23, 2021
@Dylan-DPC
Rollup merge of rust-lang#82091 - henryboisdequin:use-place-ref-more,…
16ce41b 
… r=RalfJung

use PlaceRef abstractions more consistently

Addresses this [comment](https://github.com/rust-lang/rust/pull/80865/files#r558978715)
Associated issue: rust-lang#80647

r? ``@RalfJung``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 23, 2021
@Dylan-DPC
Rollup merge of rust-lang#82091 - henryboisdequin:use-place-ref-more,…
cc07061 
… r=RalfJung

use PlaceRef abstractions more consistently

Addresses this [comment](https://github.com/rust-lang/rust/pull/80865/files#r558978715)
Associated issue: rust-lang#80647

r? ```@RalfJung```
@bwmf2
Copy link
Contributor

bwmf2 commented Mar 20, 2023

Is this available?

@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@RalfJung
Copy link
Member Author

Also see #82091 (comment) for another opportunity: super_projection should probably use iter_projections, and visit_projection_elem should take PlaceRef.

super_projection seems to be taken care of, but visit_projection_elem could still be refactored. Grepping for &.*\[PlaceElem shows some more potential opportunities.

@ericmarkmartin
Copy link
Contributor

@rustbot claim

This was referenced Jun 16, 2023
Merged
Merged
bors added a commit to rust-lang/rust-clippy that referenced this issue Jun 20, 2023
@bors
Auto merge of #10989 - ericmarkmartin:use-placeref-abstraction, r=Man…
1919dff 
…ishearth

Use placeref abstraction

rust-lang/rust#80647 suggests refactoring certain patterns with MIR places to use higher-level abstractions provided by the [`Place`](https://doc.rust-lang.org/stable/nightly-rustc/rustc_middle/mir/struct.Place.html)/[`PlaceRef`](https://doc.rust-lang.org/stable/nightly-rustc/rustc_middle/mir/struct.PlaceRef.html). While working on that issue, I found a couple candidates for such refactoring in clippy.

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: none
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 27, 2023
@bors
Auto merge of rust-lang#112693 - ericmarkmartin:use-more-placeref, r=…
b9ad9b7 
…spastorino

Use PlaceRef abstractions more often

Associated issue: rust-lang#80647

r? `@spastorino`
oli-obk pushed a commit to oli-obk/miri that referenced this issue Jun 28, 2023
@bors
Auto merge of #112693 - ericmarkmartin:use-more-placeref, r=spastorino
34f0d66 
Use PlaceRef abstractions more often

Associated issue: rust-lang/rust#80647

r? `@spastorino`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
@bors
Auto merge of #112693 - ericmarkmartin:use-more-placeref, r=spastorino
7b84855 
Use PlaceRef abstractions more often

Associated issue: rust-lang/rust#80647

r? `@spastorino`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
@bors
Auto merge of #112693 - ericmarkmartin:use-more-placeref, r=spastorino
b85fe2b 
Use PlaceRef abstractions more often

Associated issue: rust-lang/rust#80647

r? `@spastorino`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ericmarkmartin ericmarkmartin

Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@RalfJung @jonas-schievink @ericmarkmartin @oliviacrain @Noratrieb @henryboisdequin @bwmf2