Provide proper encapsulation of StableMIR APIs #56
Comments
|
One possible solution is to revert the crate split that was done before. I believe the main motivation was to ensure we don't use internal constructs inside the Stable APIs. But I think it is easier for us to ensure that this doesn't happen, than ensuring that our users know which things are meant to be public only to the compiler. I also think it is much easier to make things public later, than the opposite direction. |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this issue
Dec 6, 2023
Add ADT variant infomation to StableMIR and finish implementing TyKind::internal() Introduce a `VariantDef` type and a mechanism to retrieve the definition from an `AdtDef`. The `VariantDef` representation itself is just a combination of `AdtDef` and `VariantIdx`, which allow us to retrieve further information of a variant. I don't think we need to cache extra information for now, and we can translate on an on demand manner. I am leaving the fields public today due to rust-lang/project-stable-mir#56, but they shouldn't. For this PR, I've only added a method to retrieve the variant name, and its fields. I also added an implementation of `RustcInternal` that allow users to retrieve more information using Rust internal APIs. I have also finished the implementation of `RustcInternal` for `TyKind` which fixes rust-lang/project-stable-mir#46. ## Motivation Both of these changes are needed in order to properly interpret things like projections. For example, - The variant definition is used to find out which variant we are downcasting to. - Being able to create `Ty` from `TyKind` helps for example processing each stage of a projection, like the code in `place.ty()`.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this issue
Dec 6, 2023
Rollup merge of rust-lang#118516 - celinval:smir-variants, r=ouz-a Add ADT variant infomation to StableMIR and finish implementing TyKind::internal() Introduce a `VariantDef` type and a mechanism to retrieve the definition from an `AdtDef`. The `VariantDef` representation itself is just a combination of `AdtDef` and `VariantIdx`, which allow us to retrieve further information of a variant. I don't think we need to cache extra information for now, and we can translate on an on demand manner. I am leaving the fields public today due to rust-lang/project-stable-mir#56, but they shouldn't. For this PR, I've only added a method to retrieve the variant name, and its fields. I also added an implementation of `RustcInternal` that allow users to retrieve more information using Rust internal APIs. I have also finished the implementation of `RustcInternal` for `TyKind` which fixes rust-lang/project-stable-mir#46. ## Motivation Both of these changes are needed in order to properly interpret things like projections. For example, - The variant definition is used to find out which variant we are downcasting to. - Being able to create `Ty` from `TyKind` helps for example processing each stage of a projection, like the code in `place.ty()`.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jan 20, 2024
Rollup merge of rust-lang#120135 - oli-obk:smir_private, r=celinval SMIR: Make the remaining "private" fields actually private Turns out we have already created a trait that allows us to make the fields private: https://doc.rust-lang.org/nightly/nightly-rustc/stable_mir/ty/trait.IndexedVal.html fixes rust-lang/project-stable-mir#56 r? `@celinval`
|
We have other structures that have the same issue. Not just things that wrap usize. |
|
why not define the types in rustc_smir, then reexport them in stable_mir? that way they can have proper private fields. generally speaking, types should be defined alongside their constructor. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The current crate structure of StableMIR is making it very hard to properly encapsulate our stable structures. All the internal details of stable items have to somehow be exposed in order to allow
rustc_smirto build them.For example, the definition of
Tyexposes its internal representation:There is no mechanism for us today to ensure that this
usizecorresponds to an index inside StableMIR. TheContexttrait is also exposed, even though we expect users to never invoke them directly.The main problems I see with the lack of encapsulation are:
a. Their code will trigger an ICE
b. The API may return inconsistent result
c. We will have to constantly check for these invariants ourselves as much as possible and return
Resultfor everything we do.We should investigate what is the best mechanism to fix this.
Tasks
The text was updated successfully, but these errors were encountered: