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

WIP: Add missing trait implementation lints #3752

Closed
wants to merge 2 commits into from

Conversation

Susurrus
Copy link

@Susurrus Susurrus commented Feb 9, 2019

Posting this now so I can ensure I'm on the right track here.

This adds lints for if types are missing implementations of:

  • Clone
  • Copy
  • Debug
  • Default
  • Display
  • Eq
  • Hash
  • Ord
  • PartialEq
  • PartialOrd

Context here is in implementing RFC2235 we cannot auto-derive all traits. However, we still want the traits to be manually implemented. So we'd like lints to check on them.

I started this work in rust-lang/rust#58070, but it was suggested that these lints shouldn't be in the compiler and instead in Clippy. It was suggested that the existing lints for Copy and Debug be deprecated as part of this as well.

Additionally these lints are part of #1798.

To fully implement all lints here, rust-lang/rust#58070 will need to be merged (which is still a work in progress) in order to get Default, Display, and Hash added as lang_items so they can be easily checked on types.

@ghost
Copy link

ghost commented Feb 9, 2019

correctness is the wrong category for these lints. The lints in the correctness category is for lints that detect bugs. This probably belongs in the pedantic category.

Also it looks like there is no code for checking if it's even possible to implement the traits so this might have a lot of false positives.

@Susurrus
Copy link
Author

Susurrus commented Feb 9, 2019

correctness is the wrong category for these lints. The lints in the correctness category is for lints that detect bugs. This probably belongs in the pedantic category.

Where do I have them set as part of the correctness category? I believe I added them to the clippy::all group and that's it. However, I don't understand the lint code very well, so it's possible I missed something here. I wasn't certain if these lints belonged in any of those categories, which is why I tried to just add them to clippy::all.

Also it looks like there is no code for checking if it's even possible to implement the traits so this might have a lot of false positives.

This came up during the discussion in my Rust PR. I believe the lint for Copy checked whether it could be implemented. I don't, however, believe this applies to all of these. For example Clone, Debug, Default, Display, Hash, and PartialEq I would expect to be implementable for all types. Copy, Eq, Ord, and PartialOrd I'd expect to have false positives.

$trait_check:ident) => (
declare_clippy_lint! {
pub $lint_constant,
correctness,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where you give a lint its category

@oli-obk
Copy link
Contributor

oli-obk commented Feb 12, 2019

For example Clone, Debug, Default, Display, Hash, and PartialEq I would expect to be implementable for all types

You don't want Clone and Default for types where you should only be getting values from other values. One example is MutexGuard

@phansch phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Feb 24, 2019
@flip1995 flip1995 added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label Feb 24, 2019
@flip1995
Copy link
Member

Ping from triage @Susurrus. Since rust-lang/rust#58070 is S-inactive-closed, and there's no update on this in a while either, I'll also set this to S-inactive-closed. Thanks for your contribution and feel free to reopen this any time!

@flip1995 flip1995 closed this Jun 14, 2019
@flip1995 flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants