Skip to content

Commit f9509d3

Browse files
JarchoJarcho
committed
Refactor absolute_paths:
* Check the path length first * Use `is_from_proc_macro` * Use symbols instead of strings when checking crate names
1 parent 1c81105 commit f9509d3

16 files changed

+415
-225
lines changed

‎clippy_lints/src/absolute_paths.rs

+53-42
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

+5-7
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

+30-15
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

+14
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

+80
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

-71
This file was deleted.

0 commit comments

Comments
 (0)