implement lint for suspicious auto trait impls

Author: lcnr

compiler/rustc_index/src/bit_set.rs

@@ -938,6 +938,12 @@ pub struct GrowableBitSet<T: Idx> {
     bit_set: BitSet<T>,
 }
 
+impl<T: Idx> Default for GrowableBitSet<T> {
+    fn default() -> Self {
+        GrowableBitSet::new_empty()
+    }
+}
+
 impl<T: Idx> GrowableBitSet<T> {
     /// Ensure that the set can hold at least `min_domain_size` elements.
     pub fn ensure(&mut self, min_domain_size: usize) {

compiler/rustc_lint_defs/src/builtin.rs

@@ -3050,6 +3050,7 @@ declare_lint_pass! {
         DEREF_INTO_DYN_SUPERTRAIT,
         DEPRECATED_CFG_ATTR_CRATE_TYPE_NAME,
         DUPLICATE_MACRO_ATTRIBUTES,
+        SUSPICIOUS_AUTO_TRAIT_IMPLS,
     ]
 }
 
@@ -3626,3 +3627,37 @@ declare_lint! {
     Warn,
     "duplicated attribute"
 }
+
+declare_lint! {
+    /// The `suspicious_auto_trait_impls` lint checks for potentially incorrect
+    /// implementations of auto traits.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// struct Foo<T>(T);
+    ///
+    /// unsafe impl<T> Send for Foo<*const T> {}
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// A type can implement auto traits, e.g. `Send`, `Sync` and `Unpin`,
+    /// in two different ways: either by writing an explicit impl or if
+    /// all fields of the type implement that auto trait.
+    ///
+    /// The compiler disables the automatic implementation if an explicit one
+    /// exists for given type constructor. The exact rules governing this
+    /// are currently unsound and quite subtle and and will be modified in the future.
+    /// This change will cause the automatic implementation to be disabled in more
+    /// cases, potentially breaking some code.
+    pub SUSPICIOUS_AUTO_TRAIT_IMPLS,
+    Warn,
+    "the rules governing auto traits will change in the future",
+    @future_incompatible = FutureIncompatibleInfo {
+        reason: FutureIncompatibilityReason::FutureReleaseSemanticsChange,
+        reference: "issue #93367 <https://github.com/rust-lang/rust/issues/93367>",
+    };
+}

compiler/rustc_middle/src/ty/trait_def.rs

@@ -144,6 +144,23 @@ impl<'tcx> TyCtxt<'tcx> {
         });
     }
 
+    pub fn non_blanket_impls_for_ty(
+        self,
+        def_id: DefId,
+        self_ty: Ty<'tcx>,
+    ) -> impl Iterator<Item = DefId> + 'tcx {
+        let impls = self.trait_impls_of(def_id);
+        if let Some(simp) =
+            fast_reject::simplify_type(self, self_ty, SimplifyParams::No, StripReferences::No)
+        {
+            if let Some(impls) = impls.non_blanket_impls.get(&simp) {
+                return impls.iter().copied();
+            }
+        }
+
+        [].iter().copied()
+    }
+
     /// Applies function to every impl that could possibly match the self type `self_ty` and returns
     /// the first non-none value.
     pub fn find_map_relevant_impl<T, F: FnMut(DefId) -> Option<T>>(

compiler/rustc_typeck/src/coherence/orphan.rs

@@ -1,24 +1,33 @@
 //! Orphan checker: every impl either implements a trait defined in this
 //! crate or pertains to a type defined in this crate.
 
+use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::struct_span_err;
 use rustc_errors::ErrorReported;
 use rustc_hir as hir;
+use rustc_index::bit_set::GrowableBitSet;
 use rustc_infer::infer::TyCtxtInferExt;
-use rustc_middle::ty::{self, TyCtxt};
-use rustc_span::def_id::LocalDefId;
+use rustc_middle::ty::subst::{GenericArg, InternalSubsts};
+use rustc_middle::ty::{self, ImplPolarity, Ty, TyCtxt, TypeFoldable, TypeVisitor};
+use rustc_session::lint;
+use rustc_span::def_id::{DefId, LocalDefId};
 use rustc_span::Span;
 use rustc_trait_selection::traits;
+use std::ops::ControlFlow;
 
 pub(super) fn orphan_check_crate(tcx: TyCtxt<'_>, (): ()) -> &[LocalDefId] {
     let mut errors = Vec::new();
-    for (_trait, impls_of_trait) in tcx.all_local_trait_impls(()) {
+    for (&trait_def_id, impls_of_trait) in tcx.all_local_trait_impls(()) {
         for &impl_of_trait in impls_of_trait {
             match orphan_check_impl(tcx, impl_of_trait) {
                 Ok(()) => {}
                 Err(ErrorReported) => errors.push(impl_of_trait),
             }
         }
+
+        if tcx.trait_is_auto(trait_def_id) {
+            lint_auto_trait_impls(tcx, trait_def_id, impls_of_trait);
+        }
     }
     tcx.arena.alloc_slice(&errors)
 }
@@ -265,3 +274,201 @@ fn emit_orphan_check_error<'tcx>(
 
     Err(ErrorReported)
 }
+
+#[derive(Default)]
+struct AreUniqueParamsVisitor {
+    seen: GrowableBitSet<u32>,
+}
+
+#[derive(Copy, Clone)]
+enum NotUniqueParam<'tcx> {
+    DuplicateParam(GenericArg<'tcx>),
+    NotParam(GenericArg<'tcx>),
+}
+
+impl<'tcx> TypeVisitor<'tcx> for AreUniqueParamsVisitor {
+    type BreakTy = NotUniqueParam<'tcx>;
+    fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
+        match t.kind() {
+            ty::Param(p) => {
+                if self.seen.insert(p.index) {
+                    ControlFlow::CONTINUE
+                } else {
+                    ControlFlow::Break(NotUniqueParam::DuplicateParam(t.into()))
+                }
+            }
+            _ => ControlFlow::Break(NotUniqueParam::NotParam(t.into())),
+        }
+    }
+    fn visit_region(&mut self, r: ty::Region<'tcx>) -> ControlFlow<Self::BreakTy> {
+        match r {
+            ty::ReEarlyBound(p) => {
+                if self.seen.insert(p.index) {
+                    ControlFlow::CONTINUE
+                } else {
+                    ControlFlow::Break(NotUniqueParam::DuplicateParam(r.into()))
+                }
+            }
+            _ => ControlFlow::Break(NotUniqueParam::NotParam(r.into())),
+        }
+    }
+    fn visit_const(&mut self, c: &'tcx ty::Const<'tcx>) -> ControlFlow<Self::BreakTy> {
+        match c.val {
+            ty::ConstKind::Param(p) => {
+                if self.seen.insert(p.index) {
+                    ControlFlow::CONTINUE
+                } else {
+                    ControlFlow::Break(NotUniqueParam::DuplicateParam(c.into()))
+                }
+            }
+            _ => ControlFlow::Break(NotUniqueParam::NotParam(c.into())),
+        }
+    }
+}
+
+/// Lint impls of auto traits if they are likely to have
+/// unsound or surprising effects on auto impls.
+fn lint_auto_trait_impls(tcx: TyCtxt<'_>, trait_def_id: DefId, impls: &[LocalDefId]) {
+    let mut non_covering_impls = Vec::new();
+    for &impl_def_id in impls {
+        let trait_ref = tcx.impl_trait_ref(impl_def_id).unwrap();
+        if trait_ref.references_error() {
+            return;
+        }
+
+        if tcx.impl_polarity(impl_def_id) != ImplPolarity::Positive {
+            return;
+        }
+
+        assert_eq!(trait_ref.substs.len(), 1);
+        let self_ty = trait_ref.self_ty();
+        let (self_type_did, substs) = match self_ty.kind() {
+            ty::Adt(def, substs) => (def.did, substs),
+            _ => {
+                // FIXME: should also lint for stuff like `&i32` but
+                // considering that auto traits are unstable, that
+                // isn't too important for now as this only affects
+                // crates using `nightly`, and std.
+                continue;
+            }
+        };
+
+        // Impls which completely cover a given root type are fine as they
+        // disable auto impls entirely. So only lint if the substs
+        // are not a permutation of the identity substs.
+        match substs.visit_with(&mut AreUniqueParamsVisitor::default()) {
+            ControlFlow::Continue(()) => {} // ok
+            ControlFlow::Break(arg) => {
+                // Ideally:
+                //
+                // - compute the requirements for the auto impl candidate
+                // - check whether these are implied by the non covering impls
+                // - if not, emit the lint
+                //
+                // What we do here is a bit simpler:
+                //
+                // - badly check if an auto impl candidate definitely does not apply
+                //   for the given simplified type
+                // - if so, do not lint
+                if fast_reject_auto_impl(tcx, trait_def_id, self_ty) {
+                    // ok
+                } else {
+                    non_covering_impls.push((impl_def_id, self_type_did, arg));
+                }
+            }
+        }
+    }
+
+    for &(impl_def_id, self_type_did, arg) in &non_covering_impls {
+        tcx.struct_span_lint_hir(
+            lint::builtin::SUSPICIOUS_AUTO_TRAIT_IMPLS,
+            tcx.hir().local_def_id_to_hir_id(impl_def_id),
+            tcx.def_span(impl_def_id),
+            |err| {
+                let mut err = err.build(&format!(
+                    "cross-crate traits with a default impl, like `{}`, \
+                         should not be specialized",
+                    tcx.def_path_str(trait_def_id),
+                ));
+                let item_span = tcx.def_span(self_type_did);
+                let self_descr = tcx.def_kind(self_type_did).descr(self_type_did);
+                err.span_note(
+                    item_span,
+                    &format!(
+                        "try using the same sequence of generic parameters as the {} definition",
+                        self_descr,
+                    ),
+                );
+                match arg {
+                    NotUniqueParam::DuplicateParam(arg) => {
+                        err.note(&format!("`{}` is mentioned multiple times", arg));
+                    }
+                    NotUniqueParam::NotParam(arg) => {
+                        err.note(&format!("`{}` is not a generic parameter", arg));
+                    }
+                }
+                err.emit();
+            },
+        );
+    }
+}
+
+fn fast_reject_auto_impl<'tcx>(tcx: TyCtxt<'tcx>, trait_def_id: DefId, self_ty: Ty<'tcx>) -> bool {
+    struct DisableAutoTraitVisitor<'tcx> {
+        tcx: TyCtxt<'tcx>,
+        trait_def_id: DefId,
+        self_ty_root: Ty<'tcx>,
+        seen: FxHashSet<DefId>,
+    }
+
+    impl<'tcx> TypeVisitor<'tcx> for DisableAutoTraitVisitor<'tcx> {
+        type BreakTy = ();
+        fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
+            let tcx = self.tcx;
+            if t != self.self_ty_root {
+                for impl_def_id in tcx.non_blanket_impls_for_ty(self.trait_def_id, t) {
+                    match tcx.impl_polarity(impl_def_id) {
+                        ImplPolarity::Negative => return ControlFlow::BREAK,
+                        ImplPolarity::Reservation => {}
+                        // FIXME(@lcnr): That's probably not good enough, idk
+                        //
+                        // We might just want to take the rustdoc code and somehow avoid
+                        // explicit impls for `Self`.
+                        ImplPolarity::Positive => return ControlFlow::CONTINUE,
+                    }
+                }
+            }
+
+            match t.kind() {
+                ty::Adt(def, substs) => {
+                    // @lcnr: This is the only place where cycles can happen. We avoid this
+                    // by only visiting each `DefId` once.
+                    //
+                    // This will be is incorrect in subtle cases, but I don't care :)
+                    if self.seen.insert(def.did) {
+                        for ty in def.all_fields().map(|field| field.ty(tcx, substs)) {
+                            ty.visit_with(self)?;
+                        }
+                    }
+
+                    ControlFlow::CONTINUE
+                }
+                _ => t.super_visit_with(self),
+            }
+        }
+    }
+
+    let self_ty_root = match self_ty.kind() {
+        ty::Adt(def, _) => tcx.mk_adt(def, InternalSubsts::identity_for_item(tcx, def.did)),
+        _ => unimplemented!("unexpected self ty {:?}", self_ty),
+    };
+
+    self_ty_root
+        .visit_with(&mut DisableAutoTraitVisitor {
+            tcx,
+            self_ty_root,
+            trait_def_id,
+            seen: FxHashSet::default(),
+        })
+        .is_break()
+}

src/test/ui/auto-traits/suspicious-impls-lint.rs

@@ -0,0 +1,34 @@
+#![deny(suspicious_auto_trait_impls)]
+
+struct MayImplementSendOk<T>(T);
+unsafe impl<T: Send> Send for MayImplementSendOk<T> {} // ok
+
+struct MayImplementSendErr<T>(T);
+unsafe impl<T: Send> Send for MayImplementSendErr<&T> {}
+//~^ ERROR
+//~| WARNING this will change its meaning
+
+struct ContainsNonSendDirect<T>(*const T);
+unsafe impl<T: Send> Send for ContainsNonSendDirect<&T> {} // ok
+
+struct ContainsPtr<T>(*const T);
+struct ContainsIndirectNonSend<T>(ContainsPtr<T>);
+unsafe impl<T: Send> Send for ContainsIndirectNonSend<&T> {} // ok
+
+struct ContainsVec<T>(Vec<T>);
+unsafe impl Send for ContainsVec<i32> {}
+//~^ ERROR
+//~| WARNING this will change its meaning
+
+struct TwoParams<T, U>(T, U);
+unsafe impl<T: Send, U: Send> Send for TwoParams<T, U> {} // ok
+
+struct TwoParamsFlipped<T, U>(T, U);
+unsafe impl<T: Send, U: Send> Send for TwoParamsFlipped<U, T> {} // ok
+
+struct TwoParamsSame<T, U>(T, U);
+unsafe impl<T: Send> Send for TwoParamsSame<T, T> {}
+//~^ ERROR
+//~| WARNING this will change its meaning
+
+fn main() {}