Skip to content

Conversation

@jsgf
Copy link
Contributor

@jsgf jsgf commented Dec 10, 2025

Rather than deriving a fixed set of traits for _BindgenOpaqueArray and similar wrapper types, derive the same traits that the types using them need. This avoids deriving unused traits, and also supports custom traits.

@jsgf jsgf force-pushed the fix-wrapper-derives branch 9 times, most recently from bc2ab16 to 182e2cc Compare December 11, 2025 04:31
Rather than deriving a fixed set of traits for _BindgenOpaqueArray and
similar wrapper types, derive the same traits that the types using them
need. This avoids deriving unused traits, and also supports custom traits.
@jsgf jsgf force-pushed the fix-wrapper-derives branch from 182e2cc to afb8740 Compare December 13, 2025 20:57
Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

This seems like a significant amount of work for every generated struct... I wonder if instead we could just apply the general custom derive logic to the built-in types?

That'd be a bit more consistent with the non-built-in types (even though it requires configuration). It's a bit more explicit than automatically propagating it, too...

So, wdyt of just doing something like calling add_derives(... { "__BindgenBitfieldUnit", Struct }) etc when generating those types instead?

I think that'd be less special, cheaper, and equally powerful (or more? I can imagine wanting to derive a custom thingie for __BindgienBitfieldUnit or so only, perhaps?).

*(core::ptr::addr_of!((*this).storage) as *const u8)
.offset(byte_index as isize)
*core::ptr::addr_of!((*this).storage).cast::<u8>()
.add(byte_index)
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these seem drive-by / probably from a separate patch? But looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I had several changes on the go, and I didn't completely separate them.

@@ -2583,8 +2593,81 @@ impl CodeGenerator for CompInfo {
CanDerive::Manually;
}

let mut derives: Vec<_> = derivable_traits.into();
let mut derives = BTreeSet::new();
let derivable: Vec<&'static str> = derivable_traits.into();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid this intermediate vec here and above?

@jsgf
Copy link
Contributor Author

jsgf commented Dec 17, 2025

This seems like a significant amount of work for every generated struct... I wonder if instead we could just apply the general custom derive logic to the built-in types?

That'd be a bit more consistent with the non-built-in types (even though it requires configuration). It's a bit more explicit than automatically propagating it, too...

So, wdyt of just doing something like calling add_derives(... { "__BindgenBitfieldUnit", Struct }) etc when generating those types instead?

I think that'd be less special, cheaper, and equally powerful (or more? I can imagine wanting to derive a custom thingie for __BindgienBitfieldUnit or so only, perhaps?).

I guess it really depends on how much you want to treat these internal structures as public interface. If you require users to explicitly derive types traits for them, then:

  1. You're exposing that they exist at all, and users need to work out where and when they are used (probably by inspection or trial and error), and
  2. You're committing to them unless you're willing to introduce back-compat breaks when you change how they get emitted, or add new ones.

It seemed to me that it's better to treat them as internal implementation details, so they just inherit the traits their containing structures have. I've run into some cases where they won't derive (eg padding doesn't work with bytemuck's Pod trait), but so far that's been legit (Pod types aren't supposed to have padding anyway).

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.

2 participants