Skip to content
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

Derive + blacklisted types #950

Merged

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Sep 5, 2017

  • document blacklisting + derive interaction
  • derive Copy
  • derive Debug
  • --impl-debug
  • derive Default
  • derive Hash
  • derive PartialEq

Presumably they are blacklisted because we can't understand them properly, so be
pessimistic about what we can derive for it.

Fixes rust-lang#944.
@highfive
Copy link

highfive commented Sep 5, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@fitzgen fitzgen changed the title [WIP] Derive + blacklisted types Derive + blacklisted types Sep 5, 2017
@fitzgen
Copy link
Member Author

fitzgen commented Sep 5, 2017

r? @emilio

@upsuper
Copy link
Contributor

upsuper commented Sep 5, 2017

Although bindgen shouldn't put any assumption on blacklisted type, it could potentially be a trouble to ban all the derivation on them as well. (I haven't tried, but Debug seems to be relied widely, so it might become a problem.)

Probably we can allow people to specify which blacklisted type can derive what trait.

Maybe I should try this patch first and see if it really becomes a problem.

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 looks ok to me, but probably we should give it a try on mozjs / stylo.

This makes it so that whitelisted items and edges outgoing from them are black,
while non-whitelisted items and their edges are gray.
We already have a test for more complicated template instantiations and
blacklisting and their affects on deriving `Hash`, but it is good to have sanity
tests for the simple cases too.
@fitzgen fitzgen force-pushed the no-derive-copy-for-blacklisted-types branch from ff21cdb to 641337d Compare September 6, 2017 20:47
@fitzgen
Copy link
Member Author

fitzgen commented Sep 6, 2017

This patch works fine with mozjs.

@fitzgen
Copy link
Member Author

fitzgen commented Sep 6, 2017

With Stylo, I get the following error:

error: this trait cannot be derived for unions
    --> /home/fitzgen/mozilla-central/obj-x86_64-pc-linux-gnu/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-14c013fa10f9d775/out/gecko/structs_release.rs:3238:22
     |
3238 |             #[derive(Debug, Copy)]
     |                      ^^^^^

on this struct:

            #[repr(C)]
            #[derive(Debug, Copy)]
            pub union OwningNodeOrString_Value {
                pub _bindgen_opaque_blob: [u64; 2usize],
            }

But let me double check that against master and see whether or not that same error occurs.

@fitzgen
Copy link
Member Author

fitzgen commented Sep 6, 2017

Hm yeah with master I get tons of "copy could not be implemented for this type".

@fitzgen
Copy link
Member Author

fitzgen commented Sep 6, 2017

Here is a minimal test case:

// bindgen-flags: --opaque-type DoggoOrNull

class Doggo {};

class None {};

union DoggoOrNull {
    Doggo doggo;
    None none;
};

Testing if this is also a bug on master or not.

@fitzgen
Copy link
Member Author

fitzgen commented Sep 6, 2017

It kind of seems to me that we shouldn't be emitting unions for opaque unions -- just structs of the right size/align...

@upsuper
Copy link
Contributor

upsuper commented Sep 6, 2017

You need to make bindgen target 1.0 for building stylo.

@fitzgen
Copy link
Member Author

fitzgen commented Sep 6, 2017

Ok, so I targeted 1.0 and still getting errors that seem unrelated to deriving:

error[E0432]: unresolved import `gecko_bindings::structs::ServoStyleSheet`
  --> /home/fitzgen/mozilla-central/servo/components/style/gecko/data.rs:10:77
   |
10 | use gecko_bindings::structs::{RawGeckoPresContextOwned, ServoStyleSetSizes, ServoStyleSheet};
   |                                                                             ^^^^^^^^^^^^^^^ no `ServoStyleSheet` in `gecko_bindings::structs`. Did you mean to use `ServoStyleContext`?

But I verified that root::mozilla::ServoStyleSheet is defined in the structs_*.rs bindings. Did it not used to be namespaced or something?

Anyways, I don't think these errors are related to this PR, and it fixes other errors that get hit when trying to build Stylo with master (the copy deriving) so I think we should land this.

Unions don't support deriving these things, even if there is only one
variant (the opaque layout field).
@fitzgen
Copy link
Member Author

fitzgen commented Sep 6, 2017

@bors-servo r=emilio

@bors-servo
Copy link

📌 Commit 74e2aea has been approved by emilio

@bors-servo
Copy link

⌛ Testing commit 74e2aea with merge cb10597...

bors-servo pushed a commit that referenced this pull request Sep 6, 2017
…emilio

Derive + blacklisted types

* [X] document blacklisting + derive interaction
* [X] derive `Copy`
* [X] derive `Debug`
* [X] `--impl-debug`
* [x] derive `Default`
* [x] derive `Hash`
* [x] derive `PartialEq`
@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: emilio
Pushing cb10597 to master...

@bors-servo bors-servo merged commit 74e2aea into rust-lang:master Sep 6, 2017
@fitzgen fitzgen deleted the no-derive-copy-for-blacklisted-types branch September 6, 2017 23:38
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.

5 participants