From 539062e84ca6837d7d6b8f91f4cafd7b8657078e Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Fri, 30 Jun 2023 09:55:33 -0700 Subject: [PATCH 1/5] Add a lint for closure trait impls --- doc/src/config-lints.md | 26 ++++++ plrust/src/lib.rs | 1 + .../plrustc/src/lints/closure_trait_impl.rs | 81 +++++++++++++++++++ plrustc/plrustc/src/lints/mod.rs | 3 + plrustc/plrustc/uitests/static_closure.rs | 25 ++++++ plrustc/plrustc/uitests/static_closure.stderr | 10 +++ 6 files changed, 146 insertions(+) create mode 100644 plrustc/plrustc/src/lints/closure_trait_impl.rs create mode 100644 plrustc/plrustc/uitests/static_closure.rs create mode 100644 plrustc/plrustc/uitests/static_closure.stderr diff --git a/doc/src/config-lints.md b/doc/src/config-lints.md index 547603a3..736cc4b9 100644 --- a/doc/src/config-lints.md +++ b/doc/src/config-lints.md @@ -284,3 +284,29 @@ impl AsRef 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 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> Trait for F { + // This is the problem + type Assoc = R; +} +``` diff --git a/plrust/src/lib.rs b/plrust/src/lib.rs index f9eaa8ae..b13458a4 100644 --- a/plrust/src/lib.rs +++ b/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, \ diff --git a/plrustc/plrustc/src/lints/closure_trait_impl.rs b/plrustc/plrustc/src/lints/closure_trait_impl.rs new file mode 100644 index 00000000..b4c1dd16 --- /dev/null +++ b/plrustc/plrustc/src/lints/closure_trait_impl.rs @@ -0,0 +1,81 @@ +//! 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> 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. +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 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(..) => { + // Don't care about other traits (I think) + } + hir::GenericBound::Trait(poly_trait, ..) => { + let Some(impl_did) = poly_trait.trait_ref.path.res.opt_def_id() else { + continue + }; + 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), + ]; + if fntraits.contains(&Some(impl_did)) { + 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(..) => { + // Don't care about these. + } + } + } + } + } +} diff --git a/plrustc/plrustc/src/lints/mod.rs b/plrustc/plrustc/src/lints/mod.rs index a4bd5a46..93918d31 100644 --- a/plrustc/plrustc/src/lints/mod.rs +++ b/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> = 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)); diff --git a/plrustc/plrustc/uitests/static_closure.rs b/plrustc/plrustc/uitests/static_closure.rs new file mode 100644 index 00000000..beb3fe60 --- /dev/null +++ b/plrustc/plrustc/uitests/static_closure.rs @@ -0,0 +1,25 @@ +#![crate_type = "lib"] +use std::fmt; +trait Trait { + type Associated; +} + +impl R> Trait for F { + type Associated = R; +} + +fn static_transfers_to_associated( + _: &T, + x: T::Associated, +) -> Box +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 { + let f = || -> &'a str { "" }; + // problem is: the closure type of `f` is 'static + static_transfers_to_associated(&f, s) +} diff --git a/plrustc/plrustc/uitests/static_closure.stderr b/plrustc/plrustc/uitests/static_closure.stderr new file mode 100644 index 00000000..b403c47a --- /dev/null +++ b/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> Trait for F { + | ^^^^^^^^^^^ + | + = note: `-F plrust-closure-trait-impl` implied by `-F plrust-lints` + +error: aborting due to previous error + From acb28dbdd0c517d33bd80b97dc5a5839abe79d8f Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Fri, 30 Jun 2023 12:51:44 -0700 Subject: [PATCH 2/5] Comment why this is hard --- plrustc/plrustc/src/lints/closure_trait_impl.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/plrustc/plrustc/src/lints/closure_trait_impl.rs b/plrustc/plrustc/src/lints/closure_trait_impl.rs index b4c1dd16..e046864f 100644 --- a/plrustc/plrustc/src/lints/closure_trait_impl.rs +++ b/plrustc/plrustc/src/lints/closure_trait_impl.rs @@ -11,6 +11,19 @@ //! 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 { type Assoc; } +//! impl GivesAssoc for A { type Assoc = A; } +//! +//! trait Child where Self: GivesAssoc {} +//! impl R> Child 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. use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass, LintContext}; From 9b982702948b61b1da7a2649e815a795d27cdda5 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Fri, 30 Jun 2023 12:57:04 -0700 Subject: [PATCH 3/5] Share trait detection code between closure_trait_impl and fn_ptr lint --- .../plrustc/src/lints/closure_trait_impl.rs | 12 +--------- plrustc/plrustc/src/lints/fn_ptr.rs | 23 ++++++------------- plrustc/plrustc/src/lints/utils.rs | 14 +++++++++++ 3 files changed, 22 insertions(+), 27 deletions(-) diff --git a/plrustc/plrustc/src/lints/closure_trait_impl.rs b/plrustc/plrustc/src/lints/closure_trait_impl.rs index e046864f..154c7177 100644 --- a/plrustc/plrustc/src/lints/closure_trait_impl.rs +++ b/plrustc/plrustc/src/lints/closure_trait_impl.rs @@ -64,17 +64,7 @@ impl<'tcx> LateLintPass<'tcx> for PlrustClosureTraitImpl { // Don't care about other traits (I think) } hir::GenericBound::Trait(poly_trait, ..) => { - let Some(impl_did) = poly_trait.trait_ref.path.res.opt_def_id() else { - continue - }; - 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), - ]; - if fntraits.contains(&Some(impl_did)) { + 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", diff --git a/plrustc/plrustc/src/lints/fn_ptr.rs b/plrustc/plrustc/src/lints/fn_ptr.rs index 8c1e938c..bfae9899 100644 --- a/plrustc/plrustc/src/lints/fn_ptr.rs +++ b/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), + ); } } } diff --git a/plrustc/plrustc/src/lints/utils.rs b/plrustc/plrustc/src/lints/utils.rs index 4dd24e27..cbeb77fa 100644 --- a/plrustc/plrustc/src/lints/utils.rs +++ b/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)) +} From b07c6f51f26012c748a01267cd7166c7ceb5e2d4 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Mon, 3 Jul 2023 13:03:12 -0700 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com> --- plrustc/plrustc/src/lints/closure_trait_impl.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plrustc/plrustc/src/lints/closure_trait_impl.rs b/plrustc/plrustc/src/lints/closure_trait_impl.rs index 154c7177..0dfc44c6 100644 --- a/plrustc/plrustc/src/lints/closure_trait_impl.rs +++ b/plrustc/plrustc/src/lints/closure_trait_impl.rs @@ -44,7 +44,7 @@ impl<'tcx> LateLintPass<'tcx> for PlrustClosureTraitImpl { continue }; // TODO: should we ignore cases where `bound_pred.bounded_ty` isn't - // from from one of `item.generics.params`? + // from one of `item.generics.params`? for bound in bound_pred.bounds { match bound { hir::GenericBound::LangItemTrait( @@ -61,7 +61,7 @@ impl<'tcx> LateLintPass<'tcx> for PlrustClosureTraitImpl { ); } hir::GenericBound::LangItemTrait(..) => { - // Don't care about other traits (I think) + // 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) { @@ -75,7 +75,7 @@ impl<'tcx> LateLintPass<'tcx> for PlrustClosureTraitImpl { // try_normalize_erasing_regions and retry? } hir::GenericBound::Outlives(..) => { - // Don't care about these. + // Lifetime generics are irrelevant for guards against type projection } } } From 170342e47664122eff4d34684e5f12cd2e497253 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Fri, 7 Jul 2023 04:21:07 -0700 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com> --- plrustc/plrustc/src/lints/closure_trait_impl.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/plrustc/plrustc/src/lints/closure_trait_impl.rs b/plrustc/plrustc/src/lints/closure_trait_impl.rs index 0dfc44c6..d0a66370 100644 --- a/plrustc/plrustc/src/lints/closure_trait_impl.rs +++ b/plrustc/plrustc/src/lints/closure_trait_impl.rs @@ -1,3 +1,9 @@ +//! 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) @@ -24,6 +30,9 @@ //! 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};