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.

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

RFC: TypeInfo #2738

Open
wants to merge 8 commits into
base: master
from

Conversation

@leo60228
Copy link

commented Aug 2, 2019

Add a new unsafe trait TypeInfo to core::any, and implement it for all types. This has a method type_id with the signature fn type_id(&self) -> TypeId where Self: 'static, as well as a method type_name with the signature fn type_name(&self) -> &'static str. Traits wanting to support downcasting can add it as a supertrait, without breaking backwards compatibility.

This was originally written to enable the functionality of Error::type_id to be soundly utilized in stable Rust. However, this is a more impactful change, since it would possibly remove an existing, albeit unstable, method on a widely-used trait.

Pre-RFC

Rendered

@leo60228 leo60228 force-pushed the leo60228:master branch from af4e830 to 8b307a1 Aug 2, 2019

@crlf0710

This comment has been minimized.

Copy link

commented Aug 3, 2019

I think the idea itself is fine, but it's actually pushing Any another step towards deprecation, since Any is not useful anyway, and even less useful after this is added.

cc #2280

@crlf0710

This comment has been minimized.

Copy link

commented Aug 3, 2019

And I think this trait can be named "TypeInfo", and can have another method type_name which calls std::any::type_name.

text/0000-knowntypeid.md Outdated Show resolved Hide resolved

@leo60228 leo60228 changed the title RFC: KnownTypeId RFC: TypeInfo Aug 3, 2019

@leo60228

This comment has been minimized.

Copy link
Author

commented Aug 3, 2019

I've added the two suggestions from @KrishnaSannasi ("We could say that the blanket impl is for convenience and the unsafe marking makes it safe even in the presense of specialization (whatever form that takes)") and @crlf0710 ("And I think this trait can be named "TypeInfo", and can have another method type_name which calls std::any::type_name.")

@KrishnaSannasi

This comment has been minimized.

Copy link

commented Aug 3, 2019

@leo60228 can you update the pr description to include type_name.

Also, we could make the is method safer by also checking type_name for equality. This way even if there are hash collisions the type_name will likely be different, so it will be harder to transmute types.

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

@KrishnaSannasi type_name exists purely for debugging purposes and shouldn't be relied upon for correctness.

@KrishnaSannasi

This comment has been minimized.

Copy link

commented Aug 3, 2019

@Centril I wasn't proposing that it be the only way to check for correctness, just another measure we could take in addition to using the typeid hash. Of course this could also be folded into TypeId itself, but I thought it would be good to point this out.

This only relies on the fact that the same type will always give the same type name, so this should be fine (same as typeid, the same type gives the same hash).

@crlf0710

This comment has been minimized.

Copy link

commented Aug 3, 2019

Personally, actually I think it's compiler responsibility to detect TypeId hash collisions at compile time, and replace TypeId for either of the collision types to a hash value that's not used. And if every possible hash value is used, i think a compile error should be raised at compile time.

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

@KrishnaSannasi That seems like a hack for dealing with the soundness hole in rust-lang/rust#10389. Preferably we'd fix the problem at its root instead of papering over it.

@programmerjake

This comment has been minimized.

Copy link

commented Aug 4, 2019

maybe there should be a note about how including TypeInfo as a super-trait makes type_id visible to those who don't use std::any::TypeInfo

@clarfon

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

I'm extremely confused why this type is different from Any. And also, confused why Self: 'static is required to get a TypeId but not to get a type name as a string.

Honestly, as someone unfamiliar to how the Any magic works, this seems just… more magical. And I feel like the RFC needs to do a way better job to explain this.

What's to stop a user who wants downcasting to Just Work from using the wrong one of the two and causing loads of problems down the line? I don't know how this is affected by this RFC.

My gut says that TypeInfo should be called UnsafeAny and be a supertrait of Any. But this is only because I don't know what the difference between these traits is beyond the fact that Any doesn't always work… which is confusing.

@KrishnaSannasi

This comment has been minimized.

Copy link

commented Aug 5, 2019

TypeInfo isn't all that different from Any, it just moves the Self: 'static bound to where it is actually needed instead of forcing it on the entire trait. So from a safety perspective, nothing has changed, and it doesn't matter which one you use. But this allows other traits, like Error to safely use downcasting.
type_name is just debug info.

@clarfon

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Then why not just give this treatment to Any instead? Wouldn't relaxing the constraints not be a breaking change?

@KrishnaSannasi

This comment has been minimized.

Copy link

commented Aug 6, 2019

It would be a breaking change, any code that relies on T: Any also implying T: 'static. For example,

fn foo<T: Any>(t: T) -> Box<dyn Any> {
    Box::new(t)
}

Because Box<dyn Any> means Box<dyn Any + 'static>, so T must be 'static. If we remove the 'static bound on Any it will break any code that relies on this sort of behavior.

@bill-myers

This comment has been minimized.

Copy link

commented Aug 9, 2019

Why is the 'static bound needed at all?

Why not remove it and give a type id for the lifetime-erased version of the type, and put the 'static bound on the downcasting functions instead?

@KrishnaSannasi

This comment has been minimized.

Copy link

commented Aug 9, 2019

It is needed to prevent transmuting lifetimes, this is because lifetimes cannot affect code-gen. So all references to a type T have the same TypeId (even if they have different lifetimes). This means that you could put in one lifetime and get out a different lifetime in completely safe code. To prevent this, we only allow 1 lifetime, 'static. We don't want to give the illusion that you can even possibly use TypeId for non-'static types.

edit: Based on what @RalfJung posted below, 'static is more of a conservative guarantee that lifetimes don't play a role. In most cases lifetimes don't affect code-gen, but sometimes they do. But we can't distinguish between them in the type-system.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

To prevent this, we only allow 1 lifetime, 'static.

But Rust also has higher-ranked lifetime quantification, so you can introduce new lifetimes... I tried to break this but failed:

use std::any::TypeId;

type T1 = for<'a> fn(&'a i32) -> &'a i32;
type T2 = for<'a> fn(&'a i32) -> &'static i32;

fn main() {
    // somehow it can distinguish these even though they only differ in their lifetime
    assert_ne!(TypeId::of::<T1>(), TypeId::of::<T2>());
}

Do we have tests ensuring this?^^

Centril added a commit to Centril/rust that referenced this pull request Aug 9, 2019

Rollup merge of rust-lang#63419 - RalfJung:typeid, r=alexcrichton
check against more collisions for TypeId of fn pointer

Cc rust-lang/rfcs#2738 (comment)

Centril added a commit to Centril/rust that referenced this pull request Aug 10, 2019

Rollup merge of rust-lang#63419 - RalfJung:typeid, r=alexcrichton
check against more collisions for TypeId of fn pointer

Cc rust-lang/rfcs#2738 (comment)

Centril added a commit to Centril/rust that referenced this pull request Aug 10, 2019

Rollup merge of rust-lang#63419 - RalfJung:typeid, r=alexcrichton
check against more collisions for TypeId of fn pointer

Cc rust-lang/rfcs#2738 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.