Change push_next() to unsafe fn extend() and add push() alternative

[MarijnS95]

Fixes #905

push_next() is totally unsafe because of dereferencing a chain of p_next pointers to find the end of the chain to insert, which was obfuscated by a large unsafe block for the BaseOutStructure pointer cast in commit c8c8f69 ("next can contain a pointer chain and we need to correct insert it.").

While this function should definitely be marked unsafe, wrapping builders in unsafe {} en masse in user code isn't all too desirable, especially when this soundness issue only exists to optionally walk a p_next chain while most users are likely inserting bare structs without pointer chains most of the time. push() is introduced for this reason, remaining safe to call without any unintended raw pointer dereferences. push_next() has been renamed to extend() to more closely align with standard naming, and force existing users to notice this change and hopefully analyze their uses to decide which are safe/correct to switch over to safe push().

[MarijnS95]

Rebased and marked as ready for review, since we're now accepting breaking changes in light of other required breaking changes for ash 0.39 with Vulkan 1.4.

push_next_one() didn't sound right to me (as in: push "the" next one), suggestions are welcome.

[Ralith]

LGTM semantically. I don't love either name, TBH. push_one feels like a weirdly verbose synonym of bare push, and both feel a bit obscure. extend might be apt for adding an extension struct, except Vec::extend inserts multiple items, so that's differently confusing.

Do we have any evidence that everyone's ever used the multi-item function of push_next on purpose? Asserting against that might not be the end of the world, though I guess it could be a really unpleasant surprise if someone supports environments they don't often test...

I guess I hate plain push the least, if we need a new name. Being more concise is a decent consolation prize for the churn.

[MarijnS95]

I guess I hate plain push the least, if we need a new name. Being more concise is a decent consolation prize for the churn.

Good one! I forgot to mention, though recall that I originally used push_next_one() to have the next word explicitly in the function name to refer to pNext chains, which isn't in the new push_one() range.

Of course, if we stick with push() and add next to it for that reason, we end up with the already-existing push_next() function name again (see below).

On the other hand, since pNext chains are an integral part of Vulkan structures, push() in isolation should be clear enough to refer to these chains.

One final drawback: at that point push_next() no longer clearly distinguishes itself from push() as being the "unsafely deal with an existing pointer chain" variant, perhaps again what you're alluding to below.

Do we have any evidence that everyone's ever used the multi-item function of push_next on purpose? Asserting against that might not be the end of the world, though I guess it could be a really unpleasant surprise if someone supports environments they don't often test...

Are you suggesting to change push_next() to be the "push only one item, or panic" variant? Or to rename it altogether in light of the above?

With this PR as written, users will either notice the change in the changelog, have to wrap push_next() in unsafe or don't even notice because they put all their Vulkan code in one big unsafe {} block altogether, unless we do such a rename.

extend might be apt for adding an extension struct, except Vec::extend inserts multiple items, so that's differently confusing.

Summarizing all the above, perhaps it is worth renaming push_one() to push(), and push_next() to extend()? That way everyone will have to go through churn, but everyone will have to make a case-by-case decision which variant to switch to (or find-replace, run their app, and find where the assert!() goes off).

---

tl;dr:

1. We must mark push_next() unsafe, because its behaviour is inherently unsafe;
2. We can add a "push just one" variant that doesn't use unsafe at the detriment of not supporting existing chains (and would panic);
3. We can/must (?) make the user aware of this change, via breaking renames?
   • Rather than changing behaviour of the existing push_next(), without changing its signature?

[Ralith]

On the other hand, since pNext chains are an integral part of Vulkan structures, push() in isolation should be clear enough to refer to these chains.

That's my thinking, yeah. Risk of collision with future setters seems low.

Are you suggesting to change push_next() to be the "push only one item, or panic" variant?

Right, changing the existing behavior avoids the trouble of having to come up with a new name, but at some difficult-to-quantify risk of runtime surprises downstream.

Summarizing all the above, perhaps it is worth renaming push_one() to push(), and push_next() to extend()?

That sounds like a great outcome to me. We end up with exactly the Rust container idioms, improved concision all around, and no breakage risk. We could even keep push_next, plus unsafe, a deprecation annotation, and hidden from docs, as an alias of extend for easier incremental migration.

[MarijnS95]

Right, changing the existing behavior avoids the trouble of having to come up with a new name, but at some difficult-to-quantify risk of runtime surprises downstream.

I might have still added push_next_multiple() for that, but that risk of runtime surprises remains.

We could even keep push_next, plus unsafe, a deprecation annotation, and hidden from docs, as an alias of extend for easier incremental migration.

While that could smooth out the transition, it might be too much of a burden to keep this as generated code in the codebase until the next breaking release, given that we're already planning to do a breaking release now.

[MarijnS95]

Also, given doc(hidden) there's no real place to justify the change, and I feel like #[deprecated] is too short for that. IIRC it doesn't support intradoc links to refer to the new functions either.

Pushed it regardless, LMKWYT.

[MarijnS95]

These casts are somewhat annoying and misleading, with the main problem highlighted in #970:

We treat every next as a BaseOutStructure, even if we should "technically" only be adding BaseOutStructures to p_next: *mut c_void and BaseInStructures to p_next: *const c_void. At that point the cast only exists to change the type.

[MarijnS95]

@Ralith also added some extra safety docs explaining writing to the pointer chain in p_next.

[Ralith]

LGTM.