Decouple next lifetime from itself in TaggedStructure::push() and ::extend()

[SteveIntensifies]

Right now TaggedStructure::push() is defined by #994 as:

fn push<T: Extends<Self> + TaggedStructure<'a>>(mut self, next: &'a mut T) -> Self

This requires that the extending structure next has the same lifetime as the contents of the base structure self: TaggedStructure<'a> because of next: &'a mut T, and next itself because of T: TaggedStructure<'a> which is unnecessarily restricting. Specifically, even after self is no longer live (i.e. it is no longer mutably borrowing next) next cannot be accessed because it is mutably borrowed within itself through the fixed/shared lifetime of 'a as demonstrated by a new test in commit 4942dc3. The same restriction exists for TaggedStructure::extend().

This PR decouples lifetime 'a for the contents of self and the borrow of next from a new lifetime 'b for the contents of next in push() and extend() to require that the contents of next outlive the borrow of next rather than considering next to remain mutably borrowed within itself.

[Ralith]

I can't think of any reason why this would be wrong, though I'm not super great at reasoning about variance.

[MarijnS95]

This manually-reimplemented lifetime/borrow-checking surrounding a bunch of raw pointers is tricky and finicky to get right, that's exactly why we have a bunch of tests around it to ensure expected correctness as well as usability.

As part of this change I'd appreciate to see an additional test that demonstrates what currently isn't supported (I can vaguely assume), and so that this doesn't regress in the future.

Also, welcome back ;)

[SteveIntensifies]

That's a fair request. You don't need an additional test because the existing test would cover this case with minor modifications. I just had to make variable_pointers and corner to have different scopes.

Also, welcome back ;)

I'm totally new here 😅

[Ralith]

Thanks for the test. I agree that this case should work, and I confirm that it fails without this change.

There is at least one subtlety here: x.push(&mut y) may update y.p_next to point at an extension struct previously stored in x.p_next, which might have a shorter lifetime than y despite being equally or longer-lived than x. This could lead to y.p_next becoming a dangling pointer, but not before the lifetime of x ends.

However, I think that's okay: if y is passed to a future push call, then the dangling pointer is replaced, and since y is an extension struct it can't be passed to a Vulkan API call on its own. Code which traverses existing extension chains (e.g. extend) must be unsafe with an explicit invariant on their validity regardless, so I think this is still both technically sound and consistent with the practical usefulness of the safe push API: if you only build extension chains via push, then you'll never create a root struct with a dangling p_next in its extension chain.

tl;dr I'm reasonably confident this change is correct and desirable.

[krakow10]

As an example, I ran into this using the VideoCapabilitiesKHR, VideoDecodeCapabilitiesKHR pointer chain. I cannot access the decode capabilities because the lifetime of &mut video_decode_capabilities is fixed by video_capabilities.extend(&mut video_decode_capabilities).

See source code here
This patch allows the lifetimes to be different and fixes the compilation error.

[MarijnS95]

Thanks for taking the time to demonstrate this in a test, which matches what I expected to have been broken. I did however specifically request an additional test because the existing ones are quite convoluted without documentation on the expected behaviour, something which I've started rectifying when adding trybuild failures. The one you modified merely concerns itself with pointer values and wasn't intended to test lifetimes, for which I believe we should instead copy and modify the trybuild failure into a working test together with explanatory comments. Together, they can be easily compared to understand why one is designed to compile while the other is not.

I've pushed the result of that to master, showcasing that the project is currently "broken" in CI and this PR on top will make it successful once again when it is rebased.

---

Also, welcome back ;)

I'm totally new here 😅

Your previous account could already be used here on the ash repo, and as I've mentioned it's been unblocked allowing you to once again interact with my PRs and comments too. Do with that information as you wish, but don't feel forced to use a new account just to get around a limitation that's no longer there.

---

@Ralith I concur that those "nested" pointer chains remain and may be ugly, though not exploitable with them being raw pointers and possible traversal (extend() and any Vulkan API call) adequately marked unsafe. With those structures themselves likely not being "root" structures that are accepted by a Vulkan API call anyway, the only way to really "mess things up" is by unsafely calling .extend() with existing chains (and I use "mess things up" to both describe UB through dangling or mutable pointers, and unintended user behaviour where the builder pattern didn't make clear that a hypothetical a.push(b).push(c) suddenly makes c point to b, which the user may not want later when constructing a new chain including c). At least that is why push() asserts on p_next being NULL.

[MarijnS95]

Since this is now moved to a different test with adequate explanation (not let _ =... 😉) and to not clobber this pointer-check test, we can remove it when rebasing on master.

[MarijnS95]

Thanks a lot, I've rebased this to make it compatible with master and demonstrate that your suggestion fixes the test as described, as well as extended the title/description to explain what this is fixing and how.

Not yet sure how to better describe that next is still borrowed for the lifetime of 'a though while the contents of next via TaggedStructure<'b> are now decoupled instead; will need some more tuning of the wording.

[MarijnS95]

Specifically, if we look at the failing test:

error[E0502]: cannot borrow `api` as immutable because it is also borrowed as mutable
   --> ash/src/vk.rs:346:14
    |
343 |         let _pdev_props = vk::PhysicalDeviceProperties2::default().push(&mut api);
    |                                                                         -------- mutable borrow occurs here
...
346 |         dbg!(&api);
    |              ^^^^
    |              |
    |              immutable borrow occurs here
    |              mutable borrow later used here

having next: &'a mut impl TaggedStructure<'a> is implying api is mutably borrowed within api now because of those similar lifetimes, and have little to do with the base structure self as this error triggers even when _pdev_props is dropped and/or out of scope.

[Ralith]

will need some more tuning of the wording.

Which wording, specifically? The comments in the vicinity of the change still seem correct to me.

[MarijnS95]

Which wording, specifically? The comments in the vicinity of the change still seem correct to me.

That in the PR description and commit body, which was originally missing until I typed something up. Specifically, the issue doesn't seem to be related to next being coupled to self, but to &'a mut next being coupled to the content of itself.

[Ralith]

Commit text LGTM, and I don't think it merits going over with a super fine toothed comb because it's descriptive and not part of our public docs.

[MarijnS95]

I just prefer my contributions (including descriptions of complex lifetime changes) to be reviewed as much as I review any other change. Thanks for confirming, and merging now to unblock CI.