Add richer structure for Stable MIR Projections #117517

klinvill · 2023-11-02T15:24:18Z

Resolves rust-lang/project-stable-mir#49.

Projections in Stable MIR are currently just strings. This PR replaces that representation with a richer structure, namely projections become vectors of ProjectionElems, just as in MIR. The ProjectionElem enum is heavily based off of the MIR ProjectionElem.

This PR is a draft since there are several outstanding issues to resolve, including:

How should UserTypeProjections be represented in Stable MIR? In MIR, the projections are just a vector of ProjectionElem<(),()>, meaning ProjectionElems that don't have Local or Type arguments (for Index, Field, etc. objects). Should UserTypeProjections be represented this way in Stable MIR as well? Or is there a more user-friendly representation that wouldn't drag along all the ProjectionElem variants that presumably can't appear?
What is the expected behavior of a Place's ty function? Should it resolve down the chain of projections so that something like *_1.f would return the type referenced by field f?
Tests should be added for UserTypeProjection

rustbot · 2023-11-02T15:24:27Z

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

klinvill · 2023-11-02T15:25:30Z

r? @celinval
(not quite ready for review, just tagging you now so you'll be assigned as the reviewer)

rustbot · 2023-11-02T15:25:33Z

Failed to set assignee to celinval: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

celinval · 2023-11-02T21:14:22Z

@klinvill I'm not an official reviewer, I'll take a look at it though.

r? @spastorino

can you take a look at it please?

celinval

I would like to suggest that you crate a new test instead of extending the existing one. Thanks for doing this though!

compiler/stable_mir/src/mir/body.rs

tests/ui-fulldeps/stable-mir/crate-info.rs

celinval · 2023-11-03T19:29:32Z

compiler/stable_mir/src/mir/body.rs

+    pub projection: Vec<ProjectionElem<Local, Ty>>,
+}
+
+// TODO(klinvill): in MIR ProjectionElem is parameterized on the second Field argument and the Index


I don't have a strong opinion here since I haven't used UserTypeProjection. That said, if not all elements are valid for UserTypeProjection, I would probably keep them separate.

@spastorino?

This commit includes richer projections for both Places and UserTypeProjections. However, the tests only touch on Places. There are also outstanding TODOs regarding how projections should be resolved to produce Place types, and regarding if UserTypeProjections should just contain ProjectionElem<(),()> objects as in MIR.

Asserting is_empty is slightly more concise.

It's not clear to me (klinvill) that UserTypeProjections are produced anymore with the removal of type ascriptions as per rust-lang/rfcs#3307. Furthermore, it's not clear to me which variants of ProjectionElem could appear in such projections. For these reasons, I'm reverting projections in UserTypeProjections to simple strings until I can get more clarity on UserTypeProjections.

rustbot · 2023-11-11T00:22:24Z

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

klinvill · 2023-11-11T00:35:00Z

@spastorino I'm ready for a review on this PR. A few notable decisions/outstanding questions are:

Within ProjectionElem, the Downcast variant in MIR includes an Option<Symbol> argument that can contain the name of the variant and comments state that it is used for printing the MIR. However, I believe this information can be obtained just using the VariantIdx argument. I've therefore omitted the Option<Symbol> argument. Is there instead a reason to keep it around?
I haven't been able to generate MIR that contains the UserTypeProjection struct. Is it still possible to generate UserTypeProjection MIR after type ascriptions were removed in accordance with De-RFC: Remove type ascription rfcs#3307? If so, in what cases is this struct generated? In the meantime I've left the projections field in UserTypeProjection unimplemented (specifically it's still a string).
In MIR, the projections in UserTypeProjection and Place use different instantiations of the same generic enum ProjectionElem. In particular, it seems like UserTypeProjection projections only support a subset of the variants. I've therefore opted to hardcode ProjectionElem to the instantiation for Place, and will create a separate enum if UserTypeProjections should still be included in Stable MIR.

@celinval I also added a visitor for Places that visits each of the projection elements. Can you give the visitor changes a quick look to make sure I didn't break any important conventions?

ouz-a · 2023-11-11T08:41:38Z

* Within `ProjectionElem`, the `Downcast` variant in MIR includes an `Option<Symbol>` argument that can contain the name of the variant and comments state that it is used for printing the MIR. However, I believe this information can be obtained just using the `VariantIdx` argument. I've therefore omitted the `Option<Symbol>` argument. Is there instead a reason to keep it around?

* I haven't been able to generate MIR that contains the `UserTypeProjection` struct. Is it still possible to generate `UserTypeProjection` MIR after type ascriptions were removed in accordance with [De-RFC: Remove type ascription rfcs#3307](https://github.com/rust-lang/rfcs/pull/3307)? If so, in what cases is this struct generated? In the meantime I've left the projections field in `UserTypeProjection` unimplemented (specifically it's still a string).

* In MIR, the projections in `UserTypeProjection` and `Place` use different instantiations of the same generic enum `ProjectionElem`. In particular, it seems like `UserTypeProjection` projections only support a subset of the variants. I've therefore opted to hardcode `ProjectionElem` to the instantiation for `Place`, and will create a separate enum if `UserTypeProjection`s should still be included in Stable MIR.

I will try to address these.

Imo we should always try to have printable version of a thing available for error reporting and debug printing also this will be useful for Emit smir #117745 once it lands
(/3) I think we can leave them for now and if we hit UserTypeProjection we know we have a case for it and we can implement it. This can be done randomly testing stuff in ui

celinval · 2023-11-14T12:30:25Z

Hi @klinvill, I think it makes sense to remove the Option<Symbol> as long as we provide an utility function that retrieves that information. Otherwise, we might want to keep it as pointed out by @ouz-a.

Can you change the projection field of UserTypeProjection to Opaque type for now? Thanks

celinval · 2023-11-14T12:30:50Z

r? @celinval

@spastorino I'll take this one.

klinvill · 2023-11-15T02:03:22Z

Thanks for the suggestions @ouz-a and @celinval. The inconsistency of having Option<Symbol> only to go along with the VariantIdx in Downcast, and not having a similar field to go with FieldIdx in Field bugs me a bit. I think it would be better long-term to be consistent with either passing around the additional information, or having that information accessible through a lookup based on the index.

I'd advocate for having that information accessible through a lookup based on the index rather than passing around additional information (which would potentially need to be updated as Variants and Fields evolve). However, such a lookup is currently neither possible (due to AdtDef not being implemented) nor convenient (due to Place::ty not being implemented). Currently, looking up the variant in a projection element would require accessing the local field in the parent Place object, then using that (index) to locate the appropriate LocalDecl, then following down the projection elements that precede the Downcast to get the appropriate AdtDef, then retrieving the proper VariantDef from the variants in the AdtDef (which currently isn't implemented). Ideally, we could instead create a copy of the Place definition with the projection elements truncated to either the desired Downcast projection or its parent, and then just call the ty function to get the appropriate AdtDef, but the ty function also isn't implemented yet.

So I suppose this is my long-winded way of proposing to drop the Option<Symbol> field with the intention of having utility functions that can do the lookup based on the index. However, since it's currently not possible to do this, I propose deferring the utility functions (at least for variant lookups for Downcast) until AdtDef and Place::ty are implemented. Does that sound reasonable to you both @ouz-a and @celinval?

celinval · 2023-11-15T02:09:46Z

Sounds reasonable to me. Can you please capture your comment above into an issue in project-stable-mir? We should revisit this in the near term, and create a follow up PR.

Thank you

ouz-a · 2023-11-15T06:01:52Z

This is probably nothing
@bors r+ rollup

bors · 2023-11-15T06:01:55Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors · 2023-11-15T06:01:56Z

📌 Commit c036a10 has been approved by ouz-a

It is now in the queue for this repository.

bors · 2023-11-15T06:03:05Z

⌛ Testing commit c036a10 with merge 9af957e...

Add richer structure for Stable MIR Projections Resolves rust-lang/project-stable-mir#49. Projections in Stable MIR are currently just strings. This PR replaces that representation with a richer structure, namely projections become vectors of `ProjectionElem`s, just as in MIR. The `ProjectionElem` enum is heavily based off of the MIR `ProjectionElem`. This PR is a draft since there are several outstanding issues to resolve, including: - How should `UserTypeProjection`s be represented in Stable MIR? In MIR, the projections are just a vector of `ProjectionElem<(),()>`, meaning `ProjectionElem`s that don't have Local or Type arguments (for `Index`, `Field`, etc. objects). Should `UserTypeProjection`s be represented this way in Stable MIR as well? Or is there a more user-friendly representation that wouldn't drag along all the `ProjectionElem` variants that presumably can't appear? - What is the expected behavior of a `Place`'s `ty` function? Should it resolve down the chain of projections so that something like `*_1.f` would return the type referenced by field `f`? - Tests should be added for `UserTypeProjection`

ouz-a · 2023-11-15T06:04:01Z

Maybe we should re-try this again before rolling it up
@bors r-

ouz-a · 2023-11-15T06:04:45Z

@bors r+

bors · 2023-11-15T06:04:47Z

📌 Commit c036a10 has been approved by ouz-a

It is now in the queue for this repository.

bors · 2023-11-15T06:05:57Z

⌛ Testing commit c036a10 with merge 698fcc8...

rust-log-analyzer · 2023-11-15T06:08:40Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2023-11-15T08:01:58Z

☀️ Test successful - checks-actions
Approved by: ouz-a
Pushing 698fcc8 to master...

rust-timer · 2023-11-15T09:35:12Z

Finished benchmarking commit (698fcc8): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.2%	[3.8%, 4.5%]	2
Improvements ✅ (primary)	-0.6%	[-0.7%, -0.5%]	3
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-0.6%	[-0.7%, -0.5%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.4%, 0.4%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 674.399s -> 674.6s (0.03%)
Artifact size: 311.08 MiB -> 311.10 MiB (0.00%)

Add VarDebugInfo to Stable MIR Previously we omitted `VarDebugInfo` because we didn't have `Projection` now that rust-lang#117517 is merged it's possible to add `VarDebugInfo` information in `Body`. This PR adds stable version of the `VarDebugInfo` to `Body` r? `@celinval`

Add VarDebugInfo to Stable MIR Previously we omitted `VarDebugInfo` because we didn't have `Projection` now that rust-lang#117517 is merged it's possible to add `VarDebugInfo` information in `Body`. This PR adds stable version of the `VarDebugInfo` to `Body` r? ``@celinval``

Add VarDebugInfo to Stable MIR Previously we omitted `VarDebugInfo` because we didn't have `Projection` now that rust-lang#117517 is merged it's possible to add `VarDebugInfo` information in `Body`. This PR adds stable version of the `VarDebugInfo` to `Body` r? ```@celinval```

Rollup merge of rust-lang#117972 - ouz-a:stable_debuginfo, r=celinval Add VarDebugInfo to Stable MIR Previously we omitted `VarDebugInfo` because we didn't have `Projection` now that rust-lang#117517 is merged it's possible to add `VarDebugInfo` information in `Body`. This PR adds stable version of the `VarDebugInfo` to `Body` r? ```@celinval```

rustbot assigned TaKO8Ki Nov 2, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 2, 2023

rustbot assigned spastorino and unassigned TaKO8Ki Nov 2, 2023

celinval reviewed Nov 3, 2023

View reviewed changes

klinvill added 6 commits November 9, 2023 20:56

Move SMIR projections tests to new file

98228e6

Replace match assertions against empty slices with is_empty assertions

30d6733

Asserting is_empty is slightly more concise.

Defer Place ty implementation in Stable Mir to later PR

998aa38

Add SMIR visitor for Places and projections

d517a1c

klinvill force-pushed the smir-projections branch from 3270247 to d517a1c Compare November 10, 2023 18:36

This comment has been minimized.

Sign in to view

Ignore FieldIdx and VariantIdx examples in docs

ae1726b

klinvill marked this pull request as ready for review November 11, 2023 00:22

This comment has been minimized.

Sign in to view

rustbot assigned celinval and unassigned spastorino Nov 14, 2023

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2023

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 15, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 15, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 15, 2023

bors merged commit 698fcc8 into rust-lang:master Nov 15, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 15, 2023

klinvill mentioned this pull request Nov 15, 2023

Implement richer debug information for ProjectionElem rust-lang/project-stable-mir#53

Open

klinvill deleted the smir-projections branch November 15, 2023 20:05

ouz-a mentioned this pull request Nov 16, 2023

Add VarDebugInfo to Stable MIR #117972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add richer structure for Stable MIR Projections #117517

Add richer structure for Stable MIR Projections #117517

klinvill commented Nov 2, 2023

rustbot commented Nov 2, 2023

klinvill commented Nov 2, 2023 •

edited

rustbot commented Nov 2, 2023

celinval commented Nov 2, 2023

celinval left a comment

celinval Nov 3, 2023

This comment has been minimized.

rustbot commented Nov 11, 2023

This comment has been minimized.

klinvill commented Nov 11, 2023

ouz-a commented Nov 11, 2023 •

edited

celinval commented Nov 14, 2023

celinval commented Nov 14, 2023

klinvill commented Nov 15, 2023 •

edited

celinval commented Nov 15, 2023

ouz-a commented Nov 15, 2023

bors commented Nov 15, 2023

bors commented Nov 15, 2023

bors commented Nov 15, 2023

ouz-a commented Nov 15, 2023

ouz-a commented Nov 15, 2023

bors commented Nov 15, 2023

bors commented Nov 15, 2023

rust-log-analyzer commented Nov 15, 2023

bors commented Nov 15, 2023

rust-timer commented Nov 15, 2023

Add richer structure for Stable MIR Projections #117517

Add richer structure for Stable MIR Projections #117517

Conversation

klinvill commented Nov 2, 2023

rustbot commented Nov 2, 2023

klinvill commented Nov 2, 2023 • edited

rustbot commented Nov 2, 2023

celinval commented Nov 2, 2023

celinval left a comment

Choose a reason for hiding this comment

celinval Nov 3, 2023

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Nov 11, 2023

This comment has been minimized.

klinvill commented Nov 11, 2023

ouz-a commented Nov 11, 2023 • edited

celinval commented Nov 14, 2023

celinval commented Nov 14, 2023

klinvill commented Nov 15, 2023 • edited

celinval commented Nov 15, 2023

ouz-a commented Nov 15, 2023

bors commented Nov 15, 2023

bors commented Nov 15, 2023

bors commented Nov 15, 2023

ouz-a commented Nov 15, 2023

ouz-a commented Nov 15, 2023

bors commented Nov 15, 2023

bors commented Nov 15, 2023

rust-log-analyzer commented Nov 15, 2023

bors commented Nov 15, 2023

rust-timer commented Nov 15, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

klinvill commented Nov 2, 2023 •

edited

ouz-a commented Nov 11, 2023 •

edited

klinvill commented Nov 15, 2023 •

edited