Add Clippy version to Clippy's lint list #7813

xFrednet · 2021-10-12T21:39:21Z

Hey, hey, the semester is finally over, and I wanted to get back into hacking on Clippy. It has also been some time since our metadata collection monster has been feed. So, this PR adds a new attribute clippy::version to document which version a lint was stabilized. I considered using git blame but that would be very hacky and probably not accurate.

I'm also thinking that this attribute can be used to have a clippy::nightly lint group which is allow-by-default that delays setting the actual lint group until the defined version is reached. Just something to consider regarding #6623 🙃

This PR only adds the version to 4 lints to keep it reviewable. I'll do a followup PR to add the version to other lints if the implementation is accepted 🙃

Also, mobile approved xD

r? @flip1995

cc: #7172

closes: #6492

changelog: Clippy's lint list now displays the version a lint was added. 🎉

Example lint declaration after this update:

declare_clippy_lint! {
    /// [...]
    ///
    /// ### Example
    /// ```rust
    /// // Bad
    /// let x = 3.14;
    /// // Good
    /// let x = std::f32::consts::PI;
    /// ```
    #[clippy::version = "pre 1.29.0"]
    pub APPROX_CONSTANT,
    correctness,
    "the approximate of a known float constant (in `std::fXX::consts`)"
}

xFrednet · 2021-10-12T22:38:30Z

I'm currently stuck on the attribute regex for cargo dev update_lints 😅

The current status is (?:\s+\#\[clippy::version = "[^"]*"\])? which sadly doesn't work -.-

(Solved, regex ignores spaces, the needed to be replaced with \s+)

flip1995 · 2021-10-13T09:33:08Z

I don't think the Clippy version a lint was introduced in, is really helpful for Clippy users, since 0.0.105 doesn't mean anything to them. I would just specify the Rust version the lint was stabilized in. And for lints that existed before rustup deployed Clippy, I would just specify "pre" or some other string. That would be some more initial work to mark all the lints though.

Did you think about adding an (optional) field to the macro instead of an attribute? What benefit does the attribute have?

xFrednet · 2021-10-13T10:38:57Z

I don't think the Clippy version a lint was introduced in, is really helpful for Clippy users, since 0.0.105 doesn't mean anything to them. I would just specify the Rust version the lint was stabilized in. And for lints that existed before rustup deployed Clippy, I would just specify "pre" or some other string. That would be some more initial work to mark all the lints though.

It'll be some work either way. Marking a bunch of lints with "pre" would actually be a big help. 🙃 I would suggest using Rust 1.29 as a cut-off point as that was also the first Rust version listed on the lint list version index. Setting the version to "pre 1.29.0" will work. (Updated screenshots)

Did you think about adding an (optional) field to the macro instead of an attribute? What benefit does the attribute have?

The attribute enables the metadata collection lint to easily get the version, other possibilities would be to put it in a doc string or something. Having an attribute like this was straightforward to parse. I thought about adding an extra field to the macro and only adding the attribute inside the expansion. That would make lint description look like this:

declare_clippy_lint! {
    /// ### What it does
    /// Collects metadata about clippy lints for the website.
    ///
    pub INTERNAL_METADATA_COLLECTOR,
    "pre 1.57.0",
    internal_warn,
    "A busy bee collection metadata about lints"
}

I like the look with the attribute a bit more :)

flip1995 · 2021-10-13T12:03:40Z

Yes, I would also make the cut off where we first started creating Clippy releases with every Rust release.

Getting added lints per release could look somthing like this:

git checkout the release
Use the export.py/Metadata collector to generate lints.json file
Use a script to extract the lint list / just new lints from that file

Shouldn't be hard to write such a script.

Seeing this in the code, I don't think attribute vs in-macro makes much of a difference. Also as you wrote we may be able to reuse this to mark lints with clippy::version = "nightly" to only have those lint in nightly Clippy.

xFrednet · 2021-10-13T12:35:01Z

That's an interesting idea, the JSON files can also be downloaded from the gh-pages branch. Most of the work will probably be to add the version in Clippy.

Seeing this in the code, I don't think attribute vs in-macro makes much of a difference.

That's what I was guessing, just wanted to throw that idea out there ^^

Also as you wrote we may be able to reuse this to mark lints with clippy::version = "nightly" to only have those lint in nightly Clippy.

I think that our current setup would make it hard to have the nightly build have exclusive lints, as we reuse lint passes for several lints. My idea was to handle these with a new nightly lint group that is allow-by-default. This wouldn't prevent ICEs but FPs if clippy::nightly is not enabled. The idea was to let the code handle assigning the lint level and group based on the defined stabilizing version, like this:

The macro adds some code that compares the stabilizing version with the current compilation version. (retrieved from environment values)
- Option 1: clippy::version > current_version -> The lint is allow-by-default and only added to clippy::nightly
- Option 2: clippy::version <= current_version -> The lint group and level will be assigned as defined in the macro

That's just an idea, though 🙃

flip1995 · 2021-10-14T14:40:57Z

Most of the work will probably be to add the version in Clippy.

That can be done with just another script :)

My idea was to handle these with a new nightly lint group that is allow-by-default.

I want to avoid to introduce a new lint group for nightly. I want that using Clippy on nightly vs on stable is exactly the same and you don't have to add anything to your code or CI configuration to use the new nightly lints. Just use Clippy from the nightly toolchain instead of stable and you get all the new nightly lints how they may end up in stable one day.

I could imaging to include this in our new span_lint utils. So that we have a check, if the lint is in nightly and only emit it if nightly Clippy is run.

xFrednet · 2021-10-14T19:31:47Z

Most of the work will probably be to add the version in Clippy.

That can be done with just another script :)

Guess I'll do some scripting ^^. Should I push the commit on this branch and make the attribute mandatory?

I want that using Clippy on nightly vs on stable is exactly the same and you don't have to add anything to your code

True, that would be nicer! An idea to keep in mind. Are you okay moving this forward as it is, to display the version on the website? 🙃

flip1995 · 2021-10-14T21:41:26Z

Should I push the commit on this branch and make the attribute mandatory?

Yes, but please keep it isolated in an extra commit.

Are you okay moving this forward as it is, to display the version on the website?

Yes, this seems good to me

xFrednet · 2021-10-17T12:43:46Z

Hey, I'm currently adjusting the new_lint command to automatically include the attribute. There is no environment value for the rustc version, but I found the rustc_version crate maintained by @djc. Is it okay if I add that crate to the dependencies of clippy_dev? 🙃

flip1995 · 2021-10-19T14:06:32Z

Why not use cargo_metadata on our Cargo.toml and just remove the leading 0.. This should give you the version for the lints more accurately.

Also if we should reuse this for marking lints as nightly-only, we can just hardcode the string "nightly" in cargo dev new_lint

xFrednet · 2021-10-19T14:28:11Z

Why not use cargo_metadata on our Cargo.toml and just remove the leading 0.. This should give you the version for the lints more accurately.

No real reason just haven't thought of that ^^

Also if we should reuse this for marking lints as nightly-only, we can just hardcode the string "nightly" in cargo dev new_lint

True, that would also be my plan later down the line, depending on how we're going to implement it. Until that is implemented I thought it would be good to automatically fill that field. But we can set it to "nightly" if you prefer? 🤔

flip1995 · 2021-10-19T14:41:24Z

Until that is implemented I thought it would be good to automatically fill that field. But we can set it to "nightly" if you prefer? thinking

Depends on how hard it is to fill it. But with cargp_metadata it should be doable in only a few lines of code.

xFrednet · 2021-10-19T21:46:56Z

I was able to use cargo_metadata without any problems 👍. I've only modified the commit 40877dc the others are still the same 🙃

xFrednet · 2021-10-20T09:52:39Z

This PR needs to be rebased to add another #[clippy::version] attribute now that #7775 has been merged. I'll do that later today. It's still ready for review :)

flip1995

And I decided to write a script using nushell because why not? This was a mistake... I spend way more time on this than I would like to admit. It has definitely been more than 4 hours

4 hours is not too bad for this.

I would have written a few simple python scripts for this.

Since my suggestions may require some new scripts, let me know if you want me to write them or want some help writing them. (I can only help with python scripts, because I don't know anything about nushell)

clippy_lints/src/deprecated_lints.rs

clippy_lints/src/absurd_extreme_comparisons.rs

xFrednet · 2021-10-21T10:11:07Z

And I decided to write a script using nushell because why not? This was a mistake... I spend way more time on this than I would like to admit. It has definitely been more than 4 hours

4 hours is not too bad for this.

I always underestimate the time a small change will take. A coworker once told me that it's usually a good idea to multiply the early estimated time for a task by 2. For me, this multiplier is about 5 times, as I always oversimplify stuff xD.

I would have written a few simple python scripts for this.

That would have probably been the smart move. Python is a nice scripting language, it has been some time since I used it, though. 😅

Since my suggestions may require some new scripts, let me know if you want me to write them or want some help writing them. (I can only help with python scripts, because I don't know anything about nushell)

The adjustments should be pretty simple with some wizardry ✨. I can simply rename the v0.0.212 folder to rust-1.28.0 run the script again and then use sed to replace each #[clippy::version = "1.28.0"] with #[clippy::version = "pre 1.29.0"]. The other changes required some manual adjustment anyway, they are luckily limited to a few lints 🙃. Thank you for the offer, though, I'll ping you if I get stuck.

I don't know anything about nushell

BTW. really understandable. Nushell is basically a console prototype that organizes data in tables and allows the definition of queries. The idea is fascinating IMO. However, it's still in its early stages and I mainly used it as a gimmick. It's still better to use a normal console or any other scripting language for most use cases. Still a fun learning experience. :)

bors · 2021-10-21T10:35:40Z

☔ The latest upstream changes (presumably #7853) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet · 2021-10-21T21:15:55Z

I accidentally created another monster with 700+ changes. Sorry, it likes humans, don't worry xD

flip1995 · 2021-11-10T16:36:24Z

I like the "Added in" version most. Thanks for providing the comparison!

flip1995

I also noticed, that no lints were actually added in 1.29.0. I guess this is because 0.1.29 is the same version as 0.0.212. I don't think an action is required here.

Sorry to kill the fun in the comments 😛

clippy_dev/src/update_lints.rs

clippy_lints/src/utils/internal_lints.rs

clippy_lints/src/utils/internal_lints/metadata_collector.rs

clippy_dev/src/new_lint.rs

clippy_dev/Cargo.toml

So, some context for this, well, more a story. I'm not used to scripting, I've never really scripted anything, even if it's a valuable skill. I just never really needed it. Now, `@flip1995` correctly suggested using a script for this in `rust-clippy#7813`... And I decided to write a script using nushell because why not? This was a mistake... I spend way more time on this than I would like to admit. It has definitely been more than 4 hours. It shouldn't take that long, but me being new to scripting and nushell just wasn't a good mixture... Anyway, here is the script that creates another script which adds the versions. Fun... Just execute this on the `gh-pages` branch and the resulting `replacer.sh` in `clippy_lints` and it should all work. ```nu mv v0.0.212 rust-1.00.0; mv beta rust-1.57.0; mv master rust-1.58.0; let paths = (open ./rust-1.58.0/lints.json | select id id_span | flatten | select id path); let versions = ( ls | where name =~ "rust-" | select name | format {name}/lints.json | each { open $it | select id | insert version $it | str substring "5,11" version} | group-by id | rotate counter-clockwise id version | update version {get version | first 1} | flatten | select id version); $paths | each { |row| let version = ($versions | where id == ($row.id) | format {version}) let idu = ($row.id | str upcase) $"sed -i '0,/($idu),/{s/pub ($idu),/#[clippy::version = "($version)"]\n pub ($idu),/}' ($row.path)" } | str collect ";" | str find-replace --all '1.00.0' 'pre 1.29.0' | save "replacer.sh"; ``` And this still has some problems, but at this point I just want to be done -.-

xFrednet · 2021-11-10T19:14:31Z

I also noticed, that no lints were actually added in 1.29.0. I guess this is because 0.1.29 is the same version as 0.0.212. I don't think an action is required here.

Ups, good to know ^^. The difference shouldn't really matter :)

Sorry to kill the fun in the comments

No problem ^^. I'm sometimes a bit unsure what is okay and what not, when it comes to these kinds of things. 🙃 It should be ready now :)

flip1995

What happens when the version is not defined for a lint? Do we have an automated check, that makes sure that a version is defined for every lint?

clippy_lints/src/deprecated_lints.rs

xFrednet · 2021-11-11T11:09:49Z

What happens when the version is not defined for a lint? Do we have an automated check, that makes sure that a version is defined for every lint?

No, we don't. I misunderstood you before and thought it shouldn't be mandatory. I'll add a new lint internal lint to make the attribute mandatory. That ensures nice error messages and enables us to allow it for internal lints 🙃

flip1995 · 2021-11-11T12:20:55Z

I misunderstood you before and thought it shouldn't be mandatory. I'll add a new lint internal lint to make the attribute mandatory.

Oh I think I said that before 😄 But now, I think it's better to just require it for every lint to avoid getting to a point where we mark some of our new lints when we remember to do so, but get more inconsistent over time.

flip1995

LGTM. Let me quickly try to build the website locally, and this should be ready to merge.

flip1995 · 2021-11-11T12:37:58Z

@bors r+

Thanks! And sorry for taking so long for reviewing such a bit-rotty PR.

(The next possible improvement for our website would be to add some more filters. E.g. filtering for version added, applicability, ... This will also involve making the existent filters more usable)

bors · 2021-11-11T12:38:00Z

📌 Commit 8c45fd8 has been approved by flip1995

bors · 2021-11-11T12:38:07Z

⌛ Testing commit 8c45fd8 with merge 3bfe98d...

bors · 2021-11-11T12:50:56Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 3bfe98d to master...

xFrednet · 2021-11-11T13:09:14Z

No worries 🙃, thanks for the review!

(The next possible improvement for our website would be to add some more filters. E.g. filtering for version added, applicability, ... This will also involve making the existent filters more usable)

My next goal for the website was collecting renamed lints and create some kind of indicator, when somebody searches for the old name. Expanding on the filters is also a good idea, a copy-lint-name function would also be nice. Currently, I'm trying to wrap up my larger project before starting on my bachelor thesis in December. I'll create issues for the proposed improvements, these can also be labeled as good-first-issues 🙃

xFrednet · 2021-11-11T13:40:03Z

I've created #7958 and #7959. Could you maybe expand your suggestion a bit, what you meant with "making the existent filters more usable" in #7958.

rust-highfive assigned flip1995 Oct 12, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 12, 2021

xFrednet force-pushed the 6492-lint-version branch from 9cf6f3b to 05c2fa2 Compare October 17, 2021 15:33

xFrednet force-pushed the 6492-lint-version branch from a03df14 to 90810cd Compare October 19, 2021 21:43

xFrednet force-pushed the 6492-lint-version branch 2 times, most recently from 3c032ae to cb321f4 Compare October 19, 2021 22:03

xFrednet mentioned this pull request Oct 20, 2021

Fix doc heading of transmute_num_to_bytes #7848

Merged

xFrednet force-pushed the 6492-lint-version branch 3 times, most recently from 6ab38e9 to a6dda40 Compare October 20, 2021 21:54

flip1995 reviewed Oct 21, 2021

View reviewed changes

clippy_lints/src/deprecated_lints.rs Outdated Show resolved Hide resolved

clippy_lints/src/deprecated_lints.rs Outdated Show resolved Hide resolved

clippy_lints/src/absurd_extreme_comparisons.rs Outdated Show resolved Hide resolved

xFrednet force-pushed the 6492-lint-version branch 2 times, most recently from 5d0278d to f5702bc Compare October 21, 2021 19:14

flip1995 removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Nov 5, 2021

flip1995 reviewed Nov 10, 2021

View reviewed changes

xFrednet added 7 commits November 10, 2021 19:48

Added clippy::version attribute as lint metadata

7c1e62d

Add clippy version to Clippy's lint list

8565fc4

Document new clippy::version attribute and make it mandatory

23ed792

Manually add clippy::version attribute to deprecated lints

63cb410

Add lint to check for invalid clippy:version attributes

9d9d06d

Updated version label and style on Clippy's lint list

d68408f

xFrednet force-pushed the 6492-lint-version branch from a0a8acc to 6f4221e Compare November 10, 2021 19:08

Address review feedback

94bc0a1

xFrednet force-pushed the 6492-lint-version branch from 6f4221e to 94bc0a1 Compare November 10, 2021 19:14

flip1995 reviewed Nov 11, 2021

View reviewed changes

clippy_lints/src/deprecated_lints.rs Show resolved Hide resolved

New internal lint to make clippy::version attribute mandatory

8c45fd8

xFrednet force-pushed the 6492-lint-version branch from 9435077 to 8c45fd8 Compare November 11, 2021 12:03

flip1995 approved these changes Nov 11, 2021

View reviewed changes

bors merged commit 3bfe98d into rust-lang:master Nov 11, 2021

xFrednet deleted the 6492-lint-version branch November 11, 2021 13:09

This was referenced Nov 11, 2021

new lint: borrow_deref_ref #7930

Merged

Add lint ref_mut_iter_method_chain #7688

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Clippy version to Clippy's lint list #7813

Add Clippy version to Clippy's lint list #7813

xFrednet commented Oct 12, 2021 •

edited

xFrednet commented Oct 12, 2021 •

edited

flip1995 commented Oct 13, 2021

xFrednet commented Oct 13, 2021 •

edited

flip1995 commented Oct 13, 2021

xFrednet commented Oct 13, 2021

flip1995 commented Oct 14, 2021

xFrednet commented Oct 14, 2021

flip1995 commented Oct 14, 2021

xFrednet commented Oct 17, 2021

flip1995 commented Oct 19, 2021

xFrednet commented Oct 19, 2021

flip1995 commented Oct 19, 2021

xFrednet commented Oct 19, 2021 •

edited

xFrednet commented Oct 20, 2021

flip1995 left a comment

xFrednet commented Oct 21, 2021 •

edited

bors commented Oct 21, 2021

xFrednet commented Oct 21, 2021 •

edited

flip1995 commented Nov 10, 2021

flip1995 left a comment •

edited

xFrednet commented Nov 10, 2021

flip1995 left a comment

xFrednet commented Nov 11, 2021 •

edited

flip1995 commented Nov 11, 2021

flip1995 left a comment

flip1995 commented Nov 11, 2021

bors commented Nov 11, 2021

bors commented Nov 11, 2021

bors commented Nov 11, 2021

xFrednet commented Nov 11, 2021

xFrednet commented Nov 11, 2021

Add Clippy version to Clippy's lint list #7813

Add Clippy version to Clippy's lint list #7813

Conversation

xFrednet commented Oct 12, 2021 • edited

xFrednet commented Oct 12, 2021 • edited

flip1995 commented Oct 13, 2021

xFrednet commented Oct 13, 2021 • edited

flip1995 commented Oct 13, 2021

xFrednet commented Oct 13, 2021

flip1995 commented Oct 14, 2021

xFrednet commented Oct 14, 2021

flip1995 commented Oct 14, 2021

xFrednet commented Oct 17, 2021

flip1995 commented Oct 19, 2021

xFrednet commented Oct 19, 2021

flip1995 commented Oct 19, 2021

xFrednet commented Oct 19, 2021 • edited

xFrednet commented Oct 20, 2021

flip1995 left a comment

Choose a reason for hiding this comment

xFrednet commented Oct 21, 2021 • edited

bors commented Oct 21, 2021

xFrednet commented Oct 21, 2021 • edited

flip1995 commented Nov 10, 2021

flip1995 left a comment • edited

Choose a reason for hiding this comment

xFrednet commented Nov 10, 2021

flip1995 left a comment

Choose a reason for hiding this comment

xFrednet commented Nov 11, 2021 • edited

flip1995 commented Nov 11, 2021

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Nov 11, 2021

bors commented Nov 11, 2021

bors commented Nov 11, 2021

bors commented Nov 11, 2021

xFrednet commented Nov 11, 2021

xFrednet commented Nov 11, 2021

xFrednet commented Oct 12, 2021 •

edited

xFrednet commented Oct 12, 2021 •

edited

xFrednet commented Oct 13, 2021 •

edited

xFrednet commented Oct 19, 2021 •

edited

xFrednet commented Oct 21, 2021 •

edited

xFrednet commented Oct 21, 2021 •

edited

flip1995 left a comment •

edited

xFrednet commented Nov 11, 2021 •

edited