Auto merge of #5564 - MrAwesome:master, r=flip1995

Allow `use super::*;` glob imports changelog: Allow super::* glob imports fixes #5554 fixes #5569 A first pass at #5554 - this allows all `use super::*` to pass, which may or may not be desirable. The original issue was around allowing test modules to import their entire parent modules - I'm happy to modify this to do that instead, may just need some guidance on how to implement that (I played around a bit with #[cfg(test)] but from what I can gather, clippy itself isn't in test mode when running, even if the code in question is being checked for the test target).
2020-05-09 18:43:28 +00:00 · 2020-05-09 18:43:28 +00:00 · 43a1777b89
commit 43a1777b89
parent 43d9cdc7b3 b69200b846
7 changed files with 245 additions and 14 deletions
--- 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());
--- a/clippy_lints/src/utils/conf.rs
+++ b/clippy_lints/src/utils/conf.rs
@ -158,6 +158,8 @@ fn $config() -> $Ty {
    (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 {
--- a/clippy_lints/src/wildcard_imports.rs
+++ b/clippy_lints/src/wildcard_imports.rs
@ -3,10 +3,10 @@
 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};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::BytePos;

 declare_clippy_lint! {
@ -43,9 +43,14 @@
    ///
    /// 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.
@ -73,18 +78,34 @@
    "lint `use _::*` statements"
 }

-declare_lint_pass!(WildcardImports => [ENUM_GLOB_USE, WILDCARD_IMPORTS]);
+#[derive(Default)]
+pub struct WildcardImports {
+    warn_on_all: bool,
+    test_modules_deep: u32,
+}
+
+impl WildcardImports {
+    pub fn new(warn_on_all: bool) -> Self {
+        Self {
+            warn_on_all,
+            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 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 !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 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 {
@ -109,8 +130,7 @@ fn check_item(&mut self, cx: &LateContext<'_, '_>, item: &Item<'_>) {
                        span = use_path.span.with_hi(item.span.hi() - BytePos(1));
                    }
                    (
-                        span,
-                        false,
+                        span, false,
                    )
                };

@ -153,4 +173,36 @@ fn check_item(&mut self, cx: &LateContext<'_, '_>, item: &Item<'_>) {
            }
        }
    }
+
+    fn check_item_post(&mut self, _: &LateContext<'_, '_>, item: &Item<'_>) {
+        if is_test_module_or_function(item) {
+            self.test_modules_deep = self.test_modules_deep.saturating_sub(1);
+        }
+    }
+}
+
+impl WildcardImports {
+    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)
+    }
+}
+
+// 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 in tests.
+fn is_super_only_import(segments: &[PathSegment<'_>]) -> bool {
+    segments.len() == 1 && segments[0].ident.as_str() == "super"
+}
+
+fn is_test_module_or_function(item: &Item<'_>) -> bool {
+    matches!(item.kind, ItemKind::Mod(..)) && item.ident.name.as_str().contains("test")
 }
--- 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

--- a/tests/ui/wildcard_imports.fixed
+++ b/tests/ui/wildcard_imports.fixed
@ -155,3 +155,76 @@ fn test_weird_formatting() {
    exported();
    foo();
 }
+
+mod super_imports {
+    fn foofoo() {}
+
+    mod should_be_replaced {
+        use super::foofoo;
+
+        fn with_super() {
+            let _ = foofoo();
+        }
+    }
+
+    mod test_should_pass {
+        use super::*;
+
+        fn with_super() {
+            let _ = foofoo();
+        }
+    }
+
+    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;
+
+        fn with_explicit() {
+            let _ = foofoo();
+        }
+    }
+
+    mod use_double_super_should_be_replaced {
+        mod inner {
+            use super::super::foofoo;
+
+            fn with_double_super() {
+                let _ = foofoo();
+            }
+        }
+    }
+
+    mod use_super_explicit_should_be_replaced {
+        use super::super::super_imports::foofoo;
+
+        fn with_super_explicit() {
+            let _ = foofoo();
+        }
+    }
+}
--- a/tests/ui/wildcard_imports.rs
+++ b/tests/ui/wildcard_imports.rs
@ -156,3 +156,76 @@ fn test_weird_formatting() {
    exported();
    foo();
 }
+
+mod super_imports {
+    fn foofoo() {}
+
+    mod should_be_replaced {
+        use super::*;
+
+        fn with_super() {
+            let _ = foofoo();
+        }
+    }
+
+    mod test_should_pass {
+        use super::*;
+
+        fn with_super() {
+            let _ = foofoo();
+        }
+    }
+
+    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::*;
+
+        fn with_explicit() {
+            let _ = foofoo();
+        }
+    }
+
+    mod use_double_super_should_be_replaced {
+        mod inner {
+            use super::super::*;
+
+            fn with_double_super() {
+                let _ = foofoo();
+            }
+        }
+    }
+
+    mod use_super_explicit_should_be_replaced {
+        use super::super::super_imports::*;
+
+        fn with_super_explicit() {
+            let _ = foofoo();
+        }
+    }
+}
--- a/tests/ui/wildcard_imports.stderr
+++ b/tests/ui/wildcard_imports.stderr
@ -92,5 +92,35 @@ 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:164:13
+   |
+LL |         use super::*;
+   |             ^^^^^^^^ help: try: `super::foofoo`
+
+error: usage of wildcard import
+  --> $DIR/wildcard_imports.rs:199:17
+   |
+LL |             use super::*;
+   |                 ^^^^^^^^ help: try: `super::insidefoo`
+
+error: usage of wildcard import
+  --> $DIR/wildcard_imports.rs:207:13
+   |
+LL |         use super_imports::*;
+   |             ^^^^^^^^^^^^^^^^ help: try: `super_imports::foofoo`
+
+error: usage of wildcard import
+  --> $DIR/wildcard_imports.rs:216:17
+   |
+LL |             use super::super::*;
+   |                 ^^^^^^^^^^^^^^^ help: try: `super::super::foofoo`
+
+error: usage of wildcard import
+  --> $DIR/wildcard_imports.rs:225:13
+   |
+LL |         use super::super::super_imports::*;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `super::super::super_imports::foofoo`
+
+error: aborting due to 20 previous errors