From 0c14ea8ed79ebf0b7368659282136e876f247cc9 Mon Sep 17 00:00:00 2001 From: Glenn Hope Date: Sun, 3 May 2020 10:56:25 -0700 Subject: [PATCH 01/10] Allow 'use super::*;' imports --- clippy_lints/src/wildcard_imports.rs | 19 +++++++++++--- tests/ui/wildcard_imports.fixed | 38 ++++++++++++++++++++++++++++ tests/ui/wildcard_imports.rs | 38 ++++++++++++++++++++++++++++ tests/ui/wildcard_imports.stderr | 20 ++++++++++++++- 4 files changed, 111 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index f3038861cee2..70ad9a60a02c 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -3,7 +3,7 @@ use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::{ def::{DefKind, Res}, - Item, ItemKind, UseKind, + Item, ItemKind, PathSegment, UseKind, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -83,8 +83,8 @@ impl LateLintPass<'_, '_> for WildcardImports { if_chain! { if !in_macro(item.span); if let ItemKind::Use(use_path, UseKind::Glob) = &item.kind; - // don't lint prelude glob imports - if !use_path.segments.iter().last().map_or(false, |ps| ps.ident.as_str() == "prelude"); + if !is_prelude_import(use_path.segments); + if !is_super_only_import_in_test(use_path.segments); let used_imports = cx.tcx.names_imported_by_glob_use(item.hir_id.owner); if !used_imports.is_empty(); // Already handled by `unused_imports` then { @@ -154,3 +154,16 @@ impl LateLintPass<'_, '_> for WildcardImports { } } } + +// Allow "...prelude::*" imports. +// Many crates have a prelude, and it is imported as a glob by design. +fn is_prelude_import(segments: &[PathSegment<'_>]) -> bool { + segments.iter().last().map_or(false, |ps| ps.ident.as_str() == "prelude") +} + +// Allow "super::*" imports. +// This is intended primarily to ease the process of writing unit tests. +fn is_super_only_import_in_test(segments: &[PathSegment<'_>]) -> bool { + segments.iter().len() == 1 && + segments.first().map_or(false, |ps| ps.ident.as_str() == "super") +} diff --git a/tests/ui/wildcard_imports.fixed b/tests/ui/wildcard_imports.fixed index ed6cc00ef048..003f11009a06 100644 --- a/tests/ui/wildcard_imports.fixed +++ b/tests/ui/wildcard_imports.fixed @@ -155,3 +155,41 @@ fn test_weird_formatting() { exported(); foo(); } + +mod test_super_imports { + fn foofoo() {} + + mod use_super { + use super::*; + + fn with_super() { + let _ = foofoo(); + } + } + + mod use_explicit { + use test_super_imports::foofoo; + + fn with_explicit() { + let _ = foofoo(); + } + } + + mod use_double_super { + mod inner { + use super::super::foofoo; + + fn with_double_super() { + let _ = foofoo(); + } + } + } + + mod use_super_explicit { + use super::super::test_super_imports::foofoo; + + fn with_super_explicit() { + let _ = foofoo(); + } + } +} diff --git a/tests/ui/wildcard_imports.rs b/tests/ui/wildcard_imports.rs index c6d6efaece81..7bd57c7965a6 100644 --- a/tests/ui/wildcard_imports.rs +++ b/tests/ui/wildcard_imports.rs @@ -156,3 +156,41 @@ fn test_weird_formatting() { exported(); foo(); } + +mod test_super_imports { + fn foofoo() {} + + mod use_super { + use super::*; + + fn with_super() { + let _ = foofoo(); + } + } + + mod use_explicit { + use test_super_imports::*; + + fn with_explicit() { + let _ = foofoo(); + } + } + + mod use_double_super { + mod inner { + use super::super::*; + + fn with_double_super() { + let _ = foofoo(); + } + } + } + + mod use_super_explicit { + use super::super::test_super_imports::*; + + fn with_super_explicit() { + let _ = foofoo(); + } + } +} diff --git a/tests/ui/wildcard_imports.stderr b/tests/ui/wildcard_imports.stderr index 050e4c6304f0..858dc28797fa 100644 --- a/tests/ui/wildcard_imports.stderr +++ b/tests/ui/wildcard_imports.stderr @@ -92,5 +92,23 @@ LL | use crate:: fn_mod:: LL | | *; | |_________^ help: try: `crate:: fn_mod::foo` -error: aborting due to 15 previous errors +error: usage of wildcard import + --> $DIR/wildcard_imports.rs:172:13 + | +LL | use test_super_imports::*; + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `test_super_imports::foofoo` + +error: usage of wildcard import + --> $DIR/wildcard_imports.rs:181:17 + | +LL | use super::super::*; + | ^^^^^^^^^^^^^^^ help: try: `super::super::foofoo` + +error: usage of wildcard import + --> $DIR/wildcard_imports.rs:190:13 + | +LL | use super::super::test_super_imports::*; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `super::super::test_super_imports::foofoo` + +error: aborting due to 18 previous errors From bdc75dbb7bbdc379b1f8cc346151fac4e63d7deb Mon Sep 17 00:00:00 2001 From: Glenn Hope Date: Sun, 3 May 2020 11:18:10 -0700 Subject: [PATCH 02/10] Run `cargo dev fmt` --- clippy_lints/src/wildcard_imports.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index 70ad9a60a02c..e7400642b078 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -158,12 +158,14 @@ impl LateLintPass<'_, '_> for WildcardImports { // Allow "...prelude::*" imports. // Many crates have a prelude, and it is imported as a glob by design. fn is_prelude_import(segments: &[PathSegment<'_>]) -> bool { - segments.iter().last().map_or(false, |ps| ps.ident.as_str() == "prelude") + segments + .iter() + .last() + .map_or(false, |ps| ps.ident.as_str() == "prelude") } // Allow "super::*" imports. // This is intended primarily to ease the process of writing unit tests. fn is_super_only_import_in_test(segments: &[PathSegment<'_>]) -> bool { - segments.iter().len() == 1 && - segments.first().map_or(false, |ps| ps.ident.as_str() == "super") + segments.iter().len() == 1 && segments.first().map_or(false, |ps| ps.ident.as_str() == "super") } From 56f4e1c3a8be54c1c80de366618e83d8d7a6e9eb Mon Sep 17 00:00:00 2001 From: Glenn Hope Date: Thu, 7 May 2020 08:06:30 -0700 Subject: [PATCH 03/10] Check if the parent module name contains "test" --- clippy_lints/src/wildcard_imports.rs | 19 ++++++++++++------- tests/ui/wildcard_imports.fixed | 16 ++++++++++++---- tests/ui/wildcard_imports.rs | 16 ++++++++++++---- tests/ui/wildcard_imports.stderr | 14 ++++++++++---- 4 files changed, 46 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index e7400642b078..a22d0e6775de 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -84,7 +84,7 @@ impl LateLintPass<'_, '_> for WildcardImports { if !in_macro(item.span); if let ItemKind::Use(use_path, UseKind::Glob) = &item.kind; if !is_prelude_import(use_path.segments); - if !is_super_only_import_in_test(use_path.segments); + if !(is_super_only_import(use_path.segments) && is_in_test_module(cx, item)); let used_imports = cx.tcx.names_imported_by_glob_use(item.hir_id.owner); if !used_imports.is_empty(); // Already handled by `unused_imports` then { @@ -109,8 +109,7 @@ impl LateLintPass<'_, '_> for WildcardImports { span = use_path.span.with_hi(item.span.hi() - BytePos(1)); } ( - span, - false, + span, false, ) }; @@ -164,8 +163,14 @@ fn is_prelude_import(segments: &[PathSegment<'_>]) -> bool { .map_or(false, |ps| ps.ident.as_str() == "prelude") } -// Allow "super::*" imports. -// This is intended primarily to ease the process of writing unit tests. -fn is_super_only_import_in_test(segments: &[PathSegment<'_>]) -> bool { - segments.iter().len() == 1 && segments.first().map_or(false, |ps| ps.ident.as_str() == "super") +// Allow "super::*" imports in tests. +fn is_super_only_import(segments: &[PathSegment<'_>]) -> bool { + segments.len() == 1 && segments[0].ident.as_str() == "super" +} + +fn is_in_test_module(cx: &LateContext<'_, '_>, item: &Item<'_>) -> bool { + let parent = cx.tcx.hir().get_parent_node(item.hir_id); + let parent_item = cx.tcx.hir().expect_item(parent); + let parent_name = parent_item.ident.name.as_str(); + parent_name.contains("test") } diff --git a/tests/ui/wildcard_imports.fixed b/tests/ui/wildcard_imports.fixed index 003f11009a06..1c5c01f65d15 100644 --- a/tests/ui/wildcard_imports.fixed +++ b/tests/ui/wildcard_imports.fixed @@ -159,7 +159,15 @@ fn test_weird_formatting() { mod test_super_imports { fn foofoo() {} - mod use_super { + mod use_super_should_be_replaced { + use super::foofoo; + + fn with_super() { + let _ = foofoo(); + } + } + + mod use_super_in_test_should_pass { use super::*; fn with_super() { @@ -167,7 +175,7 @@ mod test_super_imports { } } - mod use_explicit { + mod use_explicit_should_be_replaced { use test_super_imports::foofoo; fn with_explicit() { @@ -175,7 +183,7 @@ mod test_super_imports { } } - mod use_double_super { + mod use_double_super_should_be_replaced { mod inner { use super::super::foofoo; @@ -185,7 +193,7 @@ mod test_super_imports { } } - mod use_super_explicit { + mod use_super_explicit_should_be_replaced { use super::super::test_super_imports::foofoo; fn with_super_explicit() { diff --git a/tests/ui/wildcard_imports.rs b/tests/ui/wildcard_imports.rs index 7bd57c7965a6..f783149ef93e 100644 --- a/tests/ui/wildcard_imports.rs +++ b/tests/ui/wildcard_imports.rs @@ -160,7 +160,7 @@ fn test_weird_formatting() { mod test_super_imports { fn foofoo() {} - mod use_super { + mod use_super_should_be_replaced { use super::*; fn with_super() { @@ -168,7 +168,15 @@ mod test_super_imports { } } - mod use_explicit { + mod use_super_in_test_should_pass { + use super::*; + + fn with_super() { + let _ = foofoo(); + } + } + + mod use_explicit_should_be_replaced { use test_super_imports::*; fn with_explicit() { @@ -176,7 +184,7 @@ mod test_super_imports { } } - mod use_double_super { + mod use_double_super_should_be_replaced { mod inner { use super::super::*; @@ -186,7 +194,7 @@ mod test_super_imports { } } - mod use_super_explicit { + mod use_super_explicit_should_be_replaced { use super::super::test_super_imports::*; fn with_super_explicit() { diff --git a/tests/ui/wildcard_imports.stderr b/tests/ui/wildcard_imports.stderr index 858dc28797fa..649d550a88d1 100644 --- a/tests/ui/wildcard_imports.stderr +++ b/tests/ui/wildcard_imports.stderr @@ -93,22 +93,28 @@ LL | | *; | |_________^ help: try: `crate:: fn_mod::foo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:172:13 + --> $DIR/wildcard_imports.rs:164:13 + | +LL | use super::*; + | ^^^^^^^^ help: try: `super::foofoo` + +error: usage of wildcard import + --> $DIR/wildcard_imports.rs:180:13 | LL | use test_super_imports::*; | ^^^^^^^^^^^^^^^^^^^^^ help: try: `test_super_imports::foofoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:181:17 + --> $DIR/wildcard_imports.rs:189:17 | LL | use super::super::*; | ^^^^^^^^^^^^^^^ help: try: `super::super::foofoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:190:13 + --> $DIR/wildcard_imports.rs:198:13 | LL | use super::super::test_super_imports::*; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `super::super::test_super_imports::foofoo` -error: aborting due to 18 previous errors +error: aborting due to 19 previous errors From ad92486d52fdede5c712dd27c868c942d8240704 Mon Sep 17 00:00:00 2001 From: Glenn Hope Date: Thu, 7 May 2020 14:41:54 -0700 Subject: [PATCH 04/10] Add check for "test" in parent name. Include flag for disabling wildcard import exceptions --- clippy_lints/src/lib.rs | 3 +- clippy_lints/src/utils/conf.rs | 2 + clippy_lints/src/wildcard_imports.rs | 48 +++++++++++++++---- .../toml_unknown_key/conf_unknown_key.stderr | 2 +- tests/ui/wildcard_imports.fixed | 17 +++++-- tests/ui/wildcard_imports.rs | 17 +++++-- tests/ui/wildcard_imports.stderr | 14 +++--- 7 files changed, 75 insertions(+), 28 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fb2e9932b8e4..4b67c84e38ed 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1058,7 +1058,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: let max_struct_bools = conf.max_struct_bools; store.register_early_pass(move || box excessive_bools::ExcessiveBools::new(max_struct_bools, max_fn_params_bools)); store.register_early_pass(|| box option_env_unwrap::OptionEnvUnwrap); - store.register_late_pass(|| box wildcard_imports::WildcardImports); + let warn_on_all_wildcard_imports = conf.warn_on_all_wildcard_imports; + store.register_late_pass(move || box wildcard_imports::WildcardImports::new(warn_on_all_wildcard_imports)); store.register_early_pass(|| box macro_use::MacroUseImports); store.register_late_pass(|| box verbose_file_reads::VerboseFileReads); store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default()); diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 4b81ff33495c..57b9eafd14db 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -158,6 +158,8 @@ define_Conf! { (max_struct_bools, "max_struct_bools": u64, 3), /// Lint: FN_PARAMS_EXCESSIVE_BOOLS. The maximum number of bools function parameters can have (max_fn_params_bools, "max_fn_params_bools": u64, 3), + /// Lint: WILDCARD_IMPORTS. Whether to allow certain wildcard imports (prelude, super in tests). + (warn_on_all_wildcard_imports, "warn_on_all_wildcard_imports": bool, false), } impl Default for Conf { diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index a22d0e6775de..43d0d1b9e96c 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -6,7 +6,7 @@ use rustc_hir::{ Item, ItemKind, PathSegment, UseKind, }; use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::BytePos; declare_clippy_lint! { @@ -73,18 +73,38 @@ declare_clippy_lint! { "lint `use _::*` statements" } -declare_lint_pass!(WildcardImports => [ENUM_GLOB_USE, WILDCARD_IMPORTS]); +#[derive(Default)] +pub struct WildcardImports { + warn_on_all: bool, + is_test_module: bool, + test_modules_deep: u32, +} + +impl WildcardImports { + pub fn new(warn_on_all: bool) -> Self { + Self { + warn_on_all, + is_test_module: false, + test_modules_deep: 0, + } + } +} + +impl_lint_pass!(WildcardImports => [ENUM_GLOB_USE, WILDCARD_IMPORTS]); impl LateLintPass<'_, '_> for WildcardImports { fn check_item(&mut self, cx: &LateContext<'_, '_>, item: &Item<'_>) { if item.vis.node.is_pub() || item.vis.node.is_pub_restricted() { return; } + if is_test_module(item) { + self.is_test_module = true; + self.test_modules_deep += 1; + } if_chain! { if !in_macro(item.span); if let ItemKind::Use(use_path, UseKind::Glob) = &item.kind; - if !is_prelude_import(use_path.segments); - if !(is_super_only_import(use_path.segments) && is_in_test_module(cx, item)); + if self.warn_on_all || !self.check_exceptions(use_path.segments); let used_imports = cx.tcx.names_imported_by_glob_use(item.hir_id.owner); if !used_imports.is_empty(); // Already handled by `unused_imports` then { @@ -152,6 +172,19 @@ impl LateLintPass<'_, '_> for WildcardImports { } } } + + fn check_item_post(&mut self, _: &LateContext<'_, '_>, _: &Item<'_>) { + if self.is_test_module { + self.is_test_module = false; + self.test_modules_deep -= 1; + } + } +} + +impl WildcardImports { + fn check_exceptions(&self, segments: &[PathSegment<'_>]) -> bool { + is_prelude_import(segments) || (is_super_only_import(segments) && self.test_modules_deep > 0) + } } // Allow "...prelude::*" imports. @@ -168,9 +201,6 @@ fn is_super_only_import(segments: &[PathSegment<'_>]) -> bool { segments.len() == 1 && segments[0].ident.as_str() == "super" } -fn is_in_test_module(cx: &LateContext<'_, '_>, item: &Item<'_>) -> bool { - let parent = cx.tcx.hir().get_parent_node(item.hir_id); - let parent_item = cx.tcx.hir().expect_item(parent); - let parent_name = parent_item.ident.name.as_str(); - parent_name.contains("test") +fn is_test_module(item: &Item<'_>) -> bool { + item.ident.name.as_str().contains("test") } diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 18f5d994ba8a..53970af41079 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -1,4 +1,4 @@ -error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-struct-bools`, `max-fn-params-bools`, `third-party` at line 5 column 1 +error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-struct-bools`, `max-fn-params-bools`, `warn-on-all-wildcard-imports`, `third-party` at line 5 column 1 error: aborting due to previous error diff --git a/tests/ui/wildcard_imports.fixed b/tests/ui/wildcard_imports.fixed index 1c5c01f65d15..98bf6acfe55f 100644 --- a/tests/ui/wildcard_imports.fixed +++ b/tests/ui/wildcard_imports.fixed @@ -156,10 +156,10 @@ fn test_weird_formatting() { foo(); } -mod test_super_imports { +mod super_imports { fn foofoo() {} - mod use_super_should_be_replaced { + mod should_be_replaced { use super::foofoo; fn with_super() { @@ -167,7 +167,7 @@ mod test_super_imports { } } - mod use_super_in_test_should_pass { + mod test_should_pass { use super::*; fn with_super() { @@ -175,8 +175,15 @@ mod test_super_imports { } } + mod inner { + fn test_should_pass() { + use super::*; + let _ = foofoo(); + } + } + mod use_explicit_should_be_replaced { - use test_super_imports::foofoo; + use super_imports::foofoo; fn with_explicit() { let _ = foofoo(); @@ -194,7 +201,7 @@ mod test_super_imports { } mod use_super_explicit_should_be_replaced { - use super::super::test_super_imports::foofoo; + use super::super::super_imports::foofoo; fn with_super_explicit() { let _ = foofoo(); diff --git a/tests/ui/wildcard_imports.rs b/tests/ui/wildcard_imports.rs index f783149ef93e..9275c5a09286 100644 --- a/tests/ui/wildcard_imports.rs +++ b/tests/ui/wildcard_imports.rs @@ -157,10 +157,10 @@ fn test_weird_formatting() { foo(); } -mod test_super_imports { +mod super_imports { fn foofoo() {} - mod use_super_should_be_replaced { + mod should_be_replaced { use super::*; fn with_super() { @@ -168,7 +168,7 @@ mod test_super_imports { } } - mod use_super_in_test_should_pass { + mod test_should_pass { use super::*; fn with_super() { @@ -176,8 +176,15 @@ mod test_super_imports { } } + mod inner { + fn test_should_pass() { + use super::*; + let _ = foofoo(); + } + } + mod use_explicit_should_be_replaced { - use test_super_imports::*; + use super_imports::*; fn with_explicit() { let _ = foofoo(); @@ -195,7 +202,7 @@ mod test_super_imports { } mod use_super_explicit_should_be_replaced { - use super::super::test_super_imports::*; + use super::super::super_imports::*; fn with_super_explicit() { let _ = foofoo(); diff --git a/tests/ui/wildcard_imports.stderr b/tests/ui/wildcard_imports.stderr index 649d550a88d1..bd000ce81616 100644 --- a/tests/ui/wildcard_imports.stderr +++ b/tests/ui/wildcard_imports.stderr @@ -99,22 +99,22 @@ LL | use super::*; | ^^^^^^^^ help: try: `super::foofoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:180:13 + --> $DIR/wildcard_imports.rs:187:13 | -LL | use test_super_imports::*; - | ^^^^^^^^^^^^^^^^^^^^^ help: try: `test_super_imports::foofoo` +LL | use super_imports::*; + | ^^^^^^^^^^^^^^^^ help: try: `super_imports::foofoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:189:17 + --> $DIR/wildcard_imports.rs:196:17 | LL | use super::super::*; | ^^^^^^^^^^^^^^^ help: try: `super::super::foofoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:198:13 + --> $DIR/wildcard_imports.rs:205:13 | -LL | use super::super::test_super_imports::*; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `super::super::test_super_imports::foofoo` +LL | use super::super::super_imports::*; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `super::super::super_imports::foofoo` error: aborting due to 19 previous errors From a42a2bdac2a6c881f85ebdbce66e84d977c74cfa Mon Sep 17 00:00:00 2001 From: Glenn Hope Date: Thu, 7 May 2020 14:48:27 -0700 Subject: [PATCH 05/10] Also have flag disable macro check --- clippy_lints/src/wildcard_imports.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index 43d0d1b9e96c..843ddda03569 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -102,7 +102,7 @@ impl LateLintPass<'_, '_> for WildcardImports { self.test_modules_deep += 1; } if_chain! { - if !in_macro(item.span); + if self.warn_on_all || !in_macro(item.span); if let ItemKind::Use(use_path, UseKind::Glob) = &item.kind; if self.warn_on_all || !self.check_exceptions(use_path.segments); let used_imports = cx.tcx.names_imported_by_glob_use(item.hir_id.owner); From 152cdcb45be7a8f0f24dbcd4177e0858d94516b6 Mon Sep 17 00:00:00 2001 From: Glenn Hope Date: Fri, 8 May 2020 18:22:27 -0700 Subject: [PATCH 06/10] Remove unnecessary field, check for Mod/Fn ItemKind --- clippy_lints/src/wildcard_imports.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index 843ddda03569..48405a00d554 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -76,7 +76,6 @@ declare_clippy_lint! { #[derive(Default)] pub struct WildcardImports { warn_on_all: bool, - is_test_module: bool, test_modules_deep: u32, } @@ -84,7 +83,6 @@ impl WildcardImports { pub fn new(warn_on_all: bool) -> Self { Self { warn_on_all, - is_test_module: false, test_modules_deep: 0, } } @@ -97,8 +95,7 @@ impl LateLintPass<'_, '_> for WildcardImports { if item.vis.node.is_pub() || item.vis.node.is_pub_restricted() { return; } - if is_test_module(item) { - self.is_test_module = true; + if is_test_module_or_function(item) { self.test_modules_deep += 1; } if_chain! { @@ -173,9 +170,8 @@ impl LateLintPass<'_, '_> for WildcardImports { } } - fn check_item_post(&mut self, _: &LateContext<'_, '_>, _: &Item<'_>) { - if self.is_test_module { - self.is_test_module = false; + fn check_item_post(&mut self, _: &LateContext<'_, '_>, item: &Item<'_>) { + if is_test_module_or_function(item) { self.test_modules_deep -= 1; } } @@ -201,6 +197,6 @@ fn is_super_only_import(segments: &[PathSegment<'_>]) -> bool { segments.len() == 1 && segments[0].ident.as_str() == "super" } -fn is_test_module(item: &Item<'_>) -> bool { - item.ident.name.as_str().contains("test") +fn is_test_module_or_function(item: &Item<'_>) -> bool { + matches!(item.kind, ItemKind::Fn(..) | ItemKind::Mod(..)) && item.ident.name.as_str().contains("test") } From 4db6abcd50eb07a7fa8349a059f80792b7b19a2e Mon Sep 17 00:00:00 2001 From: Glenn Hope Date: Sat, 9 May 2020 08:04:07 -0700 Subject: [PATCH 07/10] Remove check for Fn, reflect this in test cases, make test cases more robust/explicit --- clippy_lints/src/wildcard_imports.rs | 13 +++++++++---- tests/ui/wildcard_imports.fixed | 25 +++++++++++++++++++++++-- tests/ui/wildcard_imports.rs | 24 ++++++++++++++++++++++-- tests/ui/wildcard_imports.stderr | 14 ++++++++++---- 4 files changed, 64 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index 48405a00d554..e12a6659ab5b 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -43,9 +43,14 @@ declare_clippy_lint! { /// /// This can lead to confusing error messages at best and to unexpected behavior at worst. /// - /// Note that this will not warn about wildcard imports from modules named `prelude`; many - /// crates (including the standard library) provide modules named "prelude" specifically - /// designed for wildcard import. + /// **Exceptions:** + /// + /// Wildcard imports are allowed from modules named `prelude`. Many crates (including the standard library) + /// provide modules named "prelude" specifically designed for wildcard import. + /// + /// `use super::*` is allowed in test modules. This is defined as any module with "test" in the name. + /// + /// These exceptions can be disabled using the `warn-on-all-wildcard-imports` configuration flag. /// /// **Known problems:** If macros are imported through the wildcard, this macro is not included /// by the suggestion and has to be added by hand. @@ -198,5 +203,5 @@ fn is_super_only_import(segments: &[PathSegment<'_>]) -> bool { } fn is_test_module_or_function(item: &Item<'_>) -> bool { - matches!(item.kind, ItemKind::Fn(..) | ItemKind::Mod(..)) && item.ident.name.as_str().contains("test") + matches!(item.kind, ItemKind::Mod(..)) && item.ident.name.as_str().contains("test") } diff --git a/tests/ui/wildcard_imports.fixed b/tests/ui/wildcard_imports.fixed index 98bf6acfe55f..b47c8f23e240 100644 --- a/tests/ui/wildcard_imports.fixed +++ b/tests/ui/wildcard_imports.fixed @@ -175,13 +175,34 @@ mod super_imports { } } - mod inner { - fn test_should_pass() { + mod test_should_pass_inside_function { + fn with_super_inside_function() { use super::*; let _ = foofoo(); } } + mod test_should_pass_further_inside { + fn insidefoo() {} + mod inner { + use super::*; + fn with_super() { + let _ = insidefoo(); + } + } + } + + mod should_be_replaced_futher_inside { + fn insidefoo() {} + mod inner { + use super::insidefoo; + fn with_super() { + let _ = insidefoo(); + } + } + } + + mod use_explicit_should_be_replaced { use super_imports::foofoo; diff --git a/tests/ui/wildcard_imports.rs b/tests/ui/wildcard_imports.rs index 9275c5a09286..3ad1a29aebad 100644 --- a/tests/ui/wildcard_imports.rs +++ b/tests/ui/wildcard_imports.rs @@ -176,13 +176,33 @@ mod super_imports { } } - mod inner { - fn test_should_pass() { + mod test_should_pass_inside_function { + fn with_super_inside_function() { use super::*; let _ = foofoo(); } } + mod test_should_pass_further_inside { + fn insidefoo() {} + mod inner { + use super::*; + fn with_super() { + let _ = insidefoo(); + } + } + } + + mod should_be_replaced_futher_inside { + fn insidefoo() {} + mod inner { + use super::*; + fn with_super() { + let _ = insidefoo(); + } + } + } + mod use_explicit_should_be_replaced { use super_imports::*; diff --git a/tests/ui/wildcard_imports.stderr b/tests/ui/wildcard_imports.stderr index bd000ce81616..de07bd1d69ba 100644 --- a/tests/ui/wildcard_imports.stderr +++ b/tests/ui/wildcard_imports.stderr @@ -99,22 +99,28 @@ LL | use super::*; | ^^^^^^^^ help: try: `super::foofoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:187:13 + --> $DIR/wildcard_imports.rs:199:17 + | +LL | use super::*; + | ^^^^^^^^ help: try: `super::insidefoo` + +error: usage of wildcard import + --> $DIR/wildcard_imports.rs:208:13 | LL | use super_imports::*; | ^^^^^^^^^^^^^^^^ help: try: `super_imports::foofoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:196:17 + --> $DIR/wildcard_imports.rs:217:17 | LL | use super::super::*; | ^^^^^^^^^^^^^^^ help: try: `super::super::foofoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:205:13 + --> $DIR/wildcard_imports.rs:226:13 | LL | use super::super::super_imports::*; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `super::super::super_imports::foofoo` -error: aborting due to 19 previous errors +error: aborting due to 20 previous errors From a339766136a183327faaf13b987be30b2940872e Mon Sep 17 00:00:00 2001 From: Glenn Hope Date: Sat, 9 May 2020 08:33:35 -0700 Subject: [PATCH 08/10] Fix test from auto-formatter fix --- tests/ui/wildcard_imports.fixed | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/ui/wildcard_imports.fixed b/tests/ui/wildcard_imports.fixed index b47c8f23e240..67423e6ec1d1 100644 --- a/tests/ui/wildcard_imports.fixed +++ b/tests/ui/wildcard_imports.fixed @@ -202,7 +202,6 @@ mod super_imports { } } - mod use_explicit_should_be_replaced { use super_imports::foofoo; From 0ba61c612ee873314d252ca1f747c14a2f0161ba Mon Sep 17 00:00:00 2001 From: Glenn Hope Date: Sat, 9 May 2020 10:14:29 -0700 Subject: [PATCH 09/10] Check is_macro inside check_exceptions, update references to fix test --- clippy_lints/src/wildcard_imports.rs | 13 +++++++------ tests/ui/wildcard_imports.stderr | 6 +++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index e12a6659ab5b..2c4e24780e7d 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -101,12 +101,11 @@ impl LateLintPass<'_, '_> for WildcardImports { return; } if is_test_module_or_function(item) { - self.test_modules_deep += 1; + self.test_modules_deep = self.test_modules_deep.saturating_add(1); } if_chain! { - if self.warn_on_all || !in_macro(item.span); if let ItemKind::Use(use_path, UseKind::Glob) = &item.kind; - if self.warn_on_all || !self.check_exceptions(use_path.segments); + if self.warn_on_all || !self.check_exceptions(item, use_path.segments); let used_imports = cx.tcx.names_imported_by_glob_use(item.hir_id.owner); if !used_imports.is_empty(); // Already handled by `unused_imports` then { @@ -177,14 +176,16 @@ impl LateLintPass<'_, '_> for WildcardImports { fn check_item_post(&mut self, _: &LateContext<'_, '_>, item: &Item<'_>) { if is_test_module_or_function(item) { - self.test_modules_deep -= 1; + self.test_modules_deep = self.test_modules_deep.saturating_sub(1); } } } impl WildcardImports { - fn check_exceptions(&self, segments: &[PathSegment<'_>]) -> bool { - is_prelude_import(segments) || (is_super_only_import(segments) && self.test_modules_deep > 0) + fn check_exceptions(&self, item: &Item<'_>, segments: &[PathSegment<'_>]) -> bool { + in_macro(item.span) + || is_prelude_import(segments) + || (is_super_only_import(segments) && self.test_modules_deep > 0) } } diff --git a/tests/ui/wildcard_imports.stderr b/tests/ui/wildcard_imports.stderr index de07bd1d69ba..fab43b738eb4 100644 --- a/tests/ui/wildcard_imports.stderr +++ b/tests/ui/wildcard_imports.stderr @@ -105,19 +105,19 @@ LL | use super::*; | ^^^^^^^^ help: try: `super::insidefoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:208:13 + --> $DIR/wildcard_imports.rs:207:13 | LL | use super_imports::*; | ^^^^^^^^^^^^^^^^ help: try: `super_imports::foofoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:217:17 + --> $DIR/wildcard_imports.rs:216:17 | LL | use super::super::*; | ^^^^^^^^^^^^^^^ help: try: `super::super::foofoo` error: usage of wildcard import - --> $DIR/wildcard_imports.rs:226:13 + --> $DIR/wildcard_imports.rs:225:13 | LL | use super::super::super_imports::*; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `super::super::super_imports::foofoo` From b69200b8468434bc3f5b9ef8468733e5d40f4e01 Mon Sep 17 00:00:00 2001 From: Glenn Hope Date: Sat, 9 May 2020 10:16:47 -0700 Subject: [PATCH 10/10] Move is_test_module check to top of function --- clippy_lints/src/wildcard_imports.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index 2c4e24780e7d..32d9a45c37d7 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -97,12 +97,12 @@ impl_lint_pass!(WildcardImports => [ENUM_GLOB_USE, WILDCARD_IMPORTS]); impl LateLintPass<'_, '_> for WildcardImports { fn check_item(&mut self, cx: &LateContext<'_, '_>, item: &Item<'_>) { - if item.vis.node.is_pub() || item.vis.node.is_pub_restricted() { - return; - } if is_test_module_or_function(item) { self.test_modules_deep = self.test_modules_deep.saturating_add(1); } + if item.vis.node.is_pub() || item.vis.node.is_pub_restricted() { + return; + } if_chain! { if let ItemKind::Use(use_path, UseKind::Glob) = &item.kind; if self.warn_on_all || !self.check_exceptions(item, use_path.segments);