Treatment of unknown bits in operators is inconsistent about bitflags HOT 18 OPEN

It may be that there are different use cases that want different things. In my use case, the bits are defined by an external API, and new flags can be defined that my code doesn't know about yet, and I want my code to default to preserving such flags. In this use case, it's undesirable to have ! call from_bits_truncate, because it makes the flags & !other idiom silently clear unrecognized bits.

In other use cases, I can see how it's desirable to have complement call from_bits_truncate. If some code defines its own bitflags and never deals with bits from the outside world, having those extra bits get set by complement could make things like is_empty() unexpectedly return false.

Would it make sense to add some new syntax to the bitflags macro to allow users to specify when they have the external-API use case, and that all bits should be treated as "known"?

from bitflags.

Here's another related issue. flags - other computes a different result than flags -= other

That’s absolutely a bug and needs to be fixed. It seems like things are basically broken enough to justify fixing them all up together so that unknown bits are always respected. The other niche case we have to consider are multi-bit flags, where they’re treated as only representing a single flag when all related bits are set, not just when any are.

from bitflags.

A related issue is that the difference function's documentation says that it "is also conceptually equivalent to the “bit-clear” operation: flags & !other (and this syntax is also supported)", however it is significantly different in the presence of from_bits_retain:

use bitflags::bitflags;

bitflags! {
    #[derive(Debug, Copy, Clone)]
    pub struct Flags: u32 {
        const A = 0x0100_0000;
        const B = 0x1000_0000;
    }
}

fn main() {
    let flags = Flags::from_bits_retain(0x1111_1111);
    let other = Flags::A;
    
    println!("{:#x}", flags.difference(other));
    println!("{:#x}", flags & !other);
}

prints

0x10111111
0x10000000

from bitflags.

I think this is similar to #327. It seems like we should look at better clarifying how these methods work with unknown bits and adding alternatives that treat them better.

We could also consider it a bug and fix it, but I’m a little reluctant to change the behaviour of any existing methods.

We ended up making from_bits_retain safe because it was already trivially easy to end up with unspecified bits using #[derive]s.

from bitflags.

In other use cases, I can see how it's desirable to have complement call from_bits_truncate. If some code defines its own bitflags and never deals with bits from the outside world, having those extra bits get set by complement could make things like is_empty() unexpectedly return false.

In general, I think we’re trying to be more “unknown-bits-conscious” by default now, so if we ended up changing the behaviour of set methods to consider unknown bits we’d then better document the need to “sanitise” your flags if you support parsing or constructing them from arbitrary values.

Would it make sense to add some new syntax to the bitflags macro to allow users to specify when they have the external-API use case, and that all bits should be treated as "known"?

I’ve really tried to avoid this so the macros don’t stray too much farther into “invented syntax” territory, but can also appreciate how problematic it would be to have alternative methods that you’d need to conscientiously audit your codebase for to ensure subtle bugs wouldn’t slip through.

I think we’re basically at the limits of our complexity budget using macro_rules, but it seems like we’re due for a wholistic look at how we treat unknown bits. It really should be fully consistent so you can apply the same mental model to any code that interacts with flags.

from bitflags.

Here's another related issue. flags - other computes a different result than flags -= other:

use bitflags::bitflags;

bitflags! {
    #[derive(Debug, Copy, Clone)]
    pub struct Flags: u32 {
        const A = 0x0100_0000;
        const B = 0x1000_0000;
    }
}

fn main() {
    let mut flags = Flags::from_bits_retain(0x1111_1111);
    let other = Flags::A;
    
    println!("{:#x}", flags - other);
    
    flags -= other;
    
    println!("{:#x}", flags);

}

prints

0x10111111
0x10000000

This behavior appears to be new in bitflags "2", compared to bitflags "1" (using from_bits_unchecked).

from bitflags.

Here's another apparently related issue. Defining two constants vs. defining one with | appears to change the behavior of -= on unrecognized bits in a surprising way:

use bitflags::bitflags;

const FOO: u32 = 0x0000_100f;
const BAR: u32 = 0x100f_0000;

bitflags! {
    #[derive(Debug, Copy, Clone)]
    pub struct SomeFlags: u32 {
        const A = 0x0000_0000;
        const B = 0x0000_0010;
        const C = 0x0000_0020;
        const Y = FOO;
        const Z = BAR;
    }
}
bitflags! {
    #[derive(Debug, Copy, Clone)]
    pub struct OtherFlags: u32 {
        const A = 0x0000_0000;
        const B = 0x0000_0010;
        const C = 0x0000_0020;
        const Y_Z = FOO | BAR;
    }
}

fn main() {
    let mut some_flags = SomeFlags::from_bits_retain(0x1111_1111);
    some_flags -= SomeFlags::from_bits_retain(FOO);
 
    let mut other_flags = OtherFlags::from_bits_retain(0x1111_1111);
    other_flags -= OtherFlags::from_bits_retain(FOO);
 
    println!("{:#x}", some_flags);
    println!("{:#x}", other_flags);
}

prints

0x10010010
0x10

This is also a behavior change with bitflags "2" compared to "1".

from bitflags.

I’m going to do some work on this over the week. I’ll start by overhauling our test suite so each function is tested in its own file.

from bitflags.

@KodrAus Do you happen to know yet if you'll bump to bitflags 3 for this, or will this be a bitflags 2.x release?

from bitflags.

@sunfishcode I think this will be fixed in a minor or patch release, because it appears to actually be a regression.

from bitflags.

For use cases where the bits are defined externally, having complement mask out unknown bits is still undesirable because it doesn't support the common a & ~b idiom.

I've been thinking about adding const ALL = !0 to all of my bitflags declarations that have externally-defined flags as a way to effectively disable any masking.

For use cases where the bits are not defined externally, allowing unknown bits at all seems like a source of awkward corner cases. So, what if we said that const ALL = !0 is the idiom for enabling support for externally-defined flags, and then have bitflags just always clear or refuse unknown bits?

That would present the question of what to do with from_bits_retain, and perhaps the answer would be to remove it, which I realize isn't easy to do at this point. But, this approach feels like it would avoid surprising hazards for both kinds of use cases without introducing any new syntax, so it seems worth considering.

from bitflags.

Since it’s trivially easy to end up with unknown bits or invalid bit patterns in a flags value through serialization or parsing I think we should behave rationally with them, and from_bits_retain forces us to do that.

If we wanted to always try clear them out though, I guess that would just cost us an extra & Self::all() on each operation as a best-effort. If we wanted to go down that path we could do it as a 3.0.

I do like the ALL = !0 idea as a way to guarantee any bit pattern is valid and avoid the (!self).bits() != !self.bits() footgun.

from bitflags.

At the moment with ALL = !0 I think you might still run afoul of from_bits_truncate, because it will truncate bits that don’t fully correspond to a flag value. This is because a flag isn’t defined as occupying exactly one bit; it may cover multiple; so unless all bits belonging to a flag are set, they’re not considered part of that flag.

That’s the only rational behavior that makes flags like A = 1 and composite flags like ABC = A | B | C work. There’s nothing stopping you from defining ABC without also defining A, B, and C individually. Setting just the bit for A doesn’t on its own mean the flag ABC is also set.

I think we could change ! to perform an & Self::all() instead of from_bits_truncate to be both more performant and to avoid that issue.

from bitflags.

Ok @sunfishcode, how does this sound as a path forwards to you:

1. We merge #366 with its fixes to -= and formatting, and change ! to use & Self::all() instead of Self::from_bits_truncate in a 2.x release.
2. We recommend you define a flag const ALL = !0 for flags types that are defined externally to ensure all bits will be respected under all circumstances.
3. We define the concept of normalization as a value having only bits set that are specified in part or in full by the constants defined for the type. We specify that flags may be unnormalized, but operations may normalize them. Self::from_bits and Self::from_bits_truncate are guaranteed to only ever return normalized values. Self::from_bits_retain and parsing/serialization may produce unnormalized values.
4. We release a 3.0 that applies the same & Self::all() filter to all operators, so they all behave consistently with !.

from bitflags.

Ah, I had been assuming that from_bits_truncate was the same as & Self::all(). I see now the documentation does describe what it does. I keep assuming I can guess what bitflags will do in various situations, and keep getting surprised.

Overall, bitflags' API feels like it's somewhere between two different things. On one hand, from_bits_truncate is defined to be very strict; I imagine that could be desirable for situations where some combinations of flags are valid and others aren't, so that a library API with a bitflags parameter doesn't need to think about validating and error-handing for the invalid-combination cases. But on the other hand, from_bits_retain exists, and intersection and union are infallible, so there are no reliable invariants and library APIs need to think about the invalid-combination cases anyway.

I would understand a type that maintains an invariant that the value is always a union of the defined values, where there is no from_bits_retain, and deserializers are expected to fail if from_bits fails. And perhaps intersection, union, and complement are fallible to handle the invalid-combination cases. Or perhaps it doesn't have opinions about combinations; I'm not sure. In any case, "this type helps code be correct-by-construction".

And, I would understand a tyoe that makes no attempt to maintain invariants, so it has from_bits_retain, from_bits_truncate just does & all(), and nothing else implicitly does & all(). "This type is an integer newtype with conveniences for flags".

But, I don't think I can understand bitflags in its current form. I don't think I'll be able to remember which functions reject invalid combinations, which implicitly do & all() and which can produce unknown flags. I'll probably forget, and assume it has invariants, or assume it doesn't, and either way I'll be surprised sometimes.

from bitflags.

I think that's a valid criticism and reflects the in-between state this library currently finds itself in.

The reality of bitflags is that it has never really been either of those two things.

The fully robust type-safe flags enum is really a language feature if you want it to be watertight while enjoying privileged support from the Rust ecosystem. I think you could do something reasonable though with a proc macro*.

The simple convenience over integers is just something this, which you don't need a library for:

enum Flags {}

impl Flags {
    pub const A: u8 = 1;
    pub const B: u8 = 1 << 1;
}

So bitflags currently walks this middle ground, where the worst-of-both-worlds and the best-of-both-worlds are typically close together. I don't think bitflags necessarily needs to commit to one of those paths or the other, I think what it needs is a consistent mental model. So then you can evaluate its suitability for a particular usecase based on that model, rather than needing to unpack a whole lot of trivia.

This is very valuable food for thought, and I appreciate all the time you've spent bashing at it lately.

* I think you'd enforce the limitation that each flag occupy a single bit or that each bit is part of at most one flag, and generate a hidden internal field type that wraps the underlying integer away in a private module where it can't be reached except through code you generate. The ergonomics of deriving and implementing traits on that would be no worse than bitflags is today, but would require a proc-macro and sacrifice overlapping flags. I think there's room for such a library to exist and am almost motivated enough to write it.

from bitflags.