Auto merge of rust-lang#13041 - Jarcho:path, r=y21

Refactor `absolute_paths`

Checks are rearranged to do the more expensive checks later. Since the most likely path length will be one (locals and imported/local items) this will exclude such paths on the first check.

Tests were rewritten as they were hard to follow (annotations would have helped), spammy (lots of tests for the same thing) and insufficient.

One thing thing that came up and should be decided on now is what to do about the difference between `path::to::Trait::item` (4 segments) and `path::to::Type::item` (3 segments). The current behaviour treats these as different lengths which is terrible. I personally think these should both be three segments since the item can't actually be imported. Only the type or the trait could be. This makes `crate_name::Trait::item` the shortest absolute path which is shorter than the lint allows by default.

changelog: None

Author: bors

clippy_lints/src/absolute_paths.rs

@@ -1,13 +1,14 @@
 use clippy_config::Conf;
 use clippy_utils::diagnostics::span_lint;
-use clippy_utils::source::snippet_opt;
+use clippy_utils::is_from_proc_macro;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{DefId, CRATE_DEF_INDEX};
 use rustc_hir::{HirId, ItemKind, Node, Path};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
 use rustc_span::symbol::kw;
+use rustc_span::Symbol;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -24,6 +25,13 @@ declare_clippy_lint! {
     /// Note: One exception to this is code from macro expansion - this does not lint such cases, as
     /// using absolute paths is the proper way of referencing items in one.
     ///
+    /// ### Known issues
+    ///
+    /// There are currently a few cases which are not caught by this lint:
+    /// * Macro calls. e.g. `path::to::macro!()`
+    /// * Derive macros. e.g. `#[derive(path::to::macro)]`
+    /// * Attribute macros. e.g. `#[path::to::macro]`
+    ///
     /// ### Example
     /// ```no_run
     /// let x = std::f64::consts::PI;
@@ -48,63 +56,66 @@ impl_lint_pass!(AbsolutePaths => [ABSOLUTE_PATHS]);
 
 pub struct AbsolutePaths {
     pub absolute_paths_max_segments: u64,
-    pub absolute_paths_allowed_crates: &'static FxHashSet<String>,
+    pub absolute_paths_allowed_crates: FxHashSet<Symbol>,
 }
 
 impl AbsolutePaths {
     pub fn new(conf: &'static Conf) -> Self {
         Self {
             absolute_paths_max_segments: conf.absolute_paths_max_segments,
-            absolute_paths_allowed_crates: &conf.absolute_paths_allowed_crates,
+            absolute_paths_allowed_crates: conf
+                .absolute_paths_allowed_crates
+                .iter()
+                .map(|x| Symbol::intern(x))
+                .collect(),
         }
     }
 }
 
-impl LateLintPass<'_> for AbsolutePaths {
+impl<'tcx> LateLintPass<'tcx> for AbsolutePaths {
     // We should only lint `QPath::Resolved`s, but since `Path` is only used in `Resolved` and `UsePath`
     // we don't need to use a visitor or anything as we can just check if the `Node` for `hir_id` isn't
     // a `Use`
-    #[expect(clippy::cast_possible_truncation)]
-    fn check_path(&mut self, cx: &LateContext<'_>, path: &Path<'_>, hir_id: HirId) {
-        let Self {
-            absolute_paths_max_segments,
-            absolute_paths_allowed_crates,
-        } = self;
-
-        if !path.span.from_expansion()
-            && let node = cx.tcx.hir_node(hir_id)
-            && !matches!(node, Node::Item(item) if matches!(item.kind, ItemKind::Use(_, _)))
-            && let [first, rest @ ..] = path.segments
-            // Handle `::std`
-            && let (segment, len) = if first.ident.name == kw::PathRoot {
-                // Indexing is fine as `PathRoot` must be followed by another segment. `len() - 1`
-                // is fine here for the same reason
-                (&rest[0], path.segments.len() - 1)
-            } else {
-                (first, path.segments.len())
-            }
-            && len > *absolute_paths_max_segments as usize
-            && let Some(segment_snippet) = snippet_opt(cx, segment.ident.span)
-            && segment_snippet == segment.ident.as_str()
-        {
-            let is_abs_external =
-                matches!(segment.res, Res::Def(DefKind::Mod, DefId { index, .. }) if index == CRATE_DEF_INDEX);
-            let is_abs_crate = segment.ident.name == kw::Crate;
-
-            if is_abs_external && absolute_paths_allowed_crates.contains(segment.ident.name.as_str())
-                || is_abs_crate && absolute_paths_allowed_crates.contains("crate")
+    fn check_path(&mut self, cx: &LateContext<'tcx>, path: &Path<'tcx>, hir_id: HirId) {
+        let segments = match path.segments {
+            [] | [_] => return,
+            // Don't count enum variants and trait items as part of the length.
+            [rest @ .., _]
+                if let [.., s] = rest
+                    && matches!(s.res, Res::Def(DefKind::Enum | DefKind::Trait | DefKind::TraitAlias, _)) =>
+            {
+                rest
+            },
+            path => path,
+        };
+        if let [s1, s2, ..] = segments
+            && let has_root = s1.ident.name == kw::PathRoot
+            && let first = if has_root { s2 } else { s1 }
+            && let len = segments.len() - usize::from(has_root)
+            && len as u64 > self.absolute_paths_max_segments
+            && let crate_name = if let Res::Def(DefKind::Mod, DefId { index, .. }) = first.res
+                && index == CRATE_DEF_INDEX
             {
+                // `other_crate::foo` or `::other_crate::foo`
+                first.ident.name
+            } else if first.ident.name == kw::Crate || has_root {
+                // `::foo` or `crate::foo`
+                kw::Crate
+            } else {
                 return;
             }
-
-            if is_abs_external || is_abs_crate {
-                span_lint(
-                    cx,
-                    ABSOLUTE_PATHS,
-                    path.span,
-                    "consider bringing this path into scope with the `use` keyword",
-                );
-            }
+            && !path.span.from_expansion()
+            && let node = cx.tcx.hir_node(hir_id)
+            && !matches!(node, Node::Item(item) if matches!(item.kind, ItemKind::Use(..)))
+            && !self.absolute_paths_allowed_crates.contains(&crate_name)
+            && !is_from_proc_macro(cx, path)
+        {
+            span_lint(
+                cx,
+                ABSOLUTE_PATHS,
+                path.span,
+                "consider bringing this path into scope with the `use` keyword",
+            );
         }
     }
 }

clippy_utils/src/check_proc_macro.rs

@@ -123,16 +123,14 @@ fn qpath_search_pat(path: &QPath<'_>) -> (Pat, Pat) {
 
 fn path_search_pat(path: &Path<'_>) -> (Pat, Pat) {
     let (head, tail) = match path.segments {
-        [head, .., tail] => (head, tail),
-        [p] => (p, p),
         [] => return (Pat::Str(""), Pat::Str("")),
+        [p] => (Pat::Sym(p.ident.name), p),
+        // QPath::Resolved can have a path that looks like `<Foo as Bar>::baz` where
+        // the path (`Bar::baz`) has it's span covering the whole QPath.
+        [.., tail] => (Pat::Str(""), tail),
     };
     (
-        if head.ident.name == kw::PathRoot {
-            Pat::Str("::")
-        } else {
-            Pat::Sym(head.ident.name)
-        },
+        head,
         if tail.args.is_some() {
             Pat::Str(">")
         } else {

tests/ui-toml/absolute_paths/absolute_paths.allow_crates.stderr

@@ -1,29 +1,44 @@
 error: consider bringing this path into scope with the `use` keyword
-  --> tests/ui-toml/absolute_paths/absolute_paths.rs:40:5
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:14:13
    |
-LL |     std::f32::MAX;
-   |     ^^^^^^^^^^^^^
+LL |     let _ = std::path::is_separator(' ');
+   |             ^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = note: `-D clippy::absolute-paths` implied by `-D warnings`
-   = help: to override `-D warnings` add `#[allow(clippy::absolute_paths)]`
+note: the lint level is defined here
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:7:9
+   |
+LL | #![deny(clippy::absolute_paths)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:20:13
+   |
+LL |     let _ = ::std::path::MAIN_SEPARATOR;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:25:13
+   |
+LL |     let _ = std::collections::hash_map::HashMap::<i32, i32>::new();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: consider bringing this path into scope with the `use` keyword
-  --> tests/ui-toml/absolute_paths/absolute_paths.rs:41:5
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:28:31
    |
-LL |     core::f32::MAX;
-   |     ^^^^^^^^^^^^^^
+LL |     let _: &std::path::Path = std::path::Path::new("");
+   |                               ^^^^^^^^^^^^^^^
 
 error: consider bringing this path into scope with the `use` keyword
-  --> tests/ui-toml/absolute_paths/absolute_paths.rs:42:5
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:28:13
    |
-LL |     ::core::f32::MAX;
-   |     ^^^^^^^^^^^^^^^^
+LL |     let _: &std::path::Path = std::path::Path::new("");
+   |             ^^^^^^^^^^^^^^^
 
 error: consider bringing this path into scope with the `use` keyword
-  --> tests/ui-toml/absolute_paths/absolute_paths.rs:58:5
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:43:13
    |
-LL |     ::std::f32::MAX;
-   |     ^^^^^^^^^^^^^^^
+LL |     let _ = std::option::Option::None::<i32>;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 4 previous errors
+error: aborting due to 6 previous errors
 

tests/ui-toml/absolute_paths/absolute_paths.allow_long.stderr

@@ -0,0 +1,14 @@
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:25:13
+   |
+LL |     let _ = std::collections::hash_map::HashMap::<i32, i32>::new();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: the lint level is defined here
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:7:9
+   |
+LL | #![deny(clippy::absolute_paths)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 1 previous error
+

tests/ui-toml/absolute_paths/absolute_paths.default.stderr

@@ -0,0 +1,80 @@
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:14:13
+   |
+LL |     let _ = std::path::is_separator(' ');
+   |             ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: the lint level is defined here
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:7:9
+   |
+LL | #![deny(clippy::absolute_paths)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:20:13
+   |
+LL |     let _ = ::std::path::MAIN_SEPARATOR;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:25:13
+   |
+LL |     let _ = std::collections::hash_map::HashMap::<i32, i32>::new();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:28:31
+   |
+LL |     let _: &std::path::Path = std::path::Path::new("");
+   |                               ^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:28:13
+   |
+LL |     let _: &std::path::Path = std::path::Path::new("");
+   |             ^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:37:13
+   |
+LL |     let _ = ::core::clone::Clone::clone(&0i32);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:40:13
+   |
+LL |     let _ = <i32 as core::clone::Clone>::clone(&0i32);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:43:13
+   |
+LL |     let _ = std::option::Option::None::<i32>;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:65:17
+   |
+LL |         impl<T: core::cmp::Eq> core::fmt::Display for X<T>
+   |                 ^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:70:18
+   |
+LL |         where T: core::clone::Clone
+   |                  ^^^^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:65:32
+   |
+LL |         impl<T: core::cmp::Eq> core::fmt::Display for X<T>
+   |                                ^^^^^^^^^^^^^^^^^^
+
+error: consider bringing this path into scope with the `use` keyword
+  --> tests/ui-toml/absolute_paths/absolute_paths.rs:116:5
+   |
+LL |     crate::m1::S
+   |     ^^^^^^^^^^^^
+
+error: aborting due to 12 previous errors
+