From c20262aeeb45ca7986392680535ab0b1f8d1a810 Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Thu, 3 Oct 2024 11:00:34 -0700 Subject: [PATCH] [derive] IntoBytes on unions requires --cfg (#1804) Makes progress on #1792 --- src/lib.rs | 47 +++++++++++-------- tools/cargo-zerocopy/src/main.rs | 5 +- zerocopy-derive/Cargo.toml | 4 ++ zerocopy-derive/src/lib.rs | 16 ++++++- zerocopy-derive/src/output_tests.rs | 34 ++++++++++++++ zerocopy-derive/tests/trybuild.rs | 40 ++++++++++++++-- .../union_into_bytes_cfg.rs | 1 + .../union_into_bytes_cfg.stderr | 8 ++++ .../union_into_bytes_cfg.rs | 26 ++++++++++ .../union_into_bytes_cfg.stderr | 8 ++++ .../union_into_bytes_cfg.rs | 1 + .../union_into_bytes_cfg.stderr | 8 ++++ 12 files changed, 170 insertions(+), 28 deletions(-) create mode 120000 zerocopy-derive/tests/ui-msrv/union_into_bytes_cfg/union_into_bytes_cfg.rs create mode 100644 zerocopy-derive/tests/ui-msrv/union_into_bytes_cfg/union_into_bytes_cfg.stderr create mode 100644 zerocopy-derive/tests/ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.rs create mode 100644 zerocopy-derive/tests/ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.stderr create mode 120000 zerocopy-derive/tests/ui-stable/union_into_bytes_cfg/union_into_bytes_cfg.rs create mode 100644 zerocopy-derive/tests/ui-stable/union_into_bytes_cfg/union_into_bytes_cfg.stderr diff --git a/src/lib.rs b/src/lib.rs index 52cb67adce..26ea5786d9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4668,8 +4668,8 @@ fn mut_from_prefix_suffix( /// /// 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( /// ... /// # */ /// } -/// -/// #[derive(IntoBytes)] -/// #[repr(C)] -/// union MyUnion { -/// # variant: u8, -/// # /* -/// ... -/// # */ -/// } /// ``` /// /// [safety conditions]: trait@IntoBytes#safety @@ -4727,6 +4718,31 @@ fn mut_from_prefix_suffix( /// /// [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 diff --git a/tools/cargo-zerocopy/src/main.rs b/tools/cargo-zerocopy/src/main.rs index 17eddbcbc8..ec179b54f3 100644 --- a/tools/cargo-zerocopy/src/main.rs +++ b/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 " } } diff --git a/zerocopy-derive/Cargo.toml b/zerocopy-derive/Cargo.toml index 9364ff5354..2dcad5f1a0 100644 --- a/zerocopy-derive/Cargo.toml +++ b/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 diff --git a/zerocopy-derive/src/lib.rs b/zerocopy-derive/src/lib.rs index c6f8f193e5..3370780c6f 100644 --- a/zerocopy-derive/src/lib.rs +++ b/zerocopy-derive/src/lib.rs @@ -949,6 +949,17 @@ fn derive_into_bytes_enum(ast: &DeriveInput, enm: &DataEnum) -> Result Result { + // 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,7 +977,7 @@ fn derive_into_bytes_union(ast: &DeriveInput, unn: &DataUnion) -> Result Result, + { + fn only_derive_is_allowed_to_implement_this_trait() {} + } + } no_build + } +} + #[test] fn test_unaligned() { test! { diff --git a/zerocopy-derive/tests/trybuild.rs b/zerocopy-derive/tests/trybuild.rs index 8506d740d9..257f61c332 100644 --- a/zerocopy-derive/tests/trybuild.rs +++ b/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) + }; } diff --git a/zerocopy-derive/tests/ui-msrv/union_into_bytes_cfg/union_into_bytes_cfg.rs b/zerocopy-derive/tests/ui-msrv/union_into_bytes_cfg/union_into_bytes_cfg.rs new file mode 120000 index 0000000000..66ef0de5be --- /dev/null +++ b/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 \ No newline at end of file diff --git a/zerocopy-derive/tests/ui-msrv/union_into_bytes_cfg/union_into_bytes_cfg.stderr b/zerocopy-derive/tests/ui-msrv/union_into_bytes_cfg/union_into_bytes_cfg.stderr new file mode 100644 index 0000000000..d25c238f6e --- /dev/null +++ b/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) diff --git a/zerocopy-derive/tests/ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.rs b/zerocopy-derive/tests/ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.rs new file mode 100644 index 0000000000..280f05d410 --- /dev/null +++ b/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 , Apache License, Version 2.0 +// , or the MIT +// license , 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() {} diff --git a/zerocopy-derive/tests/ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.stderr b/zerocopy-derive/tests/ui-nightly/union_into_bytes_cfg/union_into_bytes_cfg.stderr new file mode 100644 index 0000000000..d687c22a88 --- /dev/null +++ b/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) diff --git a/zerocopy-derive/tests/ui-stable/union_into_bytes_cfg/union_into_bytes_cfg.rs b/zerocopy-derive/tests/ui-stable/union_into_bytes_cfg/union_into_bytes_cfg.rs new file mode 120000 index 0000000000..66ef0de5be --- /dev/null +++ b/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 \ No newline at end of file diff --git a/zerocopy-derive/tests/ui-stable/union_into_bytes_cfg/union_into_bytes_cfg.stderr b/zerocopy-derive/tests/ui-stable/union_into_bytes_cfg/union_into_bytes_cfg.stderr new file mode 100644 index 0000000000..e4e35d6256 --- /dev/null +++ b/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)