Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

Distinguish Place and PlaceWithHirId #3

Closed
nikomatsakis opened this issue Jun 9, 2020 · 7 comments · Fixed by rust-lang/rust#73489
Closed

Distinguish Place and PlaceWithHirId #3

nikomatsakis opened this issue Jun 9, 2020 · 7 comments · Fixed by rust-lang/rust#73489
Assignees

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jun 9, 2020

As part of refactoring to the new HIR place, we want to introduce a PlaceWithHirId that separates out the hir_id field from the rest of Place.

We can do this by:

  • rename the existing struct to PlaceWithHirId, get that to build
  • refactor to extract a few fields out of PlaceWithHirId into a new struct, Place, get that to build
  • Split projections into another structure, adding type information will be done as part of Store Type Information within projections #5
@arora-aman
Copy link
Member

arora-aman commented Jun 12, 2020

Outside of typechk Places are only used within clippy, so far:

tools/clippy/clippy_lints/src/escape.rs
tools/clippy/clippy_lints/src/loops.rs
tools/clippy/clippy_lints/src/utils/usage.rs

Search details: sexxi-goose/rust#14 (comment)

@nikomatsakis
Copy link
Contributor Author

That might be true, not sure. The compiler I think is mostly interested in finding the root variable at the moment, as those HIR places are (afaik) only used for the closure upvar analysis (within the compiler itself).

@arora-aman
Copy link
Member

arora-aman commented Jun 12, 2020

That might be true, not sure. The compiler I think is mostly interested in finding the root variable at the moment, as those HIR places are (afaik) only used for the closure upvar analysis (within the compiler itself).

Sorry that was meant to be outside of typechk, the module it is defined in.

@arora-aman
Copy link
Member

The markdown suggests using PlaceReference this might lead to someone confusing it for PlaceRef.

My suggestion is PlaceUnique, PlaceSpecific. These aren't great so might need to come up with something better 😂

@nikomatsakis
Copy link
Contributor Author

Ah, so that is what you meant by PlaceUnique on Zulip. =)

I think I like Place and PlaceWithHirId best. Or perhaps PlaceAccess?

@nikomatsakis
Copy link
Contributor Author

I found the "unique" terminology pretty confusing, and somehow assumed it was related to unique/mutable borrows or something else like that.

@arora-aman
Copy link
Member

arora-aman commented Jun 17, 2020

That is fair. PlaceWithHirId is quite descriptive, I like that. I just remember you suggesting coming up with something better on Zulip, that's why I suggested something else.

arora-aman added a commit to sexxi-goose/rust that referenced this issue Jun 18, 2020
For the following code
```rust
let c = || bar(foo.x, foo.x)
```

We generate two different `hir::Place`s for both `foo.x`.
Handling this adds overhead for analysis we need to do for RFC 2229.

We also want to store type information at each Projection to support
analysis as part of the RFC. This resembles what we have for
`mir::Place`

This commit modifies the Place as follows:
- Rename to `PlaceWithHirId`, where there `hir_id` is that of the
expressioin.
- Move any other information that describes the access out to another
struct now called `Place`.
- Removed `Span`, it can be accessed using the [hir
API](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/hir/map/struct.Map.html#method.span)
- Modify `Projection` to be a strucutre of its own, that currently only
contains the `ProjectionKind`.

Adding type information to projections wil be completed as part of rust-lang/project-rfc-2229#5

Closes rust-lang/project-rfc-2229#3

Co-authored-by: Aman Arora <[email protected]>
Co-authored-by: Roxane Fruytier <[email protected]>
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 19, 2020
…nikomatsakis

Refactor hir::Place

For the following code
```rust
let c = || bar(foo.x, foo.x)
```

We generate two different `hir::Place`s for both `foo.x`.
Handling this adds overhead for analysis we need to do for RFC 2229.

We also want to store type information at each Projection to support
analysis as part of the RFC. This resembles what we have for
`mir::Place`

This commit modifies the Place as follows:
- Rename to `PlaceWithHirId`, where there `hir_id` is that of the
expressioin.
- Move any other information that describes the access out to another
struct now called `Place`.
- Removed `Span`, it can be accessed using the [hir
API](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/hir/map/struct.Map.html#method.span)
- Modify `Projection` to be a strucutre of its own, that currently only
contains the `ProjectionKind`.

Adding type information to projections wil be completed as part of rust-lang/project-rfc-2229#5

Closes rust-lang/project-rfc-2229#3
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jun 22, 2020
For the following code
```rust
let c = || bar(foo.x, foo.x)
```

We generate two different `hir::Place`s for both `foo.x`.
Handling this adds overhead for analysis we need to do for RFC 2229.

We also want to store type information at each Projection to support
analysis as part of the RFC. This resembles what we have for
`mir::Place`

This commit modifies the Place as follows:
- Rename to `PlaceWithHirId`, where there `hir_id` is that of the
expressioin.
- Move any other information that describes the access out to another
struct now called `Place`.
- Removed `Span`, it can be accessed using the [hir
API](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/hir/map/struct.Map.html#method.span)
- Modify `Projection` to be a strucutre of its own, that currently only
contains the `ProjectionKind`.

Adding type information to projections wil be completed as part of rust-lang/project-rfc-2229#5

Closes rust-lang/project-rfc-2229#3

Co-authored-by: Aman Arora <[email protected]>
Co-authored-by: Roxane Fruytier <[email protected]>
@nikomatsakis nikomatsakis added this to the Feature complete milestone Feb 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants