[pathbuf_init_then_push]: Checks for calls to push immediately after creating a new PathBuf

Author: lengyijun

CHANGELOG.md

@@ -5591,6 +5591,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

clippy_lints/src/declared_lints.rs

@@ -590,6 +590,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,

clippy_lints/src/lib.rs

@@ -282,6 +282,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;
@@ -1110,6 +1111,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(msrv())));
     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));

clippy_lints/src/pathbuf_init_then_push.rs

@@ -0,0 +1,156 @@
+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_errors::Applicability;
+use rustc_hir::def::Res;
+use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, Local, PatKind, QPath, Stmt, StmtKind};
+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?
+    /// The `.join()` is easier to read than multiple `push` calls.
+    ///
+    /// ### Known problems
+    /// `.join()` introduces an 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::new().join("foo");
+    /// ```
+    #[clippy::version = "1.75.0"]
+    pub PATHBUF_INIT_THEN_PUSH,
+    complexity,
+    "`push` immediately after `PathBuf` creation"
+}
+
+impl_lint_pass!(PathbufThenPush => [PATHBUF_INIT_THEN_PUSH]);
+
+#[derive(Default)]
+pub struct PathbufThenPush {
+    searcher: Option<PathbufPushSearcher>,
+}
+
+struct PathbufPushSearcher {
+    local_id: HirId,
+    lhs_is_let: bool,
+    let_ty_span: Option<Span>,
+    init_val_span: Span,
+    arg_span: Option<Span>,
+    name: Symbol,
+    err_span: Span,
+}
+
+impl PathbufPushSearcher {
+    fn display_err(&self, cx: &LateContext<'_>) {
+        let Some(arg_span) = self.arg_span else { return };
+        let Some(arg_str) = snippet_opt(cx, arg_span) else {
+            return;
+        };
+        let Some(init_val) = snippet_opt(cx, self.init_val_span) else {
+            return;
+        };
+        let mut s = if self.lhs_is_let {
+            String::from("let ")
+        } else {
+            String::new()
+        };
+        s.push_str("mut ");
+        s.push_str(self.name.as_str());
+        if let Some(span) = self.let_ty_span {
+            s.push_str(": ");
+            s.push_str(&snippet(cx, span, "_"));
+        }
+        s.push_str(&format!(" = {init_val}.join({arg_str});",));
+
+        span_lint_and_sugg(
+            cx,
+            PATHBUF_INIT_THEN_PUSH,
+            self.err_span,
+            "calls to `push` immediately after creation",
+            "consider using the `.join()`",
+            s,
+            Applicability::HasPlaceholders,
+        );
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for PathbufThenPush {
+    fn check_block(&mut self, _: &LateContext<'tcx>, _: &'tcx Block<'tcx>) {
+        self.searcher = None;
+    }
+
+    fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx Local<'tcx>) {
+        if let Some(init_expr) = local.init
+            && let PatKind::Binding(BindingAnnotation::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_span: init_expr.span,
+                arg_span: None,
+            });
+        }
+    }
+
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        if self.searcher.is_none()
+            && 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_span: right.span,
+                arg_span: 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_span = Some(arg_expr.span);
+                searcher.display_err(cx);
+            }
+        }
+    }
+
+    fn check_block_post(&mut self, cx: &LateContext<'tcx>, _: &'tcx Block<'tcx>) {
+        if let Some(searcher) = self.searcher.take() {
+            searcher.display_err(cx);
+        }
+    }
+}

lintcheck/src/main.rs

@@ -223,10 +223,8 @@ impl CrateSource {
                 options,
             } => {
                 let repo_path = {
-                    let mut repo_path = PathBuf::from(LINTCHECK_SOURCES);
                     // add a -git suffix in case we have the same crate from crates.io and a git repo
-                    repo_path.push(format!("{name}-git"));
-                    repo_path
+                    PathBuf::from(LINTCHECK_SOURCES).join(format!("{name}-git"))
                 };
                 // clone the repo if we have not done so
                 if !repo_path.is_dir() {

tests/integration.rs

@@ -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([

tests/ui/path_buf_push_overwrite.fixed

@@ -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");

tests/ui/path_buf_push_overwrite.rs

@@ -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");

tests/ui/path_buf_push_overwrite.stderr

@@ -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"`

tests/ui/pathbuf_init_then_push.fixed

@@ -0,0 +1,7 @@
+#![warn(clippy::pathbuf_init_then_push)]
+
+use std::path::PathBuf;
+
+fn main() {
+    let mut path_buf = PathBuf::new().join("foo");
+}

tests/ui/pathbuf_init_then_push.rs

@@ -0,0 +1,8 @@
+#![warn(clippy::pathbuf_init_then_push)]
+
+use std::path::PathBuf;
+
+fn main() {
+    let mut path_buf = PathBuf::new();
+    path_buf.push("foo");
+}

tests/ui/pathbuf_init_then_push.stderr

@@ -0,0 +1,12 @@
+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::new().join("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: aborting due to 1 previous error
+

tests/ui/redundant_clone.fixed

@@ -4,6 +4,7 @@
 #![allow(
     clippy::drop_non_drop,
     clippy::implicit_clone,
+    clippy::pathbuf_init_then_push,
     clippy::uninlined_format_args,
     clippy::unnecessary_literal_unwrap
 )]

tests/ui/redundant_clone.rs

@@ -4,6 +4,7 @@
 #![allow(
     clippy::drop_non_drop,
     clippy::implicit_clone,
+    clippy::pathbuf_init_then_push,
     clippy::uninlined_format_args,
     clippy::unnecessary_literal_unwrap
 )]