From 2c09ac3d39c272469b4f9a581f4a5507bcc09ca1 Mon Sep 17 00:00:00 2001 From: Milo Moisson Date: Wed, 12 Jun 2024 18:10:56 +0200 Subject: [PATCH 1/2] feat: add cfg_not_test lint --- CHANGELOG.md | 1 + clippy_lints/src/cfg_not_test.rs | 60 ++++++++++++++++++++++++++++++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + 4 files changed, 64 insertions(+) create mode 100644 clippy_lints/src/cfg_not_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d919b521e715..5258cd9f41aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5254,6 +5254,7 @@ Released 2018-09-13 [`cast_sign_loss`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_sign_loss [`cast_slice_different_sizes`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_slice_different_sizes [`cast_slice_from_raw_parts`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_slice_from_raw_parts +[`cfg_not_test`]: https://rust-lang.github.io/rust-clippy/master/index.html#cfg_not_test [`char_lit_as_u8`]: https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8 [`chars_last_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_last_cmp [`chars_next_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_next_cmp diff --git a/clippy_lints/src/cfg_not_test.rs b/clippy_lints/src/cfg_not_test.rs new file mode 100644 index 000000000000..b54f392bf2fa --- /dev/null +++ b/clippy_lints/src/cfg_not_test.rs @@ -0,0 +1,60 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use rustc_ast::NestedMetaItem; +use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `cfg` that excludes code from `test` builds. (i.e., `#{cfg(not(test))]`) + /// + /// ### Why is this bad? + /// This may give the false impression that a codebase has 100% coverage, yet actually has untested code. + /// Enabling this also guards against excessive mockery as well, which is an anti-pattern. + /// + /// ### Example + /// ```rust + /// # fn important_check() {} + /// #[cfg(not(test))] + /// important_check(); // I'm not actually tested, but not including me will falsely increase coverage! + /// ``` + /// Use instead: + /// ```rust + /// # fn important_check() {} + /// important_check(); + /// ``` + #[clippy::version = "1.73.0"] + pub CFG_NOT_TEST, + restriction, + "enforce against excluding code from test builds" +} + +declare_lint_pass!(CfgNotTest => [CFG_NOT_TEST]); + +impl EarlyLintPass for CfgNotTest { + fn check_attribute(&mut self, cx: &EarlyContext<'_>, attr: &rustc_ast::Attribute) { + if attr.has_name(rustc_span::sym::cfg) && contains_not_test(attr.meta_item_list().as_deref(), false) { + span_lint_and_then( + cx, + CFG_NOT_TEST, + attr.span, + "code is excluded from test builds", + |diag| { + diag.help("consider not excluding any code from test builds"); + diag.note_once("this could increase code coverage despite not actually being tested"); + }, + ); + } + } +} + +fn contains_not_test(list: Option<&[NestedMetaItem]>, not: bool) -> bool { + list.is_some_and(|list| { + list.iter().any(|item| { + item.ident().is_some_and(|ident| match ident.name { + rustc_span::sym::not => contains_not_test(item.meta_item_list(), !not), + rustc_span::sym::test => not, + _ => contains_not_test(item.meta_item_list(), not), + }) + }) + }) +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 50661bf46a0a..0b674a7d7ab3 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -104,6 +104,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::casts::REF_AS_PTR_INFO, crate::casts::UNNECESSARY_CAST_INFO, crate::casts::ZERO_PTR_INFO, + crate::cfg_not_test::CFG_NOT_TEST_INFO, crate::checked_conversions::CHECKED_CONVERSIONS_INFO, crate::cognitive_complexity::COGNITIVE_COMPLEXITY_INFO, crate::collapsible_if::COLLAPSIBLE_ELSE_IF_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8a9d3b43fb3a..3d40b397ab9b 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -92,6 +92,7 @@ mod box_default; mod byte_char_slices; mod cargo; mod casts; +mod cfg_not_test; mod checked_conversions; mod cognitive_complexity; mod collapsible_if; @@ -1176,6 +1177,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_early_pass(|| Box::new(field_scoped_visibility_modifiers::FieldScopedVisibilityModifiers)); store.register_late_pass(|_| Box::new(set_contains_or_insert::HashsetInsertAfterContains)); store.register_early_pass(|| Box::new(byte_char_slices::ByteCharSlice)); + store.register_early_pass(|| Box::new(cfg_not_test::CfgNotTest)); // add lints here, do not remove this comment, it's used in `new_lint` } From dd37441a646c64c89150cc008dacdbb1f68d3f14 Mon Sep 17 00:00:00 2001 From: Milo Moisson Date: Wed, 12 Jun 2024 18:11:01 +0200 Subject: [PATCH 2/2] test: add cfg_not_test tests --- tests/ui/cfg_not_test.rs | 32 +++++++++++++++++++++++++ tests/ui/cfg_not_test.stderr | 45 ++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 tests/ui/cfg_not_test.rs create mode 100644 tests/ui/cfg_not_test.stderr diff --git a/tests/ui/cfg_not_test.rs b/tests/ui/cfg_not_test.rs new file mode 100644 index 000000000000..da3e29d28966 --- /dev/null +++ b/tests/ui/cfg_not_test.rs @@ -0,0 +1,32 @@ +#![allow(unused)] +#![warn(clippy::cfg_not_test)] + +fn important_check() {} + +fn main() { + // Statement + #[cfg(not(test))] + let answer = 42; + + // Expression + #[cfg(not(test))] + important_check(); + + // Make sure only not(test) are checked, not other attributes + #[cfg(not(foo))] + important_check(); +} + +#[cfg(not(not(test)))] +struct CfgNotTest; + +// Deeply nested `not(test)` +#[cfg(not(test))] +fn foo() {} +#[cfg(all(debug_assertions, not(test)))] +fn bar() {} +#[cfg(not(any(not(debug_assertions), test)))] +fn baz() {} + +#[cfg(test)] +mod tests {} diff --git a/tests/ui/cfg_not_test.stderr b/tests/ui/cfg_not_test.stderr new file mode 100644 index 000000000000..c1bf626887af --- /dev/null +++ b/tests/ui/cfg_not_test.stderr @@ -0,0 +1,45 @@ +error: code is excluded from test builds + --> tests/ui/cfg_not_test.rs:8:5 + | +LL | #[cfg(not(test))] + | ^^^^^^^^^^^^^^^^^ + | + = help: consider not excluding any code from test builds + = note: this could increase code coverage despite not actually being tested + = note: `-D clippy::cfg-not-test` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::cfg_not_test)]` + +error: code is excluded from test builds + --> tests/ui/cfg_not_test.rs:12:5 + | +LL | #[cfg(not(test))] + | ^^^^^^^^^^^^^^^^^ + | + = help: consider not excluding any code from test builds + +error: code is excluded from test builds + --> tests/ui/cfg_not_test.rs:24:1 + | +LL | #[cfg(not(test))] + | ^^^^^^^^^^^^^^^^^ + | + = help: consider not excluding any code from test builds + +error: code is excluded from test builds + --> tests/ui/cfg_not_test.rs:26:1 + | +LL | #[cfg(all(debug_assertions, not(test)))] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider not excluding any code from test builds + +error: code is excluded from test builds + --> tests/ui/cfg_not_test.rs:28:1 + | +LL | #[cfg(not(any(not(debug_assertions), test)))] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider not excluding any code from test builds + +error: aborting due to 5 previous errors +