Skip to content

Commit 957cb2d

Browse files
xFrednetxFrednet
committed
join_absolute_paths Address PR review
1 parent b61633e commit 957cb2d

File tree

4 files changed

+105
-31
lines changed

4 files changed

+105
-31
lines changed

‎clippy_lints/src/methods/join_absolute_paths.rs

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,51 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
2+
use clippy_utils::expr_or_init;
3+
use clippy_utils::source::snippet_opt;
24
use clippy_utils::ty::is_type_diagnostic_item;
35
use rustc_ast::ast::LitKind;
6+
use rustc_errors::Applicability;
47
use rustc_hir::{Expr, ExprKind};
58
use rustc_lint::LateContext;
6-
use rustc_span::symbol::sym::Path;
9+
use rustc_span::symbol::sym;
10+
use rustc_span::Span;
711

812
use super::JOIN_ABSOLUTE_PATHS;
913

10-
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, join_arg: &'tcx Expr<'tcx>) {
11-
let ty = cx.typeck_results().expr_ty(expr).peel_refs();
12-
if is_type_diagnostic_item(cx, ty, Path)
13-
&& let ExprKind::Lit(spanned) = &join_arg.kind
14+
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, recv: &'tcx Expr<'tcx>, join_arg: &'tcx Expr<'tcx>, expr_span: Span) {
15+
let ty = cx.typeck_results().expr_ty(recv).peel_refs();
16+
if (is_type_diagnostic_item(cx, ty, sym::Path) || is_type_diagnostic_item(cx, ty, sym::PathBuf))
17+
&& let ExprKind::Lit(spanned) = expr_or_init(cx, join_arg).kind
1418
&& let LitKind::Str(symbol, _) = spanned.node
15-
&& (symbol.as_str().starts_with('/') || symbol.as_str().starts_with('\\'))
19+
&& let sym_str = symbol.as_str()
20+
&& (sym_str.starts_with('/') || sym_str.starts_with('\\'))
1621
{
1722
span_lint_and_then(
1823
cx,
1924
JOIN_ABSOLUTE_PATHS,
2025
join_arg.span,
2126
"argument to `Path::join` starts with a path separator",
2227
|diag| {
23-
diag
24-
.note("joining a path starting with separator will replace the path instead")
25-
.help("if this is unintentional, try removing the starting separator")
26-
.help("if this is intentional, try creating a new Path instead");
28+
let arg_str = snippet_opt(cx, spanned.span).unwrap_or_else(|| "..".to_string());
29+
30+
let no_separator = if sym_str.starts_with('/') {
31+
arg_str.replacen('/', "", 1)
32+
} else {
33+
arg_str.replacen('\\', "", 1)
34+
};
35+
36+
diag.note("joining a path starting with separator will replace the path instead")
37+
.span_suggestion(
38+
spanned.span,
39+
"if this is unintentional, try removing the starting separator",
40+
no_separator,
41+
Applicability::Unspecified
42+
)
43+
.span_suggestion(
44+
expr_span,
45+
"if this is intentional, try creating a new Path instead",
46+
format!("PathBuf::from({arg_str})"),
47+
Applicability::Unspecified
48+
);
2749
},
2850
);
2951
}

‎clippy_lints/src/methods/mod.rs

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3684,11 +3684,15 @@ declare_clippy_lint! {
36843684
}
36853685

36863686
declare_clippy_lint! {
3687-
/// Checks for calls to `Path::join` that start with a path separator, like `\\` or `/`..
3687+
/// ### What it does
3688+
/// Checks for calls to `Path::join` that start with a path separator (`\\` or `/`).
36883689
///
36893690
/// ### Why is this bad?
3690-
/// `.join()` arguments starting with a separator (`/` or `\\`) can replace the entire path.
3691-
/// If this is intentional, prefer using `Path::new()` instead.
3691+
/// If the argument to `Path::join` starts with a separator, it will overwrite
3692+
/// the original path. If this is intentional, prefer using `Path::new` instead.
3693+
///
3694+
/// Note the behavior is platform dependent. A leading `\\` will be accepted
3695+
/// on unix systems as part of the file name
36923696
///
36933697
/// See [`Path::join()`](https://doc.rust-lang.org/std/path/struct.Path.html#method.join)
36943698
///
@@ -3716,7 +3720,7 @@ declare_clippy_lint! {
37163720
#[clippy::version = "1.75.0"]
37173721
pub JOIN_ABSOLUTE_PATHS,
37183722
correctness,
3719-
"arg to .join called on a `Path` contains leading separator"
3723+
"calls to `Path::join` which will overwrite the original path"
37203724
}
37213725

37223726
pub struct Methods {
@@ -4270,7 +4274,7 @@ impl Methods {
42704274
if let Some(("collect", _, _, span, _)) = method_call(recv) {
42714275
unnecessary_join::check(cx, expr, recv, join_arg, span);
42724276
} else {
4273-
join_absolute_paths::check(cx, recv, join_arg);
4277+
join_absolute_paths::check(cx, recv, join_arg, expr.span);
42744278
}
42754279
},
42764280
("last", []) => {

‎tests/ui/join_absolute_paths.rs

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,30 @@
1-
#![allow(unused)]
1+
//@no-rustfix
2+
3+
#![allow(clippy::needless_raw_string_hashes)]
24
#![warn(clippy::join_absolute_paths)]
35

46
use std::path::{Path, PathBuf};
57

68
fn main() {
7-
// should be linted
89
let path = Path::new("/bin");
910
path.join("/sh");
11+
//~^ ERROR: argument to `Path::join` starts with a path separator
1012

11-
// should be linted
1213
let path = Path::new("C:\\Users");
1314
path.join("\\user");
15+
//~^ ERROR: argument to `Path::join` starts with a path separator
16+
17+
let path = PathBuf::from("/bin");
18+
path.join("/sh");
19+
//~^ ERROR: argument to `Path::join` starts with a path separator
20+
21+
let path = PathBuf::from("/bin");
22+
path.join(r#"/sh"#);
23+
//~^ ERROR: argument to `Path::join` starts with a path separator
1424

15-
// should not be linted
1625
let path: &[&str] = &["/bin"];
1726
path.join("/sh");
1827

19-
// should not be linted
2028
let path = Path::new("/bin");
2129
path.join("sh");
22-
23-
// should be linted
24-
let path = PathBuf::from("/bin");
25-
path.join("/sh");
2630
}

‎tests/ui/join_absolute_paths.stderr

Lines changed: 51 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,67 @@
11
error: argument to `Path::join` starts with a path separator
2-
--> $DIR/join_absolute_paths.rs:9:15
2+
--> $DIR/join_absolute_paths.rs:10:15
33
|
44
LL | path.join("/sh");
55
| ^^^^^
66
|
77
= note: joining a path starting with separator will replace the path instead
8-
= help: if this is unintentional, try removing the starting separator
9-
= help: if this is intentional, try creating a new Path instead
108
= note: `-D clippy::join-absolute-paths` implied by `-D warnings`
9+
help: if this is unintentional, try removing the starting separator
10+
|
11+
LL | path.join("sh");
12+
| ~~~~
13+
help: if this is intentional, try creating a new Path instead
14+
|
15+
LL | PathBuf::from("/sh");
16+
| ~~~~~~~~~~~~~~~~~~~~
1117

1218
error: argument to `Path::join` starts with a path separator
13-
--> $DIR/join_absolute_paths.rs:13:15
19+
--> $DIR/join_absolute_paths.rs:14:15
1420
|
1521
LL | path.join("\\user");
1622
| ^^^^^^^^
1723
|
1824
= note: joining a path starting with separator will replace the path instead
19-
= help: if this is unintentional, try removing the starting separator
20-
= help: if this is intentional, try creating a new Path instead
25+
help: if this is unintentional, try removing the starting separator
26+
|
27+
LL | path.join("\user");
28+
| ~~~~~~~
29+
help: if this is intentional, try creating a new Path instead
30+
|
31+
LL | PathBuf::from("\\user");
32+
| ~~~~~~~~~~~~~~~~~~~~~~~
33+
34+
error: argument to `Path::join` starts with a path separator
35+
--> $DIR/join_absolute_paths.rs:18:15
36+
|
37+
LL | path.join("/sh");
38+
| ^^^^^
39+
|
40+
= note: joining a path starting with separator will replace the path instead
41+
help: if this is unintentional, try removing the starting separator
42+
|
43+
LL | path.join("sh");
44+
| ~~~~
45+
help: if this is intentional, try creating a new Path instead
46+
|
47+
LL | PathBuf::from("/sh");
48+
| ~~~~~~~~~~~~~~~~~~~~
49+
50+
error: argument to `Path::join` starts with a path separator
51+
--> $DIR/join_absolute_paths.rs:22:15
52+
|
53+
LL | path.join(r#"/sh"#);
54+
| ^^^^^^^^
55+
|
56+
= note: joining a path starting with separator will replace the path instead
57+
help: if this is unintentional, try removing the starting separator
58+
|
59+
LL | path.join(r#"sh"#);
60+
| ~~~~~~~
61+
help: if this is intentional, try creating a new Path instead
62+
|
63+
LL | PathBuf::from(r#"/sh"#);
64+
| ~~~~~~~~~~~~~~~~~~~~~~~
2165

22-
error: aborting due to 2 previous errors
66+
error: aborting due to 4 previous errors
2367

0 commit comments

Comments
 (0)