Auto merge of #11700 - lengyijun:pathbuf_join, r=xFrednet

[`pathbuf_init_then_push`]:  Checks for calls to `push` immediately a…

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

Just a mirror of VEC_INIT_THEN_PUSH

CHANGELOG.md

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

clippy_lints/src/declared_lints.rs

@@ -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,

clippy_lints/src/lib.rs

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

clippy_lints/src/pathbuf_init_then_push.rs

@@ -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;
+    }
+}

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,22 @@
+#![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");
+
+    let mut path_buf = PathBuf::new();
+    #[cfg(cats)]
+    path_buf.push("foo");
+}

tests/ui/pathbuf_init_then_push.rs

@@ -0,0 +1,26 @@
+#![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");
+
+    let mut path_buf = PathBuf::new();
+    #[cfg(cats)]
+    path_buf.push("foo");
+}

tests/ui/pathbuf_init_then_push.stderr

@@ -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
+

tests/ui/redundant_clone.fixed

@@ -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
 )]

tests/ui/redundant_clone.rs

@@ -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
 )]

tests/ui/redundant_clone.stderr

@@ -1,11 +1,11 @@
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:14:42
+  --> tests/ui/redundant_clone.rs:15:42
    |
 LL |     let _s = ["lorem", "ipsum"].join(" ").to_string();
    |                                          ^^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:14:14
+  --> tests/ui/redundant_clone.rs:15:14
    |
 LL |     let _s = ["lorem", "ipsum"].join(" ").to_string();
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -13,169 +13,169 @@ LL |     let _s = ["lorem", "ipsum"].join(" ").to_string();
    = help: to override `-D warnings` add `#[allow(clippy::redundant_clone)]`
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:17:15
+  --> tests/ui/redundant_clone.rs:18:15
    |
 LL |     let _s = s.clone();
    |               ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:17:14
+  --> tests/ui/redundant_clone.rs:18:14
    |
 LL |     let _s = s.clone();
    |              ^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:20:15
+  --> tests/ui/redundant_clone.rs:21:15
    |
 LL |     let _s = s.to_string();
    |               ^^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:20:14
+  --> tests/ui/redundant_clone.rs:21:14
    |
 LL |     let _s = s.to_string();
    |              ^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:23:15
+  --> tests/ui/redundant_clone.rs:24:15
    |
 LL |     let _s = s.to_owned();
    |               ^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:23:14
+  --> tests/ui/redundant_clone.rs:24:14
    |
 LL |     let _s = s.to_owned();
    |              ^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:25:42
+  --> tests/ui/redundant_clone.rs:26:42
    |
 LL |     let _s = Path::new("/a/b/").join("c").to_owned();
    |                                          ^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:25:14
+  --> tests/ui/redundant_clone.rs:26:14
    |
 LL |     let _s = Path::new("/a/b/").join("c").to_owned();
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:27:42
+  --> tests/ui/redundant_clone.rs:28:42
    |
 LL |     let _s = Path::new("/a/b/").join("c").to_path_buf();
    |                                          ^^^^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:27:14
+  --> tests/ui/redundant_clone.rs:28:14
    |
 LL |     let _s = Path::new("/a/b/").join("c").to_path_buf();
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:29:29
+  --> tests/ui/redundant_clone.rs:30:29
    |
 LL |     let _s = OsString::new().to_owned();
    |                             ^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:29:14
+  --> tests/ui/redundant_clone.rs:30:14
    |
 LL |     let _s = OsString::new().to_owned();
    |              ^^^^^^^^^^^^^^^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:31:29
+  --> tests/ui/redundant_clone.rs:32:29
    |
 LL |     let _s = OsString::new().to_os_string();
    |                             ^^^^^^^^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:31:14
+  --> tests/ui/redundant_clone.rs:32:14
    |
 LL |     let _s = OsString::new().to_os_string();
    |              ^^^^^^^^^^^^^^^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:42:19
+  --> tests/ui/redundant_clone.rs:43:19
    |
 LL |     let _t = tup.0.clone();
    |                   ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:42:14
+  --> tests/ui/redundant_clone.rs:43:14
    |
 LL |     let _t = tup.0.clone();
    |              ^^^^^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:74:25
+  --> tests/ui/redundant_clone.rs:75:25
    |
 LL |     if b { (a.clone(), a.clone()) } else { (Alpha, a) }
    |                         ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:74:24
+  --> tests/ui/redundant_clone.rs:75:24
    |
 LL |     if b { (a.clone(), a.clone()) } else { (Alpha, a) }
    |                        ^
 
-error: redundant clone
-  --> tests/ui/redundant_clone.rs:131:15
-   |
-LL |     let _s = s.clone();
-   |               ^^^^^^^^ help: remove this
-   |
-note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:131:14
-   |
-LL |     let _s = s.clone();
-   |              ^
-
 error: redundant clone
   --> tests/ui/redundant_clone.rs:132:15
    |
-LL |     let _t = t.clone();
+LL |     let _s = s.clone();
    |               ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
   --> tests/ui/redundant_clone.rs:132:14
    |
+LL |     let _s = s.clone();
+   |              ^
+
+error: redundant clone
+  --> tests/ui/redundant_clone.rs:133:15
+   |
+LL |     let _t = t.clone();
+   |               ^^^^^^^^ help: remove this
+   |
+note: this value is dropped without further use
+  --> tests/ui/redundant_clone.rs:133:14
+   |
 LL |     let _t = t.clone();
    |              ^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:142:19
+  --> tests/ui/redundant_clone.rs:143:19
    |
 LL |         let _f = f.clone();
    |                   ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:142:18
+  --> tests/ui/redundant_clone.rs:143:18
    |
 LL |         let _f = f.clone();
    |                  ^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:154:14
+  --> tests/ui/redundant_clone.rs:155:14
    |
 LL |     let y = x.clone().join("matthias");
    |              ^^^^^^^^ help: remove this
    |
 note: cloned value is neither consumed nor mutated
-  --> tests/ui/redundant_clone.rs:154:13
+  --> tests/ui/redundant_clone.rs:155:13
    |
 LL |     let y = x.clone().join("matthias");
    |             ^^^^^^^^^
 
 error: redundant clone
-  --> tests/ui/redundant_clone.rs:208:11
+  --> tests/ui/redundant_clone.rs:209:11
    |
 LL |     foo(&x.clone(), move || {
    |           ^^^^^^^^ help: remove this
    |
 note: this value is dropped without further use
-  --> tests/ui/redundant_clone.rs:208:10
+  --> tests/ui/redundant_clone.rs:209:10
    |
 LL |     foo(&x.clone(), move || {
    |          ^