Remove Hash, PartialEq, Eq, PartialOrd, Ord from DynMetadata #112052

nbdd0121 · 2023-05-28T23:43:03Z

Since comparison on DynMetadata doesn't yield much information, remove (Partial)?(Eq|Ord) and Hash from DynMetadata.

Since we currently allow unsized pointers to be hashed, this also involves changing the impl to preserve the current behaviour.

This idea was discussed briefly during last week's lang triage meeting.

cc #106447, @rust-lang/lang

@rustbot labels: +T-lang +T-libs-api

rustbot · 2023-05-28T23:43:09Z

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

thomcc · 2023-05-29T20:22:20Z

r=me on the impl, I don't know who has to sign off for this to merge though (definitely someone other than me, perhaps it's already done? Not sure). Punting back to you to figure that out.

@rustbot author

BoxyUwU · 2023-05-29T20:24:15Z

library/core/src/hash/mod.rs

@@ -960,17 +960,21 @@ mod impls {
        fn hash<H: Hasher>(&self, state: &mut H) {
            let (address, metadata) = self.to_raw_parts();
            state.write_usize(address.addr());
-            metadata.hash(state);
+            if mem::size_of_val(&metadata) == mem::size_of::<usize>() {


surely it would be better to just require metadata to be Hash instead of special casing usize size'd metadata via a transmute. this also means that we cannot have usize sized metadata with padding it

doing this would make it extremely hard to ever support unsizing away arbitrary const generics with adt_const_params since if you had a usize size'd type for your const arg it would now implicitly get hashed whereas if you added a byte of padding you wouldnt.

Perhaps we could have a (private to core) intrinsic like type_id that works for non 'static types that just returns bool for whether the metadata here is a usize instead of assuming that the only usize size'd metadata we are ever going to have is a usize

I'd be in favor of that, the impl is kind of gross as is.

I guess the metadata can also be pointers to vtables which would be usize sized, is this supposed to affect those too? I guess the intrinsic would have to be more complex if so

It'll be confusing to have DynMetadata of the same type to hash to different values.

The impl here is essentially just going back to it's old state before we have the unstable ptr metadata API (the old one uses ptr casting while I chose to use transmute_copy, but the essence is the same).

One possible solution to avoid this is to have a private-to-core trait MetadataHash that's implemented both for (), usize and DynMetadata, and we have that in the bound instead of Hash.

@nbdd0121 just so I'm clear: What is the longer-term goal w.r.t. to DynMetadata (i.e. *dyn Trait` metadata i.e. pointers to vtables)?

My understanding is that this PR continues to hash them (by hashing their raw address, transmuted to a usize), but it may be safer to exclude them, due to the issues pointed out in #106447 (comment) .

Excluding the metadata in that case will make hashes collide for two distinct &dyn Trait1 and &dyn Trait2 that has the same pointer address, but... I think that's acceptable?

It'll be confusing to have DynMetadata of the same type to hash to different values.

Also, I didn't understand what the "of the same type" was saying here. The same Type for distinct Trait1/Trait2? Or the same instance of the vtable (i.e. the same address)? Or the same Trait1 but distinct vtables (and thus the distinct hash values)?

BoxyUwU · 2023-05-29T21:04:05Z

Is == for raw pointers implemented as a buitlin impl? this PR doesnt make it impossible to == or change the Eq impl for raw pointers so why are you removing Eq from metadata which should be required in order to == raw pointers. You're removing bounds from Metadata but still acting as if all metadata can be hashed and eq'd. Tbh this PR seems like its going to be a future compat hazard and a soundness hazard

nbdd0121 · 2023-05-30T00:29:52Z

The == of raw pointer is builtin.

We can't remove PartialEq/Eq/Hash on dyn trait pointers anymore because that'll break backward compatibility. The point of this PR is to prevent us continuing our mistakes for ptr metadata API; we can still change it while it's unstable.

BoxyUwU · 2023-05-30T01:23:40Z

How is it true that you can stop promising that metadata is Eq/Hash because the API is "unstable" when we stably expose that raw pointers are eq and hash for arbitrary metadata? The only way that makes any sense is if we are not hash/eq the metadata which is not what this PR is doing.

nbdd0121 · 2023-05-30T10:23:24Z

No, not hashing/eq them will make it behave as if they are always equal.

BoxyUwU · 2023-05-31T00:36:38Z

I don't think i really have the energy to continue this discussion. I don't particularly understand what you're saying either which is unfortunate. I'm not going to object to this PR since it would be unfair to argue this shouldn't land while also simaltaneously not being able to engage with it lol

Dylan-DPC · 2023-08-15T06:18:16Z

r? compiler

thomcc · 2023-08-28T06:09:08Z

I don't know who should review this but it shouldn't be me. It's probably t-lang.

r? lang

pnkfelix · 2023-08-30T15:22:30Z

I'll be better able to review this once I get answers to #112052 (comment)

@rustbot label: +S-waiting-on-author -S-waiting-on-review

Dylan-DPC · 2023-10-31T13:14:15Z

@nbdd0121 any updates on this?

Dylan-DPC · 2024-03-15T12:06:52Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

rustbot assigned thomcc May 28, 2023

This comment has been minimized.

Sign in to view

Remove Hash, PartialEq, Eq, PartialOrd, Ord from DynMetadata

7ad7c2f

nbdd0121 force-pushed the dyn_metadata branch from 3bbd0d2 to 7ad7c2f Compare May 28, 2023 23:56

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2023

BoxyUwU reviewed May 29, 2023

View reviewed changes

rustbot assigned TaKO8Ki and unassigned thomcc Aug 15, 2023

Dylan-DPC assigned thomcc Aug 15, 2023

Dylan-DPC added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 15, 2023

rustbot assigned pnkfelix and unassigned thomcc and TaKO8Ki Aug 28, 2023

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 30, 2023

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2023

Dylan-DPC closed this Mar 15, 2024

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Hash, PartialEq, Eq, PartialOrd, Ord from DynMetadata #112052

Remove Hash, PartialEq, Eq, PartialOrd, Ord from DynMetadata #112052

nbdd0121 commented May 28, 2023

rustbot commented May 28, 2023

This comment has been minimized.

thomcc commented May 29, 2023 •

edited

Loading

BoxyUwU May 29, 2023

BoxyUwU May 29, 2023

BoxyUwU May 29, 2023 •

edited

Loading

thomcc May 29, 2023

BoxyUwU May 29, 2023

nbdd0121 May 30, 2023

pnkfelix Aug 30, 2023 •

edited

Loading

pnkfelix Aug 30, 2023

BoxyUwU commented May 29, 2023 •

edited

Loading

nbdd0121 commented May 30, 2023

BoxyUwU commented May 30, 2023

nbdd0121 commented May 30, 2023

BoxyUwU commented May 31, 2023 •

edited

Loading

Dylan-DPC commented Aug 15, 2023

thomcc commented Aug 28, 2023

pnkfelix commented Aug 30, 2023

Dylan-DPC commented Oct 31, 2023

Dylan-DPC commented Mar 15, 2024

Remove Hash, PartialEq, Eq, PartialOrd, Ord from DynMetadata #112052

Remove Hash, PartialEq, Eq, PartialOrd, Ord from DynMetadata #112052

Conversation

nbdd0121 commented May 28, 2023

rustbot commented May 28, 2023

This comment has been minimized.

thomcc commented May 29, 2023 • edited Loading

BoxyUwU May 29, 2023

Choose a reason for hiding this comment

BoxyUwU May 29, 2023

Choose a reason for hiding this comment

BoxyUwU May 29, 2023 • edited Loading

Choose a reason for hiding this comment

thomcc May 29, 2023

Choose a reason for hiding this comment

BoxyUwU May 29, 2023

Choose a reason for hiding this comment

nbdd0121 May 30, 2023

Choose a reason for hiding this comment

pnkfelix Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

pnkfelix Aug 30, 2023

Choose a reason for hiding this comment

BoxyUwU commented May 29, 2023 • edited Loading

nbdd0121 commented May 30, 2023

BoxyUwU commented May 30, 2023

nbdd0121 commented May 30, 2023

BoxyUwU commented May 31, 2023 • edited Loading

Dylan-DPC commented Aug 15, 2023

thomcc commented Aug 28, 2023

pnkfelix commented Aug 30, 2023

Dylan-DPC commented Oct 31, 2023

Dylan-DPC commented Mar 15, 2024

thomcc commented May 29, 2023 •

edited

Loading

BoxyUwU May 29, 2023 •

edited

Loading

pnkfelix Aug 30, 2023 •

edited

Loading

BoxyUwU commented May 29, 2023 •

edited

Loading

BoxyUwU commented May 31, 2023 •

edited

Loading