Skip to content

Commit

Permalink
[derive] IntoBytes on unions requires --cfg
Browse files Browse the repository at this point in the history
Makes progress on #1792
  • Loading branch information
joshlf committed Oct 3, 2024
1 parent a9f09d7 commit 8e21a86
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 8 deletions.
5 changes: 3 additions & 2 deletions tools/cargo-zerocopy/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
}
}

Expand Down
4 changes: 4 additions & 0 deletions zerocopy-derive/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
16 changes: 14 additions & 2 deletions zerocopy-derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -934,6 +934,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"));
Expand All @@ -951,15 +962,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:
Expand Down
34 changes: 34 additions & 0 deletions zerocopy-derive/src/output_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Expand Down
40 changes: 36 additions & 4 deletions zerocopy-derive/tests/trybuild.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@
// This file may not be copied, modified, or distributed except according to
// those terms.

use std::{env, mem, sync::Mutex};
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.
Expand All @@ -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));
}

static ENV_MTX: Mutex<()> = Mutex::new(());

#[test]
#[cfg_attr(miri, ignore)]
fn ui() {
let guard = ENV_MTX.lock().unwrap();
test("");
mem::drop(guard);
}

#[test]
#[cfg_attr(miri, ignore)]
fn ui_union_into_bytes_cfg() {
// 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 rustflags = rustflags.replace("--cfg zerocopy_derive_union_into_bytes", "");

let guard = ENV_MTX.lock().unwrap();
// SAFETY: Thanks to holding `ENV_MTX`, we guarantee that the `ui` test is
// not running in this critical section. It's still 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", rustflags)
};
mem::drop(guard);

test("union_into_bytes_cfg");
}

0 comments on commit 8e21a86

Please sign in to comment.