Make Copy unsafe to implement for ADTs with unsafe fields

As a rule, the application of `unsafe` to a declaration requires that use-sites
of that declaration also require `unsafe`. For example, a field declared
`unsafe` may only be read in the lexical context of an `unsafe` block.

For nearly all safe traits, the safety obligations of fields are explicitly
discharged when they are mentioned in method definitions. For example,
idiomatically implementing `Clone` (a safe trait) for a type with unsafe fields
will require `unsafe` to clone those fields.

Prior to this commit, `Copy` violated this rule. The trait is marked safe, and
although it has no explicit methods, its implementation permits reads of `Self`.

This commit resolves this by making `Copy` conditionally safe to implement. It
remains safe to implement for ADTs without unsafe fields, but unsafe to
implement for ADTs with unsafe fields.

Tracking: #132922

Author: jswrenn

compiler/rustc_hir_analysis/src/coherence/builtin.rs

@@ -103,7 +103,7 @@ fn visit_implementation_of_copy(checker: &Checker<'_>) -> Result<(), ErrorGuaran
     }
 
     let cause = traits::ObligationCause::misc(DUMMY_SP, impl_did);
-    match type_allowed_to_implement_copy(tcx, param_env, self_type, cause) {
+    match type_allowed_to_implement_copy(tcx, param_env, self_type, cause, impl_header.safety) {
         Ok(()) => Ok(()),
         Err(CopyImplementationError::InfringingFields(fields)) => {
             let span = tcx.hir().expect_item(impl_did).expect_impl().self_ty.span;
@@ -123,6 +123,12 @@ fn visit_implementation_of_copy(checker: &Checker<'_>) -> Result<(), ErrorGuaran
             let span = tcx.hir().expect_item(impl_did).expect_impl().self_ty.span;
             Err(tcx.dcx().emit_err(errors::CopyImplOnTypeWithDtor { span }))
         }
+        Err(CopyImplementationError::HasUnsafeFields) => {
+            let span = tcx.hir().expect_item(impl_did).expect_impl().self_ty.span;
+            Err(tcx
+                .dcx()
+                .span_delayed_bug(span, format!("cannot implement `Copy` for `{}`", self_type)))
+        }
     }
 }
 

compiler/rustc_hir_analysis/src/coherence/unsafety.rs

@@ -3,7 +3,7 @@
 
 use rustc_errors::codes::*;
 use rustc_errors::struct_span_code_err;
-use rustc_hir::Safety;
+use rustc_hir::{LangItem, Safety};
 use rustc_middle::ty::ImplPolarity::*;
 use rustc_middle::ty::print::PrintTraitRefExt as _;
 use rustc_middle::ty::{ImplTraitHeader, TraitDef, TyCtxt};
@@ -20,7 +20,20 @@ pub(super) fn check_item(
         tcx.generics_of(def_id).own_params.iter().find(|p| p.pure_wrt_drop).map(|_| "may_dangle");
     let trait_ref = trait_header.trait_ref.instantiate_identity();
 
-    match (trait_def.safety, unsafe_attr, trait_header.safety, trait_header.polarity) {
+    let is_copy = tcx.is_lang_item(trait_def.def_id, LangItem::Copy);
+    let trait_def_safety = if is_copy {
+        use rustc_type_ir::inherent::*;
+        // If `Self` has unsafe fields, `Copy` is unsafe to implement.
+        if trait_header.trait_ref.skip_binder().self_ty().has_unsafe_fields() {
+            rustc_hir::Safety::Unsafe
+        } else {
+            rustc_hir::Safety::Safe
+        }
+    } else {
+        trait_def.safety
+    };
+
+    match (trait_def_safety, unsafe_attr, trait_header.safety, trait_header.polarity) {
         (Safety::Safe, None, Safety::Unsafe, Positive | Reservation) => {
             let span = tcx.def_span(def_id);
             return Err(struct_span_code_err!(
@@ -48,12 +61,22 @@ pub(super) fn check_item(
                 "the trait `{}` requires an `unsafe impl` declaration",
                 trait_ref.print_trait_sugared()
             )
-            .with_note(format!(
-                "the trait `{}` enforces invariants that the compiler can't check. \
-                    Review the trait documentation and make sure this implementation \
-                    upholds those invariants before adding the `unsafe` keyword",
-                trait_ref.print_trait_sugared()
-            ))
+            .with_note(if is_copy {
+                format!(
+                    "the trait `{}` cannot be safely implemented for `{}` \
+                        because it has unsafe fields. Review the invariants \
+                        of those fields before adding an `unsafe impl`",
+                    trait_ref.print_trait_sugared(),
+                    trait_ref.self_ty(),
+                )
+            } else {
+                format!(
+                    "the trait `{}` enforces invariants that the compiler can't check. \
+                        Review the trait documentation and make sure this implementation \
+                        upholds those invariants before adding the `unsafe` keyword",
+                    trait_ref.print_trait_sugared()
+                )
+            })
             .with_span_suggestion_verbose(
                 span.shrink_to_lo(),
                 "add `unsafe` to this trait implementation",

compiler/rustc_lint/src/builtin.rs

@@ -625,6 +625,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingCopyImplementations {
             cx.param_env,
             ty,
             traits::ObligationCause::misc(item.span, item.owner_id.def_id),
+            hir::Safety::Safe,
         )
         .is_ok()
         {

compiler/rustc_trait_selection/src/traits/misc.rs

@@ -18,6 +18,7 @@ pub enum CopyImplementationError<'tcx> {
     InfringingFields(Vec<(&'tcx ty::FieldDef, Ty<'tcx>, InfringingFieldsReason<'tcx>)>),
     NotAnAdt,
     HasDestructor,
+    HasUnsafeFields,
 }
 
 pub enum ConstParamTyImplementationError<'tcx> {
@@ -39,12 +40,19 @@ pub enum InfringingFieldsReason<'tcx> {
 ///
 /// If it's not an ADT, int ty, `bool`, float ty, `char`, raw pointer, `!`,
 /// a reference or an array returns `Err(NotAnAdt)`.
+///
+/// If the impl is `Safe`, `self_type` must not have unsafe fields. When used to
+/// generate suggestions in lints, `Safe` should be supplied so as to not
+/// suggest implementing `Copy` for types with unsafe fields.
 pub fn type_allowed_to_implement_copy<'tcx>(
     tcx: TyCtxt<'tcx>,
     param_env: ty::ParamEnv<'tcx>,
     self_type: Ty<'tcx>,
     parent_cause: ObligationCause<'tcx>,
+    impl_safety: hir::Safety,
 ) -> Result<(), CopyImplementationError<'tcx>> {
+    use rustc_type_ir::inherent::*;
+
     let (adt, args) = match self_type.kind() {
         // These types used to have a builtin impl.
         // Now libcore provides that impl.
@@ -78,6 +86,10 @@ pub fn type_allowed_to_implement_copy<'tcx>(
         return Err(CopyImplementationError::HasDestructor);
     }
 
+    if impl_safety == hir::Safety::Safe && self_type.has_unsafe_fields() {
+        return Err(CopyImplementationError::HasUnsafeFields);
+    }
+
     Ok(())
 }
 

src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs

@@ -200,6 +200,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue {
                                 cx.param_env,
                                 ty,
                                 traits::ObligationCause::dummy_with_span(span),
+                                rustc_hir::Safety::Safe,
                             )
                             .is_ok()
                             {

tests/ui/unsafe-fields/copy-trait.rs

@@ -0,0 +1,41 @@
+//@ compile-flags: --crate-type=lib
+
+#![feature(unsafe_fields)]
+#![allow(incomplete_features)]
+#![deny(missing_copy_implementations)]
+
+mod good_safe_impl {
+    enum SafeEnum {
+        Safe(u8),
+    }
+
+    impl Copy for SafeEnum {}
+}
+
+mod bad_safe_impl {
+    enum UnsafeEnum {
+        Safe(u8),
+        Unsafe { unsafe field: u8 },
+    }
+
+    impl Copy for UnsafeEnum {}
+    //~^ ERROR the trait `Copy` requires an `unsafe impl` declaration
+}
+
+mod good_unsafe_impl {
+    enum UnsafeEnum {
+        Safe(u8),
+        Unsafe { unsafe field: u8 },
+    }
+
+    unsafe impl Copy for UnsafeEnum {}
+}
+
+mod bad_unsafe_impl {
+    enum SafeEnum {
+        Safe(u8),
+    }
+
+    unsafe impl Copy for SafeEnum {}
+    //~^ ERROR implementing the trait `Copy` is not unsafe
+}

tests/ui/unsafe-fields/copy-trait.stderr

@@ -0,0 +1,28 @@
+error[E0200]: the trait `Copy` requires an `unsafe impl` declaration
+  --> $DIR/copy-trait.rs:21:5
+   |
+LL |     impl Copy for UnsafeEnum {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: the trait `Copy` cannot be safely implemented for `bad_safe_impl::UnsafeEnum` because it has unsafe fields. Review the invariants of those fields before adding an `unsafe impl`
+help: add `unsafe` to this trait implementation
+   |
+LL |     unsafe impl Copy for UnsafeEnum {}
+   |     ++++++
+
+error[E0199]: implementing the trait `Copy` is not unsafe
+  --> $DIR/copy-trait.rs:39:5
+   |
+LL |     unsafe impl Copy for SafeEnum {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: remove `unsafe` from this trait implementation
+   |
+LL -     unsafe impl Copy for SafeEnum {}
+LL +     impl Copy for SafeEnum {}
+   |
+
+error: aborting due to 2 previous errors
+
+Some errors have detailed explanations: E0199, E0200.
+For more information about an error, try `rustc --explain E0199`.