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

carbotaniuman · 2022-07-12T18:14:57Z

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 · 2022-07-12T18:15:00Z

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

rust-highfive · 2022-07-12T18:15:01Z

r? @joshtriplett

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

library/core/src/any.rs

the8472 · 2022-07-12T22:49:11Z

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

klensy · 2022-07-12T23:12:32Z

library/core/src/any.rs

+#[derive(Clone, Copy, PartialOrd, Ord, Debug, Hash)]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub struct TypeId {
+    pad: u64,


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

carbotaniuman · 2022-07-16T00:44:37Z

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.

KodrAus · 2022-07-18T03:48:49Z

src/test/ui/consts/issue-73976-monomorphic.rs

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


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.

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

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

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.

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

bjorn3 · 2022-07-21T18:30:23Z

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.

bors · 2022-07-27T15:47:53Z

☔ The latest upstream changes (presumably #99802) made this pull request unmergeable. Please resolve the merge conflicts.

thomcc · 2022-07-27T20:53:16Z

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.

eddyb · 2022-07-28T01:24:12Z

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.

m-ou-se · 2022-08-03T09:40:15Z

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.

carbotaniuman · 2022-08-10T00:19:40Z

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.

m-ou-se · 2022-08-10T14:07:31Z

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

…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. See also rust-lang#99189, rust-lang#101698

…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. See also rust-lang#99189, rust-lang#101698

…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. See also #99189, #101698

Pad size of typeid and remove structuraleq

642b8e1

rust-highfive assigned joshtriplett Jul 12, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 12, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 12, 2022

fbstj reviewed Jul 12, 2022

View reviewed changes

library/core/src/any.rs Outdated Show resolved Hide resolved

carbotaniuman added 2 commits July 12, 2022 13:32

Actually delete derive

61e9395

Add stability attrs

0bb7737

This comment has been minimized.

Sign in to view

klensy reviewed Jul 12, 2022

View reviewed changes

Update tests

7985d93

KodrAus reviewed Jul 18, 2022

View reviewed changes

eddyb mentioned this pull request Jul 21, 2022

Tracking Issue for const fn type_id #77125

Closed

4 tasks

carbotaniuman added 5 commits July 21, 2022 15:52

Try MaybeUninit

b35362c

Impl traits manually

34b2cce

Fullly qualify

e531a9a

Fix

ee6d19b

Dead code warnings

53dcb86

thomcc added the I-libs-nominated Nominated for discussion during a libs team meeting. label Jul 27, 2022

CAD97 mentioned this pull request Aug 1, 2022

TypeId: use a (v0) mangled type to remain sound in the face of hash collisions. #95845

Closed

m-ou-se removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Aug 10, 2022

carbotaniuman closed this Aug 26, 2022

eddyb mentioned this pull request Oct 13, 2022

Constify TypeId ordering impls #101698

Merged

raldone01 mentioned this pull request Oct 20, 2022

Remove structural match from TypeId #103291

Merged

tgross35 mentioned this pull request Jul 14, 2024

ACP: add std::any::is_same_type::<T, U>() -> bool rust-lang/libs-team#411

Closed

Uh oh!

Conversation

carbotaniuman commented Jul 12, 2022

Uh oh!

rustbot commented Jul 12, 2022

Uh oh!

rust-highfive commented Jul 12, 2022

Uh oh!

Uh oh!

This comment has been minimized.

the8472 commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klensy Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

carbotaniuman commented Jul 16, 2022

Uh oh!

KodrAus Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

carbotaniuman Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

KodrAus Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

KodrAus Aug 1, 2022

Choose a reason for hiding this comment

Uh oh!

carbotaniuman Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

bjorn3 commented Jul 21, 2022

Uh oh!

bors commented Jul 27, 2022

Uh oh!

thomcc commented Jul 27, 2022

Uh oh!

eddyb commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-ou-se commented Aug 3, 2022

Uh oh!

carbotaniuman commented Aug 10, 2022

Uh oh!

m-ou-se commented Aug 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

the8472 commented Jul 12, 2022 •

edited

Loading

eddyb commented Jul 28, 2022 •

edited

Loading