Skip to content

Commit

Permalink
Make Copy unsafe to implement for ADTs with unsafe fields
Browse files Browse the repository at this point in the history
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: rust-lang#132922
  • Loading branch information
jswrenn committed Dec 7, 2024
1 parent 9c707a8 commit ae4c896
Show file tree
Hide file tree
Showing 11 changed files with 148 additions and 36 deletions.
40 changes: 20 additions & 20 deletions compiler/rustc_codegen_cranelift/example/mini_core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,26 +55,26 @@ impl<T: ?Sized> LegacyReceiver for &mut T {}
impl<T: ?Sized> LegacyReceiver for Box<T> {}

#[lang = "copy"]
pub unsafe trait Copy {}

unsafe impl Copy for bool {}
unsafe impl Copy for u8 {}
unsafe impl Copy for u16 {}
unsafe impl Copy for u32 {}
unsafe impl Copy for u64 {}
unsafe impl Copy for u128 {}
unsafe impl Copy for usize {}
unsafe impl Copy for i8 {}
unsafe impl Copy for i16 {}
unsafe impl Copy for i32 {}
unsafe impl Copy for isize {}
unsafe impl Copy for f32 {}
unsafe impl Copy for f64 {}
unsafe impl Copy for char {}
unsafe impl<'a, T: ?Sized> Copy for &'a T {}
unsafe impl<T: ?Sized> Copy for *const T {}
unsafe impl<T: ?Sized> Copy for *mut T {}
unsafe impl<T: Copy> Copy for Option<T> {}
pub trait Copy {}

impl Copy for bool {}
impl Copy for u8 {}
impl Copy for u16 {}
impl Copy for u32 {}
impl Copy for u64 {}
impl Copy for u128 {}
impl Copy for usize {}
impl Copy for i8 {}
impl Copy for i16 {}
impl Copy for i32 {}
impl Copy for isize {}
impl Copy for f32 {}
impl Copy for f64 {}
impl Copy for char {}
impl<'a, T: ?Sized> Copy for &'a T {}
impl<T: ?Sized> Copy for *const T {}
impl<T: ?Sized> Copy for *mut T {}
impl<T: Copy> Copy for Option<T> {}

#[lang = "sync"]
pub unsafe trait Sync {}
Expand Down
8 changes: 7 additions & 1 deletion compiler/rustc_hir_analysis/src/coherence/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)))
}
}
}

Expand Down
38 changes: 30 additions & 8 deletions compiler/rustc_hir_analysis/src/coherence/unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -20,7 +20,19 @@ 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 {
// 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!(
Expand Down Expand Up @@ -48,12 +60,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",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
6 changes: 1 addition & 5 deletions compiler/rustc_middle/src/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -980,11 +980,7 @@ impl<'tcx> rustc_type_ir::inherent::Ty<TyCtxt<'tcx>> for Ty<'tcx> {
}

fn has_unsafe_fields(self) -> bool {
if let ty::Adt(adt_def, ..) = self.kind() {
adt_def.all_fields().any(|x| x.safety == hir::Safety::Unsafe)
} else {
false
}
Ty::has_unsafe_fields(self)
}
}

Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_middle/src/ty/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1288,6 +1288,15 @@ impl<'tcx> Ty<'tcx> {
}
}

/// Checks whether this type is an ADT that has unsafe fields.
pub fn has_unsafe_fields(self) -> bool {
if let ty::Adt(adt_def, ..) = self.kind() {
adt_def.all_fields().any(|x| x.safety == hir::Safety::Unsafe)
} else {
false
}
}

/// Get morphology of the async drop glue, needed for types which do not
/// use async drop. To get async drop glue morphology for a definition see
/// [`TyCtxt::async_drop_glue_morphology`]. Used for `AsyncDestruct::Destructor`
Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_trait_selection/src/traits/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub enum CopyImplementationError<'tcx> {
InfringingFields(Vec<(&'tcx ty::FieldDef, Ty<'tcx>, InfringingFieldsReason<'tcx>)>),
NotAnAdt,
HasDestructor,
HasUnsafeFields,
}

pub enum ConstParamTyImplementationError<'tcx> {
Expand All @@ -39,11 +40,16 @@ 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>> {
let (adt, args) = match self_type.kind() {
// These types used to have a builtin impl.
Expand Down Expand Up @@ -78,6 +84,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(())
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -795,8 +795,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
| ty::Never
| ty::Tuple(_)
| ty::CoroutineWitness(..) => {
use rustc_type_ir::inherent::*;

// Only consider auto impls of unsafe traits when there are
// no unsafe fields.
if self.tcx().trait_is_unsafe(def_id) && self_ty.has_unsafe_fields() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
41 changes: 41 additions & 0 deletions tests/ui/unsafe-fields/copy-trait.rs
Original file line number Diff line number Diff line change
@@ -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
}
28 changes: 28 additions & 0 deletions tests/ui/unsafe-fields/copy-trait.stderr
Original file line number Diff line number Diff line change
@@ -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`.

0 comments on commit ae4c896

Please sign in to comment.