Allow external impls of Bits and BitFlags about bitflags HOT 12 CLOSED

Ok, I think I've got a design that should resolve #351, and avoid needing to add direct support for unstable libraries like #325.

The idea is you define your flags type manually, with the underlying bits type as a field, and then just use the bitflags! macro to generate the boilerplate without any internal field involved. That means if you #[derive(Serialize)] etc you won't get the flags-aware implementation, but we can expose that for you to use. If you want to derive zerocopy or bevy or integrate with other libraries that bitflags doesn't directly support then you can do that yourself. It looks like this:

// First: Define your flags type. It needs to be a newtype over its underlying bits type
pub struct ManualFlags(u32);

// Next: use `impl Flags` instead of `struct Flags`
bitflags! {
    impl ManualFlags: u32 {
        const A = 0b00000001;
        const B = 0b00000010;
        const C = 0b00000100;
        const ABC = Self::A.bits() | Self::B.bits() | Self::C.bits();
    }
}

from bitflags.

Yeh I think this changes the calculus of what libraries we’ll support directly going forwards to just those that are ubiquitous, and have something to gain from knowing the type is a flags enum. For others, you can now implement them yourself with minimal loss of functionality. It’s better for everyone this way I think.

from bitflags.

@KodrAus Thanks! That will massively reduce the burden on bitflags as well as get rid of the "slowdown" when users inevitably want to derive new/custom implementations.

Really curious how you'll expose the flags-aware Display/Debug (and (De)serialize) derives though :)

I could see cases where I would want to just serialize as a u8 for network bandwidth reasons, and other cases where the human readable format makes more sense, so having the choice is great!

@paul-hansen wasn't this already implemented in bitflags 2 where the serializer "detects" whether the target format is binary or intended to be "user readable" with strings?

https://github.com/bitflags/bitflags/releases/tag/2.0.0

The default serialization format with serde has changed if you #[derive(Serialize, Deserialize)] on your generated flags types. It will now use a formatted string for human-readable formats and the underlying bits type for compact formats.

from bitflags.

As for Bits, it can be reasonably reduced to:

pub trait Bits:
    Clone
    + Copy
    + BitAnd<Output = Self>
    + BitOr<Output = Self>
    + BitXor<Output = Self>
    + Not<Output = Self>
    + PartialEq
    + Sized
    + 'static
{
    /// The value of `Self` where no bits are set.
    const EMPTY: Self;

    /// The value of `Self` where all bits are set.
    const ALL: Self;
}

The Copy and 'static bounds are pretty limiting, but doesn't rule out some more exotic backing storage like [bool; N].

from bitflags.

The main thing you don't get from just the BitFlags macro right now, and what stops us being able to immediately replace a lot of generated code with it, is the const definitions, and const versions of those trait functions. We could consider a few utility macros that we transition bitflags! to, that will define costs and impl operators for you:

impl_consts! {
    MyFlags: u32 {
        const A = 0b0000_0001;
        const B = 0b0000_0010;
        const C = 0b0000_0100;
    }
}

would generate:

impl MyFlags {
    const FLAGS: &'static [(&'static str, Self::Bits)] = &[
        ("A", 0b0000_0001),
        ("B", 0b0000_0010),
        ("C", 0b0000_0100),
    ];

    pub const A: Self = Self::from_bits_retain(0b0000_0001);
    pub const B: Self = Self::from_bits_retain(0b0000_0010);
    pub const C: Self = Self::from_bits_retain(0b0000_0100);
}

you could then use Self::FLAGS as the basis for the const in the BitFlags trait, so there's still only a single definition of what those flags are.

from bitflags.

Some other open design questions are:

1. Should we use slices like &[(&str, T)], or some wrapper type, like Flags that also implements indexing.
2. Should we use tuples like (&str, T), or some flags type like Flag. Having a separate type would let us add metadata in the future, such as whether or not the flag is a composite of other flags.

I'm happy with &[(&str, T)], but open to trying a more encapsulated API if it doesn't affect ergonomics or create too many extra types. It's certainly more future proof.

from bitflags.

Nice! Personally I'm liking the look of that a lot. While admirable that you were willing to do implementations for all those crates, I think leaving it up to the end user will be more flexible and probably cause a lot less version churn in this crate. I could see cases where I would want to just serialize as a u8 for network bandwidth reasons, and other cases where the human readable format makes more sense, so having the choice is great!

from bitflags.

@MarijnS95, the way I've done this in #351 is by exposing functions from bitflags when those features are enabled. For serde::Serialize it looks like this:

/// Serialize a set of flags as a human-readable string or their underlying bits.
pub fn serialize<B: Flags, S: Serializer>(
    flags: &B,
    serializer: S,
) -> Result<S::Ok, S::Error>
where
    B::Bits: LowerHex + Serialize,
{
    // Serialize human-readable flags as a string like `"A | B"`
    if serializer.is_human_readable() {
        serializer.collect_str(&parser::AsDisplay(flags))
    }
    // Serialize non-human-readable flags directly as the underlying bits
    else {
        flags.bits().serialize(serializer)
    }
}

You don't get automatic #[derive] support, but can implement traits like Serialize yourself by forwarding to these functions:

impl Serialize for MyFlags {
    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
        bitflags::serde::serialize(self, serializer)
    }
}

I'll be sure to add an example of these in #351.

from bitflags.

Surely add an example. It's a bit too verbose for my taste, anything we could do to automate this as part of the bitflags! { impl MyType system?

Does this new/separate system also leave any reason to have the double-newtype indirection from bitflags 2.0.0?

from bitflags.

anything we could do to automate this as part of the bitflags! { impl MyType system?

This was the case that internal field was originally added for; so that you could #[derive] traits on your flags type and get flags-aware implementations. I wouldn't want to take the bitflags! macro itself any further into inventing semantics or attributes, but we could consider a crate that redefined a few custom derives that forwarded to the bitflags specialized implementation:

#[derive(bitflags_serde::Serialize)]
pub struct MyFlags(u32);

bitflags! {
    impl MyFlags: u32 { .. }
}

It's starting to get a bit complicated there by needing a few different kinds of macros, but could be experimented with.

from bitflags.

Ok, I think I've got a design that should resolve #351, and avoid needing to add direct support for unstable libraries like #325.

The idea is you define your flags type manually, with the underlying bits type as a field, and then just use the bitflags! macro to generate the boilerplate without any internal field involved. That means if you #[derive(Serialize)] etc you won't get the flags-aware implementation, but we can expose that for you to use. If you want to derive zerocopy or bevy or integrate with other libraries that bitflags doesn't directly support then you can do that yourself. It looks like this:

// First: Define your flags type. It needs to be a newtype over its underlying bits type
pub struct ManualFlags(u32);

// Next: use `impl Flags` instead of `struct Flags`
bitflags! {
    impl ManualFlags: u32 {
        const A = 0b00000001;
        const B = 0b00000010;
        const C = 0b00000100;
        const ABC = Self::A.bits() | Self::B.bits() | Self::C.bits();
    }
}

I assume this has already been considered, but I couldn't find any discussion of it, so I figured I'd ask just in case. Is there a reason that you couldn't just copy user-supplied attributes to the generated InternalBitFlags type? I did a quick hack to get the following to work:

bitflags! {
    #[derive(zerocopy::FromBytes)]
    struct Flags: u32 {
        const A = 0b00000001;
        const B = 0b00000010;
        const C = 0b00000100;
        const ABC = Self::A.bits() | Self::B.bits() | Self::C.bits();
    }
}

This has the advantage of not requiring bitflags to know anything about the crate that supplies your custom derive, so it should Just Work in most cases (I assume there are cases where the behavior of the derive would result in it rejecting the internal type, or not composing when derived on both the internal and external types, etc).

Here's the diff. Obviously this is nowhere near complete or correct, and breaks lots of tests and stuff, but I just wanted to confirm that something like this would work in principle.

$ git diff
diff --git a/examples/custom_derive.rs b/examples/custom_derive.rs
index 5a85afb..61e5084 100644
--- a/examples/custom_derive.rs
+++ b/examples/custom_derive.rs
@@ -20,4 +20,15 @@ bitflags! {
     }
 }
 
+bitflags! {
+    #[derive(zerocopy::FromBytes)]
+    struct Flags: u32 {
+        const A = 0b00000001;
+        const B = 0b00000010;
+        const C = 0b00000100;
+        const ABC = Self::A.bits() | Self::B.bits() | Self::C.bits();
+    }
+}
+
 fn main() {}
diff --git a/src/internal.rs b/src/internal.rs
index aca1ac4..9c2843a 100644
--- a/src/internal.rs
+++ b/src/internal.rs
@@ -10,12 +10,14 @@
 #[doc(hidden)]
 macro_rules! __declare_internal_bitflags {
     (
+        $(#[$outer:meta])*
         $vis:vis struct $InternalBitFlags:ident: $T:ty
     ) => {
         // NOTE: The ABI of this type is _guaranteed_ to be the same as `T`
         // This is relied on by some external libraries like `bytemuck` to make
         // its `unsafe` trait impls sound.
-        #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+        $(#[$outer])*
+        #[derive(Copy, Clone)]
         #[repr(transparent)]
         $vis struct $InternalBitFlags($T);
     };
diff --git a/src/lib.rs b/src/lib.rs
index 3d5377b..afad109 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -487,6 +487,7 @@ macro_rules! bitflags {
             // Declared in a "hidden" scope that can't be reached directly
             // These types don't appear in the end-user's API
             __declare_internal_bitflags! {
+                $(#[$outer])*
                 $vis struct InternalBitFlags: $T
             }

from bitflags.

Friendly ping on this 🙂 I'd like to figure out how zerocopy can support bitflags, as a lot of our users use bitflags.

from bitflags.