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

Add new lint DEPRECATED_CLIPPY_CFG_ATTR #12292

Merged
merged 4 commits into from
Feb 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5071,6 +5071,7 @@ Released 2018-09-13
[`default_trait_access`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_trait_access
[`default_union_representation`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_union_representation
[`deprecated_cfg_attr`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_cfg_attr
[`deprecated_clippy_cfg_attr`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_clippy_cfg_attr
[`deprecated_semver`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver
[`deref_addrof`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_addrof
[`deref_by_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_by_slicing
Expand Down
9 changes: 3 additions & 6 deletions book/src/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,17 +113,14 @@ found [here](https://rust-lang.github.io/rust-clippy/master/index.html#msrv)

Very rarely, you may wish to prevent Clippy from evaluating certain sections of code entirely. You can do this with
[conditional compilation](https://doc.rust-lang.org/reference/conditional-compilation.html) by checking that the
`cargo-clippy` feature is not set. You may need to provide a stub so that the code compiles:
`clippy` cfg is not set. You may need to provide a stub so that the code compiles:

```rust
#[cfg(not(feature = "cargo-clippy"))]
#[cfg(not(clippy)]
include!(concat!(env!("OUT_DIR"), "/my_big_function-generated.rs"));

#[cfg(feature = "cargo-clippy")]
#[cfg(clippy)]
fn my_big_function(_input: &str) -> Option<MyStruct> {
None
}
```

This feature is not actually part of your crate, so specifying `--all-features` to other tools, e.g. `cargo test
--all-features`, will not disable it.
125 changes: 97 additions & 28 deletions clippy_lints/src/attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,32 @@ declare_clippy_lint! {
"prevent from misusing the wrong attr name"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for `#[cfg_attr(feature = "cargo-clippy", ...)]` and for
/// `#[cfg(feature = "cargo-clippy")]` and suggests to replace it with
/// `#[cfg_attr(clippy, ...)]` or `#[cfg(clippy)]`.
///
/// ### Why is this bad?
/// This feature has been deprecated for years and shouldn't be used anymore.
///
/// ### Example
/// ```no_run
/// #[cfg(feature = "cargo-clippy")]
/// struct Bar;
/// ```
///
/// Use instead:
/// ```no_run
/// #[cfg(clippy)]
/// struct Bar;
/// ```
#[clippy::version = "1.78.0"]
pub DEPRECATED_CLIPPY_CFG_ATTR,
suspicious,
"usage of `cfg(feature = \"cargo-clippy\")` instead of `cfg(clippy)`"
}

declare_lint_pass!(Attributes => [
ALLOW_ATTRIBUTES_WITHOUT_REASON,
INLINE_ALWAYS,
Expand Down Expand Up @@ -794,6 +820,7 @@ impl_lint_pass!(EarlyAttributes => [
EMPTY_LINE_AFTER_DOC_COMMENTS,
NON_MINIMAL_CFG,
MAYBE_MISUSED_CFG,
DEPRECATED_CLIPPY_CFG_ATTR,
]);

impl EarlyLintPass for EarlyAttributes {
Expand All @@ -803,6 +830,7 @@ impl EarlyLintPass for EarlyAttributes {

fn check_attribute(&mut self, cx: &EarlyContext<'_>, attr: &Attribute) {
check_deprecated_cfg_attr(cx, attr, &self.msrv);
check_deprecated_cfg(cx, attr);
check_mismatched_target_os(cx, attr);
check_minimal_cfg_condition(cx, attr);
check_misused_cfg(cx, attr);
Expand Down Expand Up @@ -857,39 +885,80 @@ fn check_empty_line_after_outer_attr(cx: &EarlyContext<'_>, item: &rustc_ast::It
}
}

fn check_cargo_clippy_attr(cx: &EarlyContext<'_>, item: &rustc_ast::MetaItem) {
if item.has_name(sym::feature) && item.value_str().is_some_and(|v| v.as_str() == "cargo-clippy") {
span_lint_and_sugg(
cx,
DEPRECATED_CLIPPY_CFG_ATTR,
item.span,
"`feature = \"cargo-clippy\"` was replaced by `clippy`",
"replace with",
"clippy".to_string(),
Comment on lines +895 to +896
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm we did. ;)

I'll likely send a follow-up PR.

Applicability::MachineApplicable,
);
}
}

fn check_deprecated_cfg_recursively(cx: &EarlyContext<'_>, attr: &rustc_ast::MetaItem) {
if let Some(ident) = attr.ident() {
if ["any", "all", "not"].contains(&ident.name.as_str()) {
let Some(list) = attr.meta_item_list() else { return };
for item in list.iter().filter_map(|item| item.meta_item()) {
check_deprecated_cfg_recursively(cx, item);
}
} else {
check_cargo_clippy_attr(cx, attr);
}
}
}

fn check_deprecated_cfg(cx: &EarlyContext<'_>, attr: &Attribute) {
if attr.has_name(sym::cfg)
&& let Some(list) = attr.meta_item_list()
{
for item in list.iter().filter_map(|item| item.meta_item()) {
check_deprecated_cfg_recursively(cx, item);
}
}
}

fn check_deprecated_cfg_attr(cx: &EarlyContext<'_>, attr: &Attribute, msrv: &Msrv) {
if msrv.meets(msrvs::TOOL_ATTRIBUTES)
// check cfg_attr
&& attr.has_name(sym::cfg_attr)
// check cfg_attr
if attr.has_name(sym::cfg_attr)
&& let Some(items) = attr.meta_item_list()
&& items.len() == 2
// check for `rustfmt`
&& let Some(feature_item) = items[0].meta_item()
&& feature_item.has_name(sym::rustfmt)
// check for `rustfmt_skip` and `rustfmt::skip`
&& let Some(skip_item) = &items[1].meta_item()
&& (skip_item.has_name(sym!(rustfmt_skip))
|| skip_item
.path
.segments
.last()
.expect("empty path in attribute")
.ident
.name
== sym::skip)
// Only lint outer attributes, because custom inner attributes are unstable
// Tracking issue: https://github.com/rust-lang/rust/issues/54726
&& attr.style == AttrStyle::Outer
{
span_lint_and_sugg(
cx,
DEPRECATED_CFG_ATTR,
attr.span,
"`cfg_attr` is deprecated for rustfmt and got replaced by tool attributes",
"use",
"#[rustfmt::skip]".to_string(),
Applicability::MachineApplicable,
);
// check for `rustfmt`
if feature_item.has_name(sym::rustfmt)
&& msrv.meets(msrvs::TOOL_ATTRIBUTES)
flip1995 marked this conversation as resolved.
Show resolved Hide resolved
// check for `rustfmt_skip` and `rustfmt::skip`
&& let Some(skip_item) = &items[1].meta_item()
&& (skip_item.has_name(sym!(rustfmt_skip))
|| skip_item
.path
.segments
.last()
.expect("empty path in attribute")
.ident
.name
== sym::skip)
// Only lint outer attributes, because custom inner attributes are unstable
// Tracking issue: https://github.com/rust-lang/rust/issues/54726
&& attr.style == AttrStyle::Outer
{
span_lint_and_sugg(
cx,
DEPRECATED_CFG_ATTR,
attr.span,
"`cfg_attr` is deprecated for rustfmt and got replaced by tool attributes",
"use",
"#[rustfmt::skip]".to_string(),
Applicability::MachineApplicable,
);
} else {
check_deprecated_cfg_recursively(cx, feature_item);
}
}
}

Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::attrs::ALLOW_ATTRIBUTES_WITHOUT_REASON_INFO,
crate::attrs::BLANKET_CLIPPY_RESTRICTION_LINTS_INFO,
crate::attrs::DEPRECATED_CFG_ATTR_INFO,
crate::attrs::DEPRECATED_CLIPPY_CFG_ATTR_INFO,
crate::attrs::DEPRECATED_SEMVER_INFO,
crate::attrs::EMPTY_LINE_AFTER_DOC_COMMENTS_INFO,
crate::attrs::EMPTY_LINE_AFTER_OUTER_ATTR_INFO,
Expand Down
2 changes: 2 additions & 0 deletions src/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,9 @@ pub fn main() {
},
_ => 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()])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, no problem.

.chain(vec!["--cfg".into(), "clippy".into()])
flip1995 marked this conversation as resolved.
Show resolved Hide resolved
.collect::<Vec<String>>();

// We enable Clippy if one of the following conditions is met
Expand Down
13 changes: 13 additions & 0 deletions tests/ui/cfg_attr_cargo_clippy.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#![warn(clippy::deprecated_clippy_cfg_attr)]
#![allow(clippy::non_minimal_cfg)]
#![cfg_attr(clippy, doc = "a")] //~ ERROR: `feature = "cargo-clippy"` was

#[cfg_attr(clippy, derive(Debug))] //~ ERROR: `feature = "cargo-clippy"` was
#[cfg_attr(not(clippy), derive(Debug))] //~ ERROR: `feature = "cargo-clippy"` was
#[cfg(clippy)] //~ ERROR: `feature = "cargo-clippy"` was
#[cfg(not(clippy))] //~ ERROR: `feature = "cargo-clippy"` was
#[cfg(any(clippy))] //~ ERROR: `feature = "cargo-clippy"` was
#[cfg(all(clippy))] //~ ERROR: `feature = "cargo-clippy"` was
pub struct Bar;

fn main() {}
13 changes: 13 additions & 0 deletions tests/ui/cfg_attr_cargo_clippy.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#![warn(clippy::deprecated_clippy_cfg_attr)]
#![allow(clippy::non_minimal_cfg)]
#![cfg_attr(feature = "cargo-clippy", doc = "a")] //~ ERROR: `feature = "cargo-clippy"` was

#[cfg_attr(feature = "cargo-clippy", derive(Debug))] //~ ERROR: `feature = "cargo-clippy"` was
#[cfg_attr(not(feature = "cargo-clippy"), derive(Debug))] //~ ERROR: `feature = "cargo-clippy"` was
#[cfg(feature = "cargo-clippy")] //~ ERROR: `feature = "cargo-clippy"` was
#[cfg(not(feature = "cargo-clippy"))] //~ ERROR: `feature = "cargo-clippy"` was
#[cfg(any(feature = "cargo-clippy"))] //~ ERROR: `feature = "cargo-clippy"` was
#[cfg(all(feature = "cargo-clippy"))] //~ ERROR: `feature = "cargo-clippy"` was
pub struct Bar;

fn main() {}
47 changes: 47 additions & 0 deletions tests/ui/cfg_attr_cargo_clippy.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
error: `feature = "cargo-clippy"` was replaced by `clippy`
--> $DIR/cfg_attr_cargo_clippy.rs:5:12
|
LL | #[cfg_attr(feature = "cargo-clippy", derive(Debug))]
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `clippy`
|
= note: `-D clippy::deprecated-clippy-cfg-attr` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::deprecated_clippy_cfg_attr)]`

error: `feature = "cargo-clippy"` was replaced by `clippy`
--> $DIR/cfg_attr_cargo_clippy.rs:6:16
|
LL | #[cfg_attr(not(feature = "cargo-clippy"), derive(Debug))]
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `clippy`

error: `feature = "cargo-clippy"` was replaced by `clippy`
--> $DIR/cfg_attr_cargo_clippy.rs:7:7
|
LL | #[cfg(feature = "cargo-clippy")]
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `clippy`

error: `feature = "cargo-clippy"` was replaced by `clippy`
--> $DIR/cfg_attr_cargo_clippy.rs:8:11
|
LL | #[cfg(not(feature = "cargo-clippy"))]
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `clippy`

error: `feature = "cargo-clippy"` was replaced by `clippy`
--> $DIR/cfg_attr_cargo_clippy.rs:9:11
|
LL | #[cfg(any(feature = "cargo-clippy"))]
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `clippy`

error: `feature = "cargo-clippy"` was replaced by `clippy`
--> $DIR/cfg_attr_cargo_clippy.rs:10:11
|
LL | #[cfg(all(feature = "cargo-clippy"))]
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `clippy`

error: `feature = "cargo-clippy"` was replaced by `clippy`
--> $DIR/cfg_attr_cargo_clippy.rs:3:13
|
LL | #![cfg_attr(feature = "cargo-clippy", doc = "a")]
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `clippy`

error: aborting due to 7 previous errors

2 changes: 1 addition & 1 deletion tests/ui/useless_attribute.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#![feature(rustc_private)]

#![allow(dead_code)]
#![cfg_attr(feature = "cargo-clippy", allow(dead_code))]
#![cfg_attr(clippy, allow(dead_code))]
#[rustfmt::skip]
#[allow(unused_imports)]
#[allow(unused_extern_crates)]
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/useless_attribute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#![feature(rustc_private)]

#[allow(dead_code)]
#[cfg_attr(feature = "cargo-clippy", allow(dead_code))]
#[cfg_attr(clippy, allow(dead_code))]
#[rustfmt::skip]
#[allow(unused_imports)]
#[allow(unused_extern_crates)]
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/useless_attribute.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ LL | #[allow(dead_code)]
error: useless lint attribute
--> $DIR/useless_attribute.rs:9:1
|
LL | #[cfg_attr(feature = "cargo-clippy", allow(dead_code))]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: if you just forgot a `!`, use: `#![cfg_attr(feature = "cargo-clippy", allow(dead_code)`
LL | #[cfg_attr(clippy, allow(dead_code))]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: if you just forgot a `!`, use: `#![cfg_attr(clippy, allow(dead_code)`

error: useless lint attribute
--> $DIR/useless_attribute.rs:20:5
Expand Down
Loading