Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from impl ExtendsC for E to impl ExtendableFrom<E> for C #879

Open
Friz64 opened this issue Mar 16, 2024 · 5 comments
Open

Switch from impl ExtendsC for E to impl ExtendableFrom<E> for C #879

Friz64 opened this issue Mar 16, 2024 · 5 comments

Comments

@Friz64
Copy link
Contributor

Friz64 commented Mar 16, 2024

Here I use C for "carrier" and E for "element". A carrier is the struct that gets consumed by Vulkan API functions, and is the head of the pointer chain. An element is the opposite, just a part of the pointer chain.

The elements a carrier may be extended by is defined by the Vulkan registry. We leverage this information to provide a compile-time safe API, by generating a push_next builder function for every carrier which only takes elements that implement ExtendsC (a trait only implemented for valid elements).

In erupt, I switched this around: The trait is now implemented for the carriers, and the elements are in the generic parameter position. This means that the push_next function can now become a trait function with a default implementation, which works everywhere.

See the ExtendableFrom trait from erupt.

Side note: ash generates a separate trait for every carrier struct, where it could use generics (Extends<C>) instead.

@Ralith
Copy link
Collaborator

Ralith commented Mar 16, 2024

the push_next function can now become a trait function with a default implementation

This is a major drawback:

  • Trait functions must be imported explicitly, whereas inherent functions are always available
  • A trait method that is never implemented is misleading, normally only justified in extension traits

@MarijnS95
Copy link
Collaborator

In erupt, I switched this around: The trait is now implemented for the carriers, and the elements are in the generic parameter position. This means that the push_next function can now become a trait function with a default implementation, which works everywhere.

Is this (and this issue in general) implying that we must swap around the trait to make this work? Or specifically to have fn extend_from() inside trait ExtendableFrom? I'd argue that, given that we already have a ("unrelated" name-wise) TaggedStructure trait, we could also follow the original direction of structextends with a (likewise named) StructExtends trait:

/// Implement this for extension struct `E` when it is allowed in the base struct `B` (i.e. `E` has `structextends="B"` in `vk.xml`).
pub trait StructExtends<B>: TaggedStructure {}
impl StructExtends<vk::DeviceQueueCreateInfo<'_>>
    for vk::DeviceQueueGlobalPriorityCreateInfoKHR<'_>
{
}

/// Structures implementing this trait are layout-compatible with [`vk::BaseInStructure`] and
/// [`vk::BaseOutStructure`]. Such structures have an `s_type` field indicating its type, which
/// must always match the value of [`TaggedStructure::STRUCTURE_TYPE`].
pub unsafe trait TaggedStructure: Sized {
    const STRUCTURE_TYPE: vk::StructureType;

    fn push<T: StructExtends<Self>>(mut self, next: &mut T) -> Self {
        // XXX: UB Risk? https://github.com/ash-rs/ash/pull/953#issuecomment-2521132802
        // SAFETY: All implementors of `TaggedStructure` are required to have the `BaseOutStructure` layout
        let slf = unsafe { &mut *<*mut _>::cast::<vk::BaseOutStructure<'_>>(&mut self) };
        let next = unsafe { &mut *<*mut _>::cast::<vk::BaseOutStructure<'_>>(next) };
        assert!(
            next.p_next.is_null(),
            "push() expects a struct without an existing p_next pointer chain (equal to NULL)"
        );
        next.p_next = slf.p_next;
        slf.p_next = next;
        self
    }
}

#[test]
fn possible() {
    let x = vk::DeviceQueueCreateInfo::default();
    let mut y = vk::DeviceQueueGlobalPriorityCreateInfoKHR::default();
    x.push(&mut y);
    // y.push(&mut x); // Obviously invalid
}

Regardless, this violates both points by @Ralith (despite me giving a lot of value to the former, but not much to the latter).


If we decide to reduce these functions via traits, I'd appreciate to start small with something like the above.

@Ralith
Copy link
Collaborator

Ralith commented Dec 6, 2024

Taking advantage of the existing TaggedStructure sounds nice to me.

I do still feel that traits can be a bit awkward, but we should balance that against the duplicate generated code and the fact that we've already embraced traits requiring import with Handle. Having users do use ash::{vk, TaggedStructure} instead of just use ash::vk; isn't a terribly high cost, especially given good LSP support.

There's a compromise solution, too: offer a top-level function fn push<T, U: StructExtends<T>>(base: T, ext: &mut U) -> T in ash. This doesn't need to be imported, avoids code duplication, and doesn't strictly require we even have a provided method. I think this is slightly less ergonomic for the usual "postfix > prefix" reasons, so I still lean slightly towards the extension trait pattern, but it is appealing otherwise.

@MarijnS95
Copy link
Collaborator

Thanks for the insights! Would appreciate a suggestion for renaming a TaggedStructure in that case; it's still tagged but would appreciate to have something close to how Vulkan defines them... VulkanStructure?


Such a push function sounds nice, though still has to be imported as use ash::push; or qualified as ash::push()?

Finally, notice that we have a prelude in both ash::prelude and ash::vk::prelude. Would the trait fit there, and should we suggest glob imports from a prelude like "some other crates"? That kind of protrudes what we want to achieve here, but cleaning up our prelude to be useful like that might be worth a discussion of its own.

@Ralith
Copy link
Collaborator

Ralith commented Dec 6, 2024

Would appreciate a suggestion for renaming a TaggedStructure in that case

I'm not sure there's a pithy term Vulkan uses here (relevant docs). TaggedStructure still seems fine to me. We could shorten it to Tagged. Typed is a little closer to spec terms (the actual tag field being named sType, but also uses a more overloaded term. Maybe ChainStructure or Chain? I don't feel strongly.

Such a push function sounds nice, though still has to be imported as use ash::push; or qualified as ash::push()?

The concise name is intended to support qualified use. It'd be a bit ambiguous if imported.

Finally, notice that we have a prelude

IMO glob imports (and the preludes that promote them) are an anti-pattern due to readability and non-semver breakage hazard. I think we should ditch those modules entirely; they're mostly vestigal anyway. Let's export public helpers from the top-level ash module directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants