Skip to content

Commit e129b5d

Browse files
xFrednetxFrednet
committed
join_absolute_paths Address PR review
1 parent 2fb4ff4 commit e129b5d

File tree

4 files changed

+104
-31
lines changed

4 files changed

+104
-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: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3566,11 +3566,14 @@ declare_clippy_lint! {
35663566

35673567
declare_clippy_lint! {
35683568
/// ### What it does
3569-
/// Checks for calls to `Path::join` that start with a path separator, like `\\` or `/`..
3569+
/// Checks for calls to `Path::join` that start with a path separator (`\\` or `/`).
35703570
///
35713571
/// ### Why is this bad?
3572-
/// `.join()` arguments starting with a separator (`/` or `\\`) can replace the entire path.
3573-
/// If this is intentional, prefer using `Path::new()` instead.
3572+
/// If the argument to `Path::join` starts with a separator, it will overwrite
3573+
/// the original path. If this is intentional, prefer using `Path::new` instead.
3574+
///
3575+
/// Note the behavior is platform dependent. A leading `\\` will be accepted
3576+
/// on unix systems as part of the file name
35743577
///
35753578
/// See [`Path::join()`](https://doc.rust-lang.org/std/path/struct.Path.html#method.join)
35763579
///
@@ -3598,7 +3601,7 @@ declare_clippy_lint! {
35983601
#[clippy::version = "1.74.0"]
35993602
pub JOIN_ABSOLUTE_PATHS,
36003603
correctness,
3601-
"arg to .join called on a `Path` contains leading separator"
3604+
"calls to `Path::join` which will overwrite the original path"
36023605
}
36033606

36043607
pub struct Methods {
@@ -4102,7 +4105,7 @@ impl Methods {
41024105
if let Some(("collect", _, _, span, _)) = method_call(recv) {
41034106
unnecessary_join::check(cx, expr, recv, join_arg, span);
41044107
} else {
4105-
join_absolute_paths::check(cx, recv, join_arg);
4108+
join_absolute_paths::check(cx, recv, join_arg, expr.span);
41064109
}
41074110
},
41084111
("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)