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

[elichai]

Hi,
This is an attempt to solve #1656
I'm not sure if the IsOpaque trait is defined only on actually opaque types. but I used that to check and not impl anything except Debug/Default

[emilio]

Observables look good, but the place to check it is not quite right.

[emilio]

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

[emilio]

Instead of this, the right place for this is returning CanDerive::No here:

rust-bindgen/src/ir/analysis/derive.rs

Line 158 in ec85170

if item.is_opaque(self.ctx, &()) {

[elichai]

Yeah. I started there an then thought that maybe I should "attack" this at the source to make sure i'm not missing other places.
I'll change.

[elichai]

One problem is that this also affects Default and Debug
And for some reason also affected stuff the current code missed like

rust-bindgen/tests/expectations/tests/template.rs

Line 366 in f686751

#[derive(Debug, Default, Copy, Clone, Hash, PartialEq, Eq)]

[emilio]

Well, sure that'd affect more traits, you can choose what it affects conditionally adding something like self.derive_trait.can_derive_opaque().

[emilio]

Sorry, I hadn't noticed this was updated (GitHub likes not to notify for force-pushes looks like).

It seems like tests/opaque_pointer.rs doesn't build and CI is red because of that, because we still derive copy on the struct around the opaque thing.

[elichai]

Yeah, I'm not really sure what's the right way forward or if this PR is even a wanted feature (it is a breaking change).

I'll play with it a bit more to see if I can get the "right way" to still derive Default and Debug

[emilio]

Well, we should clearly not be deriving Copy if any of our members can't automatically derive Copy. Other than that, this feature is fine, though it's indeed breaking I doubt people would rely on it (and if they do, manually memcpying the thing they care about is probably a better way forward).

[elichai]

I changed the implementation, but I still don't get why it doesn't implement Debug manually (with the #name: { opaque } thing).
I'll continue to debug that later

[elichai]

Ops, I should fix both the rustfmt and the big array problem.
still trying to figure out why it's not manually implementing, it seems like for a single ItemId fn constrain(&mut self, id: ItemId) -> ConstrainResult is called 6-7 times, so it starts with the correct CanDerive::Manually but ends up with either No or Yes somehow.

Edit: I just missed the fact that it's a different trait :), it's still running multiple times by for my isolated test it always ends up with Debug -> Manually:

canonical name: OtherOpaque id: ItemId(7), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(12), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(10), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(1), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(12), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(12), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(7), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(10), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(7), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(10), can derive: Debug ? Manually
canonical name: OtherOpaque id: ItemId(1), can derive: Debug ? Manually

So the constain() function isn't the problem.

The problem seem to be ctx.options().impl_debug == false

[elichai]

Found the problem :)
So, by default impl_debug: false and derive_debug: true. so by me returning CanDerive::Manually I remove the debug unless the user explicitly asked for it to be implemented.

@emilio what do you think I should do? do you even agree that it makes more sense to print something like OpaqueType { opaque } then OpaqueType{ val: [0,0,0,0] }

[emilio]

@emilio what do you think I should do? do you even agree that it makes more sense to print something like OpaqueType { opaque } then OpaqueType{ val: [0,0,0,0] }

I'd be ok with that I think... Either way, really.

[elichai]

Hi, sorry I left this as is, context switching is hard.
I'm not sure what to do with the whole union/default thing, do we want it to manually impl default? do we don't? (deriving obviously doesn't work, and that's what is failing this PR right now)

I think this comment tries to explain that but I don't quite understand: https://github.com/rust-lang/rust-bindgen/pull/1667/files#diff-ab2ef507baf5b00651f924fb3cb4241cR51

[emilio]

So regarding unions, I think emitting an opaque struct for opaque unions is better, and should save a bit of code.

But I still need to take a closer look here.

[bors-servo]

☔ The latest upstream changes (presumably 0e25962) made this pull request unmergeable. Please resolve the merge conflicts.