Implement richer debug information for ProjectionElem

[klinvill]

Similar to what Oğuz did in rust-lang/rust#115534, indexed types should have debug methods that prints out more useful information like the name of the definition/variant/field.

As mentioned in rust-lang/rust#117517 (comment), this kind of debug printing is likely not possible for ProjectionElem until AdtDef is implemented in Stable MIR. Place::ty may also need to be implemented first to easily resolve the name of a variant/field. This is because it may be easiest to create a copy of the Place definition with the projection elements truncated to either the desired Downcast/Field projection or its parent, and then just call the Place::ty function to get the appropriate AdtDef.

[celinval]

I believe this issue is unblocked now sice we have implemented AdtDef and Place::ty(). @klinvill, any thoughts?

[klinvill]

Yeah I think it should be unblocked now. I unfortunately have a few other things on my plate to take care of first, but ideally can take a stab at this in the next couple of weeks.

[m-rph]

@rustbot claim

[m-rph]

This looks simple enough, unless I am missing something it's just a matter of adding all the variants of the enum, no?

[m-rph]

Okay, it seems that I am missing something, I can't see how the implementation of Place:ty can be used to retrieve the parent of an element. If all you have is local information, e.g. source relative index as in Downcast, how can Place::ty be used to retrieve the parent ADT?

[klinvill]

First off, my apologies for the severe delay in any response/work on this issue.

@m-rph yeah we'd need more than just local ProjectionElem information, but all we should need to get enriched ProjectionElem information is the Place object containing that projection. Specifically all we need is the parent's type information (which can be used to extract the variants), then the source-relative index can be used to get the relevant variant. So a method that does this might look something like:

  fn name_of_variant(&self, place_ty: &Ty) -> Option<Symbol> {
      match self {
          ProjectionElem::Downcast(idx) => { match place_ty {
              RigidTy::Adt(def, _) => Some(def.variants().get(idx.to_index())?.name()),
              _ => None,
          } },
          _ => None,
      }
  }

However, I don't think a standalone method like this is very useful. It's better just to integrate it with the existing stable mir debug/pretty-printing functionality.

@celinval, I've been out of the loop for a while so I'm not clear on the best place to add in the symbol for variant projections. It looks like the debug implementation for Place ultimately calls Place::internal to get the mir representation of the place, then calls the debug implementation for that mir Place. Is the best place to add this extra symbol information (for variant projections) in Place::internal and ProjectionElem::internal? I'm thinking I could just fold the type information along the projections and enrich the internal variant type with the symbol if it can be recovered

[celinval]

I went back to the original comment on this, I think we should either close this issue or add the utility function variant_name you just mentioned above.

We have implemented Debug for Place, which will allow users to debug a projection. The ProjectionElem on its own doesn't provide much insight on its own anyway. @klinvill, am I missing something?

[klinvill]

ProjectionElem in mir takes an Option<Symbol> argument which is the name of the variant and is just used for printing. The current stable mir internal implementation (used by Debug) always passes None for the symbol argument (https://github.com/rust-lang/rust/blob/8337ba9189de188e2ed417018af2bf17a57d51ac/compiler/rustc_smir/src/rustc_internal/internal.rs#L537).

I was thinking that the best resolution for this issue is probably to update internal implementation of Place to try to find the symbol for each variant by propagating the type as the projections are converted to their internal representations. However, I see what Quinn may have been pointing out in that getting the type of a Place requires the locals from the function the place originates from. I don't think the locals will/should always be available when converting to the debug representation/when converting to the internal representation. So the options seem to me to either add an optional locals argument to each internal/debug method, or just close this issue. I'd advocate for just closing this issue since I think the optional argument probably won't be used much and will add a lot of clutter. Any thoughts?

Additionally, the utility function I mentioned above is easy enough to implement in the future if it's needed and the place's type information is available.

[celinval]

Sounds good to me! Thanks