Pad size of TypeId and remove structural equality by carbotaniuman · Pull Request #99189 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation20 Commits9 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This implements @eddyb's suggestions from #95845

Acouple smaller things could also be done (not sure if I'll be able to help with either):

manual PartialEq impl for TypeId (i.e. turning off "structural match")

this would disallow match-ing on TypeIds (forcing the use of == instead), as to not box ourselves into a corner if address equality is ever needed in the future

increasing TypeId's size (and cratering that change)

should help with breaking misuses (just like comments on this PR describe)

it doesn't have to be actual new data, could just be a dead field
* this is library-only, e.g. TypeId(type_hash::<T>()) -> TypeId(type_hash::<T>(), 0)

Given that the lang-team has already committed to a full length cryptographic hash, increasing the size of TypeId in preparation for a future change to a cryptographic hash should be a simple stepping stone. This also removes structural equality from TypeId for more futureproofing.

rustbot added the T-libs

Relevant to the library team, which will review and decide on the PR/issue.

label

Jul 12, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

This comment has been minimized.

Maybe the padding could be uninitialized data (MaybeUninit<u64>)? Then it's more likely to blow up when someone tries to transmute it.

#[stable(feature = "rust1", since = "1.0.0")]
pub struct TypeId {
pad: u64,

Choose a reason for hiding this comment

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

This should have some comment why that field added, as it isn't obvious.

Maybe the padding could be uninitialized data (MaybeUninit)? Then it's more likely to blow up when someone tries to transmute it.

How so? I think a regular transmute would complain because of the size difference, and I put the padding in front so a pointer cast would either simply overwrite the padding, or read from data after the provided u64 or whatever.

}

const fn check_type_id<T: 'static>() -> bool {
matches!(GetTypeId::::VALUE, GetTypeId::::VALUE)

Choose a reason for hiding this comment

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

I don't think we should lose the ability to compare type ids in CTFE. Comparison is almost the only thing you can do with a type id. It's possible to make == work here, but if that's tricky to do without potentially accidentally stabilizing it in CTFE then I think we should look at adding an unstable const function to compare them.

Choose a reason for hiding this comment

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

Note that == still works in CTFE, it's just pattern matching that's no longer allowed.

Choose a reason for hiding this comment

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

Do you know if we already have a test case that covers that @carbotaniuman?

Choose a reason for hiding this comment

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

Hmm, maybe I'm missing something but it doesn't appear that TypeId::of::<T>() == TypeId::of::<U>() works in CTFE as of nightly today:

error[[E0277]](https://doc.rust-lang.org/nightly/error-index.html#E0277): can't compare `TypeId` with `_` in const contexts
 --> src/main.rs:6:23
  |
6 |     TypeId::of::<T>() == TypeId::of::<U>()
  |                       ^^ no implementation for `TypeId == _`
  |
  = help: the trait `~const PartialEq<_>` is not implemented for `TypeId`
note: the trait `PartialEq<_>` is implemented for `TypeId`, but that implementation is not `const`
 --> src/main.rs:6:23
  |
6 |     TypeId::of::<T>() == TypeId::of::<U>()
  |                       ^^

As far as I can tell that would still be the case after this PR, except that we'd also lose the ability to compare type ids by matching them.

Choose a reason for hiding this comment

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

Yeah, it looks like I was mistaken here, and I'll likely reopen this PR with more substantial work behind it.

eddyb mentioned this pull request

Jul 21, 2022

4 tasks

Maybe the padding could be uninitialized data (MaybeUninit)? Then it's more likely to blow up when someone tries to transmute it.

How so? I think a regular transmute would complain because of the size difference, and I put the padding in front so a pointer cast would either simply overwrite the padding, or read from data after the provided u64 or whatever.

In miri doing transmute_copy with 0u64 as extra field would give 0 which doesn't behave as expected, but doesn't result in an UB error. With MaybeUninit::<u64>::uninit(), pretty much any operation on it would give an error when running in miri that an uninitialized value is used.

increasing TypeId's size (and cratering that change)

A crater run was done #75923 (comment), and @m-ou-se's comment #95845 (comment) that the breakage is small and we don't really care about this anyway is probably right (thanks to @Nilstrieb for tracking this down for me).

I'm personally not sure it really makes sense to do this incrementally -- we could also just wait until we actually have the hash. That said, it seems reasonable to discuss, so I'm nominating based on request from the author on Discord.

I'm personally not sure it really makes sense to do this incrementally -- we could also just wait until we actually have the hash.

Landing this PR now (well, mainly the "remove structural equality" part) would unblock const fn TypeId::of, since we only postponed that because we were guaranteeing too much.

CAD97 mentioned this pull request

Aug 1, 2022

Removing the structural equality might make sense, but is a breaking change that'd require a libs-api FCP.

I don't think we should be padding the struct right now with unused data, as @thomcc already said. I don't really see much of an advantage in doing that now vs doing that when we actually change to a new TypeId implementation.

The main reason I was looking to land the breaking change first was so that any follow-up implementation PR wouldn't have to deal with the fallout of crater if/when that gets brought up as I didn't want to put the work in like #75923 and then get stalled.

I can split off the structural equality part (and related fixes) now if that part is acceptable, and make a new PR with the widening implementation, assuming the libs team thinks this approach is the correct one.

I can split off the structural equality part (and related fixes) now if that part is acceptable, and make a new PR with the widening implementation, assuming the libs team thinks this approach is the correct one.

I can't promise you that the entire team considers that acceptable, but if you open a separate PR for that, I can start the FCP process to see if everyone is happy to go forward with it. :)

eddyb mentioned this pull request

Oct 13, 2022

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request

May 26, 2023

…atch, r=dtolnay

Remove structural match from TypeId

As per rust-lang#99189 (comment).

Removing the structural equality might make sense, but is a breaking change that'd require a libs-api FCP.

rust-lang#99189 (comment)

Landing this PR now (well, mainly the "remove structural equality" part) would unblock const fn TypeId::of, since we only postponed that because we were guaranteeing too much.

bors added a commit to rust-lang-ci/rust that referenced this pull request

May 26, 2023

…ch, r=dtolnay

Remove structural match from TypeId

As per rust-lang#99189 (comment).

Removing the structural equality might make sense, but is a breaking change that'd require a libs-api FCP.

rust-lang#99189 (comment)

Landing this PR now (well, mainly the "remove structural equality" part) would unblock const fn TypeId::of, since we only postponed that because we were guaranteeing too much.

saethlin pushed a commit to saethlin/miri that referenced this pull request

May 29, 2023

…lnay

Remove structural match from TypeId

As per rust-lang/rust#99189 (comment).

Removing the structural equality might make sense, but is a breaking change that'd require a libs-api FCP.

rust-lang/rust#99189 (comment)

Landing this PR now (well, mainly the "remove structural equality" part) would unblock const fn TypeId::of, since we only postponed that because we were guaranteeing too much.