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

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

Closed
sbrocket opened this issue Sep 11, 2020 · 10 comments
Closed

Comments

@sbrocket
Copy link
Contributor

sbrocket commented Sep 11, 2020

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
Copy link
Owner

dtolnay commented Sep 11, 2020

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
Copy link
Contributor Author

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

@sbrocket
Copy link
Contributor Author

sbrocket commented Sep 12, 2020

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
Copy link
Contributor Author

sbrocket commented Sep 12, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

alexeyr commented Nov 13, 2020

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
Copy link
Owner

dtolnay commented Nov 13, 2020

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;
    }
}

@dtolnay dtolnay closed this as completed Nov 13, 2020
@alexeyr
Copy link

alexeyr commented Nov 14, 2020

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

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