Skip to content

Commit

Permalink
[pathbuf_init_then_push]: Checks for calls to push immediately af…
Browse files Browse the repository at this point in the history
…ter creating a new `PathBuf`

Co-authored-by: Fridtjof Stoldt <xFrednet@gmail.com>
  • Loading branch information
lengyijun and xFrednet committed Jul 19, 2024
1 parent b31bce4 commit 53eb717
Show file tree
Hide file tree
Showing 14 changed files with 311 additions and 35 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5712,6 +5712,7 @@ Released 2018-09-13
[`partialeq_to_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_to_none
[`path_buf_push_overwrite`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_buf_push_overwrite
[`path_ends_with_ext`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_ends_with_ext
[`pathbuf_init_then_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#pathbuf_init_then_push
[`pattern_type_mismatch`]: https://rust-lang.github.io/rust-clippy/master/index.html#pattern_type_mismatch
[`permissions_set_readonly_false`]: https://rust-lang.github.io/rust-clippy/master/index.html#permissions_set_readonly_false
[`positional_named_format_parameters`]: https://rust-lang.github.io/rust-clippy/master/index.html#positional_named_format_parameters
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::partialeq_to_none::PARTIALEQ_TO_NONE_INFO,
crate::pass_by_ref_or_value::LARGE_TYPES_PASSED_BY_VALUE_INFO,
crate::pass_by_ref_or_value::TRIVIALLY_COPY_PASS_BY_REF_INFO,
crate::pathbuf_init_then_push::PATHBUF_INIT_THEN_PUSH_INFO,
crate::pattern_type_mismatch::PATTERN_TYPE_MISMATCH_INFO,
crate::permissions_set_readonly_false::PERMISSIONS_SET_READONLY_FALSE_INFO,
crate::precedence::PRECEDENCE_INFO,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ mod partial_pub_fields;
mod partialeq_ne_impl;
mod partialeq_to_none;
mod pass_by_ref_or_value;
mod pathbuf_init_then_push;
mod pattern_type_mismatch;
mod permissions_set_readonly_false;
mod precedence;
Expand Down Expand Up @@ -887,6 +888,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
});
store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(conf)));
store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter));
store.register_late_pass(|_| Box::<pathbuf_init_then_push::PathbufThenPush<'_>>::default());
store.register_late_pass(|_| Box::new(iter_over_hash_type::IterOverHashType));
store.register_late_pass(|_| Box::new(impl_hash_with_borrow_str_and_bytes::ImplHashWithBorrowStrBytes));
store.register_late_pass(|_| Box::new(repeat_vec_with_capacity::RepeatVecWithCapacity));
Expand Down
193 changes: 193 additions & 0 deletions clippy_lints/src/pathbuf_init_then_push.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::path_to_local_id;
use clippy_utils::source::{snippet, snippet_opt};
use clippy_utils::ty::is_type_diagnostic_item;
use rustc_ast::{LitKind, StrStyle};
use rustc_errors::Applicability;
use rustc_hir::def::Res;
use rustc_hir::{BindingMode, Block, Expr, ExprKind, HirId, LetStmt, PatKind, QPath, Stmt, StmtKind, TyKind};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_session::impl_lint_pass;
use rustc_span::{sym, Span, Symbol};

declare_clippy_lint! {
/// ### What it does
/// Checks for calls to `push` immediately after creating a new `PathBuf`.
///
/// ### Why is this bad?
/// Multiple `.join()` calls are usually easier to read than multiple `.push`
/// calls across multiple statements. It might also be possible to use
/// `PathBuf::from` instead.
///
/// ### Known problems
/// `.join()` introduces an implicit `clone()`. `PathBuf::from` can alternativly be
/// used when the `PathBuf` is newly constructed. This will avoild the implicit clone.
///
/// ### Example
/// ```rust
/// # use std::path::PathBuf;
/// let mut path_buf = PathBuf::new();
/// path_buf.push("foo");
/// ```
/// Use instead:
/// ```rust
/// # use std::path::PathBuf;
/// let path_buf = PathBuf::from("foo");
/// // or
/// let path_buf = PathBuf::new().join("foo");
/// ```
#[clippy::version = "1.81.0"]
pub PATHBUF_INIT_THEN_PUSH,
restriction,
"`push` immediately after `PathBuf` creation"
}

impl_lint_pass!(PathbufThenPush<'_> => [PATHBUF_INIT_THEN_PUSH]);

#[derive(Default)]
pub struct PathbufThenPush<'tcx> {
searcher: Option<PathbufPushSearcher<'tcx>>,
}

struct PathbufPushSearcher<'tcx> {
local_id: HirId,
lhs_is_let: bool,
let_ty_span: Option<Span>,
init_val: Expr<'tcx>,
arg: Option<Expr<'tcx>>,
name: Symbol,
err_span: Span,
}

impl<'tcx> PathbufPushSearcher<'tcx> {
/// Try to generate a suggestion with `PathBuf::from`.
/// Returns `None` if the suggestion would be invalid.
fn gen_pathbuf_from(&self, cx: &LateContext<'_>) -> Option<String> {
if let ExprKind::Call(iter_expr, []) = &self.init_val.kind
&& let ExprKind::Path(QPath::TypeRelative(ty, segment)) = &iter_expr.kind
&& let TyKind::Path(ty_path) = &ty.kind
&& let QPath::Resolved(None, path) = ty_path
&& let Res::Def(_, def_id) = &path.res
&& cx.tcx.is_diagnostic_item(sym::PathBuf, *def_id)
&& segment.ident.name == sym::new
&& let Some(arg) = self.arg
&& let ExprKind::Lit(x) = arg.kind
&& let LitKind::Str(_, StrStyle::Cooked) = x.node
&& let Some(s) = snippet_opt(cx, arg.span)
{
Some(format!(" = PathBuf::from({s});"))
} else {
None
}
}

fn gen_pathbuf_join(&self, cx: &LateContext<'_>) -> Option<String> {
let arg = self.arg?;
let arg_str = snippet_opt(cx, arg.span)?;
let init_val = snippet_opt(cx, self.init_val.span)?;
Some(format!(" = {init_val}.join({arg_str});"))
}

fn display_err(&self, cx: &LateContext<'_>) {
if clippy_utils::attrs::span_contains_cfg(cx, self.err_span) {
return;
}
let mut sugg = if self.lhs_is_let {
String::from("let mut ")
} else {
String::new()
};
sugg.push_str(self.name.as_str());
if let Some(span) = self.let_ty_span {
sugg.push_str(": ");
sugg.push_str(&snippet(cx, span, "_"));
}
match self.gen_pathbuf_from(cx) {
Some(value) => {
sugg.push_str(&value);
},
None => {
if let Some(value) = self.gen_pathbuf_join(cx) {
sugg.push_str(&value);
} else {
return;
}
},
}

span_lint_and_sugg(
cx,
PATHBUF_INIT_THEN_PUSH,
self.err_span,
"calls to `push` immediately after creation",
"consider using the `.join()`",
sugg,
Applicability::HasPlaceholders,
);
}
}

impl<'tcx> LateLintPass<'tcx> for PathbufThenPush<'tcx> {
fn check_block(&mut self, _: &LateContext<'tcx>, _: &'tcx Block<'tcx>) {
self.searcher = None;
}

fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx LetStmt<'tcx>) {
if let Some(init_expr) = local.init
&& let PatKind::Binding(BindingMode::MUT, id, name, None) = local.pat.kind
&& !in_external_macro(cx.sess(), local.span)
&& let ty = cx.typeck_results().pat_ty(local.pat)
&& is_type_diagnostic_item(cx, ty, sym::PathBuf)
{
self.searcher = Some(PathbufPushSearcher {
local_id: id,
lhs_is_let: true,
name: name.name,
let_ty_span: local.ty.map(|ty| ty.span),
err_span: local.span,
init_val: *init_expr,
arg: None,
});
}
}

fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if let ExprKind::Assign(left, right, _) = expr.kind
&& let ExprKind::Path(QPath::Resolved(None, path)) = left.kind
&& let [name] = &path.segments
&& let Res::Local(id) = path.res
&& !in_external_macro(cx.sess(), expr.span)
&& let ty = cx.typeck_results().expr_ty(left)
&& is_type_diagnostic_item(cx, ty, sym::PathBuf)
{
self.searcher = Some(PathbufPushSearcher {
local_id: id,
lhs_is_let: false,
let_ty_span: None,
name: name.ident.name,
err_span: expr.span,
init_val: *right,
arg: None,
});
}
}

fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
if let Some(mut searcher) = self.searcher.take() {
if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = stmt.kind
&& let ExprKind::MethodCall(name, self_arg, [arg_expr], _) = expr.kind
&& path_to_local_id(self_arg, searcher.local_id)
&& name.ident.as_str() == "push"
{
searcher.err_span = searcher.err_span.to(stmt.span);
searcher.arg = Some(*arg_expr);
searcher.display_err(cx);
}
}
}

fn check_block_post(&mut self, _: &LateContext<'tcx>, _: &'tcx Block<'tcx>) {
self.searcher = None;
}
}
6 changes: 4 additions & 2 deletions tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@ fn integration_test() {
.nth(1)
.expect("repo name should have format `<org>/<name>`");

let mut repo_dir = tempfile::tempdir().expect("couldn't create temp dir").into_path();
repo_dir.push(crate_name);
let repo_dir = tempfile::tempdir()
.expect("couldn't create temp dir")
.into_path()
.join(crate_name);

let st = Command::new("git")
.args([
Expand Down
3 changes: 2 additions & 1 deletion tests/ui/path_buf_push_overwrite.fixed
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::path::PathBuf;

#[warn(clippy::all, clippy::path_buf_push_overwrite)]
#[warn(clippy::path_buf_push_overwrite)]
#[allow(clippy::pathbuf_init_then_push)]
fn main() {
let mut x = PathBuf::from("/foo");
x.push("bar");
Expand Down
3 changes: 2 additions & 1 deletion tests/ui/path_buf_push_overwrite.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::path::PathBuf;

#[warn(clippy::all, clippy::path_buf_push_overwrite)]
#[warn(clippy::path_buf_push_overwrite)]
#[allow(clippy::pathbuf_init_then_push)]
fn main() {
let mut x = PathBuf::from("/foo");
x.push("/bar");
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/path_buf_push_overwrite.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: calling `push` with '/' or '\' (file system root) will overwrite the previous path definition
--> tests/ui/path_buf_push_overwrite.rs:6:12
--> tests/ui/path_buf_push_overwrite.rs:7:12
|
LL | x.push("/bar");
| ^^^^^^ help: try: `"bar"`
Expand Down
18 changes: 18 additions & 0 deletions tests/ui/pathbuf_init_then_push.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#![warn(clippy::pathbuf_init_then_push)]

use std::path::PathBuf;

fn main() {
let mut path_buf = PathBuf::from("foo");

path_buf = PathBuf::from("foo").join("bar");

let bar = "bar";
path_buf = PathBuf::from("foo").join(bar);

let mut path_buf = PathBuf::from("foo").join("bar").join("buz");

let mut x = PathBuf::new();
println!("{}", x.display());
x.push("Duck");
}
22 changes: 22 additions & 0 deletions tests/ui/pathbuf_init_then_push.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#![warn(clippy::pathbuf_init_then_push)]

use std::path::PathBuf;

fn main() {
let mut path_buf = PathBuf::new(); //~ ERROR: calls to `push` immediately after creation
path_buf.push("foo");

path_buf = PathBuf::from("foo"); //~ ERROR: calls to `push` immediately after creation
path_buf.push("bar");

let bar = "bar";
path_buf = PathBuf::from("foo"); //~ ERROR: calls to `push` immediately after creation
path_buf.push(bar);

let mut path_buf = PathBuf::from("foo").join("bar"); //~ ERROR: calls to `push` immediately after creation
path_buf.push("buz");

let mut x = PathBuf::new();
println!("{}", x.display());
x.push("Duck");
}
33 changes: 33 additions & 0 deletions tests/ui/pathbuf_init_then_push.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
error: calls to `push` immediately after creation
--> tests/ui/pathbuf_init_then_push.rs:6:5
|
LL | / let mut path_buf = PathBuf::new();
LL | | path_buf.push("foo");
| |_________________________^ help: consider using the `.join()`: `let mut path_buf = PathBuf::from("foo");`
|
= note: `-D clippy::pathbuf-init-then-push` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::pathbuf_init_then_push)]`

error: calls to `push` immediately after creation
--> tests/ui/pathbuf_init_then_push.rs:9:5
|
LL | / path_buf = PathBuf::from("foo");
LL | | path_buf.push("bar");
| |_________________________^ help: consider using the `.join()`: `path_buf = PathBuf::from("foo").join("bar");`

error: calls to `push` immediately after creation
--> tests/ui/pathbuf_init_then_push.rs:13:5
|
LL | / path_buf = PathBuf::from("foo");
LL | | path_buf.push(bar);
| |_______________________^ help: consider using the `.join()`: `path_buf = PathBuf::from("foo").join(bar);`

error: calls to `push` immediately after creation
--> tests/ui/pathbuf_init_then_push.rs:16:5
|
LL | / let mut path_buf = PathBuf::from("foo").join("bar");
LL | | path_buf.push("buz");
| |_________________________^ help: consider using the `.join()`: `let mut path_buf = PathBuf::from("foo").join("bar").join("buz");`

error: aborting due to 4 previous errors

1 change: 1 addition & 0 deletions tests/ui/redundant_clone.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#![allow(
clippy::drop_non_drop,
clippy::implicit_clone,
clippy::pathbuf_init_then_push,
clippy::uninlined_format_args,
clippy::unnecessary_literal_unwrap
)]
Expand Down
1 change: 1 addition & 0 deletions tests/ui/redundant_clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#![allow(
clippy::drop_non_drop,
clippy::implicit_clone,
clippy::pathbuf_init_then_push,
clippy::uninlined_format_args,
clippy::unnecessary_literal_unwrap
)]
Expand Down
Loading

0 comments on commit 53eb717

Please sign in to comment.