dtolnay / cxx

Safe interop between Rust and C++
https://cxx.rs
Apache License 2.0
5.89k stars 334 forks source link

Support for using shared types from one bridge in another bridge module #297

Closed sbrocket closed 3 years ago

sbrocket commented 4 years ago

I've been working on introducing cxx to Fuchsia as a safer, better way for us to write Rust/C++ FFIs. As part of this, I'd like to provide a library or two with some common types for others to reuse in their own FFIs.

For example, see the handle types from #274; these would be a bridge between our C++ and Rust handle wrappers. These are important to write once and reuse because there's a lot of ways to implement them in a way that would cause resource leaks. Another example is a reusable Result type; we don't use exceptions and panicking on every error result is undesirable.

To do this, I'd like to be able to reuse shared types defined in one bridge module in another bridge module without having to treat them as opaque Rust types in the using module (so that they can be passed by value without boxing). Today these seem to just be recognized as unsupported types. For example:

// handle.rs
#[cxx::bridge(namespace = zx::ffi)]
mod ffi {
    struct Process {
        raw: u32,
    }

    struct Job {
        raw: u32,
    }

    // ... and so on
}

impl Drop for ffi::Process { ... }
impl Drop for ffi::Job { ... }
// ... and so on, From impls to convert, etc.

// usage.rs
#[cxx::bridge(namespace = my_usage::ffi)]
mod ffi {
    extern "Rust" {
        fn create_process(name: &CxxString, job: &handle::ffi::Job) -> handle::ffi::Process;
    }
}
dtolnay commented 4 years ago

This is not implemented yet but I am very interested in supporting it. We do something pretty close for https://docs.rs/cxx/0.4.4/cxx/trait.ExternType.html#safely-unifying-occurrences-of-the-same-extern-type but that's for sharing extern C++ types.

sbrocket commented 4 years ago

Ok, I'm taking a crack at implementing it based on your PR #190

sbrocket commented 4 years ago

One thing I noticed that don't seem to be clearly documented: The current ExternType support requires that two bridges use the same C++ namespace.

Is that necessary? It seems like this could be made to work across namespaces. I don't think the alias would need to explicitly specify the C++ namespace either, it seems like the ExternType trait could provide it. As long as you have the handshake of "the aliased type implements ExternType so we know it's a C++ type from another cxx bridge (or someone's manual unsafe impl)", I think that's all that needs to be verified? In other words, we just have to check that it's the right "flavor" of type, and then the using bridge needs to know the correct namespace to use when generating code. The type_id could be changed to only include the type name without the namespace, and a constant added to ExternType with the namespace to use.

Or is there some bit of unsafety that I'm missing there?

I think the same will apply to supporting aliases for shared types (and it could apply to opaque Rust types but there's less need for that I think). The modified ExternType trait I'm imagining would be:

pub unsafe trait ExternType {
    type Flavor;
    type Id;
    const NAMESPACE: &'static str;
};

pub fn verify_extern_type<T: ExternType<Flavor = Flavor, Id = Id>, Flavor, Id>() {}

And then example generated usages:

unsafe impl cxx::ExternType for CppType {
    type Flavor = cxx::type_id!("cpp");
    type Id = cxx::type_id!("CppType");
    const NAMESPACE: &'static str = "remote::namespace";
}

unsafe impl cxx::ExternType for SharedType {
    type Flavor = cxx::type_id!("shared");
    type Id = cxx::type_id!("SharedType");
    const NAMESPACE: &'static str = "remote::namespace";
}
sbrocket commented 4 years ago

I ask because this is somewhat important to my use case too. (You can see I naturally used different namespaces above.) I could technically fudge everything into one C++ namespace without issue, but stylistically it's a bit weird to require that usages match the namespace of "library" types.

sbrocket commented 4 years ago

Ah, no, that doesn’t work because there wouldn’t be any way to read the namespace from the other module’s trait during the cxxbridge C++ codegen, which is what needs it. That’s too bad. I suppose this can still work with a namespace attribute attached to the alias, that’s just an annoying amount of boilerplate.

sbrocket commented 4 years ago

I’m going to put together a PR to add alias support for shared types, but before I do that could you take a look at #298 so that I know I’m not off in the woods on any of the changes there?

sbrocket commented 4 years ago

I've got a draft PR #308 now for the shared type aliases. Still a few things to clean up and do but would appreciate review on the design, and same for my follow-up comment on #298.

alexeyr commented 3 years ago

Just to clarify, for now the choice is between declaring everything in a single cxx::bridge mod and treating types from another mod as extern "Rust"?

dtolnay commented 3 years ago

This was fixed actually by the chain of PRs relating to #313. I put up #465 to demonstrate how it works, using the types from @sbrocket's use case at the top of this issue.

#[cxx::bridge(namespace = "my_usage::ffi")]
mod ffi {
    #[namespace = "zx::ffi"]
    extern "C++" {
        include!("cxx-handles-demo/src/handle.rs.h");
        type Job = crate::handle::ffi::Job;
        type Process = crate::handle::ffi::Process;
    }

    extern "Rust" {
        fn create_process(name: &CxxString, job: &Job) -> Process;
    }
}

I'll go ahead and close this issue, since this is possible now.

But as you can see it's somewhat verbose at the moment -- we'll be following up on that as part of #353 to possibly make it work with use something like:

#[cxx::bridge(namespace = "my_usage::ffi")]
mod ffi {
    #[namespace = "zx::ffi"]
    use crate::handle::ffi::{Job, Process};

    extern "Rust" {
        fn create_process(name: &CxxString, job: &Job) -> Process;
    }
}
alexeyr commented 3 years ago

Thank you very much! I did look at the linked issues/pull requests but this one wasn't among them.