-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Reflection MVP #146923
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.
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
base: main
Are you sure you want to change the base?
Reflection MVP #146923
Conversation
aab0141 to
4234855
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4234855 to
6dd9a6a
Compare
This comment has been minimized.
This comment has been minimized.
6dd9a6a to
a77bbe2
Compare
This comment has been minimized.
This comment has been minimized.
| /// It can only be called at compile time. | ||
| #[unstable(feature = "type_info", issue = "146922")] | ||
| #[rustc_const_unstable(feature = "type_info", issue = "146922")] | ||
| pub const fn info(self) -> Type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently has zero regards for semver, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, but it also only supports tuples, which was an explicit choice so we can handle semver related things when we support Adts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you mean the fact that it allows inspecting a generic param and now knowing it's a tuple. This can look through opaque types and stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right it also breaks parametricity (though specialization also does that).
Not super relevant right now, but I hope "discuss semver considerations" is a major item somewhere on the agenda for this feature. ;) (The tracking issue is still rather barebones at the moment.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe controversial, but could Type be limited to viewing types within the current crate, and anything outside it simply be Opaque/Foreign/etc.? Libraries that want the Type of one of their types to be part of the public API could then expose it via a public constant or function returning said constant.
|
It seems like... trying to obtain a |
Oh whoops, that shouldn't happen. I'll add some more tests |
library/core/src/mem/type_info.rs
Outdated
| /// Primitives | ||
| Leaf, | ||
| /// FIXME(#146922): add all the common types | ||
| Unimplemented, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://docs.rs/const-type-layout/latest/const_type_layout/struct.TypeLayoutInfo.html for some prior work
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This is an MVP. Please do not flood this PR with all your wildest reflection dreams. Anything that suggests to extend the scope of this PR is off-topic. |
|
Currently, this implementation says that, in the type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newer contributor here. basically, I just got some nits you can ignore :)
thank you so much for working on this!!! :D
|
|
||
| #[unstable(feature = "type_info", issue = "146922")] | ||
| pub mod type_info; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be on purpose, but did you intend to use the same re-export pattern as above?
Otherwise, wouldn't this make a guarantee that everything in this module would become unstable/stable at the same time? (and prevent making changes to those internals..?)
if other MVPs tend to ignore this stuff, please ignore this comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I make the module stable, its internals would not become stable. The module may also stay unstable forever. For easier usage while entirely unstable I'm just making the module publicly available so all internals can be used and played with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat!
I'm a bit nervous about exposing a Type type- or more specifically I'm nervous about exposing a TyKind equivalent 🤔 Doesn't really matter for experimenting with stuff though, can figure out what we're confident in not being a problem for future evolution of the type system some other time before stabilization.
It's kinda difficult to read the type_info.rs code because it relies so heavily on the type definitions in core but you can't really see them side-by-side with the code you're reading (they're in a whole other file). I guess there's not really anything to do about this? Just a bit unfortunate
| ) -> InterpResult<'tcx> { | ||
| // project into the `type_info::Tuple::fields` field | ||
| let fields_slice_place = self.project_field(&tuple_place, FieldIdx::ZERO)?; | ||
| // get the `type_info::Field` type from `fields: &[Field]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use a lang item instead? it seems morally weird:tm: to place arbitrary compiler expectations about whatever type happens to be used as the type of this field, rather than having Field be a lang item (clearly indicating it has requirements) and then asserting this field is of type &[Field]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess generally it's kinda funny that you wind up not needing to make everything in the libs module be a lang item even though everything in there is morally a lang item with compiler magic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea... I like this while everything is up in the air. Otherwise I need to make all variants lang items, too and that will just pollute the lang item space
| help: consider importing this struct | ||
| | | ||
| LL + use std::mem::type_info::Type; | ||
| | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol thats quite unfortunate. it would be nice if nameres diagnostics didnt tell people to import unstable stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it does so on stable, as we explicitly filter unstable things.
| | | ||
| LL | fn a<F: Fn<usize>>(f: F) {} | ||
| | ^^^^^^^^^ the trait `Tuple` is not implemented for `usize` | ||
| | ^^^^^^^^^ the trait `std::marker::Tuple` is not implemented for `usize` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's going on here? 🤔 two things with the name Tuple in core now so it explicitly specifies which one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, there's also the unstable Tuple trait
fd76b36 to
34f9cff
Compare
|
|
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
|
r=me if CI passes and you notice before i do :> |
|
@bors r=BoxyUwU |
|
@bors rollup=iffy |
| rem-labels = [ | ||
| "regression-from-stable-to-beta", | ||
| "regression-from-stable-to-nightly", | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oli-obk Is there a reason this PR reformats triagebot.toml?
Is this accidental? If not, can this be in a seperate PR in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely autoformatted by the editor which uses whatever fmter+cfg it so happens to have + unintentionally committed.
Ideally we'd fixate the fmting via ig .gitattributes, .editorconfig or sth. else? Tho those are likely not recognized by the majority of editors anyway?
Reflection MVP I am opening this PR for discussion about the general design we should start out with, as there are various options (that are not too hard to transition between each other, so we should totally just pick one and go with it and reiterate later) r? `@scottmcm` and `@joshtriplett` project goal issue: rust-lang/rust-project-goals#406 tracking issue: #146922 The design currently implemented by this PR is * `TypeId::info` (method, usually used as `id.info()` returns a `Type` struct * the `Type` struct has fields that contain information about the type * the most notable field is `kind`, which is a non-exhaustive enum over all possible type kinds and their specific information. So it has a `Tuple(Tuple)` variant, where the only field is a `Tuple` struct type that contains more information (The list of type ids that make up the tuple). * To get nested type information (like the type of fields) you need to call `TypeId::info` again. * There is only one language intrinsic to go from `TypeId` to `Type`, and it does all the work An alternative design could be * Lots of small methods (each backed by an intrinsic) on `TypeId` that return all the individual information pieces (size, align, number of fields, number of variants, ...) * This is how C++ does it (see https://lemire.me/blog/2025/06/22/c26-will-include-compile-time-reflection-why-should-you-care/ and https://isocpp.org/files/papers/P2996R13.html#member-queries) * Advantage: you only get the information you ask for, so it's probably cheaper if you get just one piece of information for lots of types (e.g. reimplementing size_of in terms of `TypeId::info` is likely expensive and wasteful) * Disadvantage: lots of method calling (and `Option` return types, or "general" methods like `num_fields` returning 0 for primitives) instead of matching and field accesses * a crates.io crate could implement `TypeId::info` in terms of this design The backing implementation is modular enough that switching from one to the other is probably not an issue, and the alternative design could be easier for the CTFE engine's implementation, just not as nice to use for end users (without crates wrapping the logic) One wart of this design that I'm fixing in separate branches is that `TypeId::info` will panic if used at runtime, while it should be uncallable
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
ah we should not format the entire triagebot.toml :> @bors r- |
I am opening this PR for discussion about the general design we should start out with, as there are various options (that are not too hard to transition between each other, so we should totally just pick one and go with it and reiterate later)
r? @scottmcm and @joshtriplett
project goal issue: rust-lang/rust-project-goals#406
tracking issue: #146922
The design currently implemented by this PR is
TypeId::info(method, usually used asid.info()returns aTypestructTypestruct has fields that contain information about the typekind, which is a non-exhaustive enum over all possible type kinds and their specific information. So it has aTuple(Tuple)variant, where the only field is aTuplestruct type that contains more information (The list of type ids that make up the tuple).TypeId::infoagain.TypeIdtoType, and it does all the workAn alternative design could be
TypeIdthat return all the individual information pieces (size, align, number of fields, number of variants, ...)TypeId::infois likely expensive and wasteful)Optionreturn types, or "general" methods likenum_fieldsreturning 0 for primitives) instead of matching and field accessesTypeId::infoin terms of this designThe backing implementation is modular enough that switching from one to the other is probably not an issue, and the alternative design could be easier for the CTFE engine's implementation, just not as nice to use for end users (without crates wrapping the logic)
One wart of this design that I'm fixing in separate branches is that
TypeId::infowill panic if used at runtime, while it should be uncallable