Make `Copy` unsafe to implement for ADTs with `unsafe` fields #134008

jswrenn · 2024-12-07T13:32:59Z

As a rule, the application of unsafe to a declaration requires that use-sites of that declaration also entail 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

r? @compiler-errors

rustbot · 2024-12-07T13:33:09Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

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

compiler-errors · 2024-12-07T18:34:21Z

compiler/rustc_hir_analysis/src/coherence/unsafety.rs

-    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::*;


don't do this lol

inherent is only for the solver

What's your recommendation? The intent of this PR is to implement the suggestion in the RFC discussion that Copy is conditionally (un)safe to implement depending on whether the Self type has unsafe fields. Is there a better way to achieve this? Misunderstood what you were commenting on.

Ah gotcha. Hm.

just duplicate the method on ty or whatever

I import inherent::Ty here because it provides the has_unsafe_fields helper. Should I also add that function as an inherent method on rustc_middle::ty::Ty?

rustbot · 2024-12-07T20:09:31Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

jswrenn · 2024-12-07T20:11:17Z

compiler/rustc_codegen_cranelift/example/mini_core.rs

 #[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> {}


I presume we don't have an obligation to support #[lang = "copy"] unsafe trait Copy {}, since no_core and lang_items are unstable.

That is correct.

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

rustbot · 2024-12-07T20:50:09Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

compiler-errors · 2024-12-09T17:24:59Z

@bors r+ rollup

bors · 2024-12-09T17:25:02Z

📌 Commit 3ce35a4 has been approved by compiler-errors

It is now in the queue for this repository.

…iler-errors 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 entail `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 r? `@compiler-errors`

Rollup of 9 pull requests Successful merges: - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133970 ([AIX] Replace sa_sigaction with sa_union.__su_sigaction for AIX) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) - rust-lang#134008 (Make `Copy` unsafe to implement for ADTs with `unsafe` fields) - rust-lang#134017 (Don't use `AsyncFnOnce::CallOnceFuture` bounds for signature deduction) - rust-lang#134023 (handle cygwin environment in `install::sanitize_sh`) - rust-lang#134041 (Use SourceMap to load debugger visualizer files) - rust-lang#134065 (Move `write_graphviz_results`) - rust-lang#134070 (Some asm! diagnostic adjustments and a papercut fix) r? `@ghost` `@rustbot` modify labels: rollup

…iler-errors 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 entail `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 r? ``@compiler-errors``

Rollup of 11 pull requests Successful merges: - rust-lang#133478 (jsondocck: Parse, don't validate commands.) - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133970 ([AIX] Replace sa_sigaction with sa_union.__su_sigaction for AIX) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) - rust-lang#134008 (Make `Copy` unsafe to implement for ADTs with `unsafe` fields) - rust-lang#134017 (Don't use `AsyncFnOnce::CallOnceFuture` bounds for signature deduction) - rust-lang#134023 (handle cygwin environment in `install::sanitize_sh`) - rust-lang#134041 (Use SourceMap to load debugger visualizer files) - rust-lang#134065 (Move `write_graphviz_results`) - rust-lang#134106 (Add compiler-maintainers who requested to be on review rotation) - rust-lang#134123 (bootstrap: Forward cargo JSON output to stdout, not stderr) Failed merges: - rust-lang#134120 (Remove Felix from ping groups and review rotation) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#134008 - jswrenn:unsafe-fields-copy, r=compiler-errors 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 entail `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 r? ```@compiler-errors```

rustbot assigned compiler-errors Dec 7, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 7, 2024

This comment has been minimized.

Sign in to view

jswrenn force-pushed the unsafe-fields-copy branch from c19cba7 to d66fa52 Compare December 7, 2024 14:16

This comment has been minimized.

Sign in to view

compiler-errors reviewed Dec 7, 2024

View reviewed changes

tests/ui/unsafe-fields/copy-trait.stderr Show resolved Hide resolved

jswrenn force-pushed the unsafe-fields-copy branch from d66fa52 to 96fc9a9 Compare December 7, 2024 18:32

compiler-errors requested changes Dec 7, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2024

jswrenn force-pushed the unsafe-fields-copy branch from 96fc9a9 to 47454f3 Compare December 7, 2024 18:51

This comment has been minimized.

Sign in to view

jswrenn force-pushed the unsafe-fields-copy branch from 47454f3 to ae4c896 Compare December 7, 2024 20:09

jswrenn commented Dec 7, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

jswrenn force-pushed the unsafe-fields-copy branch from ae4c896 to 3ce35a4 Compare December 7, 2024 20:50

jswrenn requested a review from compiler-errors December 8, 2024 16:03

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 8, 2024

compiler-errors approved these changes Dec 9, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2024

fmease mentioned this pull request Dec 9, 2024

Rollup of 9 pull requests #134097

Closed

This was referenced Dec 10, 2024

Rollup of 10 pull requests #134107

Closed

Rollup of 11 pull requests #134125

Merged

bors merged commit 3eaa785 into rust-lang:master Dec 10, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Copy` unsafe to implement for ADTs with `unsafe` fields #134008

Make `Copy` unsafe to implement for ADTs with `unsafe` fields #134008

jswrenn commented Dec 7, 2024 •

edited

Loading

rustbot commented Dec 7, 2024

This comment has been minimized.

This comment has been minimized.

compiler-errors Dec 7, 2024

compiler-errors Dec 7, 2024

jswrenn Dec 7, 2024 •

edited

Loading

jswrenn Dec 7, 2024

compiler-errors Dec 7, 2024

jswrenn Dec 7, 2024

jswrenn Dec 7, 2024

jswrenn Dec 7, 2024

This comment has been minimized.

rustbot commented Dec 7, 2024

jswrenn Dec 7, 2024

bjorn3 Dec 7, 2024

This comment has been minimized.

rustbot commented Dec 7, 2024

compiler-errors commented Dec 9, 2024

bors commented Dec 9, 2024

Make Copy unsafe to implement for ADTs with unsafe fields #134008

Make Copy unsafe to implement for ADTs with unsafe fields #134008

Conversation

jswrenn commented Dec 7, 2024 • edited Loading

rustbot commented Dec 7, 2024

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jswrenn Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Dec 7, 2024

compiler-errors commented Dec 9, 2024

bors commented Dec 9, 2024

Make `Copy` unsafe to implement for ADTs with `unsafe` fields #134008

Make `Copy` unsafe to implement for ADTs with `unsafe` fields #134008

jswrenn commented Dec 7, 2024 •

edited

Loading

jswrenn Dec 7, 2024 •

edited

Loading