Add a lint for 'static impl shenanigans (#265)

* wip: lint for impls of 'static things
* Add lint for impl blocks containing `'static` things
* Add more tests
* Lint the span of the problematic type rather than the whole impl block

* exhaustively match GenericArg
Co-authored-by: Jubilee <[email protected]>

doc/src/config-lints.md

@@ -223,6 +223,23 @@ std::io::stderr().write_all(b"foobar").unwrap();
 let _stdin_is_forbidden_too = std::io::stdin();
 ```
 
+### `plrust_static_impls`
+
+This lint forbids certain `impl` blocks for types containing `&'static` references. The precise details are somewhat obscure, but can usually be avoided by making a custom struct to contain your static reference, which avoids the particular soundness hole we're concerned with. For example:
+
+```rust
+// This is forbidden:
+impl SomeTrait for (&'static Foo, Bar) {
+    // ...
+}
+
+// Instead, do this:
+struct MyType(&'static Foo, Bar);
+impl SomeTrait for MyType {
+    // ...
+}
+```
+
 ### `plrust_autotrait_impls`
 
 This lint forbids explicit implementations of the safe auto traits, as a workaround for various soundness holes around these. It may be relaxed in the future if those are fixed.

plrust/src/lib.rs

@@ -70,6 +70,7 @@ const DEFAULT_LINTS: &'static str = "\
     plrust_lifetime_parameterized_traits, \
     implied_bounds_entailment, \
     plrust_autotrait_impls, \
+    plrust_static_impls, \
     plrust_fn_pointers, \
     plrust_filesystem_macros, \
     plrust_env_macros, \

plrustc/plrustc/src/lints.rs

@@ -62,6 +62,96 @@ 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,
+                    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),
+                hir::GenericArg::Const(_) | hir::GenericArg::Infer(_)
+                // Wasn't static
+                | hir::GenericArg::Lifetime(_) => false,
+            })
+        })
+    }
+}
+
 declare_plrust_lint!(
     pub(crate) PLRUST_AUTOTRAIT_IMPLS,
     "Disallow impls of auto traits",
@@ -535,6 +625,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 +671,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));

plrustc/plrustc/uitests/static_impl_etc.rs

@@ -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 {}
+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> {}

plrustc/plrustc/uitests/static_impl_etc.stderr

@@ -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
+

plrustc/plrustc/uitests/static_impl_unsound.rs

@@ -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>>) {
+    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()
+}

plrustc/plrustc/uitests/static_impl_unsound.stderr

@@ -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
+