2.0.0 changelog appears incomplete

[MarijnS95]

Context

I maintain a few crates that provide bitflags types in their public API, while the crates themselves don't really use these types heavily. As such I won't notice changed/missing APIs until these crates are published and used out in the wild.

Most importantly, besides listing commit/PR titles in a changelog, it should serve as a human readable "migration guide" explaining the outcome or expected action from consumer crates. Right now this appears to be lacking, forcing consumers to either blindly whack some derives on types or features on the crate dependency, or search through this repository and PRs to understand whether that's the right/intended thing to do in the first place.

Cannot derive serde on bitflags anymore

Some digging seems points to bitflags now being a newtype wrapper around an internal implementation, which requires the serde feature (disabled by default in serde: enable no-std support by @nim65s in https://github.com/bitflags/bitflags/pull/296) to be enabled to get this propagated. The entire effect of this newtype wrapper should have had a honorable mention in the changelog 🙂

EDIT: Even worse, #282 makes the following mention:

If you previously had a #[derive(Serialize)] impl, you'll need a #[serde(transparent)] attribute to ensure your format doesn't change.

Again, such kind of breakage is exactly the thing to describe in the changelog 😅

Changes to serde serialization

2.0.0-rc.2 has a big warning that the serialization format changed: https://github.com/bitflags/bitflags/blob/main/CHANGELOG.md#200-rc2, yet this warning was not propagated to the main 2.0.0 changelog: https://github.com/bitflags/bitflags/releases/tag/2.0.0

Debug, Copy, Clone, Hash, (Partial)Eq etc not automatically derived anymore

Also appears to be explicitly asked in the issues and happening as a result of the newtype wrapper, but nothing explaining this in the changelog. Would be helpful if it contains a full list of all the trait implementations that have been "removed" going from 1.3.2 to 2.0.0.

---

Regardless of all that, thanks for making all these changes and improvements! I'm sure they'll get appreciated, as long as such breaking effects get the attention they deserve in the changelog.

[MarijnS95]

On a related note (though could probably be asked in a distinct issue): how do we now derive features on the inner type? I'm used to deriving for example rkyv::Archive, rkyv::Serialize, rkyv::Deserialize and bytemuck::Pod, bytemuck::Zeroable on bitflags, but that doesn't seem possible anymore (as in: deriving these on the public type requires it to be derived/implemented on the inner type as well) unless there's a passthrough attribute?

For example rkyv provides a #[archive_attr(derive(...))] to emit these on the inner *Archive type it generates for the user.

[KodrAus]

Hi @MarijnS95 👋

Thanks for pointing this all out and for digging into it! We should update our release notes to make upgrading from 1.x to 2.x as straightforward as possible.

As for how to derive traits on the inner type, that's no longer possible from outside of bitflags directly. That was one of the goals of refactoring the internals so that we can control how external traits are implemented for it with some awareness of the fact that it's a flags type.

What we should do now is add feature flags to bitflags that enables support for external libraries like bytemuck (which is stable) and rkyv (which is unstable). For unstable libraries like rkyv, we'll need to come up with a scheme that lets us upgrade them while they remain unstable. A scheme other libraries have used in the past is a RUSTFLAGS opt-in. Those are both libraries I'd be happy to add support for in bitflags.

[MarijnS95]

@KodrAus thanks! Looking forward to the release note updates!

Indeed, that's what other crates like glam do: they manually derive/implement a great bunch of these "standard" crates, hidden behind feature flags, even things like rand:

https://github.com/bitshifter/glam-rs/blob/38345b94ddd8c443b8a2d638b9a69d6ed1e45abd/Cargo.toml#L48-L55

For unstable libraries like rkyv, we'll need to come up with a scheme that lets us upgrade them while they remain unstable.

We've seen crates like nalgebra depend on multiple versions of glam via versioned feature flags: glam022, glam023 etc to turn on wrappers for specific versions, without unintentionally affecting the public bitflags API.

[mxk]

I just finished migrating one of my projects to v2 and, in addition to the issues mentioned here, one problem I ran into is lost support for multi-bit masks. This was especially surprising when serde serialize/deserialize round-trip produced a rather confusing test failure:

#[test]
fn bit_mask() {
    bitflags::bitflags! {
        #[derive(Clone, Copy, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
        #[repr(transparent)]
        #[serde(transparent)]
        pub struct Flags: u8 {
            const BIT = 1 << 0;
            const MASK = 0xF << 1;
        }
    }
    let want = Flags::from_bits_retain(3);
    let json = serde_json::to_string(&want).unwrap();
    let have: Flags = serde_json::from_str(&json).unwrap();
    assert_eq!(have, want);
}

Output:

thread 'tests::bit_mask' panicked at 'assertion failed: `(left == right)`
  left: `Flags(BIT)`,
 right: `Flags(BIT)`'

Once I added .bits() to the assertion, I saw that all the MASK bits were getting stripped during serialization. The workaround seems to be to move the MASK const into a separate impl.

[KodrAus]

I've started work on improving the release notes: https://github.com/bitflags/bitflags/releases/tag/2.0.0 I'll keep working on them to better call out sources of breakage and mitigations for them. Thanks for all the help so far everyone!

@mxk Hmm, that looks like a bug to me. You should get the value BIT | 0x2 serialized since that extra bit doesn't fully correspond to a flag, which would then roundtrip to 3. Instead you're just getting BIT. I'll raise an issue for it.

[MarijnS95]

@KodrAus thanks, that is already looking much better! A few nits:

• Two derive( examples are missing the closing ) parenthesis;
• For traits: clarify that the listed things only all need to be derived if the user wishes to maintain some form of "backwards compatibility" with 1.x. In other words: there's no must to derive these, it's just a convenient list of exactly what was derived/provided/available in 1.x;
• .bits is the only thing under "Other notable changes", perhaps just make a :warning: `.bits` field for it next to the main changes?
• The explanation for .bits is rather verbose, perhaps there's a more concise way to explain the user simply needs to use the method now?
• Don't forget to mention the #[serde(transparent)] annotation. Or perhaps this can internally be fixed (i.e. via https://serde.rs/attr-flatten.html)? I doubt anyone would want to have the internal bitflags representation as a side-effect in their (de)serialized format.

[KodrAus]

Thanks @MarijnS95 👍 I've made a few more changes to the release notes based on your feedback.

I think the ship has probably sailed on #[serde(flatten)] since we've already published stable releases without it. It's unlikely to affect anybody, but since it would technically be a breaking change that could be picked up by code that integrates serde with other frameworks I think we should leave it.

[MarijnS95]

#[derive(Serialize, Deserialize] is still without closing parenthesis 😉

Sure, it would be another breaking change, perhaps it can be queued up for bitflags 3 😅 as I doubt anyone would want to see bitflags internals in their serialized formats - and by the time they find out it's probably already too late for them too.

Regardless, #[serde(transparent)] is added to the example but don't forget to also point this out explicitly, it might be easy to miss by those only reading the explanation and not copy-pasting the example.

Changes look good otherwise, many thanks @KodrAus!

[molpopgen]

In trying to update a crate to 2.0.2, cargo-semver-checks was failing because many functions that were pub const in 1.x are no longer const. I couldn't find this mentioned in the changelog.

[KodrAus]

@molpopgen Hmm, we shouldn't have dropped any const support. Running cargo-semver-checks myself on the following public definition:

bitflags::bitflags! {
    pub struct Flags: u8 {
        const A = 0b0000_0001;
        const B = 0b0000_0010;
        const C = 0b0000_0100;
    }
}

changing only the underlying bitflags version from 1 to 2, I get warned about missing traits and from_bits_unsafe:

Completed [   0.023s] 37 checks; 35 passed, 2 failed, 6 unnecessary

--- failure derive_trait_impl_removed: built-in derived trait no longer implemented ---

Description:
A public type has stopped deriving one or more traits. This can break downstream code that depends on those types implementing those traits.
        ref: https://doc.rust-lang.org/reference/attributes/derive.html#derive
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.19.0/src/lints/derive_trait_impl_removed.ron

Failed in:
  type Flags no longer derives Eq, in v2\src\lib.rs:1
  type Flags no longer derives PartialEq, in v2\src\lib.rs:1
  type Flags no longer derives Copy, in v2\src\lib.rs:1
  type Flags no longer derives Hash, in v2\src\lib.rs:1
  type Flags no longer derives Ord, in v2\src\lib.rs:1
  type Flags no longer derives PartialOrd, in v2\src\lib.rs:1
  type Flags no longer derives Clone, in v2\src\lib.rs:1

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.19.0/src/lints/inherent_method_missing.ron

Which functions are you seeing reported as no longer const?

[molpopgen]

I am getting this (cut for brevity):

The very strange thing is that docs.rs does indeed show those as const. But my local build of the docs shows them as not const:

EDIT: I should have been more clear: docs.rs shows these fns as const in the bitflags 2.0.x docs. Building my own docs for my crate locally shows that they are not const with 2.x. But the docs for my crate on docs.rs are const using bitflags 1.3.x, and that is what is triggering semver-checks to fail.

[KodrAus]

I've created a new issue for this odd behavior, but I think the changelog is greatly improved over its original wording so will go ahead and close this one. Any more suggestions for improvements can be given in new issues. Thanks for all the input!