From 1b2b7e629155f7bc11290147ff042425c618de6a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 30 May 2025 01:37:48 +1000 Subject: [PATCH] Overhaul `UsePath`. `UsePath` contains a `SmallVec<[Res; 3]>`. This holds up to three `Res` results, one per namespace (type, value, or macro). `lower_import_res` takes a `PerNS>>` result and lowers it into the `SmallVec`. This is pretty weird. The input `PerNS` makes it clear which `Res` belongs to which namespace, but the `SmallVec` throws that information away. And code that operates on the `SmallVec` tends to use iteration (or even just grabbing the first entry!) without knowing which namespace the `Res` belongs to. Even weirder! Also, `SmallVec` is an overly flexible type to use here, because it can contain any number of elements (even though it's optimized for 3 in this case). This commit changes `UsePath` so it also contains a `PerNS>>`. This type preserves more information and is more self-documenting. The commit also changes a lot of the use sites to access the result for a particular namespace. E.g. if you're looking up a trait, it will be in the `Res` for the type namespace if it's present; it's silly to look in the `Res` for the value namespace or macro namespace. Overall I find the new code much easier to understand. However, some use sites still iterate. These now use `present_items` because that filters out the `None` results. Also, `redundant_pub_crate.rs` gets a bigger change. A `UseKind:ListStem` item gets no `Res` results, which means the old `all` call in `is_not_macro_export` would succeed (because `all` succeeds on an empty iterator) and the `ListStem` would be ignored. This is what we want, but was more by luck than design. The new code detects `ListStem` explicitly. The commit generalizes the name of that function accordingly. Finally, the commit also removes the `use_path` arena, because `PerNS>` impls `Copy` (unlike `SmallVec`) and it can be allocated in the arena shared by all `Copy` types. --- clippy_lints/src/disallowed_types.rs | 4 ++-- clippy_lints/src/legacy_numeric_constants.rs | 4 +++- clippy_lints/src/macro_use.rs | 5 +---- clippy_lints/src/min_ident_chars.rs | 5 +++-- .../src/missing_enforced_import_rename.rs | 3 ++- clippy_lints/src/redundant_pub_crate.rs | 17 ++++++++--------- clippy_lints/src/unused_trait_names.rs | 2 +- clippy_lints/src/wildcard_imports.rs | 4 ++-- clippy_utils/src/paths.rs | 11 +++++++---- 9 files changed, 29 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/disallowed_types.rs b/clippy_lints/src/disallowed_types.rs index d0b2f0c8407f..821bb25d2cec 100644 --- a/clippy_lints/src/disallowed_types.rs +++ b/clippy_lints/src/disallowed_types.rs @@ -106,8 +106,8 @@ impl_lint_pass!(DisallowedTypes => [DISALLOWED_TYPES]); impl<'tcx> LateLintPass<'tcx> for DisallowedTypes { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { if let ItemKind::Use(path, UseKind::Single(_)) = &item.kind { - for res in &path.res { - self.check_res_emit(cx, res, item.span); + if let Some(res) = path.res.type_ns { + self.check_res_emit(cx, &res, item.span); } } } diff --git a/clippy_lints/src/legacy_numeric_constants.rs b/clippy_lints/src/legacy_numeric_constants.rs index 01b49403cac8..b3c63f022d35 100644 --- a/clippy_lints/src/legacy_numeric_constants.rs +++ b/clippy_lints/src/legacy_numeric_constants.rs @@ -51,7 +51,9 @@ impl<'tcx> LateLintPass<'tcx> for LegacyNumericConstants { // so lint on the `use` statement directly. if let ItemKind::Use(path, kind @ (UseKind::Single(_) | UseKind::Glob)) = item.kind && !item.span.in_external_macro(cx.sess().source_map()) - && let Some(def_id) = path.res[0].opt_def_id() + // use `present_items` because it could be in either type_ns or value_ns + && let Some(res) = path.res.present_items().next() + && let Some(def_id) = res.opt_def_id() && self.msrv.meets(cx, msrvs::NUMERIC_ASSOCIATED_CONSTANTS) { let module = if is_integer_module(cx, def_id) { diff --git a/clippy_lints/src/macro_use.rs b/clippy_lints/src/macro_use.rs index 98ad1f6a160d..c1a26c5a9c73 100644 --- a/clippy_lints/src/macro_use.rs +++ b/clippy_lints/src/macro_use.rs @@ -100,10 +100,7 @@ impl LateLintPass<'_> for MacroUseImports { && let hir_id = item.hir_id() && let attrs = cx.tcx.hir_attrs(hir_id) && let Some(mac_attr) = attrs.iter().find(|attr| attr.has_name(sym::macro_use)) - && let Some(id) = path.res.iter().find_map(|res| match res { - Res::Def(DefKind::Mod, id) => Some(id), - _ => None, - }) + && let Some(Res::Def(DefKind::Mod, id)) = path.res.type_ns && !id.is_local() { for kid in cx.tcx.module_children(id) { diff --git a/clippy_lints/src/min_ident_chars.rs b/clippy_lints/src/min_ident_chars.rs index 00ea9bba0d19..99f01c8001a5 100644 --- a/clippy_lints/src/min_ident_chars.rs +++ b/clippy_lints/src/min_ident_chars.rs @@ -131,8 +131,9 @@ impl Visitor<'_> for IdentVisitor<'_, '_> { // If however the identifier is different, this means it is an alias (`use foo::bar as baz`). In // this case, we need to emit the warning for `baz`. if let Some(imported_item_path) = usenode - && let Some(Res::Def(_, imported_item_defid)) = imported_item_path.res.first() - && cx.tcx.item_name(*imported_item_defid).as_str() == str + // use `present_items` because it could be in any of type_ns, value_ns, macro_ns + && let Some(Res::Def(_, imported_item_defid)) = imported_item_path.res.value_ns + && cx.tcx.item_name(imported_item_defid).as_str() == str { return; } diff --git a/clippy_lints/src/missing_enforced_import_rename.rs b/clippy_lints/src/missing_enforced_import_rename.rs index a1e621cc9f6b..eeea6dfd5f4b 100644 --- a/clippy_lints/src/missing_enforced_import_rename.rs +++ b/clippy_lints/src/missing_enforced_import_rename.rs @@ -72,7 +72,8 @@ impl_lint_pass!(ImportRename => [MISSING_ENFORCED_IMPORT_RENAMES]); impl LateLintPass<'_> for ImportRename { fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { if let ItemKind::Use(path, UseKind::Single(_)) = &item.kind { - for &res in &path.res { + // use `present_items` because it could be in any of type_ns, value_ns, macro_ns + for res in path.res.present_items() { if let Res::Def(_, id) = res && let Some(name) = self.renames.get(&id) // Remove semicolon since it is not present for nested imports diff --git a/clippy_lints/src/redundant_pub_crate.rs b/clippy_lints/src/redundant_pub_crate.rs index 743bdb945efd..3828aff4164e 100644 --- a/clippy_lints/src/redundant_pub_crate.rs +++ b/clippy_lints/src/redundant_pub_crate.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::HasSession; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; -use rustc_hir::{Item, ItemKind}; +use rustc_hir::{Item, ItemKind, UseKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_session::impl_lint_pass; @@ -49,7 +49,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantPubCrate { if cx.tcx.visibility(item.owner_id.def_id) == ty::Visibility::Restricted(CRATE_DEF_ID.to_def_id()) && !cx.effective_visibilities.is_exported(item.owner_id.def_id) && self.is_exported.last() == Some(&false) - && !is_macro_export(item) + && !is_ignorable_export(item) && !item.span.in_external_macro(cx.sess().source_map()) { let span = item @@ -86,13 +86,12 @@ impl<'tcx> LateLintPass<'tcx> for RedundantPubCrate { } } -fn is_macro_export<'tcx>(item: &'tcx Item<'tcx>) -> bool { - if let ItemKind::Use(path, _) = item.kind { - if path - .res - .iter() - .all(|res| matches!(res, Res::Def(DefKind::Macro(MacroKind::Bang), _))) - { +// We ignore macro exports. And `ListStem` uses, which aren't interesting. +fn is_ignorable_export<'tcx>(item: &'tcx Item<'tcx>) -> bool { + if let ItemKind::Use(path, kind) = item.kind { + let ignore = matches!(path.res.macro_ns, Some(Res::Def(DefKind::Macro(MacroKind::Bang), _))) + || kind == UseKind::ListStem; + if ignore { return true; } } else if let ItemKind::Macro(..) = item.kind { diff --git a/clippy_lints/src/unused_trait_names.rs b/clippy_lints/src/unused_trait_names.rs index 14ac65cf4dfe..610cec7b8c8d 100644 --- a/clippy_lints/src/unused_trait_names.rs +++ b/clippy_lints/src/unused_trait_names.rs @@ -64,7 +64,7 @@ impl<'tcx> LateLintPass<'tcx> for UnusedTraitNames { // Ignore imports that already use Underscore && ident.name != kw::Underscore // Only check traits - && let Some(Res::Def(DefKind::Trait, _)) = path.res.first() + && let Some(Res::Def(DefKind::Trait, _)) = path.res.type_ns && cx.tcx.maybe_unused_trait_imports(()).contains(&item.owner_id.def_id) // Only check this import if it is visible to its module only (no pub, pub(crate), ...) && let module = cx.tcx.parent_module_from_def_id(item.owner_id.def_id) diff --git a/clippy_lints/src/wildcard_imports.rs b/clippy_lints/src/wildcard_imports.rs index 45a5dbabeb4e..467811c586bf 100644 --- a/clippy_lints/src/wildcard_imports.rs +++ b/clippy_lints/src/wildcard_imports.rs @@ -169,8 +169,8 @@ impl LateLintPass<'_> for WildcardImports { format!("{import_source_snippet}::{imports_string}") }; - // Glob imports always have a single resolution. - let (lint, message) = if let Res::Def(DefKind::Enum, _) = use_path.res[0] { + // 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") } else { (WILDCARD_IMPORTS, "usage of wildcard import") diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 9d7f3086b05f..f37a609497eb 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -306,10 +306,13 @@ fn local_item_child_by_name(tcx: TyCtxt<'_>, local_id: LocalDefId, ns: PathNS, n let item = tcx.hir_item(item_id); if let ItemKind::Use(path, UseKind::Single(ident)) = item.kind { if ident.name == name { - path.res - .iter() - .find(|res| ns.matches(res.ns())) - .and_then(Res::opt_def_id) + let opt_def_id = |ns: Option| ns.and_then(|res| res.opt_def_id()); + match ns { + PathNS::Type => opt_def_id(path.res.type_ns), + PathNS::Value => opt_def_id(path.res.value_ns), + PathNS::Macro => opt_def_id(path.res.macro_ns), + PathNS::Arbitrary => unreachable!(), + } } else { None }