From 4c44b4e3c85a1ee3ec1d77f97737aa689234f0b6 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 9 Jun 2024 22:11:47 -0400 Subject: [PATCH 1/2] Remove `is_test_module_or_function` and use `is_in_test` instead. --- clippy_lints/src/disallowed_names.rs | 46 +++++-------------- clippy_lints/src/wildcard_imports.rs | 19 ++------ tests/ui/disallowed_names.rs | 1 + tests/ui/wildcard_imports.fixed | 3 ++ tests/ui/wildcard_imports.rs | 3 ++ tests/ui/wildcard_imports.stderr | 10 ++-- .../wildcard_imports_2021.edition2018.fixed | 3 ++ .../wildcard_imports_2021.edition2018.stderr | 10 ++-- .../wildcard_imports_2021.edition2021.fixed | 3 ++ .../wildcard_imports_2021.edition2021.stderr | 10 ++-- tests/ui/wildcard_imports_2021.rs | 3 ++ 11 files changed, 47 insertions(+), 64 deletions(-) diff --git a/clippy_lints/src/disallowed_names.rs b/clippy_lints/src/disallowed_names.rs index 2afbf184117..58809604c37 100644 --- a/clippy_lints/src/disallowed_names.rs +++ b/clippy_lints/src/disallowed_names.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::span_lint; -use clippy_utils::is_test_module_or_function; +use clippy_utils::is_in_test; use rustc_data_structures::fx::FxHashSet; -use rustc_hir::{Item, Pat, PatKind}; +use rustc_hir::{Pat, PatKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; @@ -27,52 +27,30 @@ #[derive(Clone, Debug)] pub struct DisallowedNames { disallow: FxHashSet, - test_modules_deep: u32, } impl DisallowedNames { pub fn new(disallowed_names: &[String]) -> Self { Self { disallow: disallowed_names.iter().cloned().collect(), - test_modules_deep: 0, } } - - fn in_test_module(&self) -> bool { - self.test_modules_deep != 0 - } } impl_lint_pass!(DisallowedNames => [DISALLOWED_NAMES]); impl<'tcx> LateLintPass<'tcx> for DisallowedNames { - fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { - if is_test_module_or_function(cx.tcx, item) { - self.test_modules_deep = self.test_modules_deep.saturating_add(1); - } - } - fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) { - // Check whether we are under the `test` attribute. - if self.in_test_module() { - return; - } - - if let PatKind::Binding(.., ident, _) = pat.kind { - if self.disallow.contains(&ident.name.to_string()) { - span_lint( - cx, - DISALLOWED_NAMES, - ident.span, - format!("use of a disallowed/placeholder name `{}`", ident.name), - ); - } - } - } - - fn check_item_post(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { - if is_test_module_or_function(cx.tcx, item) { - self.test_modules_deep = self.test_modules_deep.saturating_sub(1); + if let PatKind::Binding(.., ident, _) = pat.kind + && self.disallow.contains(&ident.name.to_string()) + && !is_in_test(cx.tcx, pat.hir_id) + { + span_lint( + cx, + DISALLOWED_NAMES, + ident.span, + format!("use of a disallowed/placeholder name `{}`", ident.name), + ); } } } diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index 436f0cb79fb..a0a60ca8875 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::is_test_module_or_function; +use clippy_utils::is_in_test; use clippy_utils::source::{snippet, snippet_with_applicability}; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; @@ -100,7 +100,6 @@ #[derive(Default)] pub struct WildcardImports { warn_on_all: bool, - test_modules_deep: u32, allowed_segments: FxHashSet, } @@ -108,7 +107,6 @@ impl WildcardImports { pub fn new(warn_on_all: bool, allowed_wildcard_imports: FxHashSet) -> Self { Self { warn_on_all, - test_modules_deep: 0, allowed_segments: allowed_wildcard_imports, } } @@ -122,15 +120,12 @@ fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { return; } - if is_test_module_or_function(cx.tcx, item) { - self.test_modules_deep = self.test_modules_deep.saturating_add(1); - } let module = cx.tcx.parent_module_from_def_id(item.owner_id.def_id); if cx.tcx.visibility(item.owner_id.def_id) != ty::Visibility::Restricted(module.to_def_id()) { return; } if let ItemKind::Use(use_path, UseKind::Glob) = &item.kind - && (self.warn_on_all || !self.check_exceptions(item, use_path.segments)) + && (self.warn_on_all || !self.check_exceptions(cx, item, use_path.segments)) && let used_imports = cx.tcx.names_imported_by_glob_use(item.owner_id.def_id) && !used_imports.is_empty() // Already handled by `unused_imports` && !used_imports.contains(&kw::Underscore) @@ -180,20 +175,14 @@ fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { span_lint_and_sugg(cx, lint, span, message, "try", sugg, applicability); } } - - fn check_item_post(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { - if is_test_module_or_function(cx.tcx, item) { - self.test_modules_deep = self.test_modules_deep.saturating_sub(1); - } - } } impl WildcardImports { - fn check_exceptions(&self, item: &Item<'_>, segments: &[PathSegment<'_>]) -> bool { + fn check_exceptions(&self, cx: &LateContext<'_>, item: &Item<'_>, segments: &[PathSegment<'_>]) -> bool { item.span.from_expansion() || is_prelude_import(segments) - || (is_super_only_import(segments) && self.test_modules_deep > 0) || is_allowed_via_config(segments, &self.allowed_segments) + || (is_super_only_import(segments) && is_in_test(cx.tcx, item.hir_id())) } } diff --git a/tests/ui/disallowed_names.rs b/tests/ui/disallowed_names.rs index 13c883409bf..96531bf8d88 100644 --- a/tests/ui/disallowed_names.rs +++ b/tests/ui/disallowed_names.rs @@ -60,6 +60,7 @@ fn issue_1647_ref_mut() { //~^ ERROR: use of a disallowed/placeholder name `quux` } +#[cfg(test)] mod tests { fn issue_7305() { // `disallowed_names` lint should not be triggered inside of the test code. diff --git a/tests/ui/wildcard_imports.fixed b/tests/ui/wildcard_imports.fixed index 6fdd728b9b7..46890ee9213 100644 --- a/tests/ui/wildcard_imports.fixed +++ b/tests/ui/wildcard_imports.fixed @@ -204,6 +204,7 @@ mod super_imports { } } + #[cfg(test)] mod test_should_pass { use super::*; @@ -212,6 +213,7 @@ mod super_imports { } } + #[cfg(test)] mod test_should_pass_inside_function { fn with_super_inside_function() { use super::*; @@ -219,6 +221,7 @@ mod super_imports { } } + #[cfg(test)] mod test_should_pass_further_inside { fn insidefoo() {} mod inner { diff --git a/tests/ui/wildcard_imports.rs b/tests/ui/wildcard_imports.rs index 20e06d4b366..1a5586cbb88 100644 --- a/tests/ui/wildcard_imports.rs +++ b/tests/ui/wildcard_imports.rs @@ -205,6 +205,7 @@ fn with_super() { } } + #[cfg(test)] mod test_should_pass { use super::*; @@ -213,6 +214,7 @@ fn with_super() { } } + #[cfg(test)] mod test_should_pass_inside_function { fn with_super_inside_function() { use super::*; @@ -220,6 +222,7 @@ fn with_super_inside_function() { } } + #[cfg(test)] mod test_should_pass_further_inside { fn insidefoo() {} mod inner { diff --git a/tests/ui/wildcard_imports.stderr b/tests/ui/wildcard_imports.stderr index 0c69d5262c2..8e88f216394 100644 --- a/tests/ui/wildcard_imports.stderr +++ b/tests/ui/wildcard_imports.stderr @@ -106,31 +106,31 @@ LL | use super::*; | ^^^^^^^^ help: try: `super::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports.rs:236:17 + --> tests/ui/wildcard_imports.rs:239:17 | LL | use super::*; | ^^^^^^^^ help: try: `super::insidefoo` error: usage of wildcard import - --> tests/ui/wildcard_imports.rs:244:13 + --> tests/ui/wildcard_imports.rs:247:13 | LL | use crate::super_imports::*; | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `crate::super_imports::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports.rs:253:17 + --> tests/ui/wildcard_imports.rs:256:17 | LL | use super::super::*; | ^^^^^^^^^^^^^^^ help: try: `super::super::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports.rs:262:13 + --> tests/ui/wildcard_imports.rs:265:13 | LL | use super::super::super_imports::*; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `super::super::super_imports::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports.rs:270:13 + --> tests/ui/wildcard_imports.rs:273:13 | LL | use super::*; | ^^^^^^^^ help: try: `super::foofoo` diff --git a/tests/ui/wildcard_imports_2021.edition2018.fixed b/tests/ui/wildcard_imports_2021.edition2018.fixed index 6a9fe007d65..197dd3b94df 100644 --- a/tests/ui/wildcard_imports_2021.edition2018.fixed +++ b/tests/ui/wildcard_imports_2021.edition2018.fixed @@ -198,6 +198,7 @@ mod super_imports { } } + #[cfg(test)] mod test_should_pass { use super::*; @@ -206,6 +207,7 @@ mod super_imports { } } + #[cfg(test)] mod test_should_pass_inside_function { fn with_super_inside_function() { use super::*; @@ -213,6 +215,7 @@ mod super_imports { } } + #[cfg(test)] mod test_should_pass_further_inside { fn insidefoo() {} mod inner { diff --git a/tests/ui/wildcard_imports_2021.edition2018.stderr b/tests/ui/wildcard_imports_2021.edition2018.stderr index 11e0bd37769..66adacd95dc 100644 --- a/tests/ui/wildcard_imports_2021.edition2018.stderr +++ b/tests/ui/wildcard_imports_2021.edition2018.stderr @@ -106,31 +106,31 @@ LL | use super::*; | ^^^^^^^^ help: try: `super::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports_2021.rs:230:17 + --> tests/ui/wildcard_imports_2021.rs:233:17 | LL | use super::*; | ^^^^^^^^ help: try: `super::insidefoo` error: usage of wildcard import - --> tests/ui/wildcard_imports_2021.rs:238:13 + --> tests/ui/wildcard_imports_2021.rs:241:13 | LL | use crate::super_imports::*; | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `crate::super_imports::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports_2021.rs:247:17 + --> tests/ui/wildcard_imports_2021.rs:250:17 | LL | use super::super::*; | ^^^^^^^^^^^^^^^ help: try: `super::super::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports_2021.rs:256:13 + --> tests/ui/wildcard_imports_2021.rs:259:13 | LL | use super::super::super_imports::*; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `super::super::super_imports::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports_2021.rs:264:13 + --> tests/ui/wildcard_imports_2021.rs:267:13 | LL | use super::*; | ^^^^^^^^ help: try: `super::foofoo` diff --git a/tests/ui/wildcard_imports_2021.edition2021.fixed b/tests/ui/wildcard_imports_2021.edition2021.fixed index 6a9fe007d65..197dd3b94df 100644 --- a/tests/ui/wildcard_imports_2021.edition2021.fixed +++ b/tests/ui/wildcard_imports_2021.edition2021.fixed @@ -198,6 +198,7 @@ mod super_imports { } } + #[cfg(test)] mod test_should_pass { use super::*; @@ -206,6 +207,7 @@ mod super_imports { } } + #[cfg(test)] mod test_should_pass_inside_function { fn with_super_inside_function() { use super::*; @@ -213,6 +215,7 @@ mod super_imports { } } + #[cfg(test)] mod test_should_pass_further_inside { fn insidefoo() {} mod inner { diff --git a/tests/ui/wildcard_imports_2021.edition2021.stderr b/tests/ui/wildcard_imports_2021.edition2021.stderr index 11e0bd37769..66adacd95dc 100644 --- a/tests/ui/wildcard_imports_2021.edition2021.stderr +++ b/tests/ui/wildcard_imports_2021.edition2021.stderr @@ -106,31 +106,31 @@ LL | use super::*; | ^^^^^^^^ help: try: `super::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports_2021.rs:230:17 + --> tests/ui/wildcard_imports_2021.rs:233:17 | LL | use super::*; | ^^^^^^^^ help: try: `super::insidefoo` error: usage of wildcard import - --> tests/ui/wildcard_imports_2021.rs:238:13 + --> tests/ui/wildcard_imports_2021.rs:241:13 | LL | use crate::super_imports::*; | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `crate::super_imports::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports_2021.rs:247:17 + --> tests/ui/wildcard_imports_2021.rs:250:17 | LL | use super::super::*; | ^^^^^^^^^^^^^^^ help: try: `super::super::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports_2021.rs:256:13 + --> tests/ui/wildcard_imports_2021.rs:259:13 | LL | use super::super::super_imports::*; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `super::super::super_imports::foofoo` error: usage of wildcard import - --> tests/ui/wildcard_imports_2021.rs:264:13 + --> tests/ui/wildcard_imports_2021.rs:267:13 | LL | use super::*; | ^^^^^^^^ help: try: `super::foofoo` diff --git a/tests/ui/wildcard_imports_2021.rs b/tests/ui/wildcard_imports_2021.rs index 18ebc0f5127..606ff080e77 100644 --- a/tests/ui/wildcard_imports_2021.rs +++ b/tests/ui/wildcard_imports_2021.rs @@ -199,6 +199,7 @@ fn with_super() { } } + #[cfg(test)] mod test_should_pass { use super::*; @@ -207,6 +208,7 @@ fn with_super() { } } + #[cfg(test)] mod test_should_pass_inside_function { fn with_super_inside_function() { use super::*; @@ -214,6 +216,7 @@ fn with_super_inside_function() { } } + #[cfg(test)] mod test_should_pass_further_inside { fn insidefoo() {} mod inner { From 6d61bdabeaaf806cc9b45a85fc5231fc6cf2743a Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 5 Jul 2024 02:29:23 -0400 Subject: [PATCH 2/2] Use `is_in_test` in more places. --- clippy_lints/src/functions/impl_trait_in_params.rs | 8 ++++---- clippy_lints/src/incompatible_msrv.rs | 4 ++-- clippy_lints/src/methods/unwrap_expect_used.rs | 4 ++-- clippy_lints/src/missing_assert_message.rs | 4 ++-- clippy_lints/src/write.rs | 5 ++--- clippy_utils/src/lib.rs | 10 ---------- 6 files changed, 12 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/functions/impl_trait_in_params.rs b/clippy_lints/src/functions/impl_trait_in_params.rs index 6fb38a0d6dd..cf85c74e688 100644 --- a/clippy_lints/src/functions/impl_trait_in_params.rs +++ b/clippy_lints/src/functions/impl_trait_in_params.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::is_in_test_function; +use clippy_utils::is_in_test; use rustc_hir as hir; use rustc_hir::intravisit::FnKind; @@ -41,7 +41,7 @@ fn report(cx: &LateContext<'_>, param: &GenericParam<'_>, generics: &Generics<'_ pub(super) fn check_fn<'tcx>(cx: &LateContext<'_>, kind: &'tcx FnKind<'_>, body: &'tcx Body<'_>, hir_id: HirId) { if let FnKind::ItemFn(_, generics, _) = kind && cx.tcx.visibility(cx.tcx.hir().body_owner_def_id(body.id())).is_public() - && !is_in_test_function(cx.tcx, hir_id) + && !is_in_test(cx.tcx, hir_id) { for param in generics.params { if param.is_impl_trait() { @@ -59,7 +59,7 @@ pub(super) fn check_impl_item(cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { && of_trait.is_none() && let body = cx.tcx.hir().body(body_id) && cx.tcx.visibility(cx.tcx.hir().body_owner_def_id(body.id())).is_public() - && !is_in_test_function(cx.tcx, impl_item.hir_id()) + && !is_in_test(cx.tcx, impl_item.hir_id()) { for param in impl_item.generics.params { if param.is_impl_trait() { @@ -75,7 +75,7 @@ pub(super) fn check_trait_item(cx: &LateContext<'_>, trait_item: &TraitItem<'_>, && let hir::Node::Item(item) = cx.tcx.parent_hir_node(trait_item.hir_id()) // ^^ (Will always be a trait) && !item.vis_span.is_empty() // Is public - && !is_in_test_function(cx.tcx, trait_item.hir_id()) + && !is_in_test(cx.tcx, trait_item.hir_id()) { for param in trait_item.generics.params { if param.is_impl_trait() { diff --git a/clippy_lints/src/incompatible_msrv.rs b/clippy_lints/src/incompatible_msrv.rs index 35b4481bfee..5c63d48adaf 100644 --- a/clippy_lints/src/incompatible_msrv.rs +++ b/clippy_lints/src/incompatible_msrv.rs @@ -1,6 +1,6 @@ use clippy_config::msrvs::Msrv; use clippy_utils::diagnostics::span_lint; -use clippy_utils::is_in_test_function; +use clippy_utils::is_in_test; use rustc_attr::{StabilityLevel, StableSince}; use rustc_data_structures::fx::FxHashMap; use rustc_hir::{Expr, ExprKind, HirId}; @@ -88,7 +88,7 @@ fn emit_lint_if_under_msrv(&mut self, cx: &LateContext<'_>, def_id: DefId, node: return; } let version = self.get_def_id_version(cx.tcx, def_id); - if self.msrv.meets(version) || is_in_test_function(cx.tcx, node) { + if self.msrv.meets(version) || is_in_test(cx.tcx, node) { return; } if let ExpnKind::AstPass(_) | ExpnKind::Desugaring(_) = span.ctxt().outer_expn_data().kind { diff --git a/clippy_lints/src/methods/unwrap_expect_used.rs b/clippy_lints/src/methods/unwrap_expect_used.rs index 516b8984ad7..5b0bd0f716a 100644 --- a/clippy_lints/src/methods/unwrap_expect_used.rs +++ b/clippy_lints/src/methods/unwrap_expect_used.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::ty::{is_never_like, is_type_diagnostic_item}; -use clippy_utils::{is_in_cfg_test, is_in_test_function, is_lint_allowed}; +use clippy_utils::{is_in_test, is_lint_allowed}; use rustc_hir::Expr; use rustc_lint::{LateContext, Lint}; use rustc_middle::ty; @@ -61,7 +61,7 @@ pub(super) fn check( let method_suffix = if is_err { "_err" } else { "" }; - if allow_unwrap_in_tests && (is_in_test_function(cx.tcx, expr.hir_id) || is_in_cfg_test(cx.tcx, expr.hir_id)) { + if allow_unwrap_in_tests && is_in_test(cx.tcx, expr.hir_id) { return; } diff --git a/clippy_lints/src/missing_assert_message.rs b/clippy_lints/src/missing_assert_message.rs index dd98352da86..935ed48dacc 100644 --- a/clippy_lints/src/missing_assert_message.rs +++ b/clippy_lints/src/missing_assert_message.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::is_in_test; use clippy_utils::macros::{find_assert_args, find_assert_eq_args, root_macro_call_first_node, PanicExpn}; -use clippy_utils::{is_in_cfg_test, is_in_test_function}; use rustc_hir::Expr; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; @@ -62,7 +62,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { }; // This lint would be very noisy in tests, so just ignore if we're in test context - if is_in_test_function(cx.tcx, expr.hir_id) || is_in_cfg_test(cx.tcx, expr.hir_id) { + if is_in_test(cx.tcx, expr.hir_id) { return; } diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 652ce88bd95..96e53b7ef0b 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_then}; +use clippy_utils::is_in_test; use clippy_utils::macros::{format_arg_removal_span, root_macro_call_first_node, FormatArgsStorage, MacroCall}; use clippy_utils::source::{expand_past_previous_comma, snippet_opt}; -use clippy_utils::{is_in_cfg_test, is_in_test_function}; use rustc_ast::token::LitKind; use rustc_ast::{ FormatArgPosition, FormatArgPositionKind, FormatArgs, FormatArgsPiece, FormatOptions, FormatPlaceholder, @@ -297,8 +297,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { .as_ref() .map_or(false, |crate_name| crate_name == "build_script_build"); - let allowed_in_tests = self.allow_print_in_tests - && (is_in_test_function(cx.tcx, expr.hir_id) || is_in_cfg_test(cx.tcx, expr.hir_id)); + let allowed_in_tests = self.allow_print_in_tests && is_in_test(cx.tcx, expr.hir_id); match diag_name { sym::print_macro | sym::println_macro if !allowed_in_tests => { if !is_build_script { diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 45a31fe442b..28d1eeb4e28 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -2625,16 +2625,6 @@ pub fn inherits_cfg(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool { .any(|attr| attr.has_name(sym::cfg)) } -/// Checks whether item either has `test` attribute applied, or -/// is a module with `test` in its name. -/// -/// Note: Add `//@compile-flags: --test` to UI tests with a `#[test]` function -pub fn is_test_module_or_function(tcx: TyCtxt<'_>, item: &Item<'_>) -> bool { - is_in_test_function(tcx, item.hir_id()) - || matches!(item.kind, ItemKind::Mod(..)) - && item.ident.name.as_str().split('_').any(|a| a == "test" || a == "tests") -} - /// Walks up the HIR tree from the given expression in an attempt to find where the value is /// consumed. ///