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

Add lints for all common traits #58066

Open
Susurrus opened this issue Feb 2, 2019 · 8 comments
Open

Add lints for all common traits #58066

Susurrus opened this issue Feb 2, 2019 · 8 comments
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Susurrus
Copy link
Contributor

Susurrus commented Feb 2, 2019

The interoperability API guidelines suggest that all of the following traits should be eagerly implemented for types where it's appropriate:

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

Both Debug and Copy have corresponding missing_${TRAIT}_implementations lints in rustc for checking that they exist. These lints have recently become of interest for libc because of RFC2235. However, since some of these traits rely on manual implementations, it'd be useful to have lints to fail CI should any desired lints be missed in a PR.

Given that two of these lints already exist and the API guidelines lay out a clear standard practice, I'm not certain if an RFC is required to add the rest of these lints, so I'm raising this issue. My plan is to work through adding the lints for Clone, Eq, Hash, and PartialEq, as they're required for rust-lang/libc#1217.

@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 2, 2019

cc @gnzlbg

@jonas-schievink jonas-schievink added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Feb 2, 2019
@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 2, 2019

Note that Clippy has discussed adding some of these same lints in rust-lang/rust-clippy#1798

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Feb 2, 2019
@scottmcm
Copy link
Member

scottmcm commented Feb 3, 2019

This makes we want to be able to say impl !Eq for f32; as a way to say "no, I really don't want that and never will" without having to allow the lint on those types.

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 4, 2019

I think these lints belong in clippy more like in rustc, as in they catch a bad idiom, instead of something that's "inheretely dangerous" or that can lead to incorrect programs (e.g. missing a trait impl, as long as you don't use it, isn't really that big of a deal).

@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 4, 2019

@gnzlbg I've been wondering if these should go in Clippy versus here in Rust, but since the lints for Debug and Copy already exist in rustc, that'd it'd make sense to put them all here. I can't think of a reason that these types should be split between rustc and clippy, however, so what do we do with the existing Debug and Copy lints? Should I reimplement all of this in clippy and propose removing them from rustc? Is removing an allow-by-default lint backwards a reasonable thing to do?

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 4, 2019

I don't know why these lints are in rustc and not clippy - @oli-obk might know ? Maybe the reason is only historical (clippy did not exist back then when these were added).

@oli-obk oli-obk added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Feb 5, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Feb 5, 2019

The reason definitely is historical. If we move the existing lints to clippy, then we should figure out the transition story. I think it would be fine to just deprecate the lints and move them to clippy entirely.

@Susurrus
Copy link
Contributor Author

Susurrus commented Feb 8, 2019

Hmm, this is a pain, we now have 3 separate conversations going on about this in three separate locations. The big issue I have right now is this (quoted from #8070):

So looking at the lang_items docs I'm not seeing much clarity on what a lang_item actually is. As in, whether it'd be inappropriate here to define more. Only 3 traits need this treatment: Default, Display, and Hash. What're the rules around defining new lang items here? Does it break anything anywhere else? I'd think since they're in the stdlib, that this wouldn't be a big deal, but I probably don't have enough context.

If I could get rough consensus that adding those traits is appropriate, I can alter #58070 to just add those lang_items and mark these lints as deprecated. Then I can finish my PR for Clippy, which I haven't submit yet. I'm trying to reduce the amount of code I write that then gets thrown away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants