WIP: Add lint for missing Clone impls #58070
Conversation
This comment has been minimized.
This comment has been minimized.
rust-highfive
added
the
S-waiting-on-review
|
If we are going to add all the lints we should also add them into a lint-group |
Centril
added
T-lang
|
r? @oli-obk |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Susurrus
force-pushed
the
missing_trait_impls
branch
from
b622b58
to
66c226c
Compare
|
Tests succeeded locally, so pushed the latest version here. I've only refactored the code and added both a lint for missing
|
src/librustc_lint/builtin.rs
Outdated
| if ty.is_copy_modulo_regions(cx.tcx, param_env, item.span) { | ||
| return; | ||
| } | ||
| if param_env.can_type_implement_copy(cx.tcx, ty).is_ok() { |
There was a problem hiding this comment.
Did the new version of this lose the "and we're not going to warn you if the type cannot be Copy" part?
If we're doing these for essentially everything that can be derived, maybe there's a way to approximately check whether the derive would work? (Maybe even have a structured suggestion to make it easy to add the derive? One could just turn on the lint and click the buttons in your IDE to add the derives.)
There was a problem hiding this comment.
I don't think this warned you if the type couldn't be Copy, it just didn't trigger the lint. I'm wondering if this is really a problem in practice. I'd think that if you set this lint globally, you'd want it to trigger on all types and not exclude types that it can't be implemented for, so you'd have to whitelist them explicitly.
There was a problem hiding this comment.
I think this is super important. I would never use a lint suggesting Copy if I had to suppress it any time I put a Vec or a HashSet inside my struct.
Susurrus
force-pushed
the
missing_trait_impls
branch
from
66c226c
to
1e94b95
Compare
|
I started implementing I also ran into problems implementing |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
So looking at the lang_items docs I'm not seeing much clarity on what a |
|
I'm coming back to this because I actually went and ported this change to |
These lints are now implemented in Clippy and those should be used instead.
|
☔ The latest upstream changes (presumably #58316) made this pull request unmergeable. Please resolve the merge conflicts. |
Susurrus
force-pushed
the
missing_trait_impls
branch
from
1e94b95
to
f10126c
Compare
|
Okay, I whipped up rust-lang/rust-clippy#3752, which has just Debug and Copy, which should be enough for us to finalize the work necessary here in rust. I've changed this PR to mark those lints as deprecated and also added |
|
Ok, this is totally awesome. I talked with @Manishearth at the all hands about this and we have a way to be able to not affect the lang theam here: We add a new attribute to the compiler: This will be a bit more involved. Essentially we need to
|
|
(The lang team is involved here not because of lang items, but because new lints are being added... once @oli-obk feels the implementation here is up to snuff ping me and I'll FCP-merge...) |
This might be an issue of not having a page such as https://rust-lang.github.io/rust-clippy/master/index.html for the rustc lints; if we had that then rustc lints would become more discoverable? |
|
Even so, if we don't consider them important enough to be enabled by default, why do we have them in the compiler? People looking for more static analysis usually go to clippy I presume? |
|
Presumably we have them in the compiler because clippy wasn't a well known thing at that point in time? What other allow-by-default lints do we have? If we're going for deprecation on the basis of "allow by default should be in clippy" then a more holistic approach seems preferable? |
...other listed lints I'm less familiar with. |
|
@Centril the webpage isn't what makes clippy lints discoverable, clippy allow-by-default lints are a power user tool too, and we don't expect people to find them normally. The difference is that lint levels in clippy are typically one level higher than the level the lint would have in rustc; they're discovered because they trigger warnings. I do think unsafe_code and the doc lints (also the idiom ones) should stay in rust, they are independently well-known. The rest could be moved around, probably. |
|
Oh, also, the 1.0 RFC defined what goes in rustc and what doesn't: rust-lang/rfcs#2476 It doesn't impose hard restrictions, but the rough idea was that anything that fits as a clippy |
Let me rephrase... I think we can make rustc lints more discoverable and so more useful by having a webpage like that linked in a prominent place.
(cc #53224) Sure; the RFC talks about future lints, not removing existing ones (because they already exist, and there's some churn for existing code-bases using them). I think if we want to downlift lints from rustc to clippy, let's go through them one by one and see which candidates there are and then the lang + compiler teams can confirm the deprecation. |
|
I don't think that lint discoverability is that easy. The clippy lint list gets linked to from everything clippy outputs and people still don't know about it. And yeah, the RFC talks about future lints, it also doesn't specify hard rules: I only brought it up because it specifies a standard that the community agreed upon, one that will be a useful default for making this judgement for existing lints if we are going down that path. It still should be case by case, I didn't say otherwise. |
|
ping from triage @Susurrus @oli-obk @Centril @Manishearth what's the update on this? |
|
So to remove the existing lints, I'd like to see this first converted into |
|
I think there is a positive reception to adding missing trait lints for all the traits suggested by the API guidelines in Clippy based on feedback I've gotten to my PR there (rust-lang/rust-clippy#3752). That's where I've been spending my effort. However, that PR relies on the changes I propose here in |
|
I do think the existing missing_implementations lints are correctness lints in the sense that if they are used, the author considers it a bug if |
According to clippy's lint grouping rules, that's a restriction lint, not a correctness lint. |
|
@oli-obk Yes fair enough. |
|
@Susurrus I think as a first step we should do the refactorings suggested above for just the lints that are in rustc, and then implement the other lints in clippy. This way we can punt the decision on deprecating the rustc lints into the future without being blocked or implementing all the lints in rustc |
Centril
removed
T-lang
|
Removing the labels with that understanding :) |
Centril
added
S-waiting-on-author
|
Ping from triage @Susurrus :) |
|
ping from triage @Susurrus Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again! Thanks |
Dylan-DPC-zz
added
S-inactive
This is a WIP implementation for #58066. I've just implemented a lint for missing
Cloneimplementations. For the other traits listed in #58066, I'll add separate commits for the missing ones once I've gotten some feedback here and am sure I'm on the right track.Closes #58066.