-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Tracking Issue for const fn
type_id
#77125
Comments
I'm not sure I totally understand the steps needed to re-stabilize
Does this affect the soundness of
This seems impossible. If someone wants to Motivation: It would be extremely useful to make A perhaps related concern (may need a separate tracking issue) is that |
Can this be re-stabilized now?@KodrAus Going through the comment that was the basis of reverting stabilization (#75923 (comment)): Part 1
Sure, but this suggests that retrieving a Part 2
I think @benkay86 summed this up fairly well.
Part 3And finally, @eddyb remarks that we could potentially re-stabilize it if we are unlikely to change it.
It has been nearly 10 months since it was marked unstable and there have been no changes regarding this issue. At this point it seems like the implementation of TL;DRPlus even if we assume all of these points are true, these concerns only apply to If anything, the heavy reliance on
|
Nobody tried AFAIK. 10 months is pretty short on the scale of Rust evolution, especially when you take into account the year 2020. But also I believe it got stalled because of this potential breakage: #75923 (comment) In order to guarantee code that struct TypeId {
hash: u64,
// `str` can be replaced with a length-prefixed string to keep `TypeId`'s size down
v0_mangled_type: &'static str,
}
impl PartialEq for TypeId {
fn eq(&self, other: &TypeId) -> bool {
self.hash == other.hash // fast reject (!=)
&& (
self.v0_mangled_type as *const str == other.v0_mangled_type as *const str // fast accept (==)
|| self.v0_mangled_type == other.v0_mangled_type
)
}
} That way, it's always more than 8 bytes, no matter the target pointer size, and so a As for the linker concerns, if we take this specific approach, the only difference relying on their features would make is increasingly the situations in which the "fast accept ( I could try make a PR for this, but it would likely at least require #87194 (to avoid breaking when unstable
But we don't want that. You can't change |
Maybe it would be better to use this to shrink use core::{ptr, slice};
#[repr(C)]
struct StrWithLen<T: ?Sized> {
len: u16, // assume all TypeId strings are <64kB
text: T,
}
pub struct TypeId {
hash: u64,
text: &'static StrWithLen<()>, // actually StrWithLen<str>
}
impl PartialEq for TypeId {
fn eq(&self, rhs: &Self) -> bool {
if self.hash != rhs.hash {
false
} else if ptr::eq(self.text, rhs.text) {
true
} else {
unsafe {
let lhs_text = slice::from_raw_parts(
&self.text.text as *const _ as *const u8,
self.text.len as usize,
);
let rhs_text = slice::from_raw_parts(
&rhs.text.text as *const _ as *const u8,
rhs.text.len as usize,
);
lhs_text == rhs_text
}
}
}
} |
Yeah, that's what I meant by "length-prefixed". There's a "standard" way to do that using |
Update: finally opened #95845 with a (v0) mangling-based |
blocking the stabilization of this on #97156 |
…4042) # Objective > Resolves #4504 It can be helpful to have access to type information without requiring an instance of that type. Especially for `Reflect`, a lot of the gathered type information is known at compile-time and should not necessarily require an instance. ## Solution Created a dedicated `TypeInfo` enum to store static type information. All types that derive `Reflect` now also implement the newly created `Typed` trait: ```rust pub trait Typed: Reflect { fn type_info() -> &'static TypeInfo; } ``` > Note: This trait was made separate from `Reflect` due to `Sized` restrictions. If you only have access to a `dyn Reflect`, just call `.get_type_info()` on it. This new trait method on `Reflect` should return the same value as if you had called it statically. If all you have is a `TypeId` or type name, you can get the `TypeInfo` directly from the registry using the `TypeRegistry::get_type_info` method (assuming it was registered). ### Usage Below is an example of working with `TypeInfo`. As you can see, we don't have to generate an instance of `MyTupleStruct` in order to get this information. ```rust #[derive(Reflect)] struct MyTupleStruct(usize, i32, MyStruct); let info = MyTupleStruct::type_info(); if let TypeInfo::TupleStruct(info) = info { assert!(info.is::<MyTupleStruct>()); assert_eq!(std::any::type_name::<MyTupleStruct>(), info.type_name()); assert!(info.field_at(1).unwrap().is::<i32>()); } else { panic!("Expected `TypeInfo::TupleStruct`"); } ``` ### Manual Implementations It's not recommended to manually implement `Typed` yourself, but if you must, you can use the `TypeInfoCell` to automatically create and manage the static `TypeInfo`s for you (which is very helpful for blanket/generic impls): ```rust use bevy_reflect::{Reflect, TupleStructInfo, TypeInfo, UnnamedField}; use bevy_reflect::utility::TypeInfoCell; struct Foo<T: Reflect>(T); impl<T: Reflect> Typed for Foo<T> { fn type_info() -> &'static TypeInfo { static CELL: TypeInfoCell = TypeInfoCell::generic(); CELL.get_or_insert::<Self, _>(|| { let fields = [UnnamedField::new::<T>()]; let info = TupleStructInfo::new::<Self>(&fields); TypeInfo::TupleStruct(info) }) } } ``` ## Benefits One major benefit is that this opens the door to other serialization methods. Since we can get all the type info at compile time, we can know how to properly deserialize something like: ```rust #[derive(Reflect)] struct MyType { foo: usize, bar: Vec<String> } // RON to be deserialized: ( type: "my_crate::MyType", // <- We now know how to deserialize the rest of this object value: { // "foo" is a value type matching "usize" "foo": 123, // "bar" is a list type matching "Vec<String>" with item type "String" "bar": ["a", "b", "c"] } ) ``` Not only is this more compact, but it has better compatibility (we can change the type of `"foo"` to `i32` without having to update our serialized data). Of course, serialization/deserialization strategies like this may need to be discussed and fully considered before possibly making a change. However, we will be better equipped to do that now that we can access type information right from the registry. ## Discussion Some items to discuss: 1. Duplication. There's a bit of overlap with the existing traits/structs since they require an instance of the type while the type info structs do not (for example, `Struct::field_at(&self, index: usize)` and `StructInfo::field_at(&self, index: usize)`, though only `StructInfo` is accessible without an instance object). Is this okay, or do we want to handle it in another way? 2. Should `TypeInfo::Dynamic` be removed? Since the dynamic types don't have type information available at runtime, we could consider them `TypeInfo::Value`s (or just even just `TypeInfo::Struct`). The intention with `TypeInfo::Dynamic` was to keep the distinction from these dynamic types and actual structs/values since users might incorrectly believe the methods of the dynamic type's info struct would map to some contained data (which isn't possible statically). 4. General usefulness of this change, including missing/unnecessary parts. 5. Possible changes to the scene format? (One possible issue with changing it like in the example above might be that we'd have to be careful when handling generic or trait object types.) ## Compile Tests I ran a few tests to compare compile times (as suggested [here](#4042 (comment))). I toggled `Reflect` and `FromReflect` derive macros using `cfg_attr` for both this PR (aa5178e) and main (c309acd). <details> <summary>See More</summary> The test project included 250 of the following structs (as well as a few other structs): ```rust #[derive(Default)] #[cfg_attr(feature = "reflect", derive(Reflect))] #[cfg_attr(feature = "from_reflect", derive(FromReflect))] pub struct Big001 { inventory: Inventory, foo: usize, bar: String, baz: ItemDescriptor, items: [Item; 20], hello: Option<String>, world: HashMap<i32, String>, okay: (isize, usize, /* wesize */), nope: ((String, String), (f32, f32)), blah: Cow<'static, str>, } ``` > I don't know if the compiler can optimize all these duplicate structs away, but I think it's fine either way. We're comparing times, not finding the absolute worst-case time. I only ran each build 3 times using `cargo build --timings` (thank you @devil-ira), each of which were preceeded by a `cargo clean --package bevy_reflect_compile_test`. Here are the times I got: | Test | Test 1 | Test 2 | Test 3 | Average | | -------------------------------- | ------ | ------ | ------ | ------- | | Main | 1.7s | 3.1s | 1.9s | 2.33s | | Main + `Reflect` | 8.3s | 8.6s | 8.1s | 8.33s | | Main + `Reflect` + `FromReflect` | 11.6s | 11.8s | 13.8s | 12.4s | | PR | 3.5s | 1.8s | 1.9s | 2.4s | | PR + `Reflect` | 9.2s | 8.8s | 9.3s | 9.1s | | PR + `Reflect` + `FromReflect` | 12.9s | 12.3s | 12.5s | 12.56s | </details> --- ## Future Work Even though everything could probably be made `const`, we unfortunately can't. This is because `TypeId::of::<T>()` is not yet `const` (see rust-lang/rust#77125). When it does get stabilized, it would probably be worth coming back and making things `const`. Co-authored-by: MrGVSV <[email protected]>
Previous versions of log used an alpha version of value-bag, which broke on the new nightly rustc: rust-lang/rust#77125
Previous versions of log used an alpha version of value-bag, which broke on the new nightly rustc: rust-lang/rust#77125
I think #10389 needs to be resolved first, becuase depending on the resolution, it may not be possible to calculate the TypeId at compile time; it might end up like C++ |
We can compute and equate type ids that are addresses just fine at compile time. You just can't inspect the address (so no sorting type ids, no hashing them, no printing them) |
This was recently marked fixed with #118247 Edit: started a discussion https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Const.20.60TypeId.3A.3Aof.60.2C.20.60TypeId.3A.3Amatches.60.2C.20.60type_name.60.2C Edit2: From the discussion, this is still blocked by the possibility of collision #10389 |
Any further updates on this ? |
Still blocked on #10389. |
It looks like while #10389 was closed, there's still some ongoing discussion about the soundness/collision of type ids:
Is there anything else blocking? There's still something that isn't clear to me:
I understand why #101871 might be blocked due to collision issues but why this one? Ensuring |
The problem is that the scheme we end up using might involve pointers, but currently TypeId contains no pointers. If people transmute TypeId to integers in const, their code would stop compiling when we start having pointers in TypeId. |
Is there any reason for allowing such casts to begin with? Skimming the docs, I see no mention of |
That's already the case, but it does not stop unsafe code from doing it anyway. We could say "if you do that and your build breaks later, it's your own fault", but well, that often just doesn't work -- people will have their build broken on a Rust update because a dependency did something like this, and they will blame Rust, not the dependency. |
I’m also worried about the interaction with |
Well, if it really is undefined behaviour, then I see no reason as to why it should be guaranteed to compile. I get the point that it currently does work (in practice, I guess), but whether it breaks at run-time, compile-time, or at all, is just an implementation detail IMHO. |
Thanks RalfJung, that is an interesting example. I didn't know about the TypeId layout changes and about it being possible to use transmute in const! (I can't even find a tracking issue for const_transmute.)
That's kind of the point of stability though, to avoid your code breaking between rust updates. I understand the desire to hold off. |
If we have reached a point where definitively incorrect code is considered worthwhile enough to cater the language for and where rejecting such code is considered a breaking change, then we may as well no longer call it "undefined" behaviour if a specific outcome can always be safely relied upon. |
That's a tiny bit myopic. Sure, such code does exist… but why does it exist? It's not as if people write it to make life of Rust language developers harder. The majority of people, when they reach out for the And AFAICS currently the only way to achieve that is via “transmute to integers”. You may complain about the people's desire to break the rules… but if something they very much want to do is to break the rules… they would break them anyway. If I understand correctly the code in question, that @RalfJung fears to break is still nightly-only code and it's perfectly fine to break it… but this wouldn't make people happy if it would be broken with no suitable message – and that suitable message should, ideally, point to some supported-in-the-future way of doing the same thing. So what's the solution for the most natural, obvious and desired feature of “ |
Yeah it's the usual issue in a language with unsafe: if we don't implement everything everyone wants and ship it yesterday, people will reach for broken unsafe code that has UB but sometimes happens to generate the assembly people were hoping for... and then of course we get complaints if/when that UB eventually comes back to bite people. I understand people just want to solve the problem in front of them, but putting such a hack into the wild does have larger ecosystem-wide costs. Language design takes time, and this particular issue is unfortunately tied up with some long-standing non-trivial discussions. |
Can we deliberately break this some way? E.g. by encoding the ints as pointers (and not telling const eval that they're really just integers)? |
I can't think of a good way to do that. We'd need something like a magic |
Alternatively, the primary use case for The only problem I can see is if, in the future, the compiler can't prove two types are equal at compile time... but the compiler should always be able to do this, right? |
Sure, but the only sane way forward is to do what was done with provenance: give users what they need or want in a safe way and then try to pressure them to stop using dirty hacks. Without some way to do what they want to do they would find another unsafe and bug-ridden way… and then we would end up in the situation discussed.
Such function is the only way forward IMO. I mean: what can you actually do to a One may argue if that would be enough or if we also need hashing and some ordering… but without being able to even compare two You can't exactly print them in a |
We have one option here that we did not have with provenance: don't let people use the problematic operation and type until we are confident in what we have. |
This is a tracking issue for using
TypeId::of
in constant contexts.The feature gate for the issue is
#![feature(const_type_id)]
.About tracking issues
Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.
Steps
Unresolved Questions
Implementation history
type_id
into a constant intrinsic #47892The text was updated successfully, but these errors were encountered: