-
Notifications
You must be signed in to change notification settings - Fork 14k
Optionally add type names to TypeIds.
#136148
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
Conversation
| # Make `TypeId` store a reference to the name of the type, so that it can print that name. | ||
| debug_typeid = [] |
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.
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 entirely agree that that’s a problem, and I’m not aware of any documentation for std features. I expect the main way people will discover this feature is by looking at the source code of TypeId (which is linked from rustdocs, so is trivial to find).
On the other hand, maybe there is documentation for std features, in which case this PR certainly should add to it.
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 std-dev-guide but I think that's a submodule of a repo somewhere else.
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.
Yeah, better documentation would be nice. But as it's missing for the other features as well, I don't think this should be done in this PR.
joboet
left a 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.
Sound good too me. The implementation looks fine to me, let me just ask T-libs for confirmation.
r=me if T-libs is fine with adding this feature
|
IMO, this sort of quality of life improvement is not worth the extra cargo-based feature and -Zbuild-std dependency. It makes it more painful to evolve TypeId and seems easy to land a proliferation of this sort of tweak to how std/core work in a way that's hard for us to maintain long term. It seems plausible that we could have a table of TypeIds emitted somewhere by rustc that let's you do this lookup offline though. |
|
I see your point about complexity of miscellaneous features, but regarding the specific alternative, wouldn’t a rustc feature to do that likely be even more complex? I’ve frequently heard people wishing to have such a mapping, but the nice thing about doing it this way is that no management of the mapping is required, and the lookup is completely automatic and implicit. I get rejecting this change on grounds of clutter for maintainability — if nothing else, it’s easy enough to patch into a local copy — but it seems unwise to do so with the premise that |
|
rustc features have pretty clear costs too, but they're at least on a clear interface footing (available as -Z flag or similar). Expanding the build configurations of core via cfg is much less clear in terms of either stabilization path or easy-ish availability on nightly. It's also generally harder to ensure test quality (since you need to rebuild core etc, afaik we don't run any tests outside of the default config. Only that things build). |
library/core/src/any.rs
Outdated
| { | ||
| write!(f, " = {}", self.name)?; | ||
| } | ||
| write!(f, ")") |
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 think it would be better to duplicate the complete write! rather than breaking it up, so we avoid any impact to the normal feature-disabled case.
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.
Done. I was thinking that there was no difference because it doesn't change the number of text fragments involved, but it does affect the number of write_fmt() calls, which might matter to something.
This feature is intended to provide expensive but thorough help for developers who have an unexpected `TypeId` value and need to determine what type it actually is. It causes `impl Debug for TypeId` to print the type name in addition to the opaque ID hash, and in order to do so, adds a name field to `TypeId`. The cost of this is the increased size of `TypeId` and the need to store type names in the binary; therefore, it is an optional feature. It may be enabled via `cargo -Zbuild-std -Zbuild-std-features=debug_typeid`. (Note that `-Zbuild-std-features` disables default features which you may wish to reenable in addition; see <https://doc.rust-lang.org/cargo/reference/unstable.html#build-std-features>.) Example usage and output: ``` fn main() { use std::any::{Any, TypeId}; dbg!(TypeId::of::<usize>(), drop::<usize>.type_id()); } ``` ``` TypeId::of::<usize>() = TypeId(0x763d199bccd319899208909ed1a860c6 = usize) drop::<usize>.type_id() = TypeId(0xe6a34bd13f8c92dd47806da07b8cca9a = core::mem::drop<usize>) ``` Also added feature declarations for the existing `debug_refcell` feature so it is usable from the `rust.std-features` option of `config.toml`.
|
We discussed this in the libs meeting last week and were happy to have this as an unstable feature. |
|
Alright, let's ship this! |
| is_any::<[i32]>(); | ||
| } | ||
|
|
||
| #[cfg(feature = "debug_typeid")] |
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.
The coretests crate doesn't have this feature, so this test will never actually run.
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.
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.
Opened #140155 to remove it.
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.
Sorry for not getting back to this sooner — to give some context, this PR was probably originally written before the coretests split occurred (#135937), so the error was introduced by rebasing.
This feature is intended to provide expensive but thorough help for developers who have an unexpected
TypeIdvalue and need to determine what type it actually is. It causesimpl Debug for TypeIdto print the type name in addition to the opaque ID hash, and in order to do so, adds a name field toTypeId. The cost of this is the increased size ofTypeIdand the need to store type names in the binary; therefore, it is an optional feature. It does not expose any new public API, only change theDebugimplementation.It may be enabled via
cargo -Zbuild-std -Zbuild-std-features=debug_typeid. (Note that-Zbuild-std-featuresdisables default features which you may wish to reenable in addition; seehttps://doc.rust-lang.org/cargo/reference/unstable.html#build-std-features.)
Example usage and output:
Also added feature declarations for the existing
debug_refcellfeature so it is usable from therust.std-featuresoption ofconfig.toml.Related issues:
type_name()method on Any. #68379type_name_of_idto power a betterimpl Debug for TypeId? #61533