Add a lint for closure trait impls (#346)

Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>

doc/src/config-lints.md

@@ -284,3 +284,29 @@ impl AsRef<str> for my::Foo {
     }
 }
 ```
+
+### `plrust_closure_trait_impl`
+
+This lint forbids trait impls over generic over `Fn`, `FnOnce`, `FnMut` or
+`FnPtr` types. This is to work around some soundness issues where closure types
+are incorrectly `'static`. For example, the following is forbidden:
+
+```rs
+trait Trait {}
+// This is generic over a function trait.
+impl<F: Fn()> Trait for F {}
+```
+
+However, this is currently overly strict. In the future, it may be relaxed to
+forbid only the case where the return type is projected into an associated item
+on the trait, as in:
+
+```rs
+trait Trait {
+    type Assoc;
+}
+impl<R, F: Fn() -> R> Trait for F {
+    // This is the problem
+    type Assoc = R;
+}
+```

plrust/src/lib.rs

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

plrustc/plrustc/src/lints/closure_trait_impl.rs

@@ -0,0 +1,93 @@
+//! Trait objects are [often inferred as 'static][trait-obj-default],
+//! which can result, when combined with things like boxed closures,
+//! in an incorrect lifetime also being inferred for the return type.
+//! Allowing such a lifetime to become visible in Rust allows treating
+//! `&'a mut str` as `&'static mut str`, with the "fun" that implies.
+//!
+//! Ideally we'd do this in such a way that it only forbids something that
+//! allows projection through to the return type, for example
+//! ```ignore (exposition-only)
+//! trait Trait {
+//!     type Assoc;
+//! }
+//! impl<R, F: Fn() -> R> Trait for F {
+//!     type Assoc = R;
+//! }
+//! ```
+//! allows `<_ as Trait>::Assoc` to get a functions return type. That said,
+//! actually writing this lint has totally defeated me at the moment, so this is
+//! good enough for now.
+//!
+//! For some intuition as to why this is tricky, consider cases like
+//! ```ignore (exposition-only)
+//! trait GivesAssoc<A> { type Assoc; }
+//! impl<A> GivesAssoc<A> for A { type Assoc = A; }
+//!
+//! trait Child<T> where Self: GivesAssoc<T> {}
+//! impl<R, F: Fn() -> R> Child<R> for F {}
+//! ```
+//! and similarly complicated variants. To figure this out you need to examine
+//! not just the directly implemented trait, but also all traits that are
+//! indirectly implemented via bounds as a result of the impl. This is possible,
+//! but... difficult.
+//!
+//! [trait-obj-default]: https://doc.rust-lang.org/reference/lifetime-elision.html#default-trait-object-lifetimes
+//!
+use rustc_hir as hir;
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+
+declare_plrust_lint!(
+    pub(crate) PLRUST_CLOSURE_TRAIT_IMPL,
+    "Disallow trait impls which are generic over closure type",
+);
+
+rustc_lint_defs::declare_lint_pass!(PlrustClosureTraitImpl => [PLRUST_CLOSURE_TRAIT_IMPL]);
+
+impl<'tcx> LateLintPass<'tcx> for PlrustClosureTraitImpl {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
+        let hir::ItemKind::Impl(impl_item) = item.kind else {
+            return
+        };
+        for pred in impl_item.generics.predicates {
+            let hir::WherePredicate::BoundPredicate(bound_pred) = pred else {
+                continue
+            };
+            // TODO: should we ignore cases where `bound_pred.bounded_ty` isn't
+            // from one of `item.generics.params`?
+            for bound in bound_pred.bounds {
+                match bound {
+                    hir::GenericBound::LangItemTrait(
+                        hir::LangItem::Fn
+                        | hir::LangItem::FnOnce
+                        | hir::LangItem::FnMut
+                        | hir::LangItem::FnPtrTrait,
+                        ..,
+                    ) => {
+                        cx.lint(
+                            PLRUST_CLOSURE_TRAIT_IMPL,
+                            "trait impls bounded on function traits are forbidden in PL/Rust",
+                            |b| b.set_span(bound_pred.span),
+                        );
+                    }
+                    hir::GenericBound::LangItemTrait(..) => {
+                        //  other stable builtin traits aren't useful for projecting a function's return type
+                    }
+                    hir::GenericBound::Trait(poly_trait, ..) => {
+                        if super::utils::has_fn_trait(cx, poly_trait) {
+                            cx.lint(
+                                PLRUST_CLOSURE_TRAIT_IMPL,
+                                "trait impls bounded on function traits are forbidden in PL/Rust",
+                                |b| b.set_span(bound_pred.span),
+                            );
+                        }
+                        // TODO: if that fails, do we need to
+                        // try_normalize_erasing_regions and retry?
+                    }
+                    hir::GenericBound::Outlives(..) => {
+                        // Lifetime generics are irrelevant for guards against type projection
+                    }
+                }
+            }
+        }
+    }
+}

plrustc/plrustc/src/lints/fn_ptr.rs

@@ -22,22 +22,13 @@ impl<'tcx> LateLintPass<'tcx> for PlrustFnPointer {
                 );
             }
             hir::TyKind::TraitObject(traits, ..) => {
-                for trayt in *traits {
-                    if let Some(did) = trayt.trait_ref.path.res.opt_def_id() {
-                        let fn_traits = [
-                            &["core", "ops", "function", "Fn"],
-                            &["core", "ops", "function", "FnMut"],
-                            &["core", "ops", "function", "FnOnce"],
-                        ];
-                        for fn_trait_paths in fn_traits {
-                            if super::utils::match_def_path(cx, did, fn_trait_paths) {
-                                cx.lint(
-                                    PLRUST_FN_POINTERS,
-                                    "Use of function trait objects is forbidden in PL/Rust",
-                                    |b| b.set_span(ty.span),
-                                );
-                            }
-                        }
+                for poly_trait in *traits {
+                    if super::utils::has_fn_trait(cx, poly_trait) {
+                        cx.lint(
+                            PLRUST_FN_POINTERS,
+                            "Use of function trait objects is forbidden in PL/Rust",
+                            |b| b.set_span(ty.span),
+                        );
                     }
                 }
             }

plrustc/plrustc/src/lints/mod.rs

@@ -9,6 +9,7 @@ mod utils;
 mod async_await;
 mod autotrait_impls;
 mod builtin_macros;
+mod closure_trait_impl;
 mod extern_blocks;
 mod external_mod;
 mod fn_ptr;
@@ -28,6 +29,7 @@ static PLRUST_LINTS: Lazy<Vec<&'static Lint>> = Lazy::new(|| {
     let mut v = vec![
         async_await::PLRUST_ASYNC,
         autotrait_impls::PLRUST_AUTOTRAIT_IMPLS,
+        closure_trait_impl::PLRUST_CLOSURE_TRAIT_IMPL,
         static_impls::PLRUST_STATIC_IMPLS,
         extern_blocks::PLRUST_EXTERN_BLOCKS,
         external_mod::PLRUST_EXTERNAL_MOD,
@@ -75,6 +77,7 @@ pub fn register(store: &mut LintStore, _sess: &Session) {
     );
     store.register_early_pass(move || Box::new(async_await::PlrustAsync));
     store.register_early_pass(move || Box::new(external_mod::PlrustExternalMod));
+    store.register_late_pass(move |_| Box::new(closure_trait_impl::PlrustClosureTraitImpl));
     store.register_late_pass(move |_| Box::new(sus_trait_object::PlrustSuspiciousTraitObject));
     store.register_late_pass(move |_| Box::new(autotrait_impls::PlrustAutoTraitImpls));
     store.register_late_pass(move |_| Box::new(static_impls::PlrustStaticImpls));

plrustc/plrustc/src/lints/utils.rs

@@ -130,3 +130,17 @@ pub fn match_def_path<'tcx>(cx: &LateContext<'tcx>, did: DefId, syms: &[&str]) -
         .map(|x| Symbol::intern(x))
         .eq(path.iter().copied())
 }
+
+pub fn has_fn_trait(cx: &LateContext<'_>, poly_trait: &hir::PolyTraitRef<'_>) -> bool {
+    let Some(impl_did) = poly_trait.trait_ref.path.res.opt_def_id() else {
+        return false
+    };
+    let lang_items = cx.tcx.lang_items();
+    let fntraits = [
+        lang_items.get(hir::LangItem::Fn),
+        lang_items.get(hir::LangItem::FnOnce),
+        lang_items.get(hir::LangItem::FnMut),
+        lang_items.get(hir::LangItem::FnPtrTrait),
+    ];
+    fntraits.contains(&Some(impl_did))
+}

plrustc/plrustc/uitests/static_closure.rs

@@ -0,0 +1,25 @@
+#![crate_type = "lib"]
+use std::fmt;
+trait Trait {
+    type Associated;
+}
+
+impl<R, F: Fn() -> R> Trait for F {
+    type Associated = R;
+}
+
+fn static_transfers_to_associated<T: Trait + 'static>(
+    _: &T,
+    x: T::Associated,
+) -> Box<dyn fmt::Display /* + 'static */>
+where
+    T::Associated: fmt::Display,
+{
+    Box::new(x) // T::Associated: 'static follows from T: 'static
+}
+
+pub fn make_static_displayable<'a>(s: &'a str) -> Box<dyn fmt::Display> {
+    let f = || -> &'a str { "" };
+    // problem is: the closure type of `f` is 'static
+    static_transfers_to_associated(&f, s)
+}

plrustc/plrustc/uitests/static_closure.stderr

@@ -0,0 +1,10 @@
+error: trait impls bounded on function traits are forbidden in PL/Rust
+  --> $DIR/static_closure.rs:7:10
+   |
+LL | impl<R, F: Fn() -> R> Trait for F {
+   |          ^^^^^^^^^^^
+   |
+   = note: `-F plrust-closure-trait-impl` implied by `-F plrust-lints`
+
+error: aborting due to previous error
+