diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ad1a7177eb4..693c2a5f12d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6296,6 +6296,7 @@ Released 2018-09-13 [`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive [`std_instead_of_alloc`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_alloc [`std_instead_of_core`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_core +[`std_wildcard_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_wildcard_imports [`str_split_at_newline`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_split_at_newline [`str_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string [`string_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index c3f8e02b4c06..f24a20f93d3e 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -779,6 +779,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::visibility::PUB_WITHOUT_SHORTHAND_INFO, crate::visibility::PUB_WITH_SHORTHAND_INFO, crate::wildcard_imports::ENUM_GLOB_USE_INFO, + crate::wildcard_imports::STD_WILDCARD_IMPORTS_INFO, crate::wildcard_imports::WILDCARD_IMPORTS_INFO, crate::write::PRINTLN_EMPTY_STRING_INFO, crate::write::PRINT_LITERAL_INFO, diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index d9dda6eadb2a..e2255862bcf2 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -2,15 +2,15 @@ use clippy_config::Conf; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::is_in_test; use clippy_utils::source::{snippet, snippet_with_applicability}; -use rustc_data_structures::fx::FxHashSet; +use rustc_data_structures::fx::{FxHashSet, FxIndexSet}; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; use rustc_hir::{Item, ItemKind, PathSegment, UseKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::ty; use rustc_session::impl_lint_pass; -use rustc_span::BytePos; -use rustc_span::symbol::kw; +use rustc_span::symbol::{STDLIB_STABLE_CRATES, Symbol, kw}; +use rustc_span::{BytePos, Span, sym}; declare_clippy_lint! { /// ### What it does @@ -100,6 +100,45 @@ declare_clippy_lint! { "lint `use _::*` statements" } +declare_clippy_lint! { + /// ### What it does + /// Checks for wildcard imports `use _::*` from the standard library crates. + /// + /// ### Why is this bad? + /// Wildcard imports from the standard library crates can lead to breakages due to name + /// resolution ambiguities when the standard library introduces new items with the same names + /// as locally defined items. + /// + /// ### Exceptions + /// Wildcard imports are allowed from modules whose names contain `prelude`. Many crates + /// (including the standard library) provide modules named "prelude" specifically designed + /// for wildcard imports. + /// + /// ### Example + /// ```no_run + /// use foo::bar; + /// use std::rc::*; + /// + /// # mod foo { pub fn bar() {} } + /// bar(); + /// let _ = Rc::new(5); + /// ``` + /// + /// Use instead: + /// ```no_run + /// use foo::bar; + /// use std::rc::Rc; + /// + /// # mod foo { pub fn bar() {} } + /// bar(); + /// let _ = Rc::new(5); + /// ``` + #[clippy::version = "1.89.0"] + pub STD_WILDCARD_IMPORTS, + pedantic, + "lint `use _::*` from the standard library crates" +} + pub struct WildcardImports { warn_on_all: bool, allowed_segments: FxHashSet, @@ -114,7 +153,7 @@ impl WildcardImports { } } -impl_lint_pass!(WildcardImports => [ENUM_GLOB_USE, WILDCARD_IMPORTS]); +impl_lint_pass!(WildcardImports => [ENUM_GLOB_USE, WILDCARD_IMPORTS, STD_WILDCARD_IMPORTS]); impl LateLintPass<'_> for WildcardImports { fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { @@ -129,51 +168,33 @@ impl LateLintPass<'_> for WildcardImports { return; } if let ItemKind::Use(use_path, UseKind::Glob) = &item.kind - && (self.warn_on_all || !self.check_exceptions(cx, item, use_path.segments)) + && (self.warn_on_all + || !self.check_exceptions(cx, item, use_path.segments) + || (!is_prelude_import(use_path.segments) && is_std_import(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) { let mut applicability = Applicability::MachineApplicable; let import_source_snippet = snippet_with_applicability(cx, use_path.span, "..", &mut applicability); - let (span, braced_glob) = if import_source_snippet.is_empty() { - // This is a `_::{_, *}` import - // In this case `use_path.span` is empty and ends directly in front of the `*`, - // so we need to extend it by one byte. - (use_path.span.with_hi(use_path.span.hi() + BytePos(1)), true) - } else { - // In this case, the `use_path.span` ends right before the `::*`, so we need to - // extend it up to the `*`. Since it is hard to find the `*` in weird - // formatting like `use _ :: *;`, we extend it up to, but not including the - // `;`. In nested imports, like `use _::{inner::*, _}` there is no `;` and we - // can just use the end of the item span - let mut span = use_path.span.with_hi(item.span.hi()); - if snippet(cx, span, "").ends_with(';') { - span = use_path.span.with_hi(item.span.hi() - BytePos(1)); - } - (span, false) - }; - - let mut imports: Vec<_> = used_imports.iter().map(ToString::to_string).collect(); - let imports_string = if imports.len() == 1 { - imports.pop().unwrap() - } else if braced_glob { - imports.join(", ") - } else { - format!("{{{}}}", imports.join(", ")) - }; - let sugg = if braced_glob { - imports_string - } else { - format!("{import_source_snippet}::{imports_string}") - }; + let span = whole_glob_import_span(cx, item, import_source_snippet.is_empty()) + .expect("Not a glob import statement"); + let sugg = sugg_glob_import(&import_source_snippet, used_imports); // Glob imports always have a single resolution. Enums are in the value namespace. let (lint, message) = if let Some(Res::Def(DefKind::Enum, _)) = use_path.res.value_ns { - (ENUM_GLOB_USE, "usage of wildcard import for enum variants") + ( + ENUM_GLOB_USE, + String::from("usage of wildcard import for enum variants"), + ) + } else if is_std_import(use_path.segments) { + ( + STD_WILDCARD_IMPORTS, + format!("usage of wildcard import from `{}`", use_path.segments[0].ident), + ) } else { - (WILDCARD_IMPORTS, "usage of wildcard import") + (WILDCARD_IMPORTS, String::from("usage of wildcard import")) }; span_lint_and_sugg(cx, lint, span, message, "try", sugg, applicability); @@ -201,6 +222,15 @@ fn is_super_only_import(segments: &[PathSegment<'_>]) -> bool { segments.len() == 1 && segments[0].ident.name == kw::Super } +// Checks for the standard libraries, including `test` crate. +fn is_std_import(segments: &[PathSegment<'_>]) -> bool { + let Some(first_segment_name) = segments.first().map(|ps| ps.ident.name) else { + return false; + }; + + STDLIB_STABLE_CRATES.contains(&first_segment_name) || first_segment_name == sym::test +} + // Allow skipping imports containing user configured segments, // i.e. "...::utils::...::*" if user put `allowed-wildcard-imports = ["utils"]` in `Clippy.toml` fn is_allowed_via_config(segments: &[PathSegment<'_>], allowed_segments: &FxHashSet) -> bool { @@ -208,3 +238,46 @@ fn is_allowed_via_config(segments: &[PathSegment<'_>], allowed_segments: &FxHash // a single character in the config thus skipping most of the warnings. segments.iter().any(|seg| allowed_segments.contains(seg.ident.as_str())) } + +// Returns the entire span for a given glob import statement, including the `*` symbol. +fn whole_glob_import_span(cx: &LateContext<'_>, item: &Item<'_>, braced_glob: bool) -> Option { + let ItemKind::Use(use_path, UseKind::Glob) = item.kind else { + return None; + }; + + if braced_glob { + // This is a `_::{_, *}` import + // In this case `use_path.span` is empty and ends directly in front of the `*`, + // so we need to extend it by one byte. + Some(use_path.span.with_hi(use_path.span.hi() + BytePos(1))) + } else { + // In this case, the `use_path.span` ends right before the `::*`, so we need to + // extend it up to the `*`. Since it is hard to find the `*` in weird + // formatting like `use _ :: *;`, we extend it up to, but not including the + // `;`. In nested imports, like `use _::{inner::*, _}` there is no `;` and we + // can just use the end of the item span + let mut span = use_path.span.with_hi(item.span.hi()); + if snippet(cx, span, "").ends_with(';') { + span = use_path.span.with_hi(item.span.hi() - BytePos(1)); + } + Some(span) + } +} + +// Generates a suggestion for a glob import using only the actually used items. +fn sugg_glob_import(import_source_snippet: &str, used_imports: &FxIndexSet) -> String { + let mut imports: Vec<_> = used_imports.iter().map(ToString::to_string).collect(); + let imports_string = if imports.len() == 1 { + imports.pop().unwrap() + } else if import_source_snippet.is_empty() { + imports.join(", ") + } else { + format!("{{{}}}", imports.join(", ")) + }; + + if import_source_snippet.is_empty() { + imports_string + } else { + format!("{import_source_snippet}::{imports_string}") + } +} diff --git a/tests/ui/crashes/ice-11422.fixed b/tests/ui/crashes/ice-11422.fixed index be07a9d8c1f9..73e5072e3630 100644 --- a/tests/ui/crashes/ice-11422.fixed +++ b/tests/ui/crashes/ice-11422.fixed @@ -1,7 +1,7 @@ #![warn(clippy::implied_bounds_in_impls)] use std::fmt::Debug; -use std::ops::*; +use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Sub, SubAssign}; fn r#gen() -> impl PartialOrd + Debug {} //~^ implied_bounds_in_impls diff --git a/tests/ui/crashes/ice-11422.rs b/tests/ui/crashes/ice-11422.rs index 43de882caa11..9f2b38a0e029 100644 --- a/tests/ui/crashes/ice-11422.rs +++ b/tests/ui/crashes/ice-11422.rs @@ -1,7 +1,7 @@ #![warn(clippy::implied_bounds_in_impls)] use std::fmt::Debug; -use std::ops::*; +use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Sub, SubAssign}; fn r#gen() -> impl PartialOrd + PartialEq + Debug {} //~^ implied_bounds_in_impls diff --git a/tests/ui/enum_glob_use.fixed b/tests/ui/enum_glob_use.fixed index 881493f3d5ff..33fdc17eda54 100644 --- a/tests/ui/enum_glob_use.fixed +++ b/tests/ui/enum_glob_use.fixed @@ -1,5 +1,5 @@ #![warn(clippy::enum_glob_use)] -#![allow(unused)] +#![allow(unused, clippy::std_wildcard_imports)] #![warn(unused_imports)] use std::cmp::Ordering::Less; diff --git a/tests/ui/enum_glob_use.rs b/tests/ui/enum_glob_use.rs index a510462ecb2f..b45db229c9dc 100644 --- a/tests/ui/enum_glob_use.rs +++ b/tests/ui/enum_glob_use.rs @@ -1,5 +1,5 @@ #![warn(clippy::enum_glob_use)] -#![allow(unused)] +#![allow(unused, clippy::std_wildcard_imports)] #![warn(unused_imports)] use std::cmp::Ordering::*; diff --git a/tests/ui/explicit_iter_loop.fixed b/tests/ui/explicit_iter_loop.fixed index bffa1c4cf408..44a589ffba47 100644 --- a/tests/ui/explicit_iter_loop.fixed +++ b/tests/ui/explicit_iter_loop.fixed @@ -10,7 +10,7 @@ )] use core::slice; -use std::collections::*; +use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList, VecDeque}; fn main() { let mut vec = vec![1, 2, 3, 4]; diff --git a/tests/ui/explicit_iter_loop.rs b/tests/ui/explicit_iter_loop.rs index 6a5a3dd00ba2..2074fe020091 100644 --- a/tests/ui/explicit_iter_loop.rs +++ b/tests/ui/explicit_iter_loop.rs @@ -10,7 +10,7 @@ )] use core::slice; -use std::collections::*; +use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList, VecDeque}; fn main() { let mut vec = vec![1, 2, 3, 4]; diff --git a/tests/ui/for_kv_map.fixed b/tests/ui/for_kv_map.fixed index 2a68b7443fbf..0d89e20fe58c 100644 --- a/tests/ui/for_kv_map.fixed +++ b/tests/ui/for_kv_map.fixed @@ -1,7 +1,7 @@ #![warn(clippy::for_kv_map)] #![allow(clippy::used_underscore_binding)] -use std::collections::*; +use std::collections::HashMap; use std::rc::Rc; fn main() { diff --git a/tests/ui/for_kv_map.rs b/tests/ui/for_kv_map.rs index 485a97815e3c..36dc773e21c2 100644 --- a/tests/ui/for_kv_map.rs +++ b/tests/ui/for_kv_map.rs @@ -1,7 +1,7 @@ #![warn(clippy::for_kv_map)] #![allow(clippy::used_underscore_binding)] -use std::collections::*; +use std::collections::HashMap; use std::rc::Rc; fn main() { diff --git a/tests/ui/into_iter_on_ref.fixed b/tests/ui/into_iter_on_ref.fixed index 7626569fd422..b7f8bd2cf3c8 100644 --- a/tests/ui/into_iter_on_ref.fixed +++ b/tests/ui/into_iter_on_ref.fixed @@ -2,7 +2,7 @@ #![warn(clippy::into_iter_on_ref)] struct X; -use std::collections::*; +use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList, VecDeque}; fn main() { for _ in &[1, 2, 3] {} diff --git a/tests/ui/into_iter_on_ref.rs b/tests/ui/into_iter_on_ref.rs index 286a62c69ce5..6dacf59b459b 100644 --- a/tests/ui/into_iter_on_ref.rs +++ b/tests/ui/into_iter_on_ref.rs @@ -2,7 +2,7 @@ #![warn(clippy::into_iter_on_ref)] struct X; -use std::collections::*; +use std::collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList, VecDeque}; fn main() { for _ in &[1, 2, 3] {} diff --git a/tests/ui/std_wildcard_imports.fixed b/tests/ui/std_wildcard_imports.fixed new file mode 100644 index 000000000000..c0d38e91a4df --- /dev/null +++ b/tests/ui/std_wildcard_imports.fixed @@ -0,0 +1,53 @@ +//@aux-build:wildcard_imports_helper.rs + +#![feature(test)] +#![warn(clippy::std_wildcard_imports)] + +extern crate alloc; +extern crate proc_macro; +extern crate test; +extern crate wildcard_imports_helper; + +use alloc::boxed::Box; +//~^ std_wildcard_imports +use core::cell::Cell; +//~^ std_wildcard_imports +use proc_macro::is_available; +//~^ std_wildcard_imports +use std::any::type_name; +//~^ std_wildcard_imports +use std::mem::{swap, align_of}; +//~^ std_wildcard_imports +use test::bench::black_box; +//~^ std_wildcard_imports + +use crate::fn_mod::*; + +use wildcard_imports_helper::*; + +use std::io::prelude::*; +use wildcard_imports_helper::extern_prelude::v1::*; +use wildcard_imports_helper::prelude::v1::*; + +mod fn_mod { + pub fn foo() {} +} + +mod super_imports { + use super::*; + + fn test_super() { + fn_mod::foo(); + } +} + +fn main() { + foo(); + + let _ = Box::new(()); // imported from alloc::boxed module + let _ = Cell::new(()); // imported from core::cell module + let _ = is_available(); // imported from proc_macro crate + let _ = type_name::(); // imported from std::any module + let _ = align_of::(); // imported from std::mem module + black_box(()); // imported from test crate +} diff --git a/tests/ui/std_wildcard_imports.rs b/tests/ui/std_wildcard_imports.rs new file mode 100644 index 000000000000..b8e584ecaa9d --- /dev/null +++ b/tests/ui/std_wildcard_imports.rs @@ -0,0 +1,53 @@ +//@aux-build:wildcard_imports_helper.rs + +#![feature(test)] +#![warn(clippy::std_wildcard_imports)] + +extern crate alloc; +extern crate proc_macro; +extern crate test; +extern crate wildcard_imports_helper; + +use alloc::boxed::*; +//~^ std_wildcard_imports +use core::cell::*; +//~^ std_wildcard_imports +use proc_macro::*; +//~^ std_wildcard_imports +use std::any::*; +//~^ std_wildcard_imports +use std::mem::{swap, *}; +//~^ std_wildcard_imports +use test::bench::*; +//~^ std_wildcard_imports + +use crate::fn_mod::*; + +use wildcard_imports_helper::*; + +use std::io::prelude::*; +use wildcard_imports_helper::extern_prelude::v1::*; +use wildcard_imports_helper::prelude::v1::*; + +mod fn_mod { + pub fn foo() {} +} + +mod super_imports { + use super::*; + + fn test_super() { + fn_mod::foo(); + } +} + +fn main() { + foo(); + + let _ = Box::new(()); // imported from alloc::boxed module + let _ = Cell::new(()); // imported from core::cell module + let _ = is_available(); // imported from proc_macro crate + let _ = type_name::(); // imported from std::any module + let _ = align_of::(); // imported from std::mem module + black_box(()); // imported from test crate +} diff --git a/tests/ui/std_wildcard_imports.stderr b/tests/ui/std_wildcard_imports.stderr new file mode 100644 index 000000000000..cd1e2c3a3749 --- /dev/null +++ b/tests/ui/std_wildcard_imports.stderr @@ -0,0 +1,41 @@ +error: usage of wildcard import from `alloc` + --> tests/ui/std_wildcard_imports.rs:11:5 + | +LL | use alloc::boxed::*; + | ^^^^^^^^^^^^^^^ help: try: `alloc::boxed::Box` + | + = note: `-D clippy::std-wildcard-imports` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::std_wildcard_imports)]` + +error: usage of wildcard import from `core` + --> tests/ui/std_wildcard_imports.rs:13:5 + | +LL | use core::cell::*; + | ^^^^^^^^^^^^^ help: try: `core::cell::Cell` + +error: usage of wildcard import from `proc_macro` + --> tests/ui/std_wildcard_imports.rs:15:5 + | +LL | use proc_macro::*; + | ^^^^^^^^^^^^^ help: try: `proc_macro::is_available` + +error: usage of wildcard import from `std` + --> tests/ui/std_wildcard_imports.rs:17:5 + | +LL | use std::any::*; + | ^^^^^^^^^^^ help: try: `std::any::type_name` + +error: usage of wildcard import from `std` + --> tests/ui/std_wildcard_imports.rs:19:22 + | +LL | use std::mem::{swap, *}; + | ^ help: try: `align_of` + +error: usage of wildcard import from `test` + --> tests/ui/std_wildcard_imports.rs:21:5 + | +LL | use test::bench::*; + | ^^^^^^^^^^^^^^ help: try: `test::bench::black_box` + +error: aborting due to 6 previous errors +