-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add a lint for 'static
impl shenanigans
#265
Changes from 4 commits
b7b797a
e801651
76661f8
7f9920a
1c3ce5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,94 @@ impl<'tcx> LateLintPass<'tcx> for NoExternBlockPass { | |
} | ||
} | ||
|
||
declare_plrust_lint!( | ||
pub(crate) PLRUST_STATIC_IMPLS, | ||
"Disallow impl blocks for types containing `'static` references" | ||
); | ||
|
||
declare_lint_pass!(PlrustStaticImpls => [PLRUST_STATIC_IMPLS]); | ||
|
||
impl<'tcx> LateLintPass<'tcx> for PlrustStaticImpls { | ||
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) { | ||
let hir::ItemKind::Impl(imp) = &item.kind else { | ||
return; | ||
}; | ||
if self.has_static(imp.self_ty) { | ||
cx.lint( | ||
PLRUST_STATIC_IMPLS, | ||
"`impl` blocks for types containing `'static` references are not allowed", | ||
|b| b.set_span(imp.self_ty.span), | ||
) | ||
} | ||
} | ||
} | ||
|
||
impl PlrustStaticImpls { | ||
/// This is pretty naïve and designed to match the specific patterns that | ||
/// trip up https://github.com/rust-lang/rust/issues/104005... | ||
/// | ||
/// Also, I feel like I should be able to use `hir::intravisit::walk_ty` | ||
/// here instead, but it doesn't seem to let be know the lifetime of refs, | ||
/// so... not sure... | ||
/// | ||
/// It's a method on `self` mostly to discourage use in other contexts, | ||
/// since it's probably wrong for them. | ||
fn has_static(&self, t: &hir::Ty) -> bool { | ||
use hir::{Lifetime, LifetimeName::Static, MutTy, TyKind}; | ||
match &t.kind { | ||
TyKind::Infer | ||
| TyKind::Err | ||
// Doesn't exist | ||
| TyKind::Typeof(..) | ||
// Not strictly correct but we forbid this elsewhere anyway. | ||
| TyKind::BareFn(..) | ||
// TAIT stuff, still unstable at the moment, seems very hard to | ||
// prevent this for... | ||
| TyKind::OpaqueDef(..) | ||
| TyKind::Never => false, | ||
// Found one! | ||
TyKind::Rptr(Lifetime { res: Static, .. }, _) | TyKind::TraitObject(_, Lifetime { res: Static, .. }, _) => true, | ||
// Need to keep looking. | ||
TyKind::Rptr(_, MutTy { ty, .. }) | ||
| TyKind::Ptr(MutTy { ty, .. }) | ||
| TyKind::Array(ty, _) | ||
| TyKind::Slice(ty) => self.has_static(*ty), | ||
|
||
TyKind::Tup(types) => types.iter().any(|t| self.has_static(t)), | ||
|
||
TyKind::TraitObject(polytrait, ..) => { | ||
polytrait.iter().any(|poly| { | ||
self.segments_have_static(poly.trait_ref.path.segments) | ||
}) | ||
} | ||
// Something like `Vec<T>` or `Option<T>`. Need to look inside... | ||
TyKind::Path(qpath) => { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or is this why you mentioned the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, but I think it only happens if the path refers to a function. |
||
hir::QPath::Resolved(_, path) => path.segments, | ||
hir::QPath::TypeRelative(_, seg) => std::slice::from_ref(*seg), | ||
}; | ||
self.segments_have_static(segs) | ||
} | ||
} | ||
} | ||
|
||
fn segments_have_static(&self, segs: &[hir::PathSegment]) -> bool { | ||
segs.iter().any(|seg| { | ||
seg.args().args.iter().any(|arg| match arg { | ||
hir::GenericArg::Lifetime(hir::Lifetime { | ||
res: hir::LifetimeName::Static, | ||
.. | ||
}) => true, | ||
hir::GenericArg::Type(t) => self.has_static(t), | ||
_ => false, | ||
thomcc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}) | ||
}) | ||
} | ||
} | ||
|
||
declare_plrust_lint!( | ||
pub(crate) PLRUST_AUTOTRAIT_IMPLS, | ||
"Disallow impls of auto traits", | ||
|
@@ -535,6 +623,7 @@ static PLRUST_LINTS: Lazy<Vec<&'static Lint>> = Lazy::new(|| { | |
let mut v = vec![ | ||
PLRUST_ASYNC, | ||
PLRUST_AUTOTRAIT_IMPLS, | ||
PLRUST_STATIC_IMPLS, | ||
PLRUST_EXTERN_BLOCKS, | ||
PLRUST_EXTERNAL_MOD, | ||
PLRUST_FILESYSTEM_MACROS, | ||
|
@@ -580,6 +669,7 @@ pub fn register(store: &mut LintStore, _sess: &Session) { | |
store.register_early_pass(move || Box::new(PlrustAsync)); | ||
store.register_early_pass(move || Box::new(PlrustExternalMod)); | ||
store.register_late_pass(move |_| Box::new(PlrustAutoTraitImpls)); | ||
store.register_late_pass(move |_| Box::new(PlrustStaticImpls)); | ||
store.register_late_pass(move |_| Box::new(PlrustFnPointer)); | ||
store.register_late_pass(move |_| Box::new(PlrustLeaky)); | ||
store.register_late_pass(move |_| Box::new(PlrustBuiltinMacros)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
#![crate_type = "lib"] | ||
|
||
trait Foo {} | ||
struct Bar<A, B>(core::marker::PhantomData<(A, B)>); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
impl<T> Foo for &'static mut T {} | ||
impl<T> Foo for [&'static T] {} | ||
impl<T> Foo for &[&'static T] {} | ||
impl<T> Foo for (i32, [&'static T]) {} | ||
impl<T> Foo for (i32, [&'static T; 1]) {} | ||
impl<T> Foo for *const &'static T {} | ||
impl<T> Foo for Bar<i32, &'static T> {} | ||
impl<T> Foo for Baz<'static, T> {} | ||
impl<T> Foo for dyn Quux<&'static T> {} | ||
impl<T> Foo for &'static dyn Quux<T> {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:8:17 | ||
| | ||
LL | impl<T> Foo for &'static T {} | ||
| ^^^^^^^^^^ | ||
| | ||
= note: `-F plrust-static-impls` implied by `-F plrust-lints` | ||
|
||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:9:17 | ||
| | ||
LL | impl<T> Foo for &'static mut T {} | ||
| ^^^^^^^^^^^^^^ | ||
|
||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:10:17 | ||
| | ||
LL | impl<T> Foo for [&'static T] {} | ||
| ^^^^^^^^^^^^ | ||
|
||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:11:17 | ||
| | ||
LL | impl<T> Foo for &[&'static T] {} | ||
| ^^^^^^^^^^^^^ | ||
|
||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:12:17 | ||
| | ||
LL | impl<T> Foo for (i32, [&'static T]) {} | ||
| ^^^^^^^^^^^^^^^^^^^ | ||
|
||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:13:17 | ||
| | ||
LL | impl<T> Foo for (i32, [&'static T; 1]) {} | ||
| ^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:14:17 | ||
| | ||
LL | impl<T> Foo for *const &'static T {} | ||
| ^^^^^^^^^^^^^^^^^ | ||
|
||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:15:17 | ||
| | ||
LL | impl<T> Foo for Bar<i32, &'static T> {} | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
|
||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:16:17 | ||
| | ||
LL | impl<T> Foo for Baz<'static, T> {} | ||
| ^^^^^^^^^^^^^^^ | ||
|
||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:17:17 | ||
| | ||
LL | impl<T> Foo for dyn Quux<&'static T> {} | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
|
||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_etc.rs:18:17 | ||
| | ||
LL | impl<T> Foo for &'static dyn Quux<T> {} | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
|
||
error: aborting due to 11 previous errors | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that there are 11 impl lines in the test file, and we want each of them to trigger. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
#![crate_type = "lib"] | ||
|
||
use std::fmt::Display; | ||
|
||
trait Displayable { | ||
fn display(self) -> Box<dyn Display>; | ||
} | ||
|
||
// 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>>) { | ||
Comment on lines
+9
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is making me wonder: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not as far as I know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be more concrete, we are looking somewhat blindly for |
||
fn display(self) -> Box<dyn Display> { | ||
Box::new(self.0) | ||
} | ||
} | ||
|
||
fn extend_lt<T, U>(val: T) -> Box<dyn Display> | ||
where | ||
(T, Box<Option<U>>): Displayable, | ||
{ | ||
Displayable::display((val, Box::new(None))) | ||
} | ||
|
||
pub fn get_garbage(s: &str) -> String { | ||
let val = extend_lt(&String::from(s)); | ||
val.to_string() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
error: `impl` blocks for types containing `'static` references are not allowed | ||
--> $DIR/static_impl_unsound.rs:11:34 | ||
| | ||
LL | impl<T: Display> Displayable for (T, Box<Option<&'static T>>) { | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: `-F plrust-static-impls` implied by `-F plrust-lints` | ||
|
||
error: aborting due to previous error | ||
|
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.
"Rptr" is a very confusing way for rustc to say "reference", honestly.