Add a lint for 'static impl shenanigans
#265
Conversation
thomcc
changed the title
'static shenanigans
thomcc
changed the title
'static shenanigans'static impl shenanigans
thomcc
force-pushed
the
thomcc/static_impls
branch
from
f537a97 to
b7b797a
Compare
|
Note that this deliberately doesn't pierce through |
thomcc
force-pushed
the
thomcc/static_impls
branch
from
bd2e9ca to
e801651
Compare
|
Okay, I'm pretty confident this is the right approach (and that trying to solve it by additional analysis passes is more trouble than its worth). |
|
I think the CI failure is just aws flakiness. (CC @BradyBonnette) |
|
Yes. Will rerun those tests when it settles. |
| struct Baz<'a, A>(core::marker::PhantomData<&'a A>); | ||
| trait Quux<A> {} | ||
|
|
||
| impl<T> Foo for &'static T {} |
There was a problem hiding this comment.
All of these are generic because technically this is only (known to be) unsound in a case involving generics. I have not been able to trigger the unsound situation in cases with concrete types.
That said, given that we unload functions in create or replace situations, doing too much of anything with statics isn't exactly ideal, and just because I couldn't repro it doesn't mean it can't be done.
| LL | impl<T> Foo for &'static dyn Quux<T> {} | ||
| | ^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| error: aborting due to 11 previous errors |
There was a problem hiding this comment.
Note that there are 11 impl lines in the test file, and we want each of them to trigger.
| TyKind::Rptr(Lifetime { res: Static, .. }, _) | TyKind::TraitObject(_, Lifetime { res: Static, .. }, _) => true, | ||
| // Need to keep looking. | ||
| TyKind::Rptr(_, MutTy { ty, .. }) |
There was a problem hiding this comment.
"Rptr" is a very confusing way for rustc to say "reference", honestly.
| // This is more complex than the one in the issue, to make sure the `Box`'s | ||
| // lang_item status doesn't bite us. | ||
| impl<T: Display> Displayable for (T, Box<Option<&'static T>>) { |
There was a problem hiding this comment.
This is making me wonder: Box<T> is a lang item, but more than that, it is #[fundamental]. Should we have any special concerns about other #[fundamental] types here?
There was a problem hiding this comment.
Not as far as I know.
There was a problem hiding this comment.
To be more concrete, we are looking somewhat blindly for 'static through almost everything, so fundamental or not, it doesn't make a huge difference.
| let segs = match qpath { | ||
| hir::QPath::Resolved(Some(maybe_ty), _) if self.has_static(maybe_ty) => return true, | ||
| hir::QPath::TypeRelative(t, _) if self.has_static(*t) => return true, | ||
| hir::QPath::LangItem(..) => return false, |
There was a problem hiding this comment.
Or is this why you mentioned the Box<Option<&'static T>> thing?
There was a problem hiding this comment.
Yeah, but I think it only happens if the path refers to a function.
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
This applies on top of #264 because I'm lazy.Edit: not anymoreIt basically works but needs more testing and docs. I've decided to handle it by detecting the
impl ... for <something that contains 'static in here>pattern, which should be pretty rare.