Traitify push_next and adding more utility methods for next chains

[Neo-Zhixing]

Currently, we implement push_next methods for all base types in the generator. This has two problems:

1. Bloated size for generated codes
2. Adding more utility methods like extend_next (Add extend_next method to extendable structs #907) or iter_nexts becomes more difficult, as the additional utility methods will bloat the generated code sizes further.

This PR solves the problem by doing the following two things:

• For base structs, implement the BaseTaggedStructure marker trait instead of the push_next functions.
• For extension structs, implement the Extends<BaseType> trait instead of ExtendsXXXXX trait. This was suggested in Switch from impl ExtendsC for E to impl ExtendableFrom<E> for C #879
• Adding all the utility methods related to next chains in an extension trait, NextChainExt.

Some of the additional, nice-to-have things I've done in this PR:

• Adding TaggedObject, an abstraction over vk::BaseOutStructure, which allows you to use it in the same way as a &dyn Any, but without the vtable overheads since casting is just trivial pointer casts.
• Renamed push_next to be with_next to be consistent with Rust naming conventions. push_next now has the same behavior as Vec::push: it takes &mut self and returns ().
• push_next no longer chases the p_next chain for the extension structs; instead it asserts that the p_next field in the extension structure is null. This addresses push_next being safe to call is unsound #905.

Drawbacks:
I would like to address the "major drawbacks" comment made by @Ralith in #879.

1. I added the NextChainExt trait to ash::prelude, so if the user has use ash::prelude::* (like they should), they should not be impacted. Imo this is a minor inconvenience compared to the benefit of having more usable next chain utility methods and significantly reduced generated code sizes.
2. That's why in this specific implementation, BaseTaggedStructure and Extends are marker traits only. The actual utility methods are implemented separately in NextChainExt with a blanket implementation over relevant types.

Resolves #905
Resolves #907
Resolves #879

[Ralith]

Bloated size for generated codes

Reducing code size is nice, but in and of itself has only limited value; the set of generated code will be huge no matter what. Is there a measurable impact on the compile time for ash or downstream code?

For base structs, implement the BaseTaggedStructure marker trait instead of the push_next functions.

Could we improve caller safety by making having (provided) safe cast and getter methods on this trait, rather than making it a pure marker?

Some of the additional, nice-to-have things I've done in this PR:

Could these be separate PRs, so we can review and merge them in small independent units?

if the user has use ash::prelude::* (like they should)

I disagree with this assumption. Wildcard imports are a stability hazard: new elements in the imported module are not semver-breaks, but can easily break code that performs a wildcard import. This is an especially large hazard for "prelude" modules which are by their nature grab-bags of random items that might grow unpredictably.

Further, there is currently exactly one definition in ash::prelude. The whole module is arguably vestigal; certainly I've never used it, and had forgotten it existed at all.

None of that necessarily means that moving these helpers into traits is a bad idea (we already have Handle, which is morally similar), but I don't think we should make ergonomic decisions on the assumption of, or promote use of, a prelude module.

[Neo-Zhixing]

@Ralith

Reducing code size is nice, but in and of itself has only limited value.

That is correct, but that is also not the main benefit offered by this PR.

Here's the train of thoughts:

• Add extend_next method to extendable structs #907 was dismissed because it is a lot of extra generated code
• I want not just extend_next but also iter_next.
• Clearly, a PR that generates iter_next for each base structure individually will not be accepted for the same reason Add extend_next method to extendable structs #907 was not accepted: lots of extra generated code
• Therefore, we adopt the method proposed in Switch from impl ExtendsC for E to impl ExtendableFrom<E> for C #879 because @Ralith 's counter arguments there are easier to address.
• We address the "Trait functions must be imported explicitly" problem with prelude includes
• We address "A trait method that is never implemented is misleading" by moving next-chain related functions to an extension trait, NextChainExt.

Now, going back to the original motivation: Why is this needed? Why do we need more next-chain utility functions other than push_next?

Traditionally, ash is used by applications to call into Vulkan implementations.

However, ash is also useful for creating Vulkan implementations and layers.

These applications require the ability to inspect Vulkan structures passed in by the user. One example might be:

fn getPhysicalDeviceFeatures2(
    physicalDevice: vk::PhysicalDevice,
    pFeatures: &vk::PhysicalDeviceFeatures2) {
    if let Some(ray_tracing_features) = pFeatures
        .iter_nexts_mut()
        .find(|next: &mut TaggedObject| next.tag() == vk::StructureType::PHYSICAL_DEVICE_RAY_TRACING_PIPELINE_FEATURES_KHR) {
        // Fill in ray_tracing_features
        ray_tracing_features.ray_tracing_pipeline = vk::TRUE;
    }
}

The ability to iterate on the next chain is quite essential to Vulkan implementations and layers and can be similarly justified like push_next.

Clearly, there is demand for other types of next-chain related utility functions (#907) However, as we end up with more and more next-chain utility functions, the existing infrastructure in ash becomes insufficient.

With the introduction of Extends<XXX>, BaseTaggedStructure and NextChainExt traits, adding new utility functions becomes almost trivial. And TaggedObject is the type-erased object used by extend_next and iter_next.

Wildcard imports are a stability hazard: new elements in the imported module are not semver-breaks, but can easily break code that performs a wildcard import.

This is clearly going to be a semvar-breaking change. The user impact is that they will have to use ash::prelude::* for push_next if they're not doing that already.

As such, we might as well do some of the other breaking changes simultaneously: #905, renaming push_next to with_next, etc.

Further, there is currently exactly one definition in ash::prelude. The whole module is arguably vestigal; certainly I've never used it, and had forgotten it existed at all.

I don't think we're supposed to have functions and definitions in prelude modules either. We can discuss how to clean that up later on. But in general, VkResult, Handle, and NextChainExt introduced in this PR are good candidates for inclusion in the prelude module.

Could we improve caller safety by making having (provided) safe cast and getter methods on this trait, rather than making it a pure marker?

We can discuss implementation details and review procedures once the project maintainers are onboard with the spirit of this change. I would prefer not having to do wasted work if the PR ends up in limbo like my previous contribution.

Could these be separate PRs, so we can review and merge them in small independent units?

Sure, but imo the changes introduced in this PR are pretty self-contained. Without demonstrating an implementation of iter_next and extend_next, the other changes would probably seem pointless.

cc @Friz64

[Ralith]

I agree that having methods for traversing pointer chains is appealing, as is providing pointer chain methods with a single definition. Making the addition of new pointer chain methods more palatable is a good argument for this change. I'm still interested in what the compile time impact is.

However, ash is also useful for creating Vulkan implementations and layers.

Are you working on one of these?

We address the "Trait functions must be imported explicitly" problem with prelude includes

This introduces other problems, as I noted above. I don't think having to explicitly import a trait is a deal-breaker on its own, particularly with the prevalence of rust-analyzer to do so automatically.

TaggedObject is the type-erased object used by extend_next and iter_next.

This seems useful, but we should be careful to ensure that downcasting from a reference that only covers the header of a type is sound under current proposed provenance rules. If it isn't, we may need to switch to a newtyped raw pointer or something to that effect.

[Neo-Zhixing]

I'm still interested in what the compile time impact is.

This change brings the number of lines in definition.rs from 59910 to 59449. On my machine there is no observable impact on build time - both the master branch and my branch are around 4.5 seconds when compiling with cargo build --release --no-default-features.

Are you working on one of these?

Yes.

we should be careful to ensure that downcasting from a reference that only covers the header of a type is sound under current proposed provenance rules.

These casting does not involve pointer to integer casts, so I believe that they should be sound under my understanding of the Rust provenance rules.

As an additional note, because iter_next_chain returns an impl Iterator, we need to upgrade rustc to 1.75.0, which is the version where "impl trait in trait" was stabilized.

This PR should be ready for review.

[Ralith]

These casting does not involve pointer to integer casts, so I believe that they should be sound under my understanding of the Rust provenance rules.

My concern is the conversion, via raw pointers, from a reference to a small amount of memory to a reference to a larger amount of memory.

[Neo-Zhixing]

The conversion from a large amount of memory to a smaller amount of memory is obviously ok, as it can be thought as taking a slice of the original memory.

The conversion from a smaller amount of memory to a larger amount of memory is more tricky. I would like to quote the following from the strict provenance docs:

Using Strict Provenance

Most code needs no changes to conform to strict provenance, as the only really concerning operation that wasn’t obviously already Undefined Behaviour is casts from usize to a pointer. For code which does cast a usize to a pointer, the scope of the change depends on exactly what you’re doing.

In general, you just need to make sure that if you want to convert a usize address to a pointer and then use that pointer to read/write memory, you need to keep around a pointer that has sufficient provenance to perform that read/write itself. In this way all of your casts from an address to a pointer are essentially just applying offsets/indexing.

in general, the strict_provenance API really just applies two lint rules that prohibits integer to pointer conversions, so we’re mostly fine for now. All of our conversions are pointer to pointer casts so we maintain an uninterrupted chain of custody. However, for the sake of argument, let’s assume that we’re converting from a *mut BaseOutStructure to a usize then to a *mut PhysicalDeviceFeatures2.

This is still ok, because even if we’re converting a usize to a pointer, we always “keep around a pointer that has sufficient provenance to perform that read/write itself”. This is because TaggedObject borrows the original object, and that object must still be somewhere. We do not allow the user to obtain an owned TaggedObject. All of the public constructors on TaggedObject returns a reference only.

So based on my understanding, this is fine even with strict provenance. In the future if we want to allow
Box<TaggedObject> we will have to be more careful. I do not see a need for Box<TaggedObject> for now.

[Ralith]

Thanks; I'm satisfied that this approach is sound. I think under stacked borrows it would be forbidden, but tree borrows permits it and is more representative of the long term direction of Rust.

[Ralith]

Correction: Ralf specifically calls this pattern out as a UB risk: rust-lang/unsafe-code-guidelines#256. Hence, we must not include it in a release. Instead, please always go through raw pointers; never construct a reference to a base structure that we might later use to access the derived structure.

[Neo-Zhixing]

@Ralith it's been a while and it sounds like you guys will do this on your own.

Should I close this now? Or should I work on updating the PR

[Neo-Zhixing]

Ok so apparently the other maintainer MarijnS95 has blocked me because I can no longer review or comment on their PR, follow their profile, or star their projects. I'm not sure what I did that offended them but it's clear to me that my contribution is not welcomed here and there's no possibility for this PR to be accepted. Given that, I will now close this PR.