Rewrite chains to support immutable p_next input structures

[MarijnS95]

Vulkan specifically defines a VkBaseInStructure with a constant pNext pointer for read-only inputs to APIs like info in vkGetPhysicalDeviceImageFormatProperties2() (in contrast to the writable properties as outputs).

We do not currently make use of this distinction between VkBaseOutStructure, requiring callers to unconditionally declare their "read-only" input structures as mut when using .push_next(&mut T) because of the way we build up a pointer chain.

While this might not directly be a problem, it would have been nice to have a way to construct chains "more linearly" (like in C++) while keeping these structs defined as constants locally, to signify their

Example

let mut drm_format_modifier_info = vk::PhysicalDeviceImageDrmFormatModifierInfoEXT::default()
    .drm_format_modifier(1337);
let mut external_image_format_info = vk::PhysicalDeviceExternalImageFormatInfo::default()
    .handle_type(vk::ExternalMemoryHandleTypeFlags::OPAQUE_FD);
let info = vk::PhysicalDeviceImageFormatInfo2::default()
    .push_next(&mut drm_format_modifier_info)
    .push_next(&mut external_image_format_info);
let mut external_image_format_properties = vk::ExternalImageFormatProperties::default();
let mut props = vk::ImageFormatProperties2::default()
    .push_next(&mut external_image_format_properties);
instance.get_physical_device_image_format_properties2(pdevice, &info, &mut props).unwrap();

Unfortunately external_image_format_info and drm_format_modifier_info in this example are only defined mut because of .push_next(&mut T). The second call to .push_next() has to mutate external_image_format_info.p_next = drm_format_modifier_info.p_next; to insert it in the chain.

Not to mention, by default this means structs are inserted in the chain in reverse order, i.e. the last struct to go ends up being the first struct in the chain. This is usually not a problem in practice.

---

Unfortunately I do not immediately have a solution to this "problem" (that isn't introducing multiple wrapper structs and macros to abstract chains...) aside from defining a .next() directly on these "extending structures, so that you would instead write:

let drm_format_modifier_info = vk::PhysicalDeviceImageDrmFormatModifierInfoEXT::default()
    .drm_format_modifier(1337);
let external_image_format_info = vk::PhysicalDeviceExternalImageFormatInfo::default()
    .handle_type(vk::ExternalMemoryHandleTypeFlags::OPAQUE_FD)
    .next(&drm_format_modifier_info);
let info = vk::PhysicalDeviceImageFormatInfo2::default()
    .next(&external_image_format_info);

This is much closer to what one would write in C++, making sample/code translation easier. Noting that next() here is a builder function that overwrites p_next - and probably needs an assert in case !self.p_next.is_null()?

However, it easily breaks our predicate of guarding push_next() to only take extending structs, and only implementing it on the root struct via trait ExtendsT. Moreover, we'd have to disambiguate where a struct extends multiple root structs, such as:

unsafe impl ExtendsBufferViewCreateInfo for BufferUsageFlags2CreateInfoKHR<'_> {}
unsafe impl ExtendsBufferCreateInfo for BufferUsageFlags2CreateInfoKHR<'_> {}
unsafe impl ExtendsPhysicalDeviceExternalBufferInfo for BufferUsageFlags2CreateInfoKHR<'_> {}
unsafe impl ExtendsDescriptorBufferBindingInfoEXT for BufferUsageFlags2CreateInfoKHR<'_> {}

Besides not being able to create an OR trait bound on this hypothetical fn next<T: ExtendsX | ExtendsY | ExtendsZ>, users could mix up pointer chains from different "incompatible" hierarchies at places.

Perhaps that validation is not something that is very important for Ash. It's definitely a nicety for callers and why I'd like to keep it, but not something that's in place to prevent UB.

[MarijnS95]

Assigning the 0.39 milestone to see if we can "quickly" come up with a likable "solution" to address this "problem" otherwise we'll leave it as a "nice to have" for future work.

[Ralith]

I think our current interface is safer and more readable than the C++-style one. mut on the extension structs is fundamentally reasonable because we do mutate their p_next, and the use of BaseOutStructure is sound because it has the same layout. Is there any real incentive to change things here?

[MarijnS95]

and the use of BaseOutStructure is sound because it has the same layout.

BaseInStructure also has the same layout.

mut on the extension structs is fundamentally reasonable because we do mutate their p_next

It may have gotten lost in this long read: the point is to get rid of mut in this case - but I think our only possibility to do so is by building the p_next chain individually / linearly per struct that is going to be part of the chain, and deprecate push_next(&mut T) from those BaseInStructure type structs with p_next: *const c_void. Technically we are not allowed to change the p_next of the next structure via that pointer (i.e. reinterpret_cast<BaseInStructure>(const_cast(p_next))->p_next = ...).

[Ralith]

the point is to get rid of mut in this case - but I think our only possibility to do so is by building the p_next chain individually / linearly per struct that is going to be part of the chain

Right, I'm saying I think the ergonomics of our pointer chain helpers justify the use of mut.

Technically we are not allowed to change the p_next of the next structure via that pointer

I'm not sure what you mean by this. *const vs *mut is purely cosmetic in Rust; if the underlying data is mutable then it's sound to mutate it. The only way an ash user could trigger UB here is if they manually populated p_next with a pointer to data that it would be illegal to mutate.

[MarijnS95]

I'm saying I think the ergonomics of our pointer chain helpers justify the use of mut.

And that sits wrong with me. Having mut for ergonomics is nice, as long as there's perhaps the ability for an opt-out?

I don't have a concrete use-case around other than the proposed sample, where mut looked weird given the nature of Info being read-only.

The only way an ash user could trigger UB here is if they manually populated p_next with a pointer to data that it would be illegal to mutate.

This is what I mean, if we introduce the next(&T) that I proposed while keeping a variant of push_next(&mut T) around. Or what you just said, the user sets x.p_next = &y; and then calls z.push_next(&mut foo).push_next(&mut x);.

Either way, perhaps we should add this caveat to the # Safety docs of push_next()/extend() in #909?

[Ralith]

mut looked weird given the nature of Info being read-only.

It seems normal to me to have something that's mutable while you set it up and then passed elsewhere immutably to be consumed.

And that sits wrong with me. Having mut for ergonomics is nice, as long as there's perhaps the ability for an opt-out?

I don't have a strong argument against supporting both patterns if you really want, except that it might cause minor confusion about best practices downstream. Also probably a post-#953 thing, in the interest of avoiding large amounts of new redundant code.

Either way, perhaps we should add this caveat to the # Safety docs of push_next()/extend() in #909?

Yeah, that would be useful.

[MarijnS95]

It seems normal to me to have something that's mutable while you set it up and then passed elsewhere immutably to be consumed.

mut remains as long as these elements are mutably borrowed within the chain.

And we haven't done anything about "unborrowing" them yet, but that's another beast.

I don't have a strong argument against supporting both patterns if you really want, except that it might cause minor confusion about best practices downstream.

👍

Also probably a post-#953 thing, in the interest of avoiding large amounts of new redundant code.

I proposed in #879 (comment) to take a more piecemeal, incremental approach. That PR crams in way too much. Wdyt about the proposal? You mentioned two points against traitifying, that equally apply to #953?

Pending that I can orchestrate how to land #909 relative to it.

[Ralith]

Wdyt about the proposal?

100% on board with incremental.

You mentioned two points against traitifying, that equally apply to #953?

Yep. Will discuss further in #879.