Skip to content

Commit

Permalink
join_absolute_paths Address PR review
Browse files Browse the repository at this point in the history
  • Loading branch information
xFrednet committed Sep 25, 2023
1 parent 38ac7be commit 1a024eb
Show file tree
Hide file tree
Showing 4 changed files with 105 additions and 31 deletions.
42 changes: 32 additions & 10 deletions clippy_lints/src/methods/join_absolute_paths.rs
Original file line number Diff line number Diff line change
@@ -1,29 +1,51 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::expr_or_init;
use clippy_utils::source::snippet_opt;
use clippy_utils::ty::is_type_diagnostic_item;
use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind};
use rustc_lint::LateContext;
use rustc_span::symbol::sym::Path;
use rustc_span::symbol::sym;
use rustc_span::Span;

use super::JOIN_ABSOLUTE_PATHS;

pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, join_arg: &'tcx Expr<'tcx>) {
let ty = cx.typeck_results().expr_ty(expr).peel_refs();
if is_type_diagnostic_item(cx, ty, Path)
&& let ExprKind::Lit(spanned) = &join_arg.kind
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, recv: &'tcx Expr<'tcx>, join_arg: &'tcx Expr<'tcx>, expr_span: Span) {
let ty = cx.typeck_results().expr_ty(recv).peel_refs();
if (is_type_diagnostic_item(cx, ty, sym::Path) || is_type_diagnostic_item(cx, ty, sym::PathBuf))
&& let ExprKind::Lit(spanned) = expr_or_init(cx, join_arg).kind
&& let LitKind::Str(symbol, _) = spanned.node
&& (symbol.as_str().starts_with('/') || symbol.as_str().starts_with('\\'))
&& let sym_str = symbol.as_str()
&& (sym_str.starts_with('/') || sym_str.starts_with('\\'))
{
span_lint_and_then(
cx,
JOIN_ABSOLUTE_PATHS,
join_arg.span,
"argument to `Path::join` starts with a path separator",
|diag| {
diag
.note("joining a path starting with separator will replace the path instead")
.help("if this is unintentional, try removing the starting separator")
.help("if this is intentional, try creating a new Path instead");
let arg_str = snippet_opt(cx, spanned.span).unwrap_or_else(|| "..".to_string());

let no_separator = if sym_str.starts_with('/') {
arg_str.replacen('/', "", 1)
} else {
arg_str.replacen('\\', "", 1)
};

diag.note("joining a path starting with separator will replace the path instead")
.span_suggestion(
spanned.span,
"if this is unintentional, try removing the starting separator",
no_separator,
Applicability::Unspecified
)
.span_suggestion(
expr_span,
"if this is intentional, try creating a new Path instead",
format!("PathBuf::from({arg_str})"),
Applicability::Unspecified
);
},
);
}
Expand Down
14 changes: 9 additions & 5 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3633,11 +3633,15 @@ declare_clippy_lint! {
}

declare_clippy_lint! {
/// Checks for calls to `Path::join` that start with a path separator, like `\\` or `/`..
/// ### What it does
/// Checks for calls to `Path::join` that start with a path separator (`\\` or `/`).
///
/// ### Why is this bad?
/// `.join()` arguments starting with a separator (`/` or `\\`) can replace the entire path.
/// If this is intentional, prefer using `Path::new()` instead.
/// If the argument to `Path::join` starts with a separator, it will overwrite
/// the original path. If this is intentional, prefer using `Path::new` instead.
///
/// Note the behavior is platform dependent. A leading `\\` will be accepted
/// on unix systems as part of the file name
///
/// See [`Path::join()`](https://doc.rust-lang.org/std/path/struct.Path.html#method.join)
///
Expand Down Expand Up @@ -3665,7 +3669,7 @@ declare_clippy_lint! {
#[clippy::version = "1.74.0"]
pub JOIN_ABSOLUTE_PATHS,
correctness,
"arg to .join called on a `Path` contains leading separator"
"calls to `Path::join` which will overwrite the original path"
}

pub struct Methods {
Expand Down Expand Up @@ -4178,7 +4182,7 @@ impl Methods {
if let Some(("collect", _, _, span, _)) = method_call(recv) {
unnecessary_join::check(cx, expr, recv, join_arg, span);
} else {
join_absolute_paths::check(cx, recv, join_arg);
join_absolute_paths::check(cx, recv, join_arg, expr.span);
}
},
("last", []) => {
Expand Down
22 changes: 13 additions & 9 deletions tests/ui/join_absolute_paths.rs
Original file line number Diff line number Diff line change
@@ -1,26 +1,30 @@
#![allow(unused)]
//@no-rustfix

#![allow(clippy::needless_raw_string_hashes)]
#![warn(clippy::join_absolute_paths)]

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

fn main() {
// should be linted
let path = Path::new("/bin");
path.join("/sh");
//~^ ERROR: argument to `Path::join` starts with a path separator

// should be linted
let path = Path::new("C:\\Users");
path.join("\\user");
//~^ ERROR: argument to `Path::join` starts with a path separator

let path = PathBuf::from("/bin");
path.join("/sh");
//~^ ERROR: argument to `Path::join` starts with a path separator

let path = PathBuf::from("/bin");
path.join(r#"/sh"#);
//~^ ERROR: argument to `Path::join` starts with a path separator

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

// should not be linted
let path = Path::new("/bin");
path.join("sh");

// should be linted
let path = PathBuf::from("/bin");
path.join("/sh");
}
58 changes: 51 additions & 7 deletions tests/ui/join_absolute_paths.stderr
Original file line number Diff line number Diff line change
@@ -1,23 +1,67 @@
error: argument to `Path::join` starts with a path separator
--> $DIR/join_absolute_paths.rs:9:15
--> $DIR/join_absolute_paths.rs:10:15
|
LL | path.join("/sh");
| ^^^^^
|
= note: joining a path starting with separator will replace the path instead
= help: if this is unintentional, try removing the starting separator
= help: if this is intentional, try creating a new Path instead
= note: `-D clippy::join-absolute-paths` implied by `-D warnings`
help: if this is unintentional, try removing the starting separator
|
LL | path.join("sh");
| ~~~~
help: if this is intentional, try creating a new Path instead
|
LL | PathBuf::from("/sh");
| ~~~~~~~~~~~~~~~~~~~~

error: argument to `Path::join` starts with a path separator
--> $DIR/join_absolute_paths.rs:13:15
--> $DIR/join_absolute_paths.rs:14:15
|
LL | path.join("\\user");
| ^^^^^^^^
|
= note: joining a path starting with separator will replace the path instead
= help: if this is unintentional, try removing the starting separator
= help: if this is intentional, try creating a new Path instead
help: if this is unintentional, try removing the starting separator
|
LL | path.join("\user");
| ~~~~~~~
help: if this is intentional, try creating a new Path instead
|
LL | PathBuf::from("\\user");
| ~~~~~~~~~~~~~~~~~~~~~~~

error: argument to `Path::join` starts with a path separator
--> $DIR/join_absolute_paths.rs:18:15
|
LL | path.join("/sh");
| ^^^^^
|
= note: joining a path starting with separator will replace the path instead
help: if this is unintentional, try removing the starting separator
|
LL | path.join("sh");
| ~~~~
help: if this is intentional, try creating a new Path instead
|
LL | PathBuf::from("/sh");
| ~~~~~~~~~~~~~~~~~~~~

error: argument to `Path::join` starts with a path separator
--> $DIR/join_absolute_paths.rs:22:15
|
LL | path.join(r#"/sh"#);
| ^^^^^^^^
|
= note: joining a path starting with separator will replace the path instead
help: if this is unintentional, try removing the starting separator
|
LL | path.join(r#"sh"#);
| ~~~~~~~
help: if this is intentional, try creating a new Path instead
|
LL | PathBuf::from(r#"/sh"#);
| ~~~~~~~~~~~~~~~~~~~~~~~

error: aborting due to 2 previous errors
error: aborting due to 4 previous errors

0 comments on commit 1a024eb

Please sign in to comment.