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

RFC 2235 - Implement PartialEq,Eq,Hash,Debug for all types #1217

Merged
merged 7 commits into from Feb 5, 2019

Conversation

Projects
None yet
9 participants
@Susurrus
Copy link
Contributor

Susurrus commented Jan 19, 2019

First pass at implementing RFC2235. I just started with PartialEq/Eq and tested locally on my x64 Linux system. Definitely going to run into other types for other platforms that need this treatment, but thought I'd start small.

Open question is whether this should also rely on the align feature, which should improve which types can be auto-derived versus manually implemented (as it removed a lot of odd-sized padding arrays). My first pass here doesn't do that, but I might test it out to see if it does simplify quite a few structs. I think it might also be nice to have as it makes it easier for contributors to add new structs.

Part of rust-lang/rust#57715

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Jan 19, 2019

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

Cargo.toml Outdated
@@ -27,6 +27,7 @@ default = ["use_std"]
use_std = []
align = []
rustc-dep-of-std = ['align', 'rustc-std-workspace-core']
extra_traits = []

This comment has been minimized.

@gnzlbg

gnzlbg Jan 19, 2019

Collaborator

We might want to start by using more fine grained features here (e.g. traits_eq), and having an extra_traits feature that enables them all.

@gnzlbg

gnzlbg approved these changes Jan 19, 2019

Copy link
Collaborator

gnzlbg left a comment

LGTM. I worry about the extra maintainability cost of the structs for which the traits cannot be derived automatically (because they contain arrays with more than 32 elements).

Note: the style check is failing.

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Jan 21, 2019

Open question is whether this should also rely on the align feature, which should improve which types can be auto-derived versus manually implemented (as it removed a lot of odd-sized padding arrays).

I think so. align is stable and it is only available as a feature to support older Rust versions. Since this is a new feature, if align simplifies it, then it should depend on it. I don't see any downsides here. The same applies to packed(N) - by the time this hits stable, packed(N) will already be stable.

@Susurrus Susurrus force-pushed the Susurrus:rfc_2235 branch from 177a0a5 to 600b9c8 Jan 22, 2019

@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Jan 22, 2019

We might want to start by using more fine grained features here (e.g. traits_eq), and having an extra_traits feature that enables them all.

I'd prefer to not implement it like this. When we talk about maintainability costs, having more features is harder than less features. The RFC also had discussion about this, but I'd like to find out more about why it can't be done as the single trait as I've already implemented it before changing this to individual features.

LGTM. I worry about the extra maintainability cost of the structs for which the traits cannot be derived automatically (because they contain arrays with more than 32 elements).

Do you still have this concern after seeing the implementation? I'm uncertain what the maintainability cost will be that you're concerned about. There isn't much change to types after they're added to libc, so I'm wondering what you're referring to here.

I think so. align is stable and it is only available as a feature to support older Rust versions. Since this is a new feature, if align simplifies it, then it should depend on it. I don't see any downsides here. The same applies to packed(N) - by the time this hits stable, packed(N) will already be stable.

I did this, though looking at the code I didn't see any types I manually implemented this for that would actually benefit from this. I still think it's the right call, as it may help in structs that haven't been added yet though.

So I've completed implementing everything here, though there are some open questions on what to do for the fields that I skipped in my manual implementations. I'll see if they're trivial to implement manually or I'll ask if we can set guidelines such that some fields can be skipped to make things easier.

@gnzlbg

gnzlbg approved these changes Jan 22, 2019

Copy link
Collaborator

gnzlbg left a comment

This LGTM. Thanks!

@bors: r+

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Jan 22, 2019

Oh, I see that the documentation build bots are still failing. Could you fix those?

This error sounds familiar: cc @QuietMisdreavus

rustdoc -o target/doc/x86_64-unknown-linux-gnu --target x86_64-unknown-linux-gnu src/lib.rs --cfg cross_platform_docs --crate-name libc
error[E0433]: failed to resolve: maybe a missing `extern crate clone;`?
  --> src/macros.rs:42:22
   |
42 |             #[derive(Clone, Copy)]
   |                      ^^^^^ maybe a missing `extern crate clone;`?
error[E0433]: failed to resolve: maybe a missing `extern crate marker;`?
  --> src/macros.rs:42:29
   |
42 |             #[derive(Clone, Copy)]
   |                             ^^^^ maybe a missing `extern crate marker;`?
error[E0433]: failed to resolve: maybe a missing `extern crate clone;`?
  --> src/macros.rs:54:22
   |
54 |             #[derive(Clone, Copy)]
   |                      ^^^^^ maybe a missing `extern crate clone;`?
error[E0433]: failed to resolve: maybe a missing `extern crate marker;`?
  --> src/macros.rs:54:29
   |
54 |             #[derive(Clone, Copy)]
   |                             ^^^^ maybe a missing `extern crate marker;`?
@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 22, 2019

It looks like the cross_platform_docs cfg set by the doc builders removes libcore? It doesn't seem like anything is being put in its place, for the derive macros to be able to use...

libc/src/lib.rs

Lines 16 to 17 in fb2b3da

#![cfg_attr(cross_platform_docs, feature(no_core, lang_items, const_fn))]
#![cfg_attr(cross_platform_docs, no_core)]

Maybe some kind of import for dox::{Clone, Copy} that put the traits in the right module paths would fix this? Doesn't seem like a rustdoc-specific problem, though.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Jan 22, 2019

Actually, after digging into it, why are those doc builders still going if libc's docs are going to be hosted on docs.rs (which doesn't use --cfg cross_platform_docs)?

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Jan 22, 2019

That's a good question. I'd expect that libc docs are shipped with Rust (because one can directly do extern crate libc to get the same libc as libstd) but AFAICT that's not the case.

@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Jan 23, 2019

I also wanted to ask about the Cargo.lock file. I had to update it for this, but since this is a lib, the Cargo.lock doesn't need to be committed. Should I remove it in a first commit and add it to the .gitignore?

@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Jan 23, 2019

I've also been thinking that we should probably have a test that checks that each type has the right extra traits. This will ensure any manually-implemented types won't miss traits they should have. But I don't know how to start that implementation; any ideas anyone?

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Jan 23, 2019

@Susurrus don't worry about the update to the Cargo.lock.

I've also been thinking that we should probably have a test that checks that each type has the right extra traits. This will ensure any manually-implemented types won't miss traits they should have. But I don't know how to start that implementation; any ideas anyone?

IMO the right way to do this is via a clippy lint. That is, we would have (or add) a lint to clippy, e.g., missing_debug_impl_on_public_items, that requires all public types to implement, e.g., Debug. This lint would be disabled by default in clippy for obvious reasons, and we would opt-into it by enabling it for libc at the top level:

#![cfg_attr(feature = "cargo-clippy", deny(missing_debug_impl_on_public_items))]

This lint will error if a public item does not implement Debug. If there is an item for which we explicitly don't want to implement Debug, then we can opt-out for that item manually:

#[cfg_attr(feature = "cargo-clippy", allow(missing_debug_impl_on_public_items))]
struct LookMaNoDebug;

For every trait, we would need such a lint. You can open an issue about this in clippy upstream and cite this comment as motivation. Implementing such a lint is not hard, I can try to mentor you if you want, but clippy maintainers have a quick turnaround time and if you are in the Discord clippy channel they can tell you which existing lint you can base yours on to get it done quickly.

FYI there is prior art of doing this. For example, in stdsimd we need most of the API to be #[inline] or #[inline(always)], so I added a clippy lint missing_inline_in_public_items that checks that, and we use it in stdsimd and packed_simd successfully (it has caught a couple of bugs).

EDIT: one issue with this lint is that we would need to run clippy for all targets passing it the appropriate --target for it to verify all the platform specific code. It might be worth it to submit a PR that just does this first to see if there are any problems with running clippy for all targets. If there are, those problems would need to be addresses by clippy upstream, but that's something worth doing anyways, and libc is a good library to exercise that.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 23, 2019

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

@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Jan 23, 2019

@gnzlbg Awesome idea using Clippy, didn't come to mind for me, but it makes a lot of sense. I've gone through Clippy and found the following:

  • rustc already has a missing-debug-implementations feature. That should cover our needs and can be checked by clippy
  • clippy has a lint to check that PartialEq and Hash implementations may not be equal ( derive_hash_xor_eq)
  • rustc already has a missing-copy-implementations feature as well, should probably enable that as well in CI in all cases.

clippy has in the works a check to ensure the Debug impl isn't empty (rust-lang/rust-clippy/issues/1796). rust-lang/rust-clippy/issues/1798 implies that checks for Hash, PartialEq, and Eq are implemented as well, but I don't see that, so I'm confirming in that issue.

So I've enabled some more lints and fixed some types that broke because of that as you'll see in the latest commits.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 23, 2019

r? @gnzlbg

everything looks good to me so far, thanks! I'll defer to @gnzlbg from here

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 23, 2019

wrong button...

@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Jan 23, 2019

This isn't ready for merging yet. When it is, I'll remove the WIP from the title of the PR.

To catch everyone up on where this stands:

  • I need to sort out the lints.
  • I also need to change the Copy and Clone impls to work when compiling with cross_platform_docs.
@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Jan 23, 2019

I would like some clarity on Cargo.lock. Can I remove it from the repo and add it to .gitignore? It seems like unnecessary noise for a library.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 23, 2019

That is an artifact of when a git dependency on ctest was used. That's no longer the case so I think it's fine to remove

@Susurrus Susurrus force-pushed the Susurrus:rfc_2235 branch from a401787 to 1f28bd7 Jan 23, 2019

Remove Cargo.lock
This was an artifact from when a git dependency on ctest was used. This
is no longer the case, so removing it to simplify future PRs.

@Susurrus Susurrus force-pushed the Susurrus:rfc_2235 branch from 1f28bd7 to 130e5e1 Jan 23, 2019

@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Jan 23, 2019

Removed Cargo.lock (it was already in .gitignore coincidentally) and also added the style executable to .gitignore as that shouldn't ever be committed. Fixed FreeBSD builds and rebased.

@Susurrus Susurrus force-pushed the Susurrus:rfc_2235 branch from 130e5e1 to 0881a59 Jan 23, 2019

Susurrus added some commits Jan 22, 2019

Check for Debug impls for all types
This was not compile-tested on all platforms, but instead all `pub enum`
types had a `Debug` impl derived for them.
Ignore the style executable used for testing
This is used for running style checks on the libc codebase but shouldn't
be committed, so adding it to the .gitignore file.

@Susurrus Susurrus force-pushed the Susurrus:rfc_2235 branch from 9865242 to e33f760 Feb 3, 2019

@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Feb 3, 2019

None of those failures seem related to my code, seem like CI problems. I figured out how to fix the problem I was talking about, so I think this is all ready for a merge.

@@ -44,6 +44,16 @@ activate the *align* feature. This requires Rust 1.25 or newer:
libc = { version = "0.2", features = ["align"] }
```

All structs implemented by the libc crate have the `Copy` and `Clone` traits
implemented for them. The additional traits of `Debug, `Eq`, `Hash`, and

This comment has been minimized.

@auscompgeek

auscompgeek Feb 4, 2019

Looks like there's a missing backtick after Debug.

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Feb 4, 2019

Only blocker here is what to do about implementing Hash for a struct that has an f64 in it. Any suggestions?

I'd say, don't derive Hash for this, and open an issue to discuss what to do. No need to block this PR on that.

@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Feb 4, 2019

@gnzlbg I resolved the issue by using f64::to_bits() and then hashing that. I can bring this up in the existing tracking issue for this, rust-lang/rust#57715, if you think it's worth discussing further.

@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Feb 4, 2019

Note that all failures in CI here were sporadic and things should work if someone goes in and re-runs the failures.

@Susurrus

This comment has been minimized.

Copy link
Contributor Author

Susurrus commented Feb 5, 2019

So this is pretty much ready, just needs someone to poke at CI for me as we should be all good here.

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Feb 5, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 5, 2019

📌 Commit e33f760 has been approved by gnzlbg

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 5, 2019

⌛️ Testing commit e33f760 with merge 942f6ae...

bors added a commit that referenced this pull request Feb 5, 2019

Auto merge of #1217 - Susurrus:rfc_2235, r=gnzlbg
RFC 2235 - Implement PartialEq,Eq,Hash,Debug for all types

First pass at implementing [RFC2235](https://github.com/rust-lang/rfcs/blob/master/text/2235-libc-struct-traits.md). I just started with `PartialEq`/`Eq` and tested locally on my x64 Linux system. Definitely going to run into other types for other platforms that need this treatment, but thought I'd start small.

Open question is whether this should also rely on the `align` feature, which should improve which types can be auto-derived versus manually implemented (as it removed a lot of odd-sized padding arrays). My first pass here doesn't do that, but I might test it out to see if it does simplify quite a few structs. I think it might also be nice to have as it makes it easier for contributors to add new structs.

Part of rust-lang/rust#57715
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 5, 2019

☀️ Test successful - checks-cirrus, checks-travis, status-appveyor
Approved by: gnzlbg
Pushing 942f6ae to master...

@bors bors merged commit e33f760 into rust-lang:master Feb 5, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
homu Test successful
Details
@semarie

This comment has been minimized.

Copy link
Contributor

semarie commented Feb 5, 2019

just to point that targets without CI are a bit broken now (openbsd in mind). I will try to take a look

semarie added a commit to semarie/libc that referenced this pull request Feb 6, 2019

semarie added a commit to semarie/libc that referenced this pull request Feb 6, 2019

semarie added a commit to semarie/libc that referenced this pull request Feb 6, 2019

@Susurrus Susurrus deleted the Susurrus:rfc_2235 branch Feb 9, 2019

semarie added a commit to semarie/libc that referenced this pull request Feb 9, 2019

semarie added a commit to semarie/libc that referenced this pull request Feb 9, 2019

bors added a commit that referenced this pull request Feb 9, 2019

bors added a commit that referenced this pull request Feb 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.