Derive partialeq "manually" when possible #1012

pepyakin · 2017-09-21T20:29:19Z

Fixes #879

highfive · 2017-09-21T20:29:25Z

Warning

These commits modify unsafe code. Please review it carefully!

pepyakin · 2017-09-21T20:34:26Z

src/codegen/impl_partialeq.rs

+        });
+    } else {
+        for (i, base) in comp_info.base_members().iter().enumerate() {
+            // TODO: Move base field name generation into either in IR or in context.


@fitzgen There is a similar thing as with anonymous field names.
It's kinda works but ugly. What is the best possible way there?

Let's add a field_name member to Base, compute it upon construction of the Base, and then use that name both here and in codegen.

In fact, that could even be its own little PR, and we could land it independently of the rest of this stuff.

In fact, that could even be its own little PR, and we could land it independently of the rest of this stuff.

Done #1014 !

pepyakin · 2017-09-21T20:38:24Z

src/lib.rs

@@ -247,10 +255,6 @@ impl Builder {
            output_vector.push("--no-derive-debug".into());
        }

-        if !self.options.impl_debug {


I suppose this was a bug...

This should be if self.options.impl_debug { ... }; we still want to add it to the output when set. Care to do this in a new PR?

Err, I see you did that in another place. Want to submit that as a new PR or at least make it its own standalone commit?

Yeah, I thought I would rebase & squash, but now I see that this PR is becoming too big.

pepyakin · 2017-09-21T20:46:27Z

src/ir/analysis/derive_partial_eq_or_partial_ord.rs

+                        // only for arrays greater than 32.
+                        // TODO: But is there can be only arrays? 
+                        // Why inside of the can_trivially_derive_partialeq_or_partialord is map_or(false) ?
+                        return self.insert(id, Reason::ArrayTooLarge);


l.opaque().can_trivially_derive_partialeq_or_partialord() contains a check if the array is large enough or not.

https://github.com/rust-lang-nursery/rust-bindgen/blob/30d6e0417a691da4f4959c8083328241fc02d1fd/src/ir/layout.rs#L142-L146

but it feels ugly, because it depends on the implementation of l.opaque().can_trivially_derive_partialeq_or_partialord(). Do you see a better way there?

As an aside:
Now, I realize that it is generally ok to derive PartialEq for the opaque, but what it means to derive PartialOrd for the opaque? Maybe we should split those...

We could define enum CanDerive { Yes, No(Reason) } and have CanTriviallyDerive... return No(ArrayTooLarge) so that we aren't relying on implicit information here.

Now, I realize that it is generally ok to derive PartialEq for the opaque, but what it means to derive PartialOrd for the opaque? Maybe we should split those...

Want to file a new issue to discuss this?

Want to file a new issue to discuss this?

Done #1013 !

pepyakin · 2017-09-21T20:47:22Z

src/ir/analysis/derive_partial_eq_or_partial_ord.rs

@@ -143,8 +146,9 @@ impl<'ctx> MonotoneFramework for CannotDerivePartialEqOrPartialOrd<'ctx> {
                trace!("    we can trivially derive PartialEq or PartialOrd for the layout");
                ConstrainResult::Same
            } else {
+                // TODO: opaque can depend on arrays size at the moment


Same as:

https://github.com/rust-lang-nursery/rust-bindgen/pull/1012/files#diff-3c235a7615d25690c0ef47e46b9a589dR277

pepyakin · 2017-09-21T20:48:43Z

src/codegen/impl_partialeq.rs

+                    let name = match fd.name() {
+                        Some(name) => name,
+                        None => {
+                            // TODO: Bitfield stuff


@fitzgen Is it OK to just warn and don't generate PartialEq until we rule out the situation around anonymous field names?

Yeah that's fine.

@fitzgen

Generate base names upon construction As per #1012 (comment) r? @fitzgen

bors-servo · 2017-09-22T20:23:54Z

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

pepyakin · 2017-09-22T23:26:50Z

Hm, not sure what is going on.

These tests (cd tests/expectations && cargo test) passes locally (well, almost, except those objc* ones)

The error

error[E0277]: the trait bound `[i32; 33]: std::clone::Clone` is not satisfied
  --> tests/derive-partialeq-template-inst.rs:10:5
   |
10 |     pub large: [::std::os::raw::c_int; 33usize],
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for `[i32; 33]`
   |
   = help: the following implementations were found:
             <[T; 24] as std::clone::Clone>
             <[T; 6] as std::clone::Clone>
             <[T; 16] as std::clone::Clone>
             <[T; 0] as std::clone::Clone>
           and 29 others
   = note: required by `std::clone::Clone::clone`

Seems like this struct doesn't derive Copy. Have no idea why is that.

pepyakin · 2017-09-24T10:50:34Z

I managed to reproduce this on my machine. This happens only on stable rustc 1.20 and not on beta/nightly because of all_the_clones.

bors-servo · 2017-09-24T19:54:29Z

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

fitzgen

Looks great! Just a couple nitpicks below.

Thanks @pepyakin !

fitzgen · 2017-10-02T19:32:05Z

src/codegen/impl_partialeq.rs

+    } else if comp_info.kind() == CompKind::Union {
+        tokens.push(quote! {
+            &self.bindgen_union_field[..] == &other.bindgen_union_field[..]
+        });


This will only work if we aren't emitting Rust unions, so it should check ctx.options().rust_features().untagged_union()

I guess we would have CannotDeriveReason::Other here for Rust unions so this logic is actually OK as-is. That's pretty subtle, so we should definitely assert!(!ctx.options().rust_features().untagged_union()) in this branch.

Makes sense!

fitzgen · 2017-10-02T19:36:00Z

src/codegen/impl_partialeq.rs

+            quote_equals(name_ident)
+        } else {
+            quote! {
+                &self. #name_ident [..] == &other. #name_ident [..]


Nice, we don't even need a loop ;)

I was inspired by rustc itself (Just looked how it handles cases below 32 😄 )

fitzgen · 2017-10-02T19:40:49Z

src/ir/analysis/derive_partial_eq_or_partial_ord.rs

+            Some(CannotDeriveReason::Other) => 2,
+        }
+    }
+    rank(from) <= rank(to)


It seems like this whole rank thing could be an implementation of PartialOrd for CannotDeriveReason and then we could do from <= to. Am I missing anything?

I took that way because I didn't want to make public the fact that they are can be ordered.

And by they I mean Option<CannotDeriveReason> :)

What do you think, @fitzgen , is it makes sense?

Sure, sounds good to me

fitzgen · 2017-10-02T19:52:50Z

src/ir/analysis/derive_partial_eq_or_partial_ord.rs

+        trace!("constrain: {:?}", id);
+
+        if Some(CannotDeriveReason::Other)
+            == self.cannot_derive_partialeq_or_partialord.get(&id).cloned()


Use an if-let here?

if let Some(CannotDeriveReason::Other) = ...

fitzgen · 2017-10-02T19:54:41Z

src/ir/analysis/derive_partial_eq_or_partial_ord.rs

+        }
+    }
+
+    fn constrain_join(&mut self, item: &Item) -> Option<CannotDeriveReason> {


Thanks for adding this!

fitzgen · 2017-10-02T20:03:17Z

tests/headers/derive-partialeq-union.hpp

+// bindgen-flags: --with-derive-partialeq --impl-partialeq
+
+// Deriving PartialEq for rust unions is not supported.
+union ShouldNotDerivePartialEq {


If we make this comment use /// or /** ... */ style, then it will appear in the expectations too, which is sometimes useful for checking that the trait you expect to derive or not is as expected.

fitzgen · 2017-10-02T20:03:32Z

tests/headers/derive-partialeq-union_1_0.hpp

+// bindgen-flags: --rust-target 1.0 --with-derive-partialeq --impl-partialeq
+
+// This should manually derive PartialEq.
+union ShouldDerivePartialEq {


fitzgen

👍 thanks!

fitzgen · 2017-10-02T21:05:32Z

@bors-servo r+

bors-servo · 2017-10-02T21:05:32Z

📌 Commit b7bd43a has been approved by fitzgen

bors-servo · 2017-10-02T21:05:35Z

⌛ Testing commit b7bd43a with merge 48ca92b...

@fitzgen

Derive partialeq "manually" when possible Fixes #879 r? @fitzgen

bors-servo · 2017-10-02T21:05:52Z

💔 Test failed - status-travis

bors-servo · 2017-10-02T21:13:07Z

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

bors-servo · 2017-10-02T21:34:40Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 48ca92b to master...

bors-servo · 2017-10-02T21:34:42Z

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

Remove derive-partialeq-template-inst test. Add comments. Don't implement PartialEq for incomplete arrays Handle opaque bases and template instantiations Extract constrain_type. Extract `is whitelisted?` check Add failing partialeq-anonfield join for comps Fix: return insert if not whitelisted Delegate TypeRefs and alias to constrain_join. Delegate Template instantiations to constrain_join Add derive-partialeq-pointer.hpp test Update comment. Fix layout alignment larger that array limit Add missing test for derive-partialeq-anonfield.rs Clean Clean Fix typo in opaque-template-inst-member test Remove redudant stmt Add comment on can_supersede. Format impl_partialeq and leave a comment Extract requires_storage into it's own function. Clean

pepyakin · 2017-10-02T21:41:22Z

I rebased the PR @fitzgen !

fitzgen · 2017-10-02T21:47:11Z

@bors-servo r+

bors-servo · 2017-10-02T21:47:12Z

📌 Commit 617c55b has been approved by fitzgen

bors-servo · 2017-10-02T21:47:15Z

⌛ Testing commit 617c55b with merge 4e74c39...

@fitzgen

Derive partialeq "manually" when possible Fixes #879 r? @fitzgen

bors-servo · 2017-10-02T22:18:52Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 4e74c39 to master...

highfive assigned fitzgen Sep 21, 2017

highfive added the S-awaiting-review label Sep 21, 2017

pepyakin commented Sep 21, 2017

View reviewed changes

pepyakin mentioned this pull request Sep 22, 2017

Generate base names upon construction #1014

Merged

bors-servo pushed a commit that referenced this pull request Sep 22, 2017

Auto merge of #1014 - pepyakin:generate-base-names, r=fitzgen

becdc79

Generate base names upon construction As per #1012 (comment) r? @fitzgen

highfive added the S-needs-rebase label Sep 22, 2017

pepyakin force-pushed the derive-partialeq-manually branch from 6f7622e to 86d00aa Compare September 22, 2017 22:48

highfive added the S-needs-rebase label Sep 24, 2017

pepyakin force-pushed the derive-partialeq-manually branch 3 times, most recently from a095153 to 4932c6f Compare October 2, 2017 11:10

fitzgen reviewed Oct 2, 2017

View reviewed changes

pepyakin force-pushed the derive-partialeq-manually branch from 4932c6f to b7bd43a Compare October 2, 2017 20:57

fitzgen approved these changes Oct 2, 2017

View reviewed changes

fitzgen changed the title ~~Derive partialeq "manually" when possible [WIP]~~ Derive partialeq "manually" when possible Oct 2, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Oct 2, 2017

bors-servo pushed a commit that referenced this pull request Oct 2, 2017

Auto merge of #1012 - pepyakin:derive-partialeq-manually, r=fitzgen

48ca92b

Derive partialeq "manually" when possible Fixes #879 r? @fitzgen

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 2, 2017

highfive added the S-needs-rebase label Oct 2, 2017

pepyakin added 4 commits October 3, 2017 00:37

Use if let instead of plain if.

6eb4f1e

Assert we only gen partialeq for non rust unions

b02b158

Turn comments into doc comments in tests.

617c55b

pepyakin force-pushed the derive-partialeq-manually branch from b7bd43a to 617c55b Compare October 2, 2017 21:39

highfive added S-awaiting-review and removed S-tests-failed labels Oct 2, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Oct 2, 2017

bors-servo pushed a commit that referenced this pull request Oct 2, 2017

Auto merge of #1012 - pepyakin:derive-partialeq-manually, r=fitzgen

4e74c39

Derive partialeq "manually" when possible Fixes #879 r? @fitzgen

bors-servo merged commit 617c55b into rust-lang:master Oct 2, 2017

highfive removed the S-awaiting-merge label Oct 2, 2017

pepyakin deleted the derive-partialeq-manually branch October 3, 2017 05:29

Derive partialeq "manually" when possible #1012

Derive partialeq "manually" when possible #1012

Conversation

pepyakin commented Sep 21, 2017

highfive commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Sep 22, 2017

pepyakin commented Sep 22, 2017

pepyakin commented Sep 24, 2017 • edited Loading

bors-servo commented Sep 24, 2017

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen commented Oct 2, 2017

bors-servo commented Oct 2, 2017

bors-servo commented Oct 2, 2017

bors-servo commented Oct 2, 2017

bors-servo commented Oct 2, 2017

bors-servo commented Oct 2, 2017

bors-servo commented Oct 2, 2017

pepyakin commented Oct 2, 2017

fitzgen commented Oct 2, 2017

bors-servo commented Oct 2, 2017

bors-servo commented Oct 2, 2017

bors-servo commented Oct 2, 2017

pepyakin Sep 21, 2017 •

edited

Loading

pepyakin Sep 22, 2017 •

edited

Loading

pepyakin Sep 21, 2017 •

edited

Loading

pepyakin commented Sep 24, 2017 •

edited

Loading