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

Jump to bottom

Pad size of TypeId and remove structural equality #99189

Closed
wants to merge 9 commits into from
Closed

Pad size of TypeId and remove structural equality #99189

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
642b8e1
Pad size of typeid and remove structuraleq
carbotaniuman Jul 12, 2022
61e9395
Actually delete derive
carbotaniuman Jul 12, 2022
0bb7737
Add stability attrs
carbotaniuman Jul 12, 2022
7985d93
Update tests
carbotaniuman Jul 13, 2022
b35362c
Try MaybeUninit
carbotaniuman Jul 21, 2022
34b2cce
Impl traits manually
carbotaniuman Jul 21, 2022
e531a9a
Fullly qualify
carbotaniuman Jul 21, 2022
ee6d19b
Fix
carbotaniuman Jul 21, 2022
53dcb86
Dead code warnings
carbotaniuman Jul 22, 2022
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
16 changes: 14 additions & 2 deletions library/core/src/any.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,12 +665,24 @@ impl dyn Any + Send + Sync {
/// While `TypeId` implements `Hash`, `PartialOrd`, and `Ord`, it is worth
/// noting that the hashes and ordering will vary between Rust releases. Beware
/// of relying on them inside of your code!
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
#[derive(Clone, Copy, PartialOrd, Ord, Debug, Hash)]
#[stable(feature = "rust1", since = "1.0.0")]
pub struct TypeId {
// This field is unused, and is intended solely
// to break invalid transmutes to `TypeId`.
pad: u64,
Copy link
Contributor

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.

t: u64,
}

#[stable(feature = "rust1", since = "1.0.0")]
impl PartialEq for TypeId {
fn eq(&self, other: &Self) -> bool {
self.t == other.t
}
}
#[stable(feature = "rust1", since = "1.0.0")]
impl Eq for TypeId {}

impl TypeId {
/// Returns the `TypeId` of the type this generic function has been
/// instantiated with.
Expand All @@ -691,7 +703,7 @@ impl TypeId {
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_type_id", issue = "77125")]
pub const fn of<T: ?Sized + 'static>() -> TypeId {
TypeId { t: intrinsics::type_id::<T>() }
TypeId { pad: 0, t: intrinsics::type_id::<T>() }
}
}

Expand Down
10 changes: 4 additions & 6 deletions src/test/ui/const-generics/issues/issue-90318.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,9 @@ LL | If<{ TypeId::of::<T>() != TypeId::of::<()>() }>: True,
note: impl defined here, but it is not `const`
--> $SRC_DIR/core/src/any.rs:LL:COL
|
LL | #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
| ^^^^^^^^^
LL | impl PartialEq for TypeId {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
= note: calls in constants are limited to constant functions, tuple structs and tuple variants
= note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)

error: overly complex generic constant
--> $DIR/issue-90318.rs:22:8
Expand All @@ -43,10 +42,9 @@ LL | If<{ TypeId::of::<T>() != TypeId::of::<()>() }>: True,
note: impl defined here, but it is not `const`
--> $SRC_DIR/core/src/any.rs:LL:COL
|
LL | #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
| ^^^^^^^^^
LL | impl PartialEq for TypeId {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
= note: calls in constants are limited to constant functions, tuple structs and tuple variants
= note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 4 previous errors

Expand Down
14 changes: 1 addition & 13 deletions src/test/ui/consts/issue-73976-monomorphic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,9 @@
// will be properly rejected. This test will ensure that monomorphic use of these
// would not be wrongly rejected in patterns.

#![feature(const_type_id)]
#![feature(const_type_name)]

use std::any::{self, TypeId};

pub struct GetTypeId<T>(T);

impl<T: 'static> GetTypeId<T> {
pub const VALUE: TypeId = TypeId::of::<T>();
}

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

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

}
use std::any;

pub struct GetTypeNameLen<T>(T);

Expand All @@ -31,6 +20,5 @@ const fn check_type_name_len<T: 'static>() -> bool {
}

fn main() {
assert!(check_type_id::<usize>());
assert!(check_type_name_len::<usize>());
}