Skip to content
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

Optionally add type names to TypeIds. #136148

Merged
merged 1 commit into from
Feb 21, 2025
Merged

Optionally add type names to TypeIds. #136148

merged 1 commit into from
Feb 21, 2025

Conversation

kpreid
Copy link
Contributor

@kpreid kpreid commented Jan 27, 2025

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 does not expose any new public API, only change the Debug implementation.

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.

Related issues:

@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2025

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 27, 2025
Comment on lines +26 to +27
# Make `TypeId` store a reference to the name of the type, so that it can print that name.
debug_typeid = []
Copy link
Member

@jieyouxu jieyouxu Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: not a libs reviewer, but, is this1 documented anywhere? Feels like only 3 people will ever know if these features exists otherwise lol.

Footnotes

  1. or rather, I guess the core/std options in general.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

@joboet joboet left a 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

@joboet joboet added I-libs-nominated Nominated for discussion during a libs team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2025
@Mark-Simulacrum
Copy link
Member

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.

@kpreid
Copy link
Contributor Author

kpreid commented Jan 29, 2025

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 can do it better. I don’t expect it can.

@Mark-Simulacrum
Copy link
Member

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).

{
write!(f, " = {}", self.name)?;
}
write!(f, ")")
Copy link
Member

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.

Copy link
Contributor Author

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`.
@m-ou-se
Copy link
Member

m-ou-se commented Feb 12, 2025

We discussed this in the libs meeting last week and were happy to have this as an unstable feature.

@m-ou-se m-ou-se added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-libs-nominated Nominated for discussion during a libs team meeting. labels Feb 19, 2025
@joboet
Copy link
Member

joboet commented Feb 20, 2025

Alright, let's ship this!
@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2025

📌 Commit d2ed8cf has been approved by joboet

It is now in the queue for this repository.

@bors 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 Feb 20, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 20, 2025
Optionally add type names to `TypeId`s.

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 does not expose any new public API, only change the `Debug` implementation.

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`.

Related issues:

* rust-lang#68379
* rust-lang#61533
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2025
Rollup of 12 pull requests

Successful merges:

 - rust-lang#128080 (Specify scope in `out_of_scope_macro_calls` lint)
 - rust-lang#135354 ([Debuginfo] Add MSVC Synthetic and Summary providers to LLDB)
 - rust-lang#135630 (add more `s390x` target features)
 - rust-lang#136089 (Reduce `Box::default` stack copies in debug mode)
 - rust-lang#136148 (Optionally add type names to `TypeId`s.)
 - rust-lang#137192 (Remove obsolete Windows ThinLTO+TLS workaround)
 - rust-lang#137204 (Clarify MIR dialects and phases)
 - rust-lang#137299 (Simplify `Postorder` customization.)
 - rust-lang#137302 (Use a probe to avoid registering stray region obligations when re-checking drops in MIR typeck)
 - rust-lang#137305 (Tweaks in and around `rustc_middle`)
 - rust-lang#137313 (Some codegen_llvm cleanups)
 - rust-lang#137333 (Use `edition = "2024"` in the compiler (redux))

r? `@ghost`
`@rustbot` modify labels: rollup

try-job: test-various
try-job: x86_64-msvc-1
try-job: x86_64-msvc-2
try-job: i686-msvc-1
try-job: i686-msvc-2
try-job: i686-mingw-1
try-job: i686-mingw-2
try-job: i686-mingw-3
try-job: x86_64-gnu-nopt
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2025
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#132876 (rustdoc book: acknowledge --document-hidden-items)
 - rust-lang#136148 (Optionally add type names to `TypeId`s.)
 - rust-lang#136609 (libcore/net: `IpAddr::as_octets()`)
 - rust-lang#137336 (Stabilise `os_str_display`)
 - rust-lang#137350 (Move methods from Map to TyCtxt, part 3.)
 - rust-lang#137353 (Implement `read_buf` for WASI stdin)
 - rust-lang#137361 (Refactor `OperandRef::extract_field` to prep for MCP838)
 - rust-lang#137367 (Do not exempt nonexistent platforms from platform policy)
 - rust-lang#137374 (Stacker now handles miri using a noop impl itself)
 - rust-lang#137392 (remove few unused fields)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 28164f1 into rust-lang:master Feb 21, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 21, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2025
Rollup merge of rust-lang#136148 - kpreid:type-str, r=joboet

Optionally add type names to `TypeId`s.

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 does not expose any new public API, only change the `Debug` implementation.

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`.

Related issues:

* rust-lang#68379
* rust-lang#61533
@kpreid kpreid deleted the type-str branch February 25, 2025 20:43
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 27, 2025
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#132876 (rustdoc book: acknowledge --document-hidden-items)
 - rust-lang#136148 (Optionally add type names to `TypeId`s.)
 - rust-lang#136609 (libcore/net: `IpAddr::as_octets()`)
 - rust-lang#137336 (Stabilise `os_str_display`)
 - rust-lang#137350 (Move methods from Map to TyCtxt, part 3.)
 - rust-lang#137353 (Implement `read_buf` for WASI stdin)
 - rust-lang#137361 (Refactor `OperandRef::extract_field` to prep for MCP838)
 - rust-lang#137367 (Do not exempt nonexistent platforms from platform policy)
 - rust-lang#137374 (Stacker now handles miri using a noop impl itself)
 - rust-lang#137392 (remove few unused fields)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants