Skip to content

Commit

Permalink
Refactor absolute_paths:
Browse files Browse the repository at this point in the history
* Check the path length first
* Use `is_from_proc_macro`
* Use symbols instead of strings when checking crate names
  • Loading branch information
Jarcho committed Aug 10, 2024
1 parent 1c81105 commit f9509d3
Show file tree
Hide file tree
Showing 16 changed files with 415 additions and 225 deletions.
95 changes: 53 additions & 42 deletions clippy_lints/src/absolute_paths.rs
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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;
Expand All @@ -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",
);
}
}
}
12 changes: 5 additions & 7 deletions clippy_utils/src/check_proc_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
45 changes: 30 additions & 15 deletions tests/ui-toml/absolute_paths/absolute_paths.allow_crates.stderr
Original file line number Diff line number Diff line change
@@ -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

14 changes: 14 additions & 0 deletions tests/ui-toml/absolute_paths/absolute_paths.allow_long.stderr
Original file line number Diff line number Diff line change
@@ -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

80 changes: 80 additions & 0 deletions tests/ui-toml/absolute_paths/absolute_paths.default.stderr
Original file line number Diff line number Diff line change
@@ -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

71 changes: 0 additions & 71 deletions tests/ui-toml/absolute_paths/absolute_paths.disallow_crates.stderr

This file was deleted.

Loading

0 comments on commit f9509d3

Please sign in to comment.