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

Missing Eq implementations for types implementing PartialEq #36301

Closed
3 tasks done
whitequark opened this issue Sep 6, 2016 · 4 comments
Closed
3 tasks done

Missing Eq implementations for types implementing PartialEq #36301

whitequark opened this issue Sep 6, 2016 · 4 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@whitequark
Copy link
Member

whitequark commented Sep 6, 2016

Specifically (I looked at everything in std ):

  • std::num::ParseIntError
  • std::str::ParseBoolError
  • std::num::FpCategory

There seems to be no good reason for omitting Eq there, just an oversight. All other similar types already implement Eq.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Sep 6, 2016

A quick grep suggests there are around 300 types that derive PartialEq but not Eq in all of src/, though I don't have a quick way to tell how many of those can implement Eq. But I bet at least a few dozen can and should. I wish there was an easy way to run clippy on all the code.

@whitequark
Copy link
Member Author

whitequark commented Sep 6, 2016

That is really not an accurate way of measuring it because it takes into account all of the rustc-internal types, and worse, tests, for which it probably doesn't matter much whether they implement Eq or not. Here's a nicer way:

git grep '#\[derive' | \
  egrep -v '^(test|doc|libcollectionstest|libcoretest|tools|librust|libunwind|libsyntax|libtest|libterm|librbml|libserialize|libfmt_macros)' | \
  grep PartialEq | \
  grep -v '\bEq'

This produces only 29 examples, some of which implement Eq without deriving it:

libcore/char.rs:733:#[derive(PartialEq, Debug)]
libcore/cmp.rs:187:#[derive(Clone, Copy, PartialEq, Debug, Hash)]
libcore/fmt/num.rs:103:#[derive(Clone, PartialEq)]
libcore/fmt/num.rs:107:#[derive(Clone, PartialEq)]
libcore/fmt/num.rs:111:#[derive(Clone, PartialEq)]
libcore/fmt/num.rs:115:#[derive(Clone, PartialEq)]
libcore/fmt/num.rs:119:#[derive(Clone, PartialEq)]
libcore/fmt/rt/v1.rs:34:#[derive(Copy, Clone, PartialEq)]
libcore/num/dec2flt/mod.rs:159:#[derive(Debug, Clone, PartialEq)]
libcore/num/dec2flt/mod.rs:165:#[derive(Debug, Clone, PartialEq)]
libcore/num/flt2dec/decoder.rs:26:#[derive(Copy, Clone, Debug, PartialEq)]
libcore/num/flt2dec/decoder.rs:43:#[derive(Copy, Clone, Debug, PartialEq)]
libcore/num/mod.rs:2414:#[derive(Copy, Clone, PartialEq, Debug)]
libcore/num/mod.rs:2757:#[derive(Debug, Clone, PartialEq)]
libcore/num/mod.rs:2761:#[derive(Debug, Clone, PartialEq)]
libcore/str/mod.rs:112:#[derive(Debug, Clone, PartialEq)]
liblog/lib.rs:233:#[derive(Copy, Clone, PartialEq, PartialOrd, Debug)]
librand/distributions/mod.rs:272:    #[derive(PartialEq, Debug)]
libstd/collections/hash/table.rs:138:#[derive(PartialEq, Copy, Clone)]
libstd/error.rs:461:    #[derive(Debug, PartialEq)]
libstd/error.rs:463:    #[derive(Debug, PartialEq)]
libstd/ffi/c_str.rs:158:#[derive(Clone, PartialEq, Debug)]
libstd/ffi/c_str.rs:164:#[derive(Clone, PartialEq, Debug)]
libstd/ffi/c_str.rs:170:#[derive(Clone, PartialEq, Debug)]
libstd/net/mod.rs:43:#[derive(Copy, Clone, PartialEq, Debug)]
libstd/net/parser.rs:375:#[derive(Debug, Clone, PartialEq)]
libstd/path.rs:461:#[derive(Copy, Clone, PartialEq, PartialOrd, Debug)]
libstd/sync/mpsc/select.rs:106:#[derive(PartialEq)]
libstd/sys/windows/pipe.rs:168:#[derive(PartialEq, Debug)]

sophiajt pushed a commit to sophiajt/rust that referenced this issue Sep 22, 2016
@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum
Copy link
Member

rg 'derive.*PartialEq' | rg -v Eq returns nothing, so at least those types with derived equivalence seem to all implement both. I'm not sure how many types (or what types) that implement these traits manually don't follow this though. I think it might be possible to add such a thing as a lint on the rustdoc-like level once it's reimplemented atop the RLS api.

@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 26, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 16, 2017

I checked the search from #36301 (comment) and it looks like all the public types that should implement PartialEq now do. Fixed!

@dtolnay dtolnay closed this as completed Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants