Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ZeroablePrimitive trait unsafe. #121850

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

reitermarkus
Copy link
Contributor

Tracking issue: #120257

r? @dtolnay

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 1, 2024
@Noratrieb
Copy link
Member

Not dtolnay, but this is obviously correct
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 1, 2024

📌 Commit f6d2607 has been approved by Nilstrieb

It is now in the queue for this repository.

@bors 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 Mar 1, 2024
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was correct before this PR. The boundary for unsafety to have any meaning in Rust in a public API is the module level. core::num exposes a sound API without this trait being an unsafe trait. Unsafe traits are for when implementing a trait from outside the module can cause unsoundness.

core::any is similar. Implementing Any with a bogus type_id method would cause unsoundness, but it is impossible to do so from outside the module, so there is no need for Any to be an unsafe trait.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 1, 2024
@reitermarkus
Copy link
Contributor Author

I think this was correct before this PR.

Even if it was correct already, I think this makes it more obvious why the trait needs to exist in the first place.

This feels a bit different to Any, which has a blanket implementation for any T, while ZeroablePrimitive needs to be carefully implemented for only very specific types.

@Noratrieb
Copy link
Member

I don't think it's necessarily wrong to not make it unsafe, but I think making it unsafe is clearer

@dtolnay
Copy link
Member

dtolnay commented Mar 1, 2024

The fact that the standard library needs to be careful with impls for this trait is an internal implementation detail of the standard library, which doesn't have bearing on consumers of the standard library. It's a shame that rustdoc would end up emphasizing this implementation detail in public-facing docs. Ideally there would be a way for a trait to be unsafe internally for the purpose of requiring unsafe impl, but without unsafe ending up in the public-facing docs. Analogously to how there needs to be a way for a type to be repr(transparent) internally, but without repr(transparent) ending up in public-facing documentation: #90435. (In both cases, rustdoc with --document-private-items might show it.)

Would it be just as good if Sealed is unsafe and ZeroablePrimitive is safe?

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2024
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#120646 (Fix incorrect suggestion for uninitialized binding in pattern)
 - rust-lang#121416 (Improve error messages for generics with default parameters)
 - rust-lang#121475 (Add tidy check for .stderr/.stdout files for non-existent test revisions)
 - rust-lang#121580 (make unused_imports less assertive in test modules)
 - rust-lang#121736 (Remove `Mutex::unlock` Function)
 - rust-lang#121784 (Make the success arms of `if lhs || rhs` meet up in a separate block)
 - rust-lang#121818 (CFI: Remove unused `typeid_for_fnsig`)
 - rust-lang#121819 (Handle stashing of delayed bugs)
 - rust-lang#121828 (Remove unused fluent messages)
 - rust-lang#121831 (Fix typo in comment)
 - rust-lang#121850 (Make `ZeroablePrimitive` trait unsafe.)
 - rust-lang#121853 (normalizes-to: handle negative impls)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 68dd5e6 into rust-lang:master Mar 1, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 1, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2024
Rollup merge of rust-lang#121850 - reitermarkus:generic-nonzero-unsafe-trait, r=Nilstrieb

Make `ZeroablePrimitive` trait unsafe.

Tracking issue: rust-lang#120257

r? `@dtolnay`
@reitermarkus
Copy link
Contributor Author

It's a shame that rustdoc would end up emphasizing this implementation detail in public-facing docs.

Would it be just as good if Sealed is unsafe and ZeroablePrimitive is safe?

The way I see it is: If it was stable and not sealed, it would need to be unsafe. Sealing isn't the unsafe part, it's only for forwards compatibility.

I can understand the argument for “it is safe if it is sealed”, but it doesn't feel quite as intuitive to me.

@reitermarkus reitermarkus deleted the generic-nonzero-unsafe-trait branch March 2, 2024 00:03
@dtolnay
Copy link
Member

dtolnay commented Mar 2, 2024

Got it. What do you think of whether Sealed needs to be unsafe in addition to ZeroablePrimitive being unsafe? As currently implemented, adding impl<T> Sealed for T {} in libcore would make the API of core::num unsound (for nightly), even without adding any other ZeroablePrimitive impl.

@reitermarkus
Copy link
Contributor Author

adding impl<T> Sealed for T {} in libcore would make the API of core::num unsound

How would it be unsound? The safety requirement currently states types implementing it must be primitives that are valid when zero. So technically any implementation outside core would be unsound, that makes the API unusable, but not unsound.

Of course if that requirement is relaxed at some point to allow non-primitives, the trait most likely wouldn't be sealed anymore. And at that point it would probably also be renamed to Zeroable.

@dtolnay
Copy link
Member

dtolnay commented Mar 23, 2024

It's not unsound — you are right. Good call. This is the kind of case I had been thinking of:

#![feature(generic_nonzero, nonzero_internals)]

use std::num::{NonZero, ZeroablePrimitive};

#[derive(Copy, Clone)]
#[repr(transparent)]
struct Thing([u8; 3]);

unsafe impl ZeroablePrimitive for Thing {}

fn main() {
    let _ = NonZero::new(Thing([1, 1, 1]));
}

where the existence of a genuinely unnameable Sealed trait (unnameable even for nightly) is loadbearing for upholding #120257 (comment). With this safe-code-only change in libcore:

diff --git a/library/core/src/num/nonzero.rs b/library/core/src/num/nonzero.rs
@@ -20,4 +20,11 @@ mod private {
     #[const_trait]
     pub trait Sealed {}
+
+    #[unstable(
+        feature = "nonzero_internals",
+        reason = "implementation detail which may disappear or be replaced at any time",
+        issue = "none"
+    )]
+    impl<T> const Sealed for T {}
 }

@@ -39,10 +46,3 @@ pub unsafe trait ZeroablePrimitive: Sized + Copy + private::Sealed {}
 macro_rules! impl_zeroable_primitive {
     ($primitive:ty) => {
-        #[unstable(
-            feature = "nonzero_internals",
-            reason = "implementation detail which may disappear or be replaced at any time",
-            issue = "none"
-        )]
-        impl const private::Sealed for $primitive {}
-
         #[unstable(

it would make cargo check succeed but cargo build produce this ICE:

thread 'rustc' panicked at /git/rust/compiler/rustc_abi/src/layout.rs:432:14:
nonscalar layout for layout_scalar_valid_range type: Layout {
...
query stack during panic:
#0 [layout_of] computing layout of `core::num::nonzero::NonZero<Thing>`
#1 [layout_of] computing layout of `core::option::Option<core::num::nonzero::NonZero<Thing>>`
#2 [mir_drops_elaborated_and_const_checked] elaborating drops for `main`
#3 [analysis] running analysis passes on this crate
end of query stack

But a safe standard library code change turning a compiler error into an ICE is not a soundness issue, and I am sure there must be lots of other standard library safe code changes that would do similar. A prerequisite for the standard library being unsound is that user code containing only correct unsafe code would need to compile successfully into a runnable program, instead of check-able program that causes ICE in build — and in any case the unsafe impl in that example is already incorrect, as you've called out.

And I then noticed that the same code even without feature and without any libcore change also triggers the ICE, so the Sealed impls ultimately aren't even relevant to preventing it.

// cargo +nightly-2024-03-17 build

use std::num::{NonZero, ZeroablePrimitive};

#[derive(Copy, Clone)]
#[repr(transparent)]
struct Thing([u8; 3]);

unsafe impl ZeroablePrimitive for Thing {}

fn main() {
    let _ = NonZero::new(Thing([1, 1, 1]));
}

Thanks for the responses!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants