Wrap template and opaque types in a marker. #3151
Conversation
As standard, bindgen treats C++ pointers and references the same in its
output. Downstream postprocessors might want to treat these things
differently; for example to use a CppRef<T> type for C++ references
to encode the fact that they're not (usually) null.
This PR emits references wrapped in a newtype wrapper called
bindgen_marker_Reference<T>
This behavior is enabled by an option flag; it isn't default.
This type isn't actually defined in bindgen; users are expected
to define or replace this during postprocessing (e.g. by using
syn to reinterpret bindgen's output, or perhaps there are ways
to make this useful using --raw-line or other means to inject
a transparent newtype wrapper).
The same approach is taken to types which bindgen chooses to make
opaque. This is done in various circumstances but the most common
case is for non-type template parameters.
Alternative designs considered:
* Apply an attribute to the function taking or returning
such a param, e.g.
#[bindgen_marker_takes_reference_arg(1)]
fn takes_reference(a: u32, b: *const Foo)
This avoids the marker type, but the problem here is a
more invasive change to bindgen because type information
can no longer be contained in a syn::Type; type metadata
needs to be communicated around inside bindgen.
* Make a ParseCallbacks call each time a type is opaque
or a reference. This would work for standalone opaque types,
e.g.
pub struct Bar {
pub _bindgen_opaque_blob: u8
}
but the main case where we need these is where bindgen is
using an opaque or reference type in the signature of some
function, and there's no real opportunity to describe this
in any kind of callback, since the callback would have to
describe where exactly in the function signature the
opaque or reference type has been used (and then we run into
the same problems of passing this metadata around in the
innards of bindgen).
In order to maintain the current simple design where any C++
type is represented as a simple syn::Type, the best approach
seems to be to do this wrapping in a fake marker type.
Another design decision here was to represent an RValue
reference as:
TypeKind::Reference(_, ReferenceKind::RValue)
rather than a new variant:
TypeKind::RValueReference(_)
In the majority of cases references are treated the same way
whether they're rvalue or lvalue, so this was less invasive,
but either is fine.
Part of google/autocxx#124
adetaylor
force-pushed
the
opaque-and-reference-annotations
branch
from
744fcca to
72a39cf
Compare
There was a problem hiding this comment.
No objection on the opaque wrapper if it makes stuff easier.
That said, at the very least it seems we should fix the test for the Reference wrapper to not add a separate indirection? I wonder if some other folks are going to copy paste from the test then ask why their code is crashing all over the place.
At first I thought that Reference<T> (vs the current patch's Reference<*const T> or Reference<*mut T>) was strictly better / more flexible, but I guess that does lose the cons-ness information... Do you have any thoughts there? I wonder if we should really have Reference<T> / ConstReference<T> / RValueReference<T> (to keep that distinction). I don't think I've ever seen a use case for a const RValue reference (are those even a thing?).
Separately, if we're carrying around the reference type now, it seems it'd be worth unifying it with Pointer, unless there's a reason it can't be done / it's more annoying.
| unsafe extern "C" { | ||
| #[link_name = "\u{1}_Z17receive_referenceRKi"] | ||
| pub fn receive_reference( | ||
| input: __bindgen_marker_Reference<*const ::std::os::raw::c_int>, |
There was a problem hiding this comment.
This feels wrong? an rvalue reference to an int should be a regular pointer to an int, not a pointer-to-pointer, which is what this would end up being.
| @@ -0,0 +1,23 @@ | |||
| #![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)] | |||
| #[repr(transparent)] | |||
| pub struct __bindgen_marker_Reference<T: ?Sized>(*const T); | |||
There was a problem hiding this comment.
Can you add some tests for these in structs? In particular reference members in structs are not that uncommon.
I think this as-is breaks #[derive] and such, but maybe it's not a huge issue since it's opt-in. At least we should file an issue and reference it from the code that generates these wrappers.
There was a problem hiding this comment.
Ah, I guess this is only a test issue in practice, since the way this is implemented leaves the "reference" type to the consumer.
So seems fine with this changed to (T), or with the reference wrapping the inner thing (which might be more correct / flexible? You might want to e.g. make something like struct Reference<T>(NonNull<T>), and you otherwise wouldn't be able to.
| @@ -4358,7 +4364,7 @@ impl TryToRustTy for Type { | |||
| utils::build_path(item, ctx) | |||
| } | |||
| TypeKind::Opaque => self.try_to_opaque(ctx, item), | |||
| TypeKind::Pointer(inner) | TypeKind::Reference(inner) => { | |||
| TypeKind::Pointer(inner) | TypeKind::Reference(inner, _) => { | |||
There was a problem hiding this comment.
Do we handle Pointer and Reference meaningfully differently somewhere else? I wonder if instead of adding Reference(..., ReferenceKind) we should just have Pointer(inner, PointerKind) where PointerKind { Regular, LValueReference, RValueReference } or so.
| @@ -4391,7 +4397,23 @@ impl TryToRustTy for Type { | |||
| { | |||
| Ok(ty) | |||
| } else { | |||
| Ok(ty.to_ptr(is_const)) | |||
| let ty_ptr = ty.to_ptr(is_const); | |||
There was a problem hiding this comment.
So, I guess that means that this should be something like: "if reference, then Reference<#ty> else ty.to_ptr(is_const)"?
As standard, bindgen treats C++ pointers and references the same in its output. Downstream postprocessors might want to treat these things differently; for example to use a CppRef type for C++ references to encode the fact that they're not (usually) null.
This PR emits references wrapped in a newtype wrapper called
bindgen_marker_Reference
This behavior is enabled by an option flag; it isn't default. This type isn't actually defined in bindgen; users are expected to define or replace this during postprocessing (e.g. by using syn to reinterpret bindgen's output, or perhaps there are ways to make this useful using --raw-line or other means to inject a transparent newtype wrapper).
The same approach is taken to types which bindgen chooses to make opaque. This is done in various circumstances but the most common case is for non-type template parameters.
Alternative designs considered:
Apply an attribute to the function taking or returning such a param, e.g. #[bindgen_marker_takes_reference_arg(1)] fn takes_reference(a: u32, b: *const Foo) This avoids the marker type, but the problem here is a more invasive change to bindgen because type information can no longer be contained in a syn::Type; type metadata needs to be communicated around inside bindgen.
Make a ParseCallbacks call each time a type is opaque or a reference. This would work for standalone opaque types, e.g. pub struct Bar { pub _bindgen_opaque_blob: u8 } but the main case where we need these is where bindgen is using an opaque or reference type in the signature of some function, and there's no real opportunity to describe this in any kind of callback, since the callback would have to describe where exactly in the function signature the opaque or reference type has been used (and then we run into the same problems of passing this metadata around in the innards of bindgen).
In order to maintain the current simple design where any C++ type is represented as a simple syn::Type, the best approach seems to be to do this wrapping in a fake marker type.
Another design decision here was to represent an RValue reference as:
TypeKind::Reference(, ReferenceKind::RValue)
rather than a new variant:
TypeKind::RValueReference()
In the majority of cases references are treated the same way whether they're rvalue or lvalue, so this was less invasive, but either is fine.
Part of google/autocxx#124