Skip to content

Commit a72ad23

Browse files
lengyijunlengyijun
committed
[pathbuf_init_then_push]: Checks for calls to push immediately after creating a new PathBuf 
1 parent 3b1b654 commit a72ad23

16 files changed

+286
-38
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5591,6 +5591,7 @@ Released 2018-09-13
55915591
[`partialeq_to_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_to_none
55925592
[`path_buf_push_overwrite`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_buf_push_overwrite
55935593
[`path_ends_with_ext`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_ends_with_ext
5594+
[`pathbuf_init_then_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#pathbuf_init_then_push
55945595
[`pattern_type_mismatch`]: https://rust-lang.github.io/rust-clippy/master/index.html#pattern_type_mismatch
55955596
[`permissions_set_readonly_false`]: https://rust-lang.github.io/rust-clippy/master/index.html#permissions_set_readonly_false
55965597
[`positional_named_format_parameters`]: https://rust-lang.github.io/rust-clippy/master/index.html#positional_named_format_parameters

‎c

3.6 MB
Binary file not shown.

‎clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
590590
crate::partialeq_to_none::PARTIALEQ_TO_NONE_INFO,
591591
crate::pass_by_ref_or_value::LARGE_TYPES_PASSED_BY_VALUE_INFO,
592592
crate::pass_by_ref_or_value::TRIVIALLY_COPY_PASS_BY_REF_INFO,
593+
crate::pathbuf_init_then_push::PATHBUF_INIT_THEN_PUSH_INFO,
593594
crate::pattern_type_mismatch::PATTERN_TYPE_MISMATCH_INFO,
594595
crate::permissions_set_readonly_false::PERMISSIONS_SET_READONLY_FALSE_INFO,
595596
crate::precedence::PRECEDENCE_INFO,

‎clippy_lints/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,7 @@ mod partial_pub_fields;
282282
mod partialeq_ne_impl;
283283
mod partialeq_to_none;
284284
mod pass_by_ref_or_value;
285+
mod pathbuf_init_then_push;
285286
mod pattern_type_mismatch;
286287
mod permissions_set_readonly_false;
287288
mod precedence;
@@ -1110,6 +1111,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
11101111
});
11111112
store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(msrv())));
11121113
store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter));
1114+
store.register_late_pass(|_| Box::<pathbuf_init_then_push::PathbufThenPush<'_>>::default());
11131115
store.register_late_pass(|_| Box::new(iter_over_hash_type::IterOverHashType));
11141116
store.register_late_pass(|_| Box::new(impl_hash_with_borrow_str_and_bytes::ImplHashWithBorrowStrBytes));
11151117
store.register_late_pass(|_| Box::new(repeat_vec_with_capacity::RepeatVecWithCapacity));

‎clippy_lints/src/pathbuf_init_then_push.rs

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::path_to_local_id;
3+
use clippy_utils::source::{snippet, snippet_opt};
4+
use clippy_utils::ty::is_type_diagnostic_item;
5+
use rustc_ast::{LitKind, StrStyle};
6+
use rustc_errors::Applicability;
7+
use rustc_hir::def::Res;
8+
use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, Local, PatKind, QPath, Stmt, StmtKind, TyKind};
9+
use rustc_lint::{LateContext, LateLintPass, LintContext};
10+
use rustc_middle::lint::in_external_macro;
11+
use rustc_session::impl_lint_pass;
12+
use rustc_span::{sym, Span, Symbol};
13+
14+
declare_clippy_lint! {
15+
/// ### What it does
16+
/// Checks for calls to `push` immediately after creating a new `PathBuf`.
17+
///
18+
/// ### Why is this bad?
19+
/// The `.join()` is easier to read than multiple `push` calls.
20+
///
21+
/// ### Known problems
22+
/// `.join()` introduces an implicit `clone()`
23+
///
24+
/// ### Example
25+
/// ```rust
26+
/// use std::path::PathBuf;
27+
/// let mut path_buf = PathBuf::new();
28+
/// path_buf.push("foo");
29+
/// ```
30+
/// Use instead:
31+
/// ```rust
32+
/// use std::path::PathBuf;
33+
/// let path_buf = PathBuf::new().join("foo");
34+
/// ```
35+
#[clippy::version = "1.78.0"]
36+
pub PATHBUF_INIT_THEN_PUSH,
37+
complexity,
38+
"`push` immediately after `PathBuf` creation"
39+
}
40+
41+
impl_lint_pass!(PathbufThenPush<'_> => [PATHBUF_INIT_THEN_PUSH]);
42+
43+
#[derive(Default)]
44+
pub struct PathbufThenPush<'tcx> {
45+
searcher: Option<PathbufPushSearcher<'tcx>>,
46+
}
47+
48+
struct PathbufPushSearcher<'tcx> {
49+
local_id: HirId,
50+
lhs_is_let: bool,
51+
let_ty_span: Option<Span>,
52+
init_val: Expr<'tcx>,
53+
arg: Option<Expr<'tcx>>,
54+
name: Symbol,
55+
err_span: Span,
56+
}
57+
58+
impl<'tcx> PathbufPushSearcher<'tcx> {
59+
// try to generate `PathBuf::from`
60+
fn gen_pathbuf_from(&self, cx: &LateContext<'_>) -> Option<String> {
61+
if let ExprKind::Call(iter_expr, []) = &self.init_val.kind
62+
&& let ExprKind::Path(QPath::TypeRelative(ty, segment)) = &iter_expr.kind
63+
&& let TyKind::Path(ty_path) = &ty.kind
64+
&& let QPath::Resolved(None, path) = ty_path
65+
&& let Res::Def(_, def_id) = &path.res
66+
&& cx.tcx.is_diagnostic_item(sym::PathBuf, *def_id)
67+
&& segment.ident.name == sym::new
68+
&& let Some(arg) = self.arg
69+
&& let ExprKind::Lit(x) = arg.kind
70+
&& let LitKind::Str(_, StrStyle::Cooked) = x.node
71+
&& let Some(s) = snippet_opt(cx, arg.span)
72+
{
73+
Some(format!(" = PathBuf::from({s});"))
74+
} else {
75+
None
76+
}
77+
}
78+
79+
fn gen_pathbuf_join(&self, cx: &LateContext<'_>) -> Option<String> {
80+
let arg = self.arg?;
81+
let arg_str = snippet_opt(cx, arg.span)?;
82+
let init_val = snippet_opt(cx, self.init_val.span)?;
83+
Some(format!(" = {init_val}.join({arg_str});"))
84+
}
85+
86+
fn display_err(&self, cx: &LateContext<'_>) {
87+
let mut sugg = if self.lhs_is_let {
88+
String::from("let mut ")
89+
} else {
90+
String::new()
91+
};
92+
sugg.push_str(self.name.as_str());
93+
if let Some(span) = self.let_ty_span {
94+
sugg.push_str(": ");
95+
sugg.push_str(&snippet(cx, span, "_"));
96+
}
97+
match self.gen_pathbuf_from(cx) {
98+
Some(value) => {
99+
sugg.push_str(&value);
100+
},
101+
None => {
102+
if let Some(value) = self.gen_pathbuf_join(cx) {
103+
sugg.push_str(&value);
104+
} else {
105+
return;
106+
}
107+
},
108+
}
109+
110+
span_lint_and_sugg(
111+
cx,
112+
PATHBUF_INIT_THEN_PUSH,
113+
self.err_span,
114+
"calls to `push` immediately after creation",
115+
"consider using the `.join()`",
116+
sugg,
117+
Applicability::HasPlaceholders,
118+
);
119+
}
120+
}
121+
122+
impl<'tcx> LateLintPass<'tcx> for PathbufThenPush<'tcx> {
123+
fn check_block(&mut self, _: &LateContext<'tcx>, _: &'tcx Block<'tcx>) {
124+
self.searcher = None;
125+
}
126+
127+
fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx Local<'tcx>) {
128+
if let Some(init_expr) = local.init
129+
&& let PatKind::Binding(BindingAnnotation::MUT, id, name, None) = local.pat.kind
130+
&& !in_external_macro(cx.sess(), local.span)
131+
&& let ty = cx.typeck_results().pat_ty(local.pat)
132+
&& is_type_diagnostic_item(cx, ty, sym::PathBuf)
133+
{
134+
self.searcher = Some(PathbufPushSearcher {
135+
local_id: id,
136+
lhs_is_let: true,
137+
name: name.name,
138+
let_ty_span: local.ty.map(|ty| ty.span),
139+
err_span: local.span,
140+
init_val: *init_expr,
141+
arg: None,
142+
});
143+
}
144+
}
145+
146+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
147+
if self.searcher.is_none()
148+
&& let ExprKind::Assign(left, right, _) = expr.kind
149+
&& let ExprKind::Path(QPath::Resolved(None, path)) = left.kind
150+
&& let [name] = &path.segments
151+
&& let Res::Local(id) = path.res
152+
&& !in_external_macro(cx.sess(), expr.span)
153+
&& let ty = cx.typeck_results().expr_ty(left)
154+
&& is_type_diagnostic_item(cx, ty, sym::PathBuf)
155+
{
156+
self.searcher = Some(PathbufPushSearcher {
157+
local_id: id,
158+
lhs_is_let: false,
159+
let_ty_span: None,
160+
name: name.ident.name,
161+
err_span: expr.span,
162+
init_val: *right,
163+
arg: None,
164+
});
165+
}
166+
}
167+
168+
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
169+
if let Some(mut searcher) = self.searcher.take() {
170+
if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = stmt.kind
171+
&& let ExprKind::MethodCall(name, self_arg, [arg_expr], _) = expr.kind
172+
&& path_to_local_id(self_arg, searcher.local_id)
173+
&& name.ident.as_str() == "push"
174+
{
175+
searcher.err_span = searcher.err_span.to(stmt.span);
176+
searcher.arg = Some(*arg_expr);
177+
searcher.display_err(cx);
178+
}
179+
}
180+
}
181+
182+
fn check_block_post(&mut self, cx: &LateContext<'tcx>, _: &'tcx Block<'tcx>) {
183+
if let Some(searcher) = self.searcher.take() {
184+
searcher.display_err(cx);
185+
}
186+
}
187+
}

‎lintcheck/src/main.rs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -223,10 +223,8 @@ impl CrateSource {
223223
options,
224224
} => {
225225
let repo_path = {
226-
let mut repo_path = PathBuf::from(LINTCHECK_SOURCES);
227226
// add a -git suffix in case we have the same crate from crates.io and a git repo
228-
repo_path.push(format!("{name}-git"));
229-
repo_path
227+
PathBuf::from(LINTCHECK_SOURCES).join(format!("{name}-git"))
230228
};
231229
// clone the repo if we have not done so
232230
if !repo_path.is_dir() {

‎tests/integration.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,10 @@ fn integration_test() {
2929
.nth(1)
3030
.expect("repo name should have format `<org>/<name>`");
3131

32-
let mut repo_dir = tempfile::tempdir().expect("couldn't create temp dir").into_path();
33-
repo_dir.push(crate_name);
32+
let repo_dir = tempfile::tempdir()
33+
.expect("couldn't create temp dir")
34+
.into_path()
35+
.join(crate_name);
3436

3537
let st = Command::new("git")
3638
.args([

‎tests/ui/path_buf_push_overwrite.fixed

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use std::path::PathBuf;
22

3-
#[warn(clippy::all, clippy::path_buf_push_overwrite)]
3+
#[warn(clippy::path_buf_push_overwrite)]
4+
#[allow(clippy::pathbuf_init_then_push)]
45
fn main() {
56
let mut x = PathBuf::from("/foo");
67
x.push("bar");

‎tests/ui/path_buf_push_overwrite.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use std::path::PathBuf;
22

3-
#[warn(clippy::all, clippy::path_buf_push_overwrite)]
3+
#[warn(clippy::path_buf_push_overwrite)]
4+
#[allow(clippy::pathbuf_init_then_push)]
45
fn main() {
56
let mut x = PathBuf::from("/foo");
67
x.push("/bar");

‎tests/ui/path_buf_push_overwrite.stderr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: calling `push` with '/' or '\' (file system root) will overwrite the previous path definition
2-
--> tests/ui/path_buf_push_overwrite.rs:6:12
2+
--> tests/ui/path_buf_push_overwrite.rs:7:12
33
|
44
LL | x.push("/bar");
55
| ^^^^^^ help: try: `"bar"`

‎tests/ui/pathbuf_init_then_push.fixed

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
#![warn(clippy::pathbuf_init_then_push)]
2+
3+
use std::path::PathBuf;
4+
5+
fn main() {
6+
let mut path_buf = PathBuf::from("foo");
7+
8+
path_buf = PathBuf::from("foo").join("bar");
9+
10+
let bar = "bar";
11+
path_buf = PathBuf::from("foo").join(bar);
12+
}

‎tests/ui/pathbuf_init_then_push.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
#![warn(clippy::pathbuf_init_then_push)]
2+
3+
use std::path::PathBuf;
4+
5+
fn main() {
6+
let mut path_buf = PathBuf::new();
7+
path_buf.push("foo");
8+
9+
path_buf = PathBuf::from("foo");
10+
path_buf.push("bar");
11+
12+
let bar = "bar";
13+
path_buf = PathBuf::from("foo");
14+
path_buf.push(bar);
15+
}

‎tests/ui/pathbuf_init_then_push.stderr

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
error: calls to `push` immediately after creation
2+
--> tests/ui/pathbuf_init_then_push.rs:6:5
3+
|
4+
LL | / let mut path_buf = PathBuf::new();
5+
LL | | path_buf.push("foo");
6+
| |_________________________^ help: consider using the `.join()`: `let mut path_buf = PathBuf::from("foo");`
7+
|
8+
= note: `-D clippy::pathbuf-init-then-push` implied by `-D warnings`
9+
= help: to override `-D warnings` add `#[allow(clippy::pathbuf_init_then_push)]`
10+
11+
error: calls to `push` immediately after creation
12+
--> tests/ui/pathbuf_init_then_push.rs:9:5
13+
|
14+
LL | / path_buf = PathBuf::from("foo");
15+
LL | | path_buf.push("bar");
16+
| |_________________________^ help: consider using the `.join()`: `path_buf = PathBuf::from("foo").join("bar");`
17+
18+
error: calls to `push` immediately after creation
19+
--> tests/ui/pathbuf_init_then_push.rs:13:5
20+
|
21+
LL | / path_buf = PathBuf::from("foo");
22+
LL | | path_buf.push(bar);
23+
| |_______________________^ help: consider using the `.join()`: `path_buf = PathBuf::from("foo").join(bar);`
24+
25+
error: aborting due to 3 previous errors
26+

‎tests/ui/redundant_clone.fixed

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#![allow(
55
clippy::drop_non_drop,
66
clippy::implicit_clone,
7+
clippy::pathbuf_init_then_push,
78
clippy::uninlined_format_args,
89
clippy::unnecessary_literal_unwrap
910
)]

‎tests/ui/redundant_clone.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#![allow(
55
clippy::drop_non_drop,
66
clippy::implicit_clone,
7+
clippy::pathbuf_init_then_push,
78
clippy::uninlined_format_args,
89
clippy::unnecessary_literal_unwrap
910
)]

0 commit comments

Comments
 (0)