Skip to content

Do not derive Clone/Copy/Eq*/Ord* for opaque types #1667

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/codegen/impl_partialeq.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ pub fn gen_partialeq_impl(
item: &Item,
ty_for_impl: &proc_macro2::TokenStream,
) -> Option<proc_macro2::TokenStream> {
debug_assert!(
!item.is_opaque(ctx, &()),
"You're not supposed to call this on an opaque type"
);
let mut tokens = vec![];
Copy link
Contributor

Choose a reason for hiding this comment

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

You should just debug_assert!(!item.is_opaque(..)) if we decide not to derive partialeq for these, then remove the conditional below.


if item.is_opaque(ctx, &()) {
tokens.push(quote! {
&self._bindgen_opaque_blob[..] == &other._bindgen_opaque_blob[..]
});
} else if comp_info.kind() == CompKind::Union {
if comp_info.kind() == CompKind::Union {
assert!(!ctx.options().rust_features().untagged_union);
tokens.push(quote! {
&self.bindgen_union_field[..] == &other.bindgen_union_field[..]
Expand Down
45 changes: 16 additions & 29 deletions src/ir/analysis/derive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,37 +156,14 @@ impl<'ctx> CannotDerive<'ctx> {

trace!("ty: {:?}", ty);
if item.is_opaque(self.ctx, &()) {
if !self.derive_trait.can_derive_union() &&
ty.is_union() &&
self.ctx.options().rust_features().untagged_union
{
trace!(
" cannot derive {} for Rust unions",
self.derive_trait
);
return CanDerive::No;
}

let layout_can_derive =
ty.layout(self.ctx).map_or(CanDerive::Yes, |l| {
let can_derive_opaque = self.derive_trait.can_derive_opaque();
if can_derive_opaque == CanDerive::Yes {
return ty.layout(self.ctx).map_or(can_derive_opaque, |l| {
l.opaque().array_size_within_derive_limit(self.ctx)
});

match layout_can_derive {
CanDerive::Yes => {
trace!(
" we can trivially derive {} for the layout",
self.derive_trait
);
}
_ => {
trace!(
" we cannot derive {} for the layout",
self.derive_trait
);
}
};
return layout_can_derive;
} else {
return can_derive_opaque;
}
}

match *ty.kind() {
Expand Down Expand Up @@ -513,6 +490,16 @@ impl DeriveTrait {
}
}

fn can_derive_opaque(&self) -> CanDerive {
match self {
DeriveTrait::Copy |
DeriveTrait::Hash |
DeriveTrait::PartialEqOrPartialOrd => CanDerive::No,
DeriveTrait::Default => CanDerive::Yes,
DeriveTrait::Debug => CanDerive::Manually,
}
}

fn can_derive_fnptr(&self, f: &FunctionSig) -> CanDerive {
match (self, f.function_pointers_can_derive()) {
(DeriveTrait::Copy, _) | (DeriveTrait::Default, _) | (_, true) => {
Expand Down
2 changes: 1 addition & 1 deletion tests/expectations/tests/annotation_hide.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
/// <div rustbindgen opaque></div>
#[repr(C)]
#[repr(align(4))]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct D {
pub _bindgen_opaque_blob: u32,
}
Expand Down
2 changes: 1 addition & 1 deletion tests/expectations/tests/bitfield_large_overflow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

#[repr(C)]
#[repr(align(8))]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct _bindgen_ty_1 {
pub _bindgen_opaque_blob: [u64; 10usize],
}
Expand Down
7 changes: 1 addition & 6 deletions tests/expectations/tests/doggo-or-null.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ fn bindgen_test_layout_Null() {
/// this test to make sure that opaque unions don't derive and still compile.
#[repr(C)]
#[repr(align(4))]
#[derive(Copy, Clone)]
#[derive(Default)]
pub union DoggoOrNull {
pub _bindgen_opaque_blob: u32,
}
Expand All @@ -73,8 +73,3 @@ fn bindgen_test_layout_DoggoOrNull() {
concat!("Alignment of ", stringify!(DoggoOrNull))
);
}
impl Default for DoggoOrNull {
fn default() -> Self {
unsafe { ::std::mem::zeroed() }
}
}
7 changes: 1 addition & 6 deletions tests/expectations/tests/empty-union.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,7 @@
)]

#[repr(C)]
#[derive(Copy, Clone)]
#[derive(Default)]
pub union a__bindgen_ty_1 {
pub _address: u8,
}
impl Default for a__bindgen_ty_1 {
fn default() -> Self {
unsafe { ::std::mem::zeroed() }
}
}
4 changes: 2 additions & 2 deletions tests/expectations/tests/forward_declared_opaque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@
)]

#[repr(C)]
#[derive(Copy, Clone)]
#[derive(Default)]
pub struct a {
_unused: [u8; 0],
}
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct b {
_unused: [u8; 0],
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ pub enum _bindgen_ty_1 {
}
pub type JS_Alias = u8;
#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct JS_Base {
pub f: JS_Alias,
}
Expand All @@ -27,7 +26,6 @@ impl Default for JS_Base {
}
}
#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct JS_AutoIdVector {
pub _base: JS_Base,
}
Expand Down
4 changes: 2 additions & 2 deletions tests/expectations/tests/issue-573-layout-test-failures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@
)]

#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct Outer {
pub i: u8,
}
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct AutoIdVector {
pub ar: Outer,
}
Expand Down
4 changes: 2 additions & 2 deletions tests/expectations/tests/issue-674-1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ pub mod root {
#[allow(unused_imports)]
use self::super::super::root;
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct Maybe {
pub _address: u8,
}
pub type Maybe_ValueType<T> = T;
}
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct CapturingContentInfo {
pub a: u8,
}
Expand Down
6 changes: 3 additions & 3 deletions tests/expectations/tests/issue-674-2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ pub mod root {
#[allow(unused_imports)]
use self::super::super::root;
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct Rooted {
pub _address: u8,
}
pub type Rooted_ElementType<T> = T;
}
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct c {
pub b: u8,
}
Expand All @@ -45,7 +45,7 @@ pub mod root {
);
}
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct B {
pub a: root::c,
}
Expand Down
6 changes: 3 additions & 3 deletions tests/expectations/tests/issue-674-3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ pub mod root {
#[allow(unused_imports)]
use self::super::root;
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct nsRefPtrHashtable {
pub _address: u8,
}
pub type nsRefPtrHashtable_UserDataType<PtrType> = *mut PtrType;
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct a {
pub b: u8,
}
Expand All @@ -41,7 +41,7 @@ pub mod root {
);
}
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct nsCSSValue {
pub c: root::a,
}
Expand Down
4 changes: 2 additions & 2 deletions tests/expectations/tests/issue-801-opaque-sloppiness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub struct A {
}
#[repr(C)]
#[repr(align(1))]
#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Default)]
pub struct B {
pub _bindgen_opaque_blob: u8,
}
Expand All @@ -36,7 +36,7 @@ extern "C" {
pub static mut B_a: A;
}
#[repr(C)]
#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Default)]
pub struct C {
pub b: B,
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ fn bindgen_test_layout_SuchWow() {
}
#[repr(C)]
#[repr(align(1))]
#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Default)]
pub struct Opaque {
pub _bindgen_opaque_blob: u8,
}
Expand Down Expand Up @@ -105,7 +105,7 @@ extern "C" {
pub static mut Opaque_MAJESTIC_AF: Doggo;
}
#[repr(C)]
#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Default)]
pub struct Whitelisted {
pub some_member: Opaque,
}
Expand Down
2 changes: 1 addition & 1 deletion tests/expectations/tests/no-hash-opaque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

#[repr(C)]
#[repr(align(4))]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct NoHash {
pub _bindgen_opaque_blob: u32,
}
Expand Down
2 changes: 1 addition & 1 deletion tests/expectations/tests/no-partialeq-opaque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

#[repr(C)]
#[repr(align(4))]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct NoPartialEq {
pub _bindgen_opaque_blob: u32,
}
Expand Down
2 changes: 1 addition & 1 deletion tests/expectations/tests/no_copy_opaque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

#[repr(C)]
#[repr(align(4))]
#[derive(Debug, Default)]
#[derive(Default)]
pub struct NoCopy {
pub _bindgen_opaque_blob: u32,
}
Expand Down
2 changes: 1 addition & 1 deletion tests/expectations/tests/non-type-params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
pub type Array16 = u8;
pub type ArrayInt4 = [u32; 4usize];
#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
#[derive(Default)]
pub struct UsesArray {
pub array_char_16: [u8; 16usize],
pub array_bool_8: [u8; 8usize],
Expand Down
5 changes: 2 additions & 3 deletions tests/expectations/tests/opaque-template-inst-member-2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
/// This is like `opaque-template-inst-member.hpp` except exercising the cases
/// where we are OK to derive Debug/Hash/PartialEq.
#[repr(C)]
#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Default)]
pub struct OpaqueTemplate {
pub _address: u8,
}
/// Should derive Debug/Hash/PartialEq.
#[repr(C)]
#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Default)]
pub struct ContainsOpaqueTemplate {
pub mBlah: u32,
pub mBaz: ::std::os::raw::c_int,
Expand Down Expand Up @@ -62,7 +62,6 @@ fn bindgen_test_layout_ContainsOpaqueTemplate() {
}
/// Should also derive Debug/Hash/PartialEq.
#[repr(C)]
#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
pub struct InheritsOpaqueTemplate {
pub _base: u8,
pub wow: *mut ::std::os::raw::c_char,
Expand Down
12 changes: 1 addition & 11 deletions tests/expectations/tests/opaque-template-inst-member.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
)]

#[repr(C)]
#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Default)]
pub struct OpaqueTemplate {
pub _address: u8,
}
Expand Down Expand Up @@ -63,11 +63,6 @@ impl Default for ContainsOpaqueTemplate {
unsafe { ::std::mem::zeroed() }
}
}
impl ::std::cmp::PartialEq for ContainsOpaqueTemplate {
fn eq(&self, other: &ContainsOpaqueTemplate) -> bool {
&self.mBlah[..] == &other.mBlah[..] && self.mBaz == other.mBaz
}
}
/// This should not end up deriving Debug/Hash either, for similar reasons, although
/// we're exercising base member edges now.
#[repr(C)]
Expand Down Expand Up @@ -106,8 +101,3 @@ impl Default for InheritsOpaqueTemplate {
unsafe { ::std::mem::zeroed() }
}
}
impl ::std::cmp::PartialEq for InheritsOpaqueTemplate {
fn eq(&self, other: &InheritsOpaqueTemplate) -> bool {
&self._base[..] == &other._base[..] && self.wow == other.wow
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ pub mod root {
}
}
#[repr(C)]
#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Default)]
pub struct ContainsOpaqueInstantiation {
pub opaque: u32,
}
Expand Down
2 changes: 1 addition & 1 deletion tests/expectations/tests/opaque-template-instantiation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl Default for ContainsInstantiation {
}
}
#[repr(C)]
#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Default)]
pub struct ContainsOpaqueInstantiation {
pub opaque: u32,
}
Expand Down
2 changes: 1 addition & 1 deletion tests/expectations/tests/opaque-tracing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ extern "C" {
}
#[repr(C)]
#[repr(align(4))]
#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Default)]
pub struct Container {
pub _bindgen_opaque_blob: [u32; 2usize],
}
Expand Down
Loading