bitflags icon indicating copy to clipboard operation
bitflags copied to clipboard

Enforce uniqueness of flags

Open tgross35 opened this issue 1 year ago • 3 comments
trafficstars

Would it be possible to add an optional check in the bitflags! macro that makes sure flags are unique? Since this is a rules macro, I think it should be possible to add static assertions.

bitflags::bitflags! {
    #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
    #[bitflags(unique)] // something like this would trigger the added assertion
    pub struct MyFlags: u32 {
        const A = 0b00000001;
        const B = 0b00100000;
        const C = 0b00000100;
        // Error, has the same representation as A
        const D = 0b00100000;
    }
}

// This would be generated from the macro
const _: () = {
    let all_flags = <MyFlags as bitflags::Flags>::FLAGS;
    let mut left_idx = 0usize;

    // Test all combinations of flags
    loop {
        if left_idx >= all_flags.len() {
            break;
        }
    
        let mut right_idx = left_idx + 1;
        while right_idx < all_flags.len() {
            let left = &all_flags[left_idx];
            let right = &all_flags[right_idx];
            let lhs = left.value().bits();
            let rhs = right.value().bits();
            assert!(rhs != lhs, "flags contain duplicate values");
            right_idx += 1;
        }
        left_idx += 1
    }
}; 

Resulting error:

error[E0080]: evaluation of constant value failed
  --> src/main.rs:27:13
   |
27 |             assert!(rhs != lhs, "flags contain duplicate values");
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'flags contain duplicate values', src/main.rs:27:13
   |
   = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0080`.
error: could not compile `playground` (bin "playground") due to previous error

Unfortunately, const panic isn't very flexible with format args so it isn't possible to print something like left.name(), right.name(). But this is still better than an accidental bug because two values use the same bits.

Alternatively, a way to autogenerate the flags could solve this, but that may be trickier in a rules macro.

bitflags::bitflags! {
    #[bitflags(unique)]
    pub struct Flags: u32 {
        sequential(/* minimum value */ 0b1, A, B, C, D)
    }
}

Both of these are just ideas to make it easier to maintain lots of flags with less chance of error.

tgross35 avatar Feb 06 '24 01:02 tgross35

I see the sequential idea is covered at https://github.com/bitflags/bitflags/issues/236 already

tgross35 avatar Feb 06 '24 01:02 tgross35

How would you deal with multi bit flags?

const A = 0b0001;
const B = 0b0010;
const AB = 0b0011;

How would you deal with zero bit flags?

const A = 0;

rusty-snake avatar Feb 06 '24 16:02 rusty-snake

I was thinking just to disallow them in these contexts initially. Zero wouldn't be a problem with the proposed solution since it checks equality, but intentional duplicates would need some sort of #[bitflags(allow_duplicate)] attribute to exclude them from the checking.

The proposed solution wouldn't work with an attribute as-is of course.

tgross35 avatar Feb 06 '24 19:02 tgross35

Thanks for the suggestion @tgross35! I'd like to avoid introducing any features that require custom attributes, but since you can already write that const outside of the bitflags macro I think it's something you could introduce in your own projects to enforce uniqueness.

KodrAus avatar Mar 20 '24 03:03 KodrAus