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

Support zerocopy derives #326

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ jobs:
run: cargo install cargo-hack

- name: Powerset
run: cargo hack test --feature-powerset --lib --optional-deps "std serde" --depth 3 --skip rustc-dep-of-std
run: cargo hack test --feature-powerset --lib --optional-deps "std serde zerocopy_0_6" --depth 3 --skip rustc-dep-of-std
Copy link
Member

Choose a reason for hiding this comment

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

After rebasing on main this shouldn't be necessary anymore. We automatically test all of these features.


- name: Docs
run: cargo doc --features example_generated
Expand Down
2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ exclude = ["tests", ".github"]
serde = { version = "1.0", optional = true, default-features = false }
core = { version = "1.0.0", optional = true, package = "rustc-std-workspace-core" }
compiler_builtins = { version = "0.1.2", optional = true }
zerocopy = { version = "0.6.1", optional = true, default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

I think we should do something like:

zerocopy_0_6 = { version = "0.6.1", optional = true, default-features = false, package = "zerocopy" }

here and remove the explicit feature. Otherwise users can depend on zerocopy directly, which will become confusing when a new version is released.


[dev-dependencies]
trybuild = "1.0"
Expand All @@ -35,6 +36,7 @@ serde_test = "1.0"
std = []
example_generated = []
rustc-dep-of-std = ["core", "compiler_builtins"]
zerocopy_0_6 = ["zerocopy"]

[package.metadata.docs.rs]
features = ["example_generated"]
28 changes: 28 additions & 0 deletions examples/zerocopy.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#[cfg(feature = "zerocopy_0_6")]
fn main() {
use bitflags::bitflags;
use zerocopy::{AsBytes, FromBytes};

bitflags! {
#[derive(AsBytes, Debug, Eq, FromBytes, PartialEq)]
#[repr(transparent)]
pub struct Flags: u32 {
const A = 1;
const B = 2;
const C = 4;
const D = 8;
}
}

let flags = Flags::A | Flags::B;
let bytes = flags.as_bytes();
println!("{:?} -> {:?}", flags, bytes);

let flags_from_bytes = Flags::read_from(bytes).unwrap();
println!("{:?} -> {:?}", bytes, flags_from_bytes);

assert_eq!(flags, flags_from_bytes);
}

#[cfg(not(feature = "zerocopy_0_6"))]
fn main() {}
6 changes: 6 additions & 0 deletions src/internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ macro_rules! __declare_internal_bitflags {
$iter_vis:vis struct $Iter:ident;
$iter_names_vis:vis struct $IterNames:ident;
) => {
#[cfg(feature = "zerocopy_0_6")]
use $crate::__private::zerocopy;
#[cfg(feature = "zerocopy_0_6")]
use ::core::marker::Sized;

#[cfg_attr(feature = "zerocopy_0_6", derive($crate::__private::zerocopy::AsBytes, $crate::__private::zerocopy::FromBytes))]
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this won't actually work. This code is expanded in an end-user's library where there's no knowledge of whether or not bitflags has this feature enabled. For other libraries, we instead expand to a macro that calls back into bitflags and generates appropriate impl blocks.

Since this library requires using their custom derive I think we'll need a different strategy that accepts the struct definition as input and annotates it with its attributes. We'll need a second macro that also includes those necessary use statements.

If zerocopy 0.7 that's currently in-progress has a nicer experience for implementing its traits then I think it would be good to wait for it.

Copy link

Choose a reason for hiding this comment

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

Hey, came across this while discussing google/zerocopy#287. Zerocopy 0.7 has been released now. Is the experience you're referring to the need to emit an impl for the function only_derive_is_allowed_to_implement_this_trait or something else? We've recently done some work to make the docs more precise with regards to what needs to hold in order for a trait impl to be sound, so that might help you. Let me know if there's anything else we could add that would make this easier for you!

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[repr(transparent)]
$vis struct $InternalBitFlags {
Expand Down
2 changes: 2 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,8 @@ pub mod __private {

#[cfg(feature = "serde")]
pub use serde;
#[cfg(feature = "zerocopy_0_6")]
pub use zerocopy;
}

/*
Expand Down
2 changes: 1 addition & 1 deletion src/public.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ macro_rules! __declare_public_bitflags {
$vis:vis struct $BitFlags:ident;
) => {
$(#[$outer])*
$vis struct $BitFlags(<Self as $crate::__private::PublicFlags>::Internal);
$vis struct $BitFlags(<$BitFlags as $crate::__private::PublicFlags>::Internal);
};
}

Expand Down