Github Roadmap for 2021 by flip1995 · Pull Request #6462 · rust-lang/rust-clippy...

Member

rust-highfive commented on Dec 18, 2020

r? @llogiq

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

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!

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.

@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 commented on 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 commented on Dec 18, 2020

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 commented on 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 commented on 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

djc commented on Dec 19, 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 commented on Dec 19, 2020

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.

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.

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.

@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 commented on Dec 23, 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 commented 28 days ago

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

flip1995 commented 27 days ago

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.

Manishearth commented 19 days ago

Can/should we merge this?

phansch commented 18 days ago

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 commented 18 days ago

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

bors commented 18 days ago

Testing commit d141cdc with merge 13ca5c8...

bors commented 18 days ago

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...

pickfire commented 3 days ago

Rendered link 404