Add new lint `DEPRECATED_CLIPPY_CFG_ATTR` by GuillaumeGomez · Pull Request #12292 · rust-lang/rust-clippy

GuillaumeGomez · 2024-02-14T16:56:25Z

As discussed on zulip.

This lint suggests to replace feature = "cargo-clippy" with clippy.

r? @flip1995

changelog: Add new lint DEPRECATED_CLIPPY_CFG_ATTR

flip1995

Impl LGTM. 2 open questions.

I think we want to wait with merging this, until cfg(clippy) is known by rustc/cargo? Then we'll also add --cfg clippy to the clippy-driver.

clippy_lints/src/attrs.rs

Urgau · 2024-02-14T20:19:43Z

I think we want to wait with merging this, until cfg(clippy) is known by rustc/cargo?

We can do it at the same time. It won't be an issue until clippy is synced in rust-lang/rust.
It will also probably be easier to convince rust-lang/rust folks if this PR is merged first.

GuillaumeGomez · 2024-02-15T00:25:56Z

Finally fixed doc test and dogfood. ^^'

flip1995 · 2024-02-15T10:37:47Z

It won't be an issue until clippy is synced in rust-lang/rust.

That's in exactly one week. Will rustc be ready until then. I'd rather approve the PR and leave it as S-blocked until rustc is ready. I'm happy to do an out of cycle sync for this. We need to get all of those changes in, in the same release cycle. Just merging and hoping that rustc (or any other OSS project) is ready in time doesn't work out great in my experience 😅

GuillaumeGomez · 2024-02-15T10:38:41Z

Sending the PR for rust in a few minutes.

GuillaumeGomez · 2024-02-15T10:42:33Z

Opened rust-lang/rust#121137.

tests/ui/useless_attribute.fixed

src/driver.rs

tests/ui/useless_attribute.stderr

…-clippy`

Urgau · 2024-02-15T11:02:33Z

It won't be an issue until clippy is synced in rust-lang/rust.

That's in exactly one week. Will rustc be ready until then. I'd rather approve the PR and leave it as S-blocked until rustc is ready. I'm happy to do an out of cycle sync for this. We need to get all of those changes in, in the same release cycle. Just merging and hoping that rustc (or any other OSS project) is ready in time doesn't work out great in my experience 😅

-Zcheck-cfg is unstable and opt-in, so we have technically more than a week, but yes, I had planned to submit the PR just after this one merged; as to get them in the same-ish nightly.

flip1995

LGTM and approved.

The last open question is, if this should be an extra lint or if it should be merged into DEPRECATED_CFG_ARG?

flip1995 · 2024-02-16T09:52:01Z

clippy_lints/src/attrs.rs

+            "replace with",
+            "clippy".to_string(),


One possible enhancement to this lint could be to check if the second part of the cfg_attr is an allow/warn/... and suggest to remove the cfg_attr completely.

OTOH we might want to add an extra lint for this, once people moved to cfg_attr(clippy, allow(...)).

So I wouldn't do this in this PR.

I made the same suggestion to @GuillaumeGomez in private and we also thought that it would be better done in a follow-up, as to also catch the pattern with cfg_attr(clippy, ...).

I confirm we did. ;)

I'll likely send a follow-up PR.

flip1995 · 2024-02-16T09:56:26Z

-Zcheck-cfg is unstable and opt-in

I would propose to keep -Zcheck-cfg unstable for at least one more release cycle after this PR and the cfg(clippy) PR in rustc are merged and synced. I'm not sure what timeline you have in mind for stabilization though.

Urgau · 2024-02-16T10:13:47Z

No problem, I have not yet proposed stabilization of the feature. We are currently running a Call for Testing, we may run it for a second week, after that and if all goes well, I will propose stabilization in rustc and Cargo, I expect this to take several weeks; and will make sure to have at least one full release for cfg(clippy).

flip1995 · 2024-02-16T10:18:18Z

src/driver.rs

                _ => Some(s.to_string()),
            })
+            // FIXME: remove this line in 1.79 to only keep `--cfg clippy`.
            .chain(vec!["--cfg".into(), r#"feature="cargo-clippy""#.into()])


@Urgau Can I ask you to remove this line as part of the stabilization PR of the -Zcheck-cfg feature in rust-lang/rust please? That way, the time people have to transition is as long as possible.

Sure, no problem.

flip1995 · 2024-02-16T10:23:07Z

The last open question is, if this should be an extra lint or if it should be merged into DEPRECATED_CFG_ARG?

Answering my final question myself:

After thinking about this, I prefer to use a new lint for this, because:

People that allowed the old DEPRECATED_CFG_ARG lint for some reason, still get a warning with the new lint
It will be easier for people to transition with cargo clippy --fix -- -Aclippy::all -Wclippy::deprecated_clippy_cfg_arg, without interfering with the old lint.

With that and as the cfg(clippy) PR is already in the bors queue:

@bors r+

bors · 2024-02-16T10:23:10Z

📌 Commit f4a3db8 has been approved by flip1995

It is now in the queue for this repository.

bors · 2024-02-16T10:24:18Z

⌛ Testing commit f4a3db8 with merge 32c006c...

bors · 2024-02-16T10:34:40Z

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

Urgau · 2024-02-16T10:36:28Z

Thank you @flip1995 (and @GuillaumeGomez) for your help, I really appreciate it ❤️.

Without your help this issue could very well have delayed check-cfg or even put it in limbo. Thank you very much.

GuillaumeGomez · 2024-02-16T10:37:21Z

It allows me to take a break from some compiler stuff so it's very welcome. 😄

flip1995 · 2024-02-16T11:21:06Z

rust-lang/blog.rust-lang.org#1253

I thought I'd write a blog post about this.

…rgau,Nilstrieb Add clippy into the known `cfg` list In clippy, we are removing the `feature = "cargo-clippy"` cfg to replace it with `clippy` in rust-lang/rust-clippy#12292. But for it to work, we need to declare `clippy` as cfg. It makes it more coherent with other existing tools like rustdoc. cc `@flip1995`

Rollup merge of rust-lang#121137 - GuillaumeGomez:add-clippy-cfg, r=Urgau,Nilstrieb Add clippy into the known `cfg` list In clippy, we are removing the `feature = "cargo-clippy"` cfg to replace it with `clippy` in rust-lang/rust-clippy#12292. But for it to work, we need to declare `clippy` as cfg. It makes it more coherent with other existing tools like rustdoc. cc `@flip1995`

…ip1995 Add `unnecessary_clippy_cfg` lint Follow-up of #12292. r? `@flip1995` changelog: Add `unnecessary_clippy_cfg` lint

`cfg(feature = "cargo-clippy")` has been [deprecated]. The correct way to achieve the same result is now `cfg(clippy)`. [deprecated]: rust-lang/rust-clippy#12292

…flip1995 Remove `feature=cargo-clippy` argument Roses are red, Violets are blue, Fixme was written, And now it's due --- changelog: **Important Change** Removed the implicit `cargo-clippy` feature set by Clippy as announced here: <https://blog.rust-lang.org/2024/02/28/Clippy-deprecating-feature-cargo-clippy.html> [#13246](#13246) Follow-up of: #12292 r? `@flip1995` cc: `@GuillaumeGomez`

`cfg(feature = "cargo-clippy")` has been [deprecated]. The correct way to achieve the same result is now `cfg(clippy)`. (This was already changed in the main branch, but the flat-matcher branch fell behind.) [deprecated]: rust-lang/rust-clippy#12292

rustbot assigned flip1995 Feb 14, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 14, 2024

flip1995 reviewed Feb 14, 2024

View reviewed changes

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

clippy_lints/src/attrs.rs Show resolved Hide resolved

GuillaumeGomez force-pushed the DEPRECATED_CLIPPY_CFG_ATTR branch 3 times, most recently from 7f1e19f to 22cd0dd Compare February 14, 2024 22:40

Add new lint DEPRECATED_CLIPPY_CFG_ATTR

f35eec8

GuillaumeGomez force-pushed the DEPRECATED_CLIPPY_CFG_ATTR branch from 22cd0dd to 51dc344 Compare February 14, 2024 23:40

GuillaumeGomez mentioned this pull request Feb 15, 2024

Add clippy into the known cfg list rust-lang/rust#121137

Merged

flip1995 reviewed Feb 15, 2024

View reviewed changes

tests/ui/useless_attribute.fixed Outdated Show resolved Hide resolved

src/driver.rs Show resolved Hide resolved

tests/ui/useless_attribute.stderr Show resolved Hide resolved

GuillaumeGomez added 3 commits February 15, 2024 11:52

Pass --cfg clippy to clippy-driver

e0f82af

Add ui tests for DEPRECATED_CLIPPY_CFG_ATTR

cd6f03a

Update clippy book to mention cfg(clippy) instead of feature `cargo…

f4a3db8

…-clippy`

GuillaumeGomez force-pushed the DEPRECATED_CLIPPY_CFG_ATTR branch from 51dc344 to f4a3db8 Compare February 15, 2024 10:52

flip1995 approved these changes Feb 16, 2024

View reviewed changes

flip1995 reviewed Feb 16, 2024

View reviewed changes

bors merged commit 32c006c into rust-lang:master Feb 16, 2024

GuillaumeGomez deleted the DEPRECATED_CLIPPY_CFG_ATTR branch February 16, 2024 10:35

GuillaumeGomez mentioned this pull request Feb 16, 2024

Add unnecessary_clippy_cfg lint #12303

Merged

bors added a commit that referenced this pull request Feb 22, 2024

Auto merge of #12303 - GuillaumeGomez:unneedeed_clippy_cfg_attr, r=fl…

d554bca

…ip1995 Add `unnecessary_clippy_cfg` lint Follow-up of #12292. r? `@flip1995` changelog: Add `unnecessary_clippy_cfg` lint

This was referenced Feb 28, 2024

Replace feature = "cargo-clippy" danielparks/matchgen#41

Merged

Clippy ignores feature = "cargo-clippy" in generated code #12392

Closed

xFrednet mentioned this pull request Aug 9, 2024

Remove feature=cargo-clippy argument #13246

Merged

Comments

Conversation

GuillaumeGomez commented Feb 14, 2024

Uh oh!

flip1995 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Urgau commented Feb 14, 2024

Uh oh!

GuillaumeGomez commented Feb 15, 2024

Uh oh!

flip1995 commented Feb 15, 2024

Uh oh!

GuillaumeGomez commented Feb 15, 2024

Uh oh!

GuillaumeGomez commented Feb 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Urgau commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flip1995 left a comment

Choose a reason for hiding this comment

Uh oh!

flip1995 Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

Urgau Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

flip1995 commented Feb 16, 2024

Uh oh!

Urgau commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flip1995 Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

Urgau Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

flip1995 commented Feb 16, 2024

Uh oh!

bors commented Feb 16, 2024

Uh oh!

bors commented Feb 16, 2024

Uh oh!

bors commented Feb 16, 2024

Uh oh!

Urgau commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Feb 16, 2024

Uh oh!

flip1995 commented Feb 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flip1995 left a comment •

edited

Loading

Urgau commented Feb 15, 2024 •

edited

Loading

Urgau commented Feb 16, 2024 •

edited

Loading

Urgau commented Feb 16, 2024 •

edited

Loading