[derive] IntoBytes on unions requires --cfg (#1804)

Makes progress on #1792

Author: joshlf

src/lib.rs

@@ -4668,8 +4668,8 @@ fn mut_from_prefix_suffix<T: FromBytes + KnownLayout + ?Sized>(
 ///
 /// This derive analyzes, at compile time, whether the annotated type satisfies
 /// the [safety conditions] of `IntoBytes` and implements `IntoBytes` if it is
-/// sound to do so. This derive can be applied to structs, enums, and unions;
-/// e.g.:
+/// sound to do so. This derive can be applied to structs and enums (see below
+/// for union support); e.g.:
 ///
 /// ```
 /// # use zerocopy_derive::{IntoBytes};
@@ -4689,15 +4689,6 @@ fn mut_from_prefix_suffix<T: FromBytes + KnownLayout + ?Sized>(
 ///     ...
 /// # */
 /// }
-///
-/// #[derive(IntoBytes)]
-/// #[repr(C)]
-/// union MyUnion {
-/// #   variant: u8,
-/// # /*
-///     ...
-/// # */
-/// }
 /// ```
 ///
 /// [safety conditions]: trait@IntoBytes#safety
@@ -4727,6 +4718,31 @@ fn mut_from_prefix_suffix<T: FromBytes + KnownLayout + ?Sized>(
 ///
 /// [type layout]: https://doc.rust-lang.org/reference/type-layout.html
 ///
+/// # Unions
+///
+/// Currently, union bit validity is [up in the air][union-validity], and so
+/// zerocopy does not support `#[derive(IntoBytes)]` on unions by default.
+/// However, implementing `IntoBytes` on a union type is likely sound on all
+/// existing Rust toolchains - it's just that it may become unsound in the
+/// future. You can opt-in to `#[derive(IntoBytes)]` support on unions by
+/// passing the unstable `zerocopy_derive_union_into_bytes` cfg:
+///
+/// ```shell
+/// $ RUSTFLAGS='--cfg zerocopy_derive_union_into_bytes' cargo build
+/// ```
+///
+/// We make no stability guarantees regarding this cfg, and may remove it at any
+/// point.
+///
+/// We are actively working with Rust to stabilize the necessary language
+/// guarantees to support this in a forwards-compatible way, which will enable
+/// us to remove the cfg gate. As part of this effort, we need to know how much
+/// demand there is for this feature. If you would like to use `IntoBytes` on
+/// unions, [please let us know][discussion].
+///
+/// [union-validity]: https://github.com/rust-lang/unsafe-code-guidelines/issues/438
+/// [discussion]: https://github.com/google/zerocopy/discussions/1802
+///
 /// # Analysis
 ///
 /// *This section describes, roughly, the analysis performed by this derive to
@@ -4790,15 +4806,6 @@ pub use zerocopy_derive::IntoBytes;
 ///     ...
 /// # */
 /// }
-///
-/// #[derive(IntoBytes)]
-/// #[repr(C)]
-/// union MyUnion {
-/// #   variant: u8,
-/// # /*
-///     ...
-/// # */
-/// }
 /// ```
 ///
 /// This derive performs a sophisticated, compile-time safety analysis to

tools/cargo-zerocopy/src/main.rs

@@ -164,10 +164,11 @@ fn install_toolchain_or_exit(versions: &Versions, name: &str) -> Result<(), Erro
 }
 
 fn get_rustflags(name: &str) -> &'static str {
+    // See #1792 for context on zerocopy_derive_union_into_bytes.
     if name == "nightly" {
-        "--cfg __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS "
+        "--cfg __ZEROCOPY_INTERNAL_USE_ONLY_NIGHTLY_FEATURES_IN_TESTS --cfg zerocopy_derive_union_into_bytes "
     } else {
-        ""
+        "--cfg zerocopy_derive_union_into_bytes "
     }
 }
 

zerocopy-derive/Cargo.toml

@@ -22,6 +22,10 @@ repository = "https://github.com/google/zerocopy"
 # [1] https://github.com/rust-lang/crater
 exclude = [".*", "tests/enum_from_bytes.rs", "tests/ui-nightly/enum_from_bytes_u16_too_few.rs.disabled"]
 
+[lints.rust]
+# See #1792 for more context.
+unexpected_cfgs = { level = "warn", check-cfg = ['cfg(zerocopy_derive_union_into_bytes)'] }
+
 [lib]
 proc-macro = true
 

zerocopy-derive/src/lib.rs

@@ -949,6 +949,17 @@ fn derive_into_bytes_enum(ast: &DeriveInput, enm: &DataEnum) -> Result<TokenStre
 /// - `repr(C)`, `repr(transparent)`, or `repr(packed)`
 /// - no padding (size of union equals size of each field type)
 fn derive_into_bytes_union(ast: &DeriveInput, unn: &DataUnion) -> Result<TokenStream, Error> {
+    // See #1792 for more context.
+    let cfg_compile_error = quote!(
+        const _: () = {
+            #[cfg(not(zerocopy_derive_union_into_bytes))]
+            ::zerocopy::util::macro_util::core_reexport::compile_error!(
+                "requires --cfg zerocopy_derive_union_into_bytes;
+please let us know you use this feature: https://github.com/google/zerocopy/discussions/1802"
+            );
+        };
+    );
+
     // TODO(#10): Support type parameters.
     if !ast.generics.params.is_empty() {
         return Err(Error::new(Span::call_site(), "unsupported on types with type parameters"));
@@ -966,15 +977,16 @@ fn derive_into_bytes_union(ast: &DeriveInput, unn: &DataUnion) -> Result<TokenSt
         ));
     }
 
-    Ok(impl_block(
+    let impl_block = impl_block(
         ast,
         unn,
         Trait::IntoBytes,
         FieldBounds::ALL_SELF,
         SelfBounds::None,
         Some(PaddingCheck::Union),
         None,
-    ))
+    );
+    Ok(quote!(#cfg_compile_error #impl_block))
 }
 
 /// A struct is `Unaligned` if:

zerocopy-derive/src/output_tests.rs

@@ -332,6 +332,40 @@ fn test_into_bytes() {
     }
 }
 
+#[test]
+fn test_union_into_bytes() {
+    // Rustfmt spuriously adds spaces after the newline in the middle of the
+    // string literal.
+    #[rustfmt::skip]
+    test! {
+        IntoBytes {
+            #[repr(C)]
+            union Foo {
+                a: u8,
+            }
+        } expands to {
+            const _: () = {
+                #[cfg(not(zerocopy_derive_union_into_bytes))]
+                ::zerocopy::util::macro_util::core_reexport::compile_error!(
+                    "requires --cfg zerocopy_derive_union_into_bytes;
+please let us know you use this feature: https://github.com/google/zerocopy/discussions/1802"
+                );
+            };
+            #[allow(deprecated)]
+            unsafe impl ::zerocopy::IntoBytes for Foo
+            where
+                u8: ::zerocopy::IntoBytes,
+                (): ::zerocopy::util::macro_util::PaddingFree<
+                    Foo,
+                    { ::zerocopy::union_has_padding!(Foo, [u8]) },
+                >,
+            {
+                fn only_derive_is_allowed_to_implement_this_trait() {}
+            }
+        } no_build
+    }
+}
+
 #[test]
 fn test_unaligned() {
     test! {

zerocopy-derive/tests/trybuild.rs

@@ -6,11 +6,10 @@
 // This file may not be copied, modified, or distributed except according to
 // those terms.
 
+use std::env;
 use testutil::set_rustflags_w_warnings;
 
-#[test]
-#[cfg_attr(miri, ignore)]
-fn ui() {
+fn test(subdir: &str) {
     let version = testutil::ToolchainVersion::extract_from_pwd().unwrap();
     // See the doc comment on this method for an explanation of what this does
     // and why we store source files in different directories.
@@ -21,5 +20,38 @@ fn ui() {
     set_rustflags_w_warnings();
 
     let t = trybuild::TestCases::new();
-    t.compile_fail(format!("tests/{}/*.rs", source_files_dirname));
+    t.compile_fail(format!("tests/{}/{}/*.rs", source_files_dirname, subdir));
+}
+
+#[test]
+#[cfg_attr(miri, ignore)]
+fn ui() {
+    test("");
+
+    // This tests the behavior when `--cfg zerocopy_derive_union_into_bytes` is
+    // not present, so remove it. If this logic is wrong, that's fine - it will
+    // exhibit as a test failure that we can debug at that point.
+    let rustflags = env::var("RUSTFLAGS").unwrap();
+    let new_rustflags = rustflags.replace("--cfg zerocopy_derive_union_into_bytes", "");
+
+    // SAFETY: None of our code is concurrently accessinv env vars. It's
+    // possible that the test framework has spawned other threads that are
+    // concurrently accessing env vars, but we can't do anything about that.
+    #[allow(unused_unsafe)] // `set_var` is safe on our MSRV.
+    unsafe {
+        env::set_var("RUSTFLAGS", new_rustflags)
+    };
+
+    test("union_into_bytes_cfg");
+
+    // Reset RUSTFLAGS in case we later add other tests which rely on its value.
+    // This isn't strictly necessary, but it's easier to add this now when we're
+    // thinking about the semantics of these env vars than to debug later when
+    // we've forgotten about it.
+    //
+    // SAFETY: See previous safety comment.
+    #[allow(unused_unsafe)] // `set_var` is safe on our MSRV.
+    unsafe {
+        env::set_var("RUSTFLAGS", rustflags)
+    };
 }

zerocopy-derive/tests/ui-msrv/union_into_bytes_cfg/union_into_bytes_cfg.rs

@@ -0,0 +1 @@
+../../ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.rs

zerocopy-derive/tests/ui-msrv/union_into_bytes_cfg/union_into_bytes_cfg.stderr

@@ -0,0 +1,8 @@
+error: requires --cfg zerocopy_derive_union_into_bytes;
+please let us know you use this feature: https://github.com/google/zerocopy/discussions/1802
+  --> tests/ui-msrv/union_into_bytes_cfg/union_into_bytes_cfg.rs:20:10
+   |
+20 | #[derive(IntoBytes)]
+   |          ^^^^^^^^^
+   |
+   = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

zerocopy-derive/tests/ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.rs

@@ -0,0 +1,26 @@
+// Copyright 2024 The Fuchsia Authors
+//
+// Licensed under a BSD-style license <LICENSE-BSD>, Apache License, Version 2.0
+// <LICENSE-APACHE or https://www.apache.org/licenses/LICENSE-2.0>, or the MIT
+// license <LICENSE-MIT or https://opensource.org/licenses/MIT>, at your option.
+// This file may not be copied, modified, or distributed except according to
+// those terms.
+
+//! See: https://github.com/google/zerocopy/issues/553
+//! zerocopy must still allow derives of deprecated types.
+//! This test has a hand-written impl of a deprecated type, and should result in a compilation
+//! error. If zerocopy does not tack an allow(deprecated) annotation onto its impls, then this
+//! test will fail because more than one compile error will be generated.
+#![deny(deprecated)]
+
+extern crate zerocopy;
+
+use zerocopy::IntoBytes;
+
+#[derive(IntoBytes)]
+#[repr(C)]
+union Foo {
+    a: u8,
+}
+
+fn main() {}

zerocopy-derive/tests/ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.stderr

@@ -0,0 +1,8 @@
+error: requires --cfg zerocopy_derive_union_into_bytes;
+       please let us know you use this feature: https://github.com/google/zerocopy/discussions/1802
+  --> tests/ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.rs:20:10
+   |
+20 | #[derive(IntoBytes)]
+   |          ^^^^^^^^^
+   |
+   = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

zerocopy-derive/tests/ui-stable/union_into_bytes_cfg/union_into_bytes_cfg.rs

@@ -0,0 +1 @@
+../../ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.rs

zerocopy-derive/tests/ui-stable/union_into_bytes_cfg/union_into_bytes_cfg.stderr

@@ -0,0 +1,8 @@
+error: requires --cfg zerocopy_derive_union_into_bytes;
+       please let us know you use this feature: https://github.com/google/zerocopy/discussions/1802
+  --> tests/ui-stable/union_into_bytes_cfg/union_into_bytes_cfg.rs:20:10
+   |
+20 | #[derive(IntoBytes)]
+   |          ^^^^^^^^^
+   |
+   = note: this error originates in the derive macro `IntoBytes` (in Nightly builds, run with -Z macro-backtrace for more info)