Is there a more ergonomic way of accepting either a Bitflags<MyEnum> or just MyEnum?

[joseluis]

I have a structure that accepts Bitflags for a flags field. And it works beautifully well except in the case of wanting to use just one flag:

// OK
let a = MyStruct::new(MyEnum::Flag1 | MyEnum::Flag2);

// Fails
let b = MyStruct::new(MyEnum::Flag1);
                   // ^^^^^^^^^^^^^ expected struct `enumflags2::BitFlags`, found enum `mycrate::MyEnum`

// the ugly solution 
let b = MyStruct::new(enumflags2::BitFlags::from(MyEnum::Flag1));

Do you happen know of a more ergonomic way for the user to provide just one flag? Thank you

[meithecatte]

The standard way would be to take impl Into<BitFlags<Enum>>. This is similar to what you would do with Option arguments that you don't want to wrap with Some.

[joseluis]

Thanks. My intention is to wrap an unsafe FFI binded structure, and I was already using Option like this:

pub fn new(/*...,*/ flags: Option<BitFlags<NcOptionFlag>>) -> Self {
    MyStruct {
        flags.unwrap_or(BitFlags::empty()).bits() as u64
    }
}

Which works well, and accepts either None, or Some(BitFlags). The only only "problem" was the ergonomics of providing a single flag.

Now, when trying to make it work with the Into Trait I'm encountering a wall, which is that I no longer seem to be able to cast as u64 nor using u64::from(X), since I'm not the creator of either BitFlags or u64 ... So I can't crete the necessary impl Into...

I think going this way is making it too complicated. Among the little information I've found regarding this trick, there's gtk-rs removing the usage of the Into trait for reasons I tend to agree with.

But maybe I'm missing something and I'm open to keep trying. Do you have any more ideas?

[meithecatte]

Thanks. My intention is to wrap an unsafe FFI binded structure, and I was already using Option like this: ```rust pub fn new(/*...,*/ flags: Option<BitFlags<NcOptionFlag>>) -> Self { MyStruct { flags.unwrap_or(BitFlags::empty()).bits() as u64 } } ``` Which works well, and accepts either None, or Some(BitFlags). The only only "problem" was the ergonomics of providing a single flag.

What's the motivation behind using `Option` here? Type-wise, I don't think there should be a distinction between `None` and `Some(BitFlags::empty())`. Somewhat recently, support for `NcOptionFlag::empty()` landed on master too. Would that help here?

Now, when trying to make it work with the Into Trait I'm encountering a wall, which is that I no longer seem to be able to cast `as u64` nor using `u64::from(X)`, since I'm not the creator of either BitFlags or u64 ... So I can't crete the necessary `impl Into`...

Are you still using `.bits()`? AFAICS, this should work. Also, if you add `#[repr(u64)]` onto your `enum`, you won't need the cast ;)

I think going this way is making it too complicated. Among the little information I've found regarding this trick, there's [gtk-rs removing the usage of the Into trait](https://gtk-rs.org/blog/2019/06/22/new-release.html) for reasons I tend to agree with.

I agree with the type inference aspect, though it doesn't apply in case of bit flags. I might've misremembered the part about `Option`s somewhat... As far as being explicit goes, I don't think there's much value in expressing this conversion, as it's merely type-level and compiles down to nothing. I think the trade-off should go in the direction of ergonomics in this case - the `Into` trick is a key part of what allows keeping a single flag a separate type from a set of them, and it's used [a fair amount of times](https://docs.rs/enumflags2/0.6.4/enumflags2/struct.BitFlags.html#method.intersects) in the crate itself.

[joseluis]

What's the motivation behind using Option here? Type-wise, I don't think there should be a distinction between None and Some(BitFlags::empty()).

Merely because since the original C API accepted null as a parameter for several fields, at first I wanted to remain as similar as possible by being able to pass None, but I surely prefer the most ergonomic option.

Somewhat recently, support for NcOptionFlag::empty() landed on master too. Would that help here?

Yes, that would be fantastic, but unfortunately I'm not being able to make it work in a standalone crate

EDIT: I'm sorry I just realized it does work with the master version, which is more recent than the one published in crates.io

Are you still using .bits()? AFAICS, this should work. Also, if you add #[repr(u64)] onto your enum, you won't need the cast ;)

Ha! I completely overlooked that fact. You're right. I just did that and it now works for a single flag :D

pub fn new(/*...,*/ flags: impl Into<BitFlags<NcOptionFlag>>) -> Self {
    MyStruct {
        flags.into().bits()
    }
}

[joseluis]

BTW I just found out that it's imposible to derive BitFlags for an enum with a value of 0:

The following code only works if I remove the BitFlags derive:

#[repr(u32)]
#[derive(BitFlags, Copy, Clone, Debug, PartialEq)]
pub enum NcAlign {
    Empty = 0 as u32,
    One = 1 as u32,
    Two = 2 as u32,
}

It gives the error:

error[E0080]: evaluation of constant value failed
   |     ^^^^ attempt to subtract with overflow"

If that worked it would be the best solution for specifying an empty flag, since it would avoid the need of having to include the RawBitFlags trait into scope.

Is there an unavoidable reason why that isn't allowed?

[meithecatte]

The semantics of a flag that doesn't have exactly one bit set are unclear. In particular, should .iter include it? Good arguments can be made in both directions, and there doesn't seem to be a use case. Even here, Empty isn't a true yes-or-no flag, but a separate value. I think it would be better defined as an associated constant instead:

impl NcAlign {
    pub const EMPTY: BitFlags<NcAlign> = BitFlags::empty();
}

This makes me realize that {BitFlags, RawBittFlags}::empty and {BitFlags, RawBitFlags}::all should probably be constants, and not functions, too. What do you think? I'm planning one last breaking change release that switches to an attribute macro to fix #21, so it'd be a perfect moment to improve the API.

(side note: RawBitFlags is, in hindsight, a terrible name, which is why master tentatively renames it to BitFlag - values of a type that implement it represent a single flag)

Also, what is the reason for the as u32? This shouldn't be necessary, and it triggers the workaround enabled with not_literal, where assertions are emitted to supplement expansion-time checks.

Is importing the trait something worth avoiding? I feel like this should be made opt-in with something like #[bitflags(inherent_empty_all)] - I checked, and inherent items shadow any trait impls, so this wouldn't introduce any usability issues.

Thoughts?

[joseluis]

The semantics of a flag that doesn't have exactly one bit set are unclear. In particular, should .iter include it? Good arguments can be made in both directions, and there doesn't seem to be a use case. Even here, Empty isn't a true yes-or-no flag, but a separate value.

I'm not really sure of the trade-offs... I just miss a comfortable way of dealing with 0 / empty as part of the provided abstraction. I didn't imagine it would be so hairy. For my use-case I'd be the most happy with any solution that works without forcing the user to import anything more than the enum itself.

I think it would be better defined as an associated constant instead:

impl NcAlign {
    pub const EMPTY: BitFlags<NcAlign> = BitFlags::empty();
}

That looked promising but unfortunately it doesn't seem to compile, even in nightly.:

error[E0015]: calls in constants are limited to constant functions, tuple structs and tuple variants

This makes me realize that {BitFlags, RawBittFlags}::empty and {BitFlags, RawBitFlags}::all should probably be constants, and not functions, too. What do you think? I'm planning one last breaking change release that switches to an attribute macro to fix #21, so it'd be a perfect moment to improve the API.

If you are able to make it const I'd say go for it, since I don't think it can have any disadvantages... And maybe then the previous code will work. 😄

(side note: RawBitFlags is, in hindsight, a terrible name, which is why master tentatively renames it to BitFlag - values of a type that implement it represent a single flag)

Agree. And if you already intend to do some breaking changes, it's better to do them all together, better than getting stuck with an uglier API than it could have been once it's more used.

Also, what is the reason for the as u32? This shouldn't be necessary, and it triggers the workaround enabled with not_literal, where assertions are emitted to supplement expansion-time checks.

Well I'm sharing simplifications of the real code, the original code is more like:

#[repr(u64)]
#[derive(BitFlags, Copy, Clone, Debug, PartialEq)]
pub enum NcOptionFlag {
    InhibitSetlocale = nc::NCOPTION_INHIBIT_SETLOCALE as u64,
    VerifySixel = nc::NCOPTION_VERIFY_SIXEL as u64,
    // ....
}

I could use #[repr(u32)], remove the as u64 in each line, and move it to the flags.into().bits() as u64 part so that everything works the same, but... It doesn't seem to achieve much in that regard since then I've tried using the crates.io version and disabling the not_literal feature (I saw it's automatic now in master), but it fails to compile:

error: This kind of discriminant expression is not supported.
hint: Enable the "not_literal" feature to use a workaround.
note: This is not enabled by default due to the high potential for confusing error messages (see documentation).
  --> src/options.rs:28:24
   |
28 |     InhibitSetlocale = nc::NCOPTION_INHIBIT_SETLOCALE,

Is importing the trait something worth avoiding? I feel like this should be made opt-in with something like #[bitflags(inherent_empty_all)] - I checked, and inherent items shadow any trait impls, so this wouldn't introduce any usability issues.

Depends on the trade-offs I suppose. The only thing I'm sure of is that if importing the trait wasn't needed, we would never think of making it so.

I haven't seen anything like what you propose. How would it work? Isn't it related to this pending issue?

EDIT: I also found this procedural macro.

[meithecatte]

> Is importing the trait something worth avoiding? I feel like this should be made opt-in with something like `#[bitflags(inherent_empty_all)]` - I checked, and inherent items shadow any trait impls, so this wouldn't introduce any usability issues. Depends on the trade-offs I suppose. The only thing I'm sure of is that if importing the trait wasn't needed, we would never think of making it so. I haven't seen anything like what you propose. How would it work? Isn't it related to [this pending issue](rust-lang/rfcs#2375)?

I haven't seen that RFC before. I'm just suggesting an attribute that would make the macro generate an `impl YourEnum` block that creates `empty` and `all`. Perhaps similar to `#[serde(transparent)]` syntax-wise. "Inherent implementation" is the name for a non-trait `impl` block, but from your response I can see that it isn't widely known... However, now that I think about it, the only reason to not do this by default would be backwards compatibility, so it's probably better to enable it by default, together with the other changes. I'll try to wrap up the next release sometime this week.

[joseluis]

Ok now I understand. Well, I don't think I can be indicative of what is widely known since I'm learning Rust while I try to program what I need... and there's a lot I don't know yet. At the very least I've learned several very important things more in this thread.

I'll let you to it then, and I'll be very happy to use the new release when it's ready. Thank you very much for your time.

[mokeyish]

@joseluis Is this https://github.com/mokeyish/enum-flags a better implementation for your question?😅

[joseluis]

@joseluis Is this https://github.com/mokeyish/enum-flags a better implementation for your question?sweat_smile

thank you! the library looks really good, small and on point.