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

Roadmap for 2021 #6462

Merged
merged 2 commits into from
Jan 12, 2021
Merged

Roadmap for 2021 #6462

merged 2 commits into from
Jan 12, 2021

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented Dec 17, 2020

Rendered

This is the first time Clippy gets its own roadmap. The reason for this roadmap is, that with the Rust language growing, also Clippy is growing. With this keeping track of and implementing bigger projects gets quite hard. This roadmap should help in exactly this regard.

After having the approval of this roadmap by the Clippy team, we want to know from the community:

  • What do you think in general about this roadmap?
  • Are there any pain points in Clippy, that should be included here?
  • What of the points listed here has the highest priority for you?

We're looking forward to getting your feedback!

changelog: Add roadmap for Clippy 2021

r? @rust-lang/clippy

@rust-highfive
Copy link

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 17, 2020
@flip1995 flip1995 changed the title Add Roadmap for 2021 Roadmap for 2021 Dec 17, 2020
@flip1995 flip1995 added S-needs-discussion Status: Needs further discussion before merging or work can be started and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 17, 2020
@BartMassey
Copy link

Thanks so much for Clippy! It's super-invaluable. The roadmap looks great.

To be honest, the increasing Clippy false-positive rate is a bit annoying. I would like to see a policy that has all new lints introduced into Clippy as allow by default, and only moved to warn when sufficient evidence has been provided to suggest that it would be a good idea.

A few examples from my repos:

  • Many single-character variables: This is really common in numeric functions such as gcd() that are mathematically defined. The math tends to have single-character names, and it is best practice to copy these names into the code.

  • &Vec parameter: This is great in many situations, but currently things like

    type Sequence = Vec<usize>;
    fn sum(s: &Sequence) -> usize { s.iter().cloned().sum() }
    

    produce a warning, which is less than ideal. Haven't reported as a bug, because it's a matter of style, but it seems aggressive to complain about it by default.

  • Use sort_unstable() where possible: Yes, this will be faster, but honestly in most applications it doesn't matter. Performance lints seem problematic as defaults in general to me: should maybe have their own flag or group or something?

For a wonderful, comprehensive tool like this I doubt there will ever be defaults that will make everyone happy. I personally would prefer a conservative approach though: it is customary for me to tell my students that I won't look at their code until they've fixed the Clippy warnings, so I want that to always seem like a worthwhile activity.

Looking forward to Clippy in 2021!

@kykosic
Copy link

kykosic commented Dec 18, 2020

In general, I think this is a great roadmap for the coming year.

For me personally, by far the most important feature is the "no output after cargo check". Seeing as rust-analyzer is the official language server, this leads to much frustration when trying to use clippy when editors run check on save. I have to either cargo clean and re-compile before every clippy or setup some other workaround with alternate build directories that is equally tedious.

@Manishearth
Copy link
Member

@BartMassey

A few examples from my repos:

I actually think the first two examples here are not false positives, they're the lint working as intended. The last one ... iffier.

I'll point out that Clippy is meant to be used with a decent amount of lint toggling, since preferences vary wildly. I'd encourage you to file issues on things you feel are false positives; but it does seem like your preferences are more conservative than most, and given that it might be useful to turn off some lints or groups. One thing we might end up doing is improving the lint categorization to make this easier.

@gilescope
Copy link

gilescope commented Dec 18, 2020

I assumed that new lints needed to be stabilised before they were run in stable.

As for false positives / controversial lints maybe we should think of them as optimisation levels and o1 would have the rock solid no false positives ones and that o3 might contain more aspirational lints?

To do that scientifically we need stats on which are specifically denied - It would be great if we could gather that data like Estaban’s suggestion of capturing stats on compiler errors.

@flip1995
Copy link
Member Author

I would like to see a policy that has all new lints introduced into Clippy as allow by default,

@BartMassey That probably won't happen. What we have in mind is to release them only in nightly and to make sure that they have to pass $some_test before they're released to stable. No specific plan yet though, but we'll work on improving this.

I assumed that new lints needed to be stabilised before they were run in stable.

@gilescope Until recently (sometime this year) there was no need for that and our process (just putting them out there) worked great. Something changed and now we'll need such a process. We'll work on improving this situation.

For me personally, by far the most important feature is the "no output after cargo check".

@kykosic This might actually be one of the first things we'll get fixed. @ebroto did great work towards this recently.

To do that scientifically we need stats on which are specifically denied - It would be great if we could gather that data like Estaban’s suggestion of capturing stats on compiler errors.

Metrics like this are definitely a want have.


Thanks for your opinions on this. This will definitely help prioritizing things!

@ebroto
Copy link
Member

ebroto commented Dec 18, 2020

Some feedback from reddit that has not been mentioned yet:

Now that clippy has many lints it should be possible to identify common patterns and refactor them as helper functions. Something like lint templates would also be useful

(link)

@flip1995
Copy link
Member Author

flip1995 commented Dec 18, 2020

A refactor of Clippy is definitely something interesting to look at. As a first step merging lint passes and building more modules like the methods module might make sense.

On reddit was another complaint about a FP slipping into stable, with the result that they stopped using Clippy. We should make implementing a test system a priority.

Idea to improve the FP rate situation

A bit of Off-topic, since the roadmap is about the "What", not the "How"

One idea on reddit was, to compile a list of crates that pass Clippy. We can ask people to submit their crate if they require it to pass Clippy on their CI, and then we can use it in some mini-crater to check new lints. So we would make a deal with crate authors: If you adhere to (stable) Clippy, we make sure, that Clippy doesn't break something on your end with a FP.

@djc
Copy link
Contributor

djc commented Dec 18, 2020

FWIW, I basically never use clippy in CI because I use rust-analyzer and so local clippy only gives me results if I cargo clean first -- having that fixed would be a large improvement (although having it in CI is helpful too, anyway).

As for the FP rate, maybe an idea would be to put newly stable lints in pedantic first for the one or two release cycles? People who enable pedantic are supposedly a little more invested in clippy and less likely to be bothered by false positives.

@flip1995
Copy link
Member Author

I basically never use clippy in CI because I use rust-analyzer and so local clippy only gives me results if I cargo clean first

Isn't that a reason to use it in CI, so you don't accidentally miss the Clippy errors? 🤔

As for the FP rate, maybe an idea would be to put newly stable lints in pedantic first for the one or two release cycles?

I don't want to do this, because this would be abusing the pedantic group and will involve quite a bit of work to determine when they should get out. I think the better idea would be to release lints nightly-only so people who want FP "free" lints can use stable Clippy, and people who want to help test Clippy can use nightly. Or even better: just additionally run nightly Clippy in CI and report if lints report FPs.

@matthiaskrgr
Copy link
Member

Rust analyzer allows you to set a custom command instead of cargo check to execute on save: rust-analyzer.checkOnSave.command
You can set that to run clippy instead I believe.

@jhpratt
Copy link
Member

jhpratt commented Dec 18, 2020

Yeah, it's possible to use clippy with rust-analyzer in the way @matthiaskrgr described.

@djc It's possible to just do touch src/lib.rs, which lets you run clippy without having to remove all sorts of things.

@BartMassey
Copy link

@jhpratt, @djc I have gotten into the habit of inserting and then deleting a space in my editor before running cargo check, which effectively touches a file and allows it to trigger.

(Sadly, I will probably be doing this for the rest of my life, since by the time this issue is fixed my finger macro will be cemented forever. I still type sync; sync; reboot at my Linux box because in 1985 there was a bug in Berkeley UNIX such that you needed a couple of syncs to make sure the filesystem was fully written to disk. 😳)

@Manishearth
Copy link
Member

Manishearth commented Dec 22, 2020

As for the FP rate, maybe an idea would be to put newly stable lints in pedantic first for the one or two release cycles? People who enable pedantic are supposedly a little more invested in clippy and less likely to be bothered by false positives.

We already have the nursery group for this. We have the nightly cycle for unforseen issues crop up, and we downgrade lints to nursery if there are major ones.

@llogiq
Copy link
Contributor

llogiq commented Jan 3, 2021

From my perspective, this looks great! Anyone have any concerns or should I r+ this?

@flip1995
Copy link
Member Author

flip1995 commented Jan 4, 2021

I want to add a prioritization chapter to it. I quickly go through my remaining 100+ GitHub notifications (by marking all as "done" where I'm not mentioned and not participating 😄) and then get right to it.

@flip1995 flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-needs-discussion Status: Needs further discussion before merging or work can be started labels Jan 4, 2021
@Manishearth
Copy link
Member

Can/should we merge this?

@phansch
Copy link
Member

phansch commented Jan 12, 2021

Discussed in the Clippy meeting today and we're ready to get started 🎉

@bors r=flip1995,llogiq,killercup,Manishearth,oli-obk,matthiaskrgr,phansch,mikerite,mcarton,yaahc,ebroto

@bors
Copy link
Contributor

bors commented Jan 12, 2021

📌 Commit d141cdc has been approved by flip1995,llogiq,killercup,Manishearth,oli-obk,matthiaskrgr,phansch,mikerite,mcarton,yaahc,ebroto

@bors
Copy link
Contributor

bors commented Jan 12, 2021

⌛ Testing commit d141cdc with merge 13ca5c8...

@bors
Copy link
Contributor

bors commented Jan 12, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995,llogiq,killercup,Manishearth,oli-obk,matthiaskrgr,phansch,mikerite,mcarton,yaahc,ebroto
Pushing 13ca5c8 to master...

@bors bors merged commit 13ca5c8 into rust-lang:master Jan 12, 2021
@flip1995 flip1995 deleted the roadmap branch January 12, 2021 16:47
@pickfire
Copy link
Contributor

Rendered link 404

@flip1995
Copy link
Member Author

Fixed

Also here: https://github.com/rust-lang/rust-clippy/blob/d141cdc947fb49be603fb91ec626984a961c400d/doc/roadmap-2021.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.