-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Remove 'static requirement on try_as_dyn #150161
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
base: main
Are you sure you want to change the base?
Conversation
|
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
This comment has been minimized.
This comment has been minimized.
01327be to
e7ef1ee
Compare
e7ef1ee to
29f1dba
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.
Some drive-by comments; but I'm not rustc/HIR-savy, so take these with a grain of salt 🙇
| } | ||
| LifetimeKind::ImplicitObjectLifetimeDefault | ||
| | LifetimeKind::Error | ||
| | LifetimeKind::Infer |
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.
Does this refer to '_ and/or "hidden behind &[mut]" lifetimes? I think those are analogous to unique, non-nameable-in-bounds, lifetimes, so this ought to be fine?
| // Test that downcasting to a dyn trait works as expected | ||
| const _: () = { | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&&42_i32).is_none()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&()).is_some()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&&[()]).is_some()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&&()).is_some()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&&42_u32).is_none()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&(&(), &())).is_none()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&(&(), &(), ())).is_none()); | ||
|
|
||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&Struct(&42_u32)).is_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.
Nit: it would be a bit more readable to have the const { assert! next to the impl in question, or to involve some macro to hide the outermost &; I feel like it's a tad harder to read which type is being tested exactly. But maybe it's just me 😅
| // Only valid for 'static lifetimes -> returns None | ||
| impl Trait for &'static u32 {} |
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.
Wait, does that mean that before this PR, (x: &'static u32).try_as_dyn::<dyn Trait>() would work, but with this, it won't anymore?
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.
jup 💀 Maybe that's fixable by running it twice, once if the type is 'static and once without, but idk what the best thing here is for now
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&()).is_some()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&&[()]).is_some()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&&()).is_some()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&&42_u32).is_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.
This one is a "regression", right? With the old design we could be involving a &'static u32 which is both : 'static, and implementing Trait in the classical sense. Perhaps we'll want a try_as_dyn_static() function as well, for those interested in this use case?
| if ty_lifetimes.visit_ty_unambig(self.self_ty).is_break() { | ||
| return false; |
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 think this function also needs to test for unicity of type params, lest impl<T> Trait for (T, T) {} be green-lit, which when intersected over the &u32 family of types, is just (&'t u32, &'t u32) impl in disguise.
-
If anything, do add that test case:
const _: () = { trait Homo {} impl<T> Homo for (T, T) {} // Let's pick `T = &'_ i32`. assert!(std::any::try_as_dyn::<_, dyn Homo>(&(&42_i32, &27_i32)).is_none()); };
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&&42_i32).is_none()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&()).is_some()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&&[()]).is_some()); | ||
| assert!(std::any::try_as_dyn::<_, dyn Trait>(&&()).is_some()); |
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'd expect this line to fail atm since you Break on LifetimeKind::Infer 🤔
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.
infer lifetimes are a different thing. unnamed lifetimes just get an empty name but a unique DefId.
| } | ||
| impl<'v> Visitor<'v> for LifetimeFinder { | ||
| type Result = ControlFlow<()>; | ||
| fn visit_lifetime(&mut self, lifetime: &'v Lifetime) -> Self::Result { |
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.
It would be nice in this function to stop tracking for<'any>-kind of lifetimes: those are part of the monomorphised info of the type, are they not? But this might require setting up a separate set of known-for<>-quantified lifetimes that ought to be ignored rather than seen-checked.
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.
yea, I decided to just ignore those for now (sound but pessimistic again), but should def document that
| if let TypingMode::Reflection = ecx.typing_mode() | ||
| && !cx.is_fully_generic_for_reflection(impl_def_id) | ||
| { | ||
| return Err(NoSolution); |
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 way over my head, lol, but just to sanity-check: could this branch be hoisted to the beginning of the function, as in, any impl that is not fully_generic_for_reflection(), when checking in Reflection mode, ought to result in Err(NoSolution), right? As in, it's not specific to this double-Negative branch?
|
r? BoxyUwU |
tracking issue: #144361
cc @theemathas can you find an unsoundness when combined with generic impls where there are no lifetimes, but reference other impls that have lifetimes with constraints? I couldn't find anything.
cc @ivarflakstad @izagawd