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

Added support for #[repr(transparent)] #185

Merged
merged 1 commit into from
Aug 28, 2018

Conversation

regexident
Copy link
Contributor

@regexident regexident commented Aug 3, 2018

#[repr(transparent)] was stabilized today:

Version 1.28.0 (2018-08-02)

The #[repr(transparent)] attribute is now stable. This attribute allows a Rust newtype wrapper (struct NewType(T);) to be represented as the inner type across Foreign Function Interface (FFI) boundaries.

@regexident regexident force-pushed the repr-transparent branch 4 times, most recently from 87444fc to 9774fb5 Compare August 3, 2018 00:53
@RReverser
Copy link
Contributor

One problem with that is that newtypes should be usually treated as different types, while with this approach they become interchangeable due to C/C++ side being just aliases, which can lead to mistakes... Not sure what would be a stable way to fix this.

@regexident
Copy link
Contributor Author

Oh, looks like I missed …

Coercions and casting between the transparent wrapper and its non-zero-sized types are forbidden.

… and kinda misunderstood the semantics.

What would be the appropriate/expected output for #[repr(transparent)] types then?

@RReverser
Copy link
Contributor

Well you can't coerce between them in Rust, but you can on C(++) side, and that's kinda the problem - you don't want Meters(u32) and Kilograms(u32) to be exchangeable without any type checks.

This brings us to a problem of strong typedefs in C(++) which don't exist, and hence I'm not sure what would be the proper way to simulate them.

Perhaps we should generate wrappers for each function instead of exposing it as-is, that would accept proper structures and unwrap them before passing to Rust? This would be somewhat dirty, but safe.

Otherwise, we could accept primitives where they're passed by value, but pointers to proper structs where they're passed by reference, so at least some of the calls would be checked (and this would be safe since C(++) guarantee that pointer to struct is same as pointer to first member).

Another alternative, albeit limited to only C++ and only primitives, would be to generate enum class : unsigned int and such, so that you couldn't accidentally put one in place of another without explicit casting.

I'm not sure which option would be the best, so let's wait for feedback from others.

@regexident
Copy link
Contributor Author

Had a chat with @gankro and @emilio at this week's Rust Berlin Meetup about cbindgen and #[repr(transparent)].

The consensus —as I understood it— was that the proposed implementation —albeit being inherently unsafe— is the only viable one given the lack of language support from C/C++.

@nox, you seem to have been the main author of rust-lang/rfcs#1758, could you weight in on what cbindgen should do for #[repr(transparent)]?

cc @eddyb

@eddyb
Copy link

eddyb commented Aug 11, 2018

You can use the GCC extension transparent_union. It's not ideal but AFAIK it does have the same ABI effects.

@RReverser
Copy link
Contributor

@eddyb Ohh I like this idea if it works in Clang and MSVC too.

@regexident
Copy link
Contributor Author

@RReverser At least clang seems to support it.

@RReverser
Copy link
Contributor

First, the argument corresponding to a transparent union type can be of any type in the union; no cast is required.

This doesn't sound good though, after all, the entire point is to make these types explicit wrappers.

@regexident regexident force-pushed the repr-transparent branch 3 times, most recently from eb97aee to 67f6110 Compare August 19, 2018 08:24
@eqrion
Copy link
Collaborator

eqrion commented Aug 28, 2018

It looks like this will need to be rebased. It's a bit sad that C/C++ doesn't support strong typedefs to fully get the benefit here. That being the case though, the approach this PR takes seems to be the best we'll be able to do. I'll do a review pass once it's been rebased.

@regexident
Copy link
Contributor Author

Rebased.

@eqrion eqrion merged commit 5e6d58d into mozilla:master Aug 28, 2018
@eqrion
Copy link
Collaborator

eqrion commented Aug 28, 2018

Thanks!

@regexident regexident deleted the repr-transparent branch August 28, 2018 21:56
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.

4 participants