Skip to content

Allow use super::*; glob imports #5564

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

Merged
merged 10 commits into from
May 9, 2020
3 changes: 2 additions & 1 deletion clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
74 changes: 63 additions & 11 deletions clippy_lints/src/wildcard_imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ 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};
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::BytePos;

declare_clippy_lint! {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -73,18 +78,34 @@ declare_clippy_lint! {
"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 {
Expand All @@ -109,8 +130,7 @@ impl LateLintPass<'_, '_> for WildcardImports {
span = use_path.span.with_hi(item.span.hi() - BytePos(1));
}
(
span,
false,
span, false,
)
};

Expand Down Expand Up @@ -153,4 +173,36 @@ impl LateLintPass<'_, '_> for WildcardImports {
}
}
}

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")
}
2 changes: 1 addition & 1 deletion tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr
Original file line number Diff line number Diff line change
@@ -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

73 changes: 73 additions & 0 deletions tests/ui/wildcard_imports.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
73 changes: 73 additions & 0 deletions tests/ui/wildcard_imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
32 changes: 31 additions & 1 deletion tests/ui/wildcard_imports.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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