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.

Sign up for GitHub

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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions library/core/src/hash/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>() {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@BoxyUwU BoxyUwU May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@pnkfelix pnkfelix Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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

// SAFETY: This is either `usize` or `DynMetadata`. In both case doing transmute
// is okay.
// We can't hash `DynMetadata` directly so transmute it to usize before hashing.
let metadata_as_usize = unsafe { mem::transmute_copy(&metadata) };
state.write_usize(metadata_as_usize);
}
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> Hash for *mut T {
#[inline]
fn hash<H: Hasher>(&self, state: &mut H) {
let (address, metadata) = self.to_raw_parts();
state.write_usize(address.addr());
metadata.hash(state);
self.cast_const().hash(state);
}
}
}
33 changes: 1 addition & 32 deletions library/core/src/ptr/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#![unstable(feature = "ptr_metadata", issue = "81513")]

use crate::fmt;
use crate::hash::{Hash, Hasher};

/// Provides the pointer metadata type of any pointed-to type.
///
Expand Down Expand Up @@ -57,7 +56,7 @@ pub trait Pointee {
// NOTE: Keep trait bounds in `static_assert_expected_bounds_for_metadata`
// in `library/core/src/ptr/metadata.rs`
// in sync with those here:
type Metadata: Copy + Send + Sync + Ord + Hash + Unpin;
type Metadata: Copy + Send + Sync + Unpin;
}

/// Pointers to types implementing this trait alias are “thin”.
Expand Down Expand Up @@ -241,33 +240,3 @@ impl<Dyn: ?Sized> Clone for DynMetadata<Dyn> {
*self
}
}

impl<Dyn: ?Sized> Eq for DynMetadata<Dyn> {}

impl<Dyn: ?Sized> PartialEq for DynMetadata<Dyn> {
#[inline]
fn eq(&self, other: &Self) -> bool {
crate::ptr::eq::<VTable>(self.vtable_ptr, other.vtable_ptr)
}
}

impl<Dyn: ?Sized> Ord for DynMetadata<Dyn> {
#[inline]
fn cmp(&self, other: &Self) -> crate::cmp::Ordering {
(self.vtable_ptr as *const VTable).cmp(&(other.vtable_ptr as *const VTable))
}
}

impl<Dyn: ?Sized> PartialOrd for DynMetadata<Dyn> {
#[inline]
fn partial_cmp(&self, other: &Self) -> Option<crate::cmp::Ordering> {
Some(self.cmp(other))
}
}

impl<Dyn: ?Sized> Hash for DynMetadata<Dyn> {
#[inline]
fn hash<H: Hasher>(&self, hasher: &mut H) {
crate::ptr::hash::<VTable, _>(self.vtable_ptr, hasher)
}
}
12 changes: 6 additions & 6 deletions library/core/tests/ptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,17 +801,17 @@ fn ptr_metadata() {

#[test]
fn ptr_metadata_bounds() {
fn metadata_eq_method_address<T: ?Sized>() -> usize {
// The `Metadata` associated type has an `Ord` bound, so this is valid:
<<T as Pointee>::Metadata as PartialEq>::eq as usize
fn metadata_clone_method_address<T: ?Sized>() -> usize {
// The `Metadata` associated type has an `Copy` bound, so this is valid:
<<T as Pointee>::Metadata as Clone>::clone as usize
}
// "Synthetic" trait impls generated by the compiler like those of `Pointee`
// are not checked for bounds of associated type.
// So with a buggy core we could have both:
// * `<dyn Display as Pointee>::Metadata == DynMetadata`
// * `DynMetadata: !PartialEq`
// * `DynMetadata: !Clone`
// … and cause an ICE here:
metadata_eq_method_address::<dyn Display>();
metadata_clone_method_address::<dyn Display>();

// For this reason, let’s check here that bounds are satisfied:

Expand All @@ -825,7 +825,7 @@ fn ptr_metadata_bounds() {
fn static_assert_expected_bounds_for_metadata<Meta>()
where
// Keep this in sync with the associated type in `library/core/src/ptr/metadata.rs`
Meta: Copy + Send + Sync + Ord + std::hash::Hash + Unpin,
Meta: Copy + Send + Sync + Unpin,
{
}
}
Expand Down