diff --git a/CHANGELOG.md b/CHANGELOG.md index 26c9877dbffb..d7b99f33067a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5340,6 +5340,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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 77438b27f900..1f472ca4407f 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -554,6 +554,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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a70a38ee08bf..8eb18aae1b13 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -263,6 +263,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; @@ -1070,6 +1071,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, 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::::default()); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/pathbuf_init_then_push.rs b/clippy_lints/src/pathbuf_init_then_push.rs new file mode 100644 index 000000000000..eae21d06ae4f --- /dev/null +++ b/clippy_lints/src/pathbuf_init_then_push.rs @@ -0,0 +1,151 @@ +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::{declare_tool_lint, 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. + /// + /// ### Example + /// ```rust + /// let mut path_buf = PathBuf::new(); + /// path_buf.push("foo"); + /// ``` + /// Use instead: + /// ```rust + /// let path_buf = PathBuf::new().join("foo"); + /// ``` + #[clippy::version = "1.75.0"] + pub PATHBUF_INIT_THEN_PUSH, + complexity, + "default lint description" +} + +impl_lint_pass!(PathbufThenPush => [PATHBUF_INIT_THEN_PUSH]); + +#[derive(Default)] +pub struct PathbufThenPush { + searcher: Option, +} + +struct PathbufPushSearcher { + local_id: HirId, + lhs_is_let: bool, + let_ty_span: Option, + init_val_span: Span, + arg_span: Option, + 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); + } + } +} diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index 58cb42316fd2..3416a23646dd 100644 --- a/lintcheck/src/main.rs +++ b/lintcheck/src/main.rs @@ -216,10 +216,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() { diff --git a/tests/integration.rs b/tests/integration.rs index 031982edbe9e..7a0e5809c50d 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -29,8 +29,10 @@ fn integration_test() { .nth(1) .expect("repo name should have format `/`"); - 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([ diff --git a/tests/ui/path_buf_push_overwrite.fixed b/tests/ui/path_buf_push_overwrite.fixed index 86e3e5bbdafd..1a46d72378f8 100644 --- a/tests/ui/path_buf_push_overwrite.fixed +++ b/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"); diff --git a/tests/ui/path_buf_push_overwrite.rs b/tests/ui/path_buf_push_overwrite.rs index 460cc254e942..3e3f84b17a4b 100644 --- a/tests/ui/path_buf_push_overwrite.rs +++ b/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"); diff --git a/tests/ui/path_buf_push_overwrite.stderr b/tests/ui/path_buf_push_overwrite.stderr index 1453d020c412..2c2b2ba3fe43 100644 --- a/tests/ui/path_buf_push_overwrite.stderr +++ b/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 - --> $DIR/path_buf_push_overwrite.rs:6:12 + --> $DIR/path_buf_push_overwrite.rs:7:12 | LL | x.push("/bar"); | ^^^^^^ help: try: `"bar"` diff --git a/tests/ui/pathbuf_init_then_push.fixed b/tests/ui/pathbuf_init_then_push.fixed new file mode 100644 index 000000000000..5412a44b43ec --- /dev/null +++ b/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"); +} diff --git a/tests/ui/pathbuf_init_then_push.rs b/tests/ui/pathbuf_init_then_push.rs new file mode 100644 index 000000000000..473654c62c17 --- /dev/null +++ b/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"); +} diff --git a/tests/ui/pathbuf_init_then_push.stderr b/tests/ui/pathbuf_init_then_push.stderr new file mode 100644 index 000000000000..8aca66303053 --- /dev/null +++ b/tests/ui/pathbuf_init_then_push.stderr @@ -0,0 +1,12 @@ +error: calls to `push` immediately after creation + --> $DIR/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 previous error + diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed index 867f5b210171..550a1607de24 100644 --- a/tests/ui/redundant_clone.fixed +++ b/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 )] diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index adcbd01e819c..0e3f13633297 100644 --- a/tests/ui/redundant_clone.rs +++ b/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 )] diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr index 4115fcf21228..54f7ac519ddb 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -1,11 +1,11 @@ error: redundant clone - --> $DIR/redundant_clone.rs:15:42 + --> $DIR/redundant_clone.rs:16:42 | LL | let _s = ["lorem", "ipsum"].join(" ").to_string(); | ^^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:15:14 + --> $DIR/redundant_clone.rs:16: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 - --> $DIR/redundant_clone.rs:18:15 + --> $DIR/redundant_clone.rs:19:15 | LL | let _s = s.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:18:14 + --> $DIR/redundant_clone.rs:19:14 | LL | let _s = s.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:21:15 + --> $DIR/redundant_clone.rs:22:15 | LL | let _s = s.to_string(); | ^^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:21:14 + --> $DIR/redundant_clone.rs:22:14 | LL | let _s = s.to_string(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:24:15 + --> $DIR/redundant_clone.rs:25:15 | LL | let _s = s.to_owned(); | ^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:24:14 + --> $DIR/redundant_clone.rs:25:14 | LL | let _s = s.to_owned(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:26:42 + --> $DIR/redundant_clone.rs:27:42 | LL | let _s = Path::new("/a/b/").join("c").to_owned(); | ^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:26:14 + --> $DIR/redundant_clone.rs:27:14 | LL | let _s = Path::new("/a/b/").join("c").to_owned(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:28:42 + --> $DIR/redundant_clone.rs:29:42 | LL | let _s = Path::new("/a/b/").join("c").to_path_buf(); | ^^^^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:28:14 + --> $DIR/redundant_clone.rs:29:14 | LL | let _s = Path::new("/a/b/").join("c").to_path_buf(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:30:29 + --> $DIR/redundant_clone.rs:31:29 | LL | let _s = OsString::new().to_owned(); | ^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:30:14 + --> $DIR/redundant_clone.rs:31:14 | LL | let _s = OsString::new().to_owned(); | ^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:32:29 + --> $DIR/redundant_clone.rs:33:29 | LL | let _s = OsString::new().to_os_string(); | ^^^^^^^^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:32:14 + --> $DIR/redundant_clone.rs:33:14 | LL | let _s = OsString::new().to_os_string(); | ^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:43:19 + --> $DIR/redundant_clone.rs:44:19 | LL | let _t = tup.0.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:43:14 + --> $DIR/redundant_clone.rs:44:14 | LL | let _t = tup.0.clone(); | ^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:75:25 + --> $DIR/redundant_clone.rs:76:25 | LL | if b { (a.clone(), a.clone()) } else { (Alpha, a) } | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:75:24 + --> $DIR/redundant_clone.rs:76:24 | LL | if b { (a.clone(), a.clone()) } else { (Alpha, a) } | ^ error: redundant clone - --> $DIR/redundant_clone.rs:132:15 + --> $DIR/redundant_clone.rs:133:15 | LL | let _s = s.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:132:14 + --> $DIR/redundant_clone.rs:133:14 | LL | let _s = s.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:133:15 + --> $DIR/redundant_clone.rs:134:15 | LL | let _t = t.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:133:14 + --> $DIR/redundant_clone.rs:134:14 | LL | let _t = t.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:143:19 + --> $DIR/redundant_clone.rs:144:19 | LL | let _f = f.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:143:18 + --> $DIR/redundant_clone.rs:144:18 | LL | let _f = f.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:155:14 + --> $DIR/redundant_clone.rs:156:14 | LL | let y = x.clone().join("matthias"); | ^^^^^^^^ help: remove this | note: cloned value is neither consumed nor mutated - --> $DIR/redundant_clone.rs:155:13 + --> $DIR/redundant_clone.rs:156:13 | LL | let y = x.clone().join("matthias"); | ^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:209:11 + --> $DIR/redundant_clone.rs:210:11 | LL | foo(&x.clone(), move || { | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:209:10 + --> $DIR/redundant_clone.rs:210:10 | LL | foo(&x.clone(), move || { | ^