Should `ash` have separate types for `*Flags` and `*FlagBits`?

[MarijnS95]

Should ash treat these as separate entities if vk.xml properly uses *Flags or *FlagBits to distinguish variables where only a single flag is allowed (i.e. some API/function call) versus variables where multiple flag-bits can be set (typically where supported flags are queried for a specific implementation).

_Originally posted by @MarijnS95 in https://github.com/ash-rs/ash/pull/671#discussion_r998503720_

---

Could look something like the following, even though it is rather verbose:

#[repr(/* Flags */ u32)]
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[doc = "<https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VkSampleCountFlagBits.html>"]
pub enum SampleCountFlagBits {
    #[doc = "Sample count 1 supported"]
    TYPE_1 = 0b1,
    #[doc = "Sample count 2 supported"]
    TYPE_2 = 0b10,
    #[doc = "Sample count 4 supported"]
    TYPE_4 = 0b100,
    #[doc = "Sample count 8 supported"]
    TYPE_8 = 0b1000,
    #[doc = "Sample count 16 supported"]
    TYPE_16 = 0b1_0000,
    #[doc = "Sample count 32 supported"]
    TYPE_32 = 0b10_0000,
    #[doc = "Sample count 64 supported"]
    TYPE_64 = 0b100_0000,
}
#[repr(transparent)]
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[doc = "<https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VkSampleCountFlags.html>"]
pub struct SampleCountFlags(pub(crate) Flags);
vk_bitflags_wrapped!(SampleCountFlags, Flags);
impl SampleCountFlags {
    #[doc = "Sample count 1 supported"]
    pub const TYPE_1: Self = Self(SampleCountFlagBits::TYPE_1 as Flags);
    #[doc = "Sample count 2 supported"]
    pub const TYPE_2: Self = Self(SampleCountFlagBits::TYPE_2 as Flags);
    #[doc = "Sample count 4 supported"]
    pub const TYPE_4: Self = Self(SampleCountFlagBits::TYPE_4 as Flags);
    #[doc = "Sample count 8 supported"]
    pub const TYPE_8: Self = Self(SampleCountFlagBits::TYPE_8 as Flags);
    #[doc = "Sample count 16 supported"]
    pub const TYPE_16: Self = Self(SampleCountFlagBits::TYPE_16 as Flags);
    #[doc = "Sample count 32 supported"]
    pub const TYPE_32: Self = Self(SampleCountFlagBits::TYPE_32 as Flags);
    #[doc = "Sample count 64 supported"]
    pub const TYPE_64: Self = Self(SampleCountFlagBits::TYPE_64 as Flags);
}

[Rua]

There may be issues in functions that return information from Vulkan. If the implementation supports an enum variant that Ash doesn't recognise, then you get an enum with an invalid variant from Rust's perspective, which is UB.

[MarijnS95]

Omitted for brevity, but libraries typically add an Unknown(u32) variant for that. Does require manually matching to convert to the enum though, but at least converting from enum to Flags/u32 is a simple as conversion.

[filnet]

I grepped FlagBits in the vulkan headers and found ~580 occurrences.

A quick look at the results shows that vulkan is quite consistent with FlagBits. They are used as input parameters (when creating structure, setting states, ...) or to read/write masks (aka. vk::*Flags). I did not find uses of FlagBits as a return value (direct or via a struct).

If the above is always true then the only place where we can encounter an undefined FlagBits is in a mask. Any "dandling" bits will not be readable (directly at least) and should not cause UB.

PS: My search was not exhaustive so there might be exceptions that I overlooked...

[Ralith]

I think there's some marginal value here, though I'm not convinced it's large. There's serious risk of making ourselves canaries for bugs in vk.xml, which tend to be common for anything that doesn't affect the C/C++ bindings.

Regardless, we shouldn't use a rust enum, because we can't both conform to the C ABI and handle unknown values. A better approach would use the pseudo-enum newtype-with-consts pattern for both. A From impl is also in order.

[kanashimia]

Here is an example (gtk4-rs code) that uses this: https://github.com/gtk-rs/gtk4-rs/blob/master/gtk4/src/auto/enums.rs

I don't really like the fact that you have to add Unknown value, and the fact that casting as u32 doesn't work, but i don't think that is a problem so much.

Also a problem is that this requires rustc 1.66 or later, so doesn't work on current stable (not a problem for gtk crate, as they don't use custom discriminant values)

On latest rust you can specify multiple repr types like this:

#[repr(C, u32)]
enum Flags {
    Verbose = 0b1,
    Info = 0b1_0000,
    Warning = 0b1_0000_0000,
    Error = 0b1_0000_0000_0000,
    Unknown(u32),
}

Enums have huge benefit that you can import their items by value, which you can't do with assosiated consts, which means you can write code like this:

use vk::DebugUtilsMessageSeverityFlagsEXT::*;
use vk::DebugUtilsMessageTypeFlagsEXT::*;
let debug_info = vk::DebugUtilsMessengerCreateInfoEXT::default()
    .message_severity(ERROR | WARNING | INFO | VERBOSE)
    .message_type(GENERAL | VALIDATION | PERFORMANCE)
    .pfn_user_callback(Some(vulkan_debug_callback));

You can just rename struct itself currently, but it doesn't look so good:

use vk::DebugUtilsMessageSeverityFlagsEXT as L;
use vk::DebugUtilsMessageTypeFlagsEXT as T;
let debug_info = vk::DebugUtilsMessengerCreateInfoEXT::default()
    .message_severity(L::ERROR | L::WARNING | L::INFO | L::VERBOSE)
    .message_type(T::GENERAL | T::VALIDATION | T::PERFORMANCE)
    .pfn_user_callback(Some(vulkan_debug_callback));

There is a crate, enumflags2, that may be useful: https://docs.rs/enumflags2/latest/enumflags2

Also is there any reason why ash has its own bitflag impl instead of using external crates (bitflags)?

[Ralith]

On latest rust you can specify multiple repr types like this:

This is still not compatible with the Vulkan ABI.

Also is there any reason why ash has its own bitflag impl instead of using external crates (bitflags)?

Because bitflags defines many operations that are incorrect on open enums.

[Friz64]

In erupt, I have separate types. FlagBits are implemented as a #[repr(transparent)] struct with impls on it (example), and Flags are implemented using the bitflags crate (example). Enums are implemented the same way as FlagBits (example)

[kanashimia]

This is still not compatible with the Vulkan ABI.

Can you explain please why is it incompatible?

[Ralith]

The representation of data-bearing enums is specified at https://github.com/rust-lang/rfcs/blob/master/text/2195-really-tagged-unions.md.

[MarijnS95]

@Ralith fair enough, these FlagBits are used in structs often making it impossible to insert a custom conversion in e.g. safe(r) wrapper code, but we could do so in the builder pattern though it makes us carry even more types then.

At that point (when implementing these as newtype wrappers with associated const) the only difference becomes what @Friz64 has shown for erupt: both types are essentially the same except that FlagBits doesn't define common bitwise operators. However, with the nested value being pub, users can trivially perform these themselves and wondering "why the API is so inconvenient" without realizing they "shouldn't do this" (or upstream vk.xml spec is wrong, though that seems less likely).

All in all, do we think this is an actionable point or shall we close it and wait and see in the generator rewrite?

[Friz64]

However, with the nested value being pub, users can trivially perform these themselves and wondering "why the API is so inconvenient" without realizing they "shouldn't do this" (or upstream vk.xml spec is wrong, though that seems less likely).

The line needs to be drawn somewhere, people can do lots of stupid things if they try hard enough. You can't make it 100% perfect, but you can guide the user in the right direction with the API. With Flags/FlagBits separation, there is a sufficient level of indication to the user that they're doing something wrong IMO. Documentation also helps.

[MarijnS95]

@Friz64 whoops, looks like I cut that sentence short, intended to include something along the lines of "I'd use into_raw()/from_raw() to further discourage users to defer to raw values (perhaps even with a debug_assert!(x.is_power_of_two())); but then again Ash never intended/intends to be an extra validation layer unless it comes to cheap things we easily check in the name of UB or even careful design of the API at compile time (what I intended to do here by using a Rust enum).

It's unfortunate the RFC shared by @Ralith defines how to store the tag for enum variants with extra data, whereas my example above would have wanted to use a tuple enum variant to "capture" all unknown tag variants. In other words, that RFC seems to point out passing repr-Int enums to C(++) is sound, but how about the reverse where we cannot strictly guarantee that we know about all tag values?

[Lokathor]

In other words, that RFC seems to point out passing repr-Int enums to C(++) is sound, but how about the reverse where we cannot strictly guarantee that we know about all tag values?

If C ever hands you a bag of bytes that you read as an enum, and the tag data isn't a real tag value, it's UB.

You should never set up your FFI in a way where data coming in from the outside world will be automatically interpreted as an enum. It's just asking for trouble.

[MarijnS95]

@Lokathor thanks but you're only stating the above in a different way, not adding any new information. The quoted bit it a question to which, to my knowledge, Rust does not have an answer yet.

The point is that for these types to work as enums for us, we need Rust to define a safe way to read/cast a bag of bytes from C to Rust enums, i.e. bidirectional FFI, and capture all unknown bit-patterns to an ::Unknown variant of sorts. RFC 2195 describes data-bearing enums (and only for Rust->C FFI?) while we don't need them to hold any data.

In the end it seems to me there's too little gain in separating *Flags from *FlagBits (see Erupt bindings for example) if we cannot represent the latter as enum type, we'd better close/postpone this issue until that perhaps ever becomes possible?

[Lokathor]

Sorry if I wasn't clear enough in my explanation.

In the end it seems to me there's too little gain in separating Flags from FlagBits

I agree, that's not a good idea

we'd better close/postpone this issue until that perhaps ever becomes possible?

I strongly suspect this will never become possible.