Skip to content
This repository was archived by the owner on May 28, 2025. It is now read-only.

Commit 780c61f

Browse files
borsbors
committed
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
2 parents 1c81105 + f9509d3 commit 780c61f

File tree

16 files changed

+415
-225
lines changed

16 files changed

+415
-225
lines changed

‎clippy_lints/src/absolute_paths.rs‎

Lines changed: 53 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
11
use clippy_config::Conf;
22
use clippy_utils::diagnostics::span_lint;
3-
use clippy_utils::source::snippet_opt;
3+
use clippy_utils::is_from_proc_macro;
44
use rustc_data_structures::fx::FxHashSet;
55
use rustc_hir::def::{DefKind, Res};
66
use rustc_hir::def_id::{DefId, CRATE_DEF_INDEX};
77
use rustc_hir::{HirId, ItemKind, Node, Path};
88
use rustc_lint::{LateContext, LateLintPass};
99
use rustc_session::impl_lint_pass;
1010
use rustc_span::symbol::kw;
11+
use rustc_span::Symbol;
1112

1213
declare_clippy_lint! {
1314
/// ### What it does
@@ -24,6 +25,13 @@ declare_clippy_lint! {
2425
/// Note: One exception to this is code from macro expansion - this does not lint such cases, as
2526
/// using absolute paths is the proper way of referencing items in one.
2627
///
28+
/// ### Known issues
29+
///
30+
/// There are currently a few cases which are not caught by this lint:
31+
/// * Macro calls. e.g. `path::to::macro!()`
32+
/// * Derive macros. e.g. `#[derive(path::to::macro)]`
33+
/// * Attribute macros. e.g. `#[path::to::macro]`
34+
///
2735
/// ### Example
2836
/// ```no_run
2937
/// let x = std::f64::consts::PI;
@@ -48,63 +56,66 @@ impl_lint_pass!(AbsolutePaths => [ABSOLUTE_PATHS]);
4856

4957
pub struct AbsolutePaths {
5058
pub absolute_paths_max_segments: u64,
51-
pub absolute_paths_allowed_crates: &'static FxHashSet<String>,
59+
pub absolute_paths_allowed_crates: FxHashSet<Symbol>,
5260
}
5361

5462
impl AbsolutePaths {
5563
pub fn new(conf: &'static Conf) -> Self {
5664
Self {
5765
absolute_paths_max_segments: conf.absolute_paths_max_segments,
58-
absolute_paths_allowed_crates: &conf.absolute_paths_allowed_crates,
66+
absolute_paths_allowed_crates: conf
67+
.absolute_paths_allowed_crates
68+
.iter()
69+
.map(|x| Symbol::intern(x))
70+
.collect(),
5971
}
6072
}
6173
}
6274

63-
impl LateLintPass<'_> for AbsolutePaths {
75+
impl<'tcx> LateLintPass<'tcx> for AbsolutePaths {
6476
// We should only lint `QPath::Resolved`s, but since `Path` is only used in `Resolved` and `UsePath`
6577
// we don't need to use a visitor or anything as we can just check if the `Node` for `hir_id` isn't
6678
// a `Use`
67-
#[expect(clippy::cast_possible_truncation)]
68-
fn check_path(&mut self, cx: &LateContext<'_>, path: &Path<'_>, hir_id: HirId) {
69-
let Self {
70-
absolute_paths_max_segments,
71-
absolute_paths_allowed_crates,
72-
} = self;
73-
74-
if !path.span.from_expansion()
75-
&& let node = cx.tcx.hir_node(hir_id)
76-
&& !matches!(node, Node::Item(item) if matches!(item.kind, ItemKind::Use(_, _)))
77-
&& let [first, rest @ ..] = path.segments
78-
// Handle `::std`
79-
&& let (segment, len) = if first.ident.name == kw::PathRoot {
80-
// Indexing is fine as `PathRoot` must be followed by another segment. `len() - 1`
81-
// is fine here for the same reason
82-
(&rest[0], path.segments.len() - 1)
83-
} else {
84-
(first, path.segments.len())
85-
}
86-
&& len > *absolute_paths_max_segments as usize
87-
&& let Some(segment_snippet) = snippet_opt(cx, segment.ident.span)
88-
&& segment_snippet == segment.ident.as_str()
89-
{
90-
let is_abs_external =
91-
matches!(segment.res, Res::Def(DefKind::Mod, DefId { index, .. }) if index == CRATE_DEF_INDEX);
92-
let is_abs_crate = segment.ident.name == kw::Crate;
93-
94-
if is_abs_external && absolute_paths_allowed_crates.contains(segment.ident.name.as_str())
95-
|| is_abs_crate && absolute_paths_allowed_crates.contains("crate")
79+
fn check_path(&mut self, cx: &LateContext<'tcx>, path: &Path<'tcx>, hir_id: HirId) {
80+
let segments = match path.segments {
81+
[] | [_] => return,
82+
// Don't count enum variants and trait items as part of the length.
83+
[rest @ .., _]
84+
if let [.., s] = rest
85+
&& matches!(s.res, Res::Def(DefKind::Enum | DefKind::Trait | DefKind::TraitAlias, _)) =>
86+
{
87+
rest
88+
},
89+
path => path,
90+
};
91+
if let [s1, s2, ..] = segments
92+
&& let has_root = s1.ident.name == kw::PathRoot
93+
&& let first = if has_root { s2 } else { s1 }
94+
&& let len = segments.len() - usize::from(has_root)
95+
&& len as u64 > self.absolute_paths_max_segments
96+
&& let crate_name = if let Res::Def(DefKind::Mod, DefId { index, .. }) = first.res
97+
&& index == CRATE_DEF_INDEX
9698
{
99+
// `other_crate::foo` or `::other_crate::foo`
100+
first.ident.name
101+
} else if first.ident.name == kw::Crate || has_root {
102+
// `::foo` or `crate::foo`
103+
kw::Crate
104+
} else {
97105
return;
98106
}
99-
100-
if is_abs_external || is_abs_crate {
101-
span_lint(
102-
cx,
103-
ABSOLUTE_PATHS,
104-
path.span,
105-
"consider bringing this path into scope with the `use` keyword",
106-
);
107-
}
107+
&& !path.span.from_expansion()
108+
&& let node = cx.tcx.hir_node(hir_id)
109+
&& !matches!(node, Node::Item(item) if matches!(item.kind, ItemKind::Use(..)))
110+
&& !self.absolute_paths_allowed_crates.contains(&crate_name)
111+
&& !is_from_proc_macro(cx, path)
112+
{
113+
span_lint(
114+
cx,
115+
ABSOLUTE_PATHS,
116+
path.span,
117+
"consider bringing this path into scope with the `use` keyword",
118+
);
108119
}
109120
}
110121
}

‎clippy_utils/src/check_proc_macro.rs‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -123,16 +123,14 @@ fn qpath_search_pat(path: &QPath<'_>) -> (Pat, Pat) {
123123

124124
fn path_search_pat(path: &Path<'_>) -> (Pat, Pat) {
125125
let (head, tail) = match path.segments {
126-
[head, .., tail] => (head, tail),
127-
[p] => (p, p),
128126
[] => return (Pat::Str(""), Pat::Str("")),
127+
[p] => (Pat::Sym(p.ident.name), p),
128+
// QPath::Resolved can have a path that looks like `<Foo as Bar>::baz` where
129+
// the path (`Bar::baz`) has it's span covering the whole QPath.
130+
[.., tail] => (Pat::Str(""), tail),
129131
};
130132
(
131-
if head.ident.name == kw::PathRoot {
132-
Pat::Str("::")
133-
} else {
134-
Pat::Sym(head.ident.name)
135-
},
133+
head,
136134
if tail.args.is_some() {
137135
Pat::Str(">")
138136
} else {

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

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,44 @@
11
error: consider bringing this path into scope with the `use` keyword
2-
--> tests/ui-toml/absolute_paths/absolute_paths.rs:40:5
2+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:14:13
33
|
4-
LL | std::f32::MAX;
5-
| ^^^^^^^^^^^^^
4+
LL | let _ = std::path::is_separator(' ');
5+
| ^^^^^^^^^^^^^^^^^^^^^^^
66
|
7-
= note: `-D clippy::absolute-paths` implied by `-D warnings`
8-
= help: to override `-D warnings` add `#[allow(clippy::absolute_paths)]`
7+
note: the lint level is defined here
8+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:7:9
9+
|
10+
LL | #![deny(clippy::absolute_paths)]
11+
| ^^^^^^^^^^^^^^^^^^^^^^
12+
13+
error: consider bringing this path into scope with the `use` keyword
14+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:20:13
15+
|
16+
LL | let _ = ::std::path::MAIN_SEPARATOR;
17+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
18+
19+
error: consider bringing this path into scope with the `use` keyword
20+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:25:13
21+
|
22+
LL | let _ = std::collections::hash_map::HashMap::<i32, i32>::new();
23+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
924

1025
error: consider bringing this path into scope with the `use` keyword
11-
--> tests/ui-toml/absolute_paths/absolute_paths.rs:41:5
26+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:28:31
1227
|
13-
LL | core::f32::MAX;
14-
| ^^^^^^^^^^^^^^
28+
LL | let _: &std::path::Path = std::path::Path::new("");
29+
| ^^^^^^^^^^^^^^^
1530

1631
error: consider bringing this path into scope with the `use` keyword
17-
--> tests/ui-toml/absolute_paths/absolute_paths.rs:42:5
32+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:28:13
1833
|
19-
LL | ::core::f32::MAX;
20-
| ^^^^^^^^^^^^^^^^
34+
LL | let _: &std::path::Path = std::path::Path::new("");
35+
| ^^^^^^^^^^^^^^^
2136

2237
error: consider bringing this path into scope with the `use` keyword
23-
--> tests/ui-toml/absolute_paths/absolute_paths.rs:58:5
38+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:43:13
2439
|
25-
LL | ::std::f32::MAX;
26-
| ^^^^^^^^^^^^^^^
40+
LL | let _ = std::option::Option::None::<i32>;
41+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2742

28-
error: aborting due to 4 previous errors
43+
error: aborting due to 6 previous errors
2944

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
error: consider bringing this path into scope with the `use` keyword
2+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:25:13
3+
|
4+
LL | let _ = std::collections::hash_map::HashMap::<i32, i32>::new();
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6+
|
7+
note: the lint level is defined here
8+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:7:9
9+
|
10+
LL | #![deny(clippy::absolute_paths)]
11+
| ^^^^^^^^^^^^^^^^^^^^^^
12+
13+
error: aborting due to 1 previous error
14+

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

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
error: consider bringing this path into scope with the `use` keyword
2+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:14:13
3+
|
4+
LL | let _ = std::path::is_separator(' ');
5+
| ^^^^^^^^^^^^^^^^^^^^^^^
6+
|
7+
note: the lint level is defined here
8+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:7:9
9+
|
10+
LL | #![deny(clippy::absolute_paths)]
11+
| ^^^^^^^^^^^^^^^^^^^^^^
12+
13+
error: consider bringing this path into scope with the `use` keyword
14+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:20:13
15+
|
16+
LL | let _ = ::std::path::MAIN_SEPARATOR;
17+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
18+
19+
error: consider bringing this path into scope with the `use` keyword
20+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:25:13
21+
|
22+
LL | let _ = std::collections::hash_map::HashMap::<i32, i32>::new();
23+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
24+
25+
error: consider bringing this path into scope with the `use` keyword
26+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:28:31
27+
|
28+
LL | let _: &std::path::Path = std::path::Path::new("");
29+
| ^^^^^^^^^^^^^^^
30+
31+
error: consider bringing this path into scope with the `use` keyword
32+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:28:13
33+
|
34+
LL | let _: &std::path::Path = std::path::Path::new("");
35+
| ^^^^^^^^^^^^^^^
36+
37+
error: consider bringing this path into scope with the `use` keyword
38+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:37:13
39+
|
40+
LL | let _ = ::core::clone::Clone::clone(&0i32);
41+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
42+
43+
error: consider bringing this path into scope with the `use` keyword
44+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:40:13
45+
|
46+
LL | let _ = <i32 as core::clone::Clone>::clone(&0i32);
47+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
48+
49+
error: consider bringing this path into scope with the `use` keyword
50+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:43:13
51+
|
52+
LL | let _ = std::option::Option::None::<i32>;
53+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
54+
55+
error: consider bringing this path into scope with the `use` keyword
56+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:65:17
57+
|
58+
LL | impl<T: core::cmp::Eq> core::fmt::Display for X<T>
59+
| ^^^^^^^^^^^^^
60+
61+
error: consider bringing this path into scope with the `use` keyword
62+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:70:18
63+
|
64+
LL | where T: core::clone::Clone
65+
| ^^^^^^^^^^^^^^^^^^
66+
67+
error: consider bringing this path into scope with the `use` keyword
68+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:65:32
69+
|
70+
LL | impl<T: core::cmp::Eq> core::fmt::Display for X<T>
71+
| ^^^^^^^^^^^^^^^^^^
72+
73+
error: consider bringing this path into scope with the `use` keyword
74+
--> tests/ui-toml/absolute_paths/absolute_paths.rs:116:5
75+
|
76+
LL | crate::m1::S
77+
| ^^^^^^^^^^^^
78+
79+
error: aborting due to 12 previous errors
80+

‎tests/ui-toml/absolute_paths/absolute_paths.disallow_crates.stderr‎

Lines changed: 0 additions & 71 deletions
This file was deleted.

0 commit comments

Comments
 (0)