-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Support trait objects in type info reflection #151239
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
Support trait objects in type info reflection #151239
Conversation
|
|
This comment was marked as resolved.
This comment was marked as resolved.
| Type { | ||
| kind: DynTrait( | ||
| DynTrait { | ||
| super_traits: [ | ||
| TypeId(0xf726af39bcd0090512f636802780d009), | ||
| TypeId(0xd3eba1307d3a0b58acd77b80e4532fbf), | ||
| ], | ||
| is_auto: false, | ||
| auto_traits: [ | ||
| TypeId(0x0d5e48167084e668b711d10061f0446a), | ||
| ], | ||
| }, | ||
| ), | ||
| size: None, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trait A {}
trait B: A {}
trait C: B {}The quoted dump is the type info for dyn C + Send. How do we determine whether it has trait C?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't think I fully understand the question. Could you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clear, I replaced the type ID number with type name in the following examples.
First, TypeId(dyn C) != TypeId(dyn C + Send), then the type info of dyn C + Send is
Type {
kind: DynTrait(
DynTrait {
super_traits: [
TypeId(B),
TypeId(A),
],
is_auto: false,
auto_traits: [
TypeId(Send),
],
},
),
size: None,
}
For the current PR implementation, if given a dyn C + Send (or type ID of it), we can know there are A, B and Send but no way of knowing C.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not include C in it since C is not a super trait of itself. I am assuming you want a field that represents the TypeId of the trait object itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly, dyn C and dyn C + Send are two different things, so it's not "the trait object itself".
Intuitively, I think DynTrait's layout should probably look something like this. Take dyn C + Send as an example again.
Type {
kind: DynTrait(
DynTrait {
predicates: [ Predicate(Trait(C)), Predicate(Trait(Send)) ]
},
),
size: None,
}
Predicate {
trait,
negative: bool, // Not sure if possible, #144241
}
Trait(C) = Trait {
supers: [ Trait(B), Trait(A) ],
is_auto: false,
}
Trait(Send) = Trait {
supers: [],
is_auto: true,
}
Trait(B) = ...
Trait(A) = ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes a lot of sense! I will go with this
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
eb13b48 to
f782c2f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a7df6d9 to
b5eecb0
Compare
This comment has been minimized.
This comment has been minimized.
Yea. BTW, I'm adding generics info for ADT types in PR #151142. Once your PR is merged, I can also add generics info to traits later. |
This comment has been minimized.
This comment has been minimized.
be15746 to
e8b0f6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, waiting for further review by @oli-obk. (I don't have r+ permission)
| #[derive(Debug)] | ||
| #[non_exhaustive] | ||
| #[unstable(feature = "type_info", issue = "146922")] | ||
| pub struct DynTrait { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very much non-blocking, but I would prefer this be named TraitObject, as that is how they are presented in the book and the reference. Definitely something that should be done in a follow-up PR, as I don't want to hold up merging this due to bikeshedding :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nomenclature has moved away from trait object, looks like we have more places to update
e8b0f6c to
714dc35
Compare
This comment has been minimized.
This comment has been minimized.
|
What else do I have to do to get this PR merged @oli-obk ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Reminder, once the PR becomes ready for a review, use |
714dc35 to
afe3fc1
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
e7e72a1 to
f4125af
Compare
d3842ef to
a1893d3
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors r=oli-obk |
Rollup of 9 pull requests Successful merges: - #151239 (Support trait objects in type info reflection) - #150893 (offload: move (un)register lib into global_ctors) - #151013 (Add some clarifications and fixes for fmt syntax) - #151666 (compiler: Rename several types/traits for per-query vtables) - #151738 (Add `extern crate core` to diagnostic tests) - #151747 (Update `askama` version to `0.15.2`) - #151759 (Update `browser-ui-test` version to `0.23.3`) - #151763 (Add FileCheck annotations to simplify_match.rs) - #151766 (Fix `x fix`, again)
Rollup merge of #151239 - izagawd:comptime-reflection-dyn-trait-variant, r=oli-obk Support trait objects in type info reflection Tracking issue: #146922 Adds type_info support for trait object types by introducing a DynTrait variant ~~I can't seem to get it to work correctly with `dyn for<'a> Foo<'a>`, though it works fine for normal `dyn Foo` trait objects~~ r? @oli-obk
Tracking issue: #146922
Adds type_info support for trait object types by introducing a DynTrait variant
I can't seem to get it to work correctly withdyn for<'a> Foo<'a>, though it works fine for normaldyn Footrait objectsr? @oli-obk