Add Clippy version to Clippy's lint list by xFrednet · Pull Request #7813 · rust...

Copy link

Member

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 commented on Oct 12 •

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

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 commented on Oct 13 •

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

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:

1. git checkout the release
2. Use the export.py/Metadata collector to generate lints.json file
3. 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 commented on Oct 13

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

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

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

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

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

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

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

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 commented on Oct 19 •

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

xFrednet commented 29 days ago

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 :)

xFrednet commented 28 days ago •

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

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

xFrednet commented 28 days ago •

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

flip1995 commented 8 days ago

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

xFrednet commented 8 days ago

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 :)

clippy_lints/src/deprecated_lints.rs

Show resolved

xFrednet commented 7 days ago •

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

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

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

Commit 8c45fd8 has been approved by flip1995

bors commented 7 days ago

Testing commit 8c45fd8 with merge 3bfe98d...

xFrednet commented 7 days ago

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

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.