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

Implement shared type aliases from another bridge module #308

Closed
wants to merge 13 commits into from

Conversation

sbrocket
Copy link
Contributor

@sbrocket sbrocket commented Sep 20, 2020

This is still a WIP but the core of this is working now. Two main things to do:

  • Figure out how to handle both bridges trying to emit the Vec/Box functions, and how to make usage in UniquePtr/CxxVector work if the remote bridge doesn't use the shared type in that way
  • Clean up TODOs about adding ui tests

My other PR (#298) will also need to rebased on this or vice versa. The first 4 commits here are duplicated between them.

Fixes #297

@sbrocket
Copy link
Contributor Author

sbrocket commented Sep 20, 2020

It seems like in order to resolve the first issue I mention above - so that the monomorphized functions for Vec/Box/UniquePtr/CxxVector can be both generated when needed and also not have duplicate definitions - I'm going to need to make the name of those symbols somehow unique within a crate.

If I could somehow get the module path within the crate then I could include that in the function names, but I'm not sure how I'd get that short of the user specifying the crate root when calling cxxbridge and then calculating it from the path of the file being processed. On the proc_macro side it doesn't seem possible at all...there's the unstable proc_macro::Span::source_file() but then I don't have the crate root.

Any other ideas? The linkage attribute (rust-lang/rust#29603) seems like it's not going anywhere anytime soon or making them weak symbols would work.

@sbrocket
Copy link
Contributor Author

sbrocket commented Sep 20, 2020

Actually, it needs to not just be unique within the crate but globally, so module path won't help.

The only other option I can think of is to unconditionally expand the Vec/Box/UniquePtr/CxxVector functions for all shared types in the module where they're declared (and never on the alias side). That's pretty brute force but not sure how else to resolve this without weak linkage. That will also get worse (in terms of code size) as more such wrapper types are added, and would complicate adding wrapper type extensibility (aka #251). The code size impact should be mitigated for folks that use LTO, at least.

Actually, I think this is an existing bug with type aliases generally, and there's just no test coverage for Vec<C> and so forth where C is aliased. Maybe it can be addressed separately from this PR?

@dtolnay
Copy link
Owner

dtolnay commented Sep 20, 2020

For Vec/etc impls, I would prefer if only the "real" module of a type (the one that the others re-export from) were able to define those. That means something like a new attribute or other syntax to request inclusion of particular impls even when they don't appear in that module's ffi function signatures. For instance we could do:

mod ffi {
    struct C {...}

    impl Vec<C> {}  // allowed as long as your module defines C
}

@dtolnay
Copy link
Owner

dtolnay commented Sep 20, 2020

The only other option I can think of is to unconditionally expand the Vec/Box/UniquePtr/CxxVector functions for all shared types in the module where they're declared

I don't want this because it doesn't work once we get to map bindings (std::unordered_map etc). Doing an impl for every possible pair of two types would be unreasonable, so we would at least need another solution for the map case, and once there is another solution anyway I think there isn't a reason to do this Vec etc.

@sbrocket
Copy link
Contributor Author

sbrocket commented Sep 20, 2020

Ok, that suggestion makes sense, and good point about map. One downside is that it means that authors of cxx type libraries need to anticipate which wrapper types clients will want to use, but that seems workable.

Would you be fine with not expanding Box/Vec for aliases (as is already done for UniquePtr/CxxVector) in this PR and then adding the new syntax in a later PR? I can file a tracking issue. That still leaves users with an equivalent workaround; they can simply define an unused function in the main module.

@adetaylor
Copy link
Collaborator

@sbrocket I've been working in a similar area, and I hadn't realized till now. Oops.

See:
#325 which also adds an ExternType::Kind, albeit for a slightly different purpose (which probably doesn't conflict except textually)
#329 which implements the impl UniquePtrTarget<T> syntax discussed above.

How close are you to landing this? If it's a matter of days, I'll rebase on top of your branch.

@sbrocket
Copy link
Contributor Author

sbrocket commented Oct 1, 2020

I can finish it over today and tomorrow. I was waiting for a response from dtolnay@ to my question above, but if you’ve got it implemented then that makes the question moot.

@adetaylor
Copy link
Collaborator

OK cool. I am unlikely to get back to mine till next week so there's no urgency. But I will eventually plan to rebase on yours if you think it will land soon.

@sbrocket sbrocket marked this pull request as ready for review October 2, 2020 22:53
@sbrocket
Copy link
Contributor Author

sbrocket commented Oct 2, 2020

OK, this should be good to go now.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This largely overlaps with the ExternType improvements made as part of #313 / #325, so I'll go ahead and close. The ExternTypeKindOpaqueCpp and ExternTypeKindShared from this PR are broadly equivalent to the cxx::kind::Opaque and cxx::kind::Trivial we arrived at in the other PR.

The remaining pieces toward the implementation I'd like of shared type aliases are tracked in #353.

@dtolnay dtolnay closed this Oct 10, 2020
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 this pull request may close these issues.

Support for using shared types from one bridge in another bridge module
3 participants