Skip to content

Commit 0e88347

Browse files
committed
new lint: needless_path_new
1 parent 8a5dc7c commit 0e88347

File tree

7 files changed

+356
-0
lines changed

7 files changed

+356
-0
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6434,6 +6434,7 @@ Released 2018-09-13
64346434
[`needless_parens_on_range_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_parens_on_range_literals
64356435
[`needless_pass_by_ref_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_ref_mut
64366436
[`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value
6437+
[`needless_path_new`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_path_new
64376438
[`needless_pub_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pub_self
64386439
[`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark
64396440
[`needless_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop

clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -553,6 +553,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
553553
crate::needless_parens_on_range_literals::NEEDLESS_PARENS_ON_RANGE_LITERALS_INFO,
554554
crate::needless_pass_by_ref_mut::NEEDLESS_PASS_BY_REF_MUT_INFO,
555555
crate::needless_pass_by_value::NEEDLESS_PASS_BY_VALUE_INFO,
556+
crate::needless_path_new::NEEDLESS_PATH_NEW_INFO,
556557
crate::needless_question_mark::NEEDLESS_QUESTION_MARK_INFO,
557558
crate::needless_update::NEEDLESS_UPDATE_INFO,
558559
crate::neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD_INFO,

clippy_lints/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,7 @@ mod needless_maybe_sized;
269269
mod needless_parens_on_range_literals;
270270
mod needless_pass_by_ref_mut;
271271
mod needless_pass_by_value;
272+
mod needless_path_new;
272273
mod needless_question_mark;
273274
mod needless_update;
274275
mod neg_cmp_op_on_partial_ord;
@@ -831,5 +832,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
831832
store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom));
832833
store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny));
833834
store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg));
835+
store.register_late_pass(|_| Box::new(needless_path_new::NeedlessPathNew));
834836
// add lints here, do not remove this comment, it's used in `new_lint`
835837
}
Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::sugg::Sugg;
3+
use clippy_utils::{expr_or_init, is_path_diagnostic_item, path_res};
4+
use rustc_errors::Applicability;
5+
use rustc_hir::def::{CtorKind, DefKind, Res};
6+
use rustc_hir::{Expr, ExprKind, QPath};
7+
use rustc_infer::infer::InferCtxt;
8+
use rustc_infer::traits::{Obligation, ObligationCause};
9+
use rustc_lint::{LateContext, LateLintPass};
10+
use rustc_middle::ty::{self, GenericPredicates, ParamTy, PredicatePolarity, Ty};
11+
use rustc_session::declare_lint_pass;
12+
use rustc_span::sym;
13+
use rustc_trait_selection::infer::TyCtxtInferExt;
14+
use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;
15+
use std::iter;
16+
17+
declare_clippy_lint! {
18+
/// ### What it does
19+
/// Detects expressions being enclosed in `Path::new` when passed to a function which would
20+
/// accept the enclosed expression directly.
21+
///
22+
/// ### Why is this bad?
23+
/// It is unnecessarily verbose
24+
///
25+
/// ### Example
26+
/// ```no_run
27+
/// # use std::{fs, path::Path};
28+
/// fs::write(Path::new("foo.txt"), "foo");
29+
/// ```
30+
/// Use instead:
31+
/// ```no_run
32+
/// # use std::{fs, path::Path};
33+
/// fs::write("foo.txt", "foo");
34+
/// ```
35+
#[clippy::version = "1.92.0"]
36+
pub NEEDLESS_PATH_NEW,
37+
nursery,
38+
"`Path::new(x)` passed as an argument where `x` would suffice"
39+
}
40+
41+
declare_lint_pass!(NeedlessPathNew => [NEEDLESS_PATH_NEW]);
42+
43+
impl<'tcx> LateLintPass<'tcx> for NeedlessPathNew {
44+
fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) {
45+
let tcx = cx.tcx;
46+
47+
let (fn_did, args) = match e.kind {
48+
ExprKind::Call(callee, args)
49+
if let Res::Def(DefKind::Fn | DefKind::AssocFn | DefKind::Ctor(_, CtorKind::Fn), did) =
50+
// re: `expr_or_init`: `callee` might be a variable storing a fn ptr, for example,
51+
// so we need to get to the actual initializer
52+
path_res(cx, expr_or_init(cx, callee)) =>
53+
{
54+
(did, args)
55+
},
56+
ExprKind::MethodCall(_, _, args, _)
57+
if let Some(did) = cx.typeck_results().type_dependent_def_id(e.hir_id) =>
58+
{
59+
(did, args)
60+
},
61+
_ => return,
62+
};
63+
64+
let sig = tcx.fn_sig(fn_did).skip_binder().skip_binder();
65+
66+
let infcx = cx.tcx.infer_ctxt().build(cx.typing_mode());
67+
68+
// `ExprKind::MethodCall` doesn't include the receiver in `args`, but does in `sig.inputs()`
69+
// -- so we iterate over both in `rev`erse in order to line them up starting from the _end_
70+
//
71+
// and for `ExprKind::Call` this is basically a no-op
72+
iter::zip(sig.inputs().iter().rev(), args.iter().rev())
73+
.enumerate()
74+
.for_each(|(arg_idx, (arg_ty, arg))| {
75+
// we want `arg` to be `Path::new(x)`
76+
if let ExprKind::Call(path_new, [x]) = arg.kind
77+
&& let ExprKind::Path(QPath::TypeRelative(path, new)) = path_new.kind
78+
&& is_path_diagnostic_item(cx, path, sym::Path)
79+
&& new.ident.name == sym::new
80+
&& let ty::Param(arg_param_ty) = arg_ty.kind()
81+
&& !is_used_anywhere_else(
82+
*arg_param_ty,
83+
sig.inputs()
84+
.iter()
85+
// `arg_idx` is based on the reversed order, so we need to reverse as well for the
86+
// `enumerate` indices to work
87+
.rev()
88+
.enumerate()
89+
.filter_map(|(i, input)| (i != arg_idx).then_some(*input)),
90+
)
91+
&& let x_ty = cx.typeck_results().expr_ty(x)
92+
&& has_required_preds(cx, &infcx, *arg_ty, x_ty, cx.tcx.predicates_of(fn_did))
93+
{
94+
let mut applicability = Applicability::MachineApplicable;
95+
let sugg = Sugg::hir_with_applicability(cx, x, "_", &mut applicability);
96+
span_lint_and_sugg(
97+
cx,
98+
NEEDLESS_PATH_NEW,
99+
arg.span,
100+
"the expression enclosed in `Path::new` can be passed directly",
101+
"try",
102+
sugg.to_string(),
103+
applicability,
104+
);
105+
}
106+
});
107+
}
108+
}
109+
110+
fn is_used_anywhere_else<'tcx>(param_ty: ParamTy, mut other_sig_tys: impl Iterator<Item = Ty<'tcx>>) -> bool {
111+
other_sig_tys.any(|sig_ty| {
112+
sig_ty.walk().any(|generic_arg| {
113+
if let Some(ty) = generic_arg.as_type()
114+
&& let ty::Param(pt) = ty.kind()
115+
&& *pt == param_ty
116+
{
117+
true
118+
} else {
119+
false
120+
}
121+
})
122+
})
123+
}
124+
125+
fn has_required_preds<'tcx>(
126+
cx: &LateContext<'tcx>,
127+
infcx: &InferCtxt<'tcx>,
128+
param_ty: Ty<'tcx>,
129+
x_ty: Ty<'tcx>,
130+
preds: GenericPredicates<'tcx>,
131+
) -> bool {
132+
let mut has_preds = false;
133+
134+
let has_required_preds = preds
135+
.predicates
136+
.iter()
137+
.filter_map(|(clause, _)| clause.as_trait_clause())
138+
.map(|pred| pred.skip_binder())
139+
.filter(|pred| {
140+
// dbg!(pred.self_ty(), param_ty);
141+
pred.self_ty() == param_ty
142+
})
143+
.all(|pred| {
144+
has_preds = true;
145+
146+
if pred.polarity != PredicatePolarity::Positive {
147+
return false;
148+
}
149+
150+
let new_pred = pred.with_replaced_self_ty(cx.tcx, x_ty);
151+
let obligation = Obligation::new(cx.tcx, ObligationCause::dummy(), cx.param_env, new_pred);
152+
infcx.predicate_must_hold_modulo_regions(&obligation)
153+
// match cx.tcx.get_diagnostic_name(pred.def_id()) {
154+
// Some(sym::AsRef) => {
155+
// // TODO: check if it's `AsRef<Path>` in paricular
156+
// },
157+
// Some(sym::Sized) => todo!(),
158+
// _ => return false,
159+
// };
160+
});
161+
162+
if !has_preds {
163+
// There were no trait clauses -- this means that the type just needs to be `Path`, so the
164+
// lint is not applicable
165+
return false;
166+
}
167+
168+
has_required_preds
169+
}

tests/ui/needless_path_new.fixed

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
#![warn(clippy::needless_path_new)]
2+
3+
use std::fs;
4+
use std::path::Path;
5+
6+
fn takes_path(_: &Path) {}
7+
fn takes_impl_path(_: impl AsRef<Path>) {}
8+
fn takes_path_and_impl_path(_: &Path, _: impl AsRef<Path>) {}
9+
fn takes_two_impl_paths_with_the_same_generic<P: AsRef<Path>>(_: P, _: P) {}
10+
fn takes_two_impl_paths_with_different_generics<P: AsRef<Path>, Q: AsRef<Path>>(_: P, _: Q) {}
11+
12+
struct Foo;
13+
14+
impl Foo {
15+
fn takes_path(_: &Path) {}
16+
fn takes_self_and_path(&self, _: &Path) {}
17+
fn takes_path_and_impl_path(_: &Path, _: impl AsRef<Path>) {}
18+
fn takes_self_and_path_and_impl_path(&self, _: &Path, _: impl AsRef<Path>) {}
19+
}
20+
21+
fn main() {
22+
let f = Foo;
23+
24+
fs::write("foo.txt", "foo"); //~ needless_path_new
25+
26+
fs::copy(
27+
"foo", //~ needless_path_new
28+
"bar", //~ needless_path_new
29+
);
30+
31+
Foo::takes_path(Path::new("foo"));
32+
33+
f.takes_self_and_path_and_impl_path(
34+
Path::new("foo"),
35+
"bar", //~ needless_path_new
36+
);
37+
38+
// We can and should change both independently
39+
takes_two_impl_paths_with_different_generics(
40+
"foo", //~ needless_path_new
41+
"bar", //~ needless_path_new
42+
);
43+
44+
let a = takes_impl_path;
45+
a("foo.txt"); //~ needless_path_new
46+
47+
// Don't lint
48+
49+
takes_path(Path::new("foo"));
50+
51+
// The paramater that _could_ be passed directly, was;
52+
// The parameter that could't, wasn't
53+
takes_path_and_impl_path(Path::new("foo"), "bar");
54+
Foo::takes_path_and_impl_path(Path::new("foo"), "bar");
55+
f.takes_self_and_path_and_impl_path(Path::new("foo"), "bar");
56+
57+
// We are conservative and don't suggest changing a parameter
58+
// if it contains a generic type used elsewhere in the function
59+
takes_two_impl_paths_with_the_same_generic(Path::new("foo"), Path::new("bar"));
60+
61+
// The type ascription specifies `Path`, not just `impl AsRef<Path>`
62+
let _: Option<&Path> = Some(Path::new("foo"));
63+
64+
// The return type requires `Path`, not just `impl AsRef<Path>`
65+
fn foo() -> Option<&'static Path> {
66+
Some(Path::new("foo.txt"))
67+
}
68+
}

tests/ui/needless_path_new.rs

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
#![warn(clippy::needless_path_new)]
2+
3+
use std::fs;
4+
use std::path::Path;
5+
6+
fn takes_path(_: &Path) {}
7+
fn takes_impl_path(_: impl AsRef<Path>) {}
8+
fn takes_path_and_impl_path(_: &Path, _: impl AsRef<Path>) {}
9+
fn takes_two_impl_paths_with_the_same_generic<P: AsRef<Path>>(_: P, _: P) {}
10+
fn takes_two_impl_paths_with_different_generics<P: AsRef<Path>, Q: AsRef<Path>>(_: P, _: Q) {}
11+
12+
struct Foo;
13+
14+
impl Foo {
15+
fn takes_path(_: &Path) {}
16+
fn takes_self_and_path(&self, _: &Path) {}
17+
fn takes_path_and_impl_path(_: &Path, _: impl AsRef<Path>) {}
18+
fn takes_self_and_path_and_impl_path(&self, _: &Path, _: impl AsRef<Path>) {}
19+
}
20+
21+
fn main() {
22+
let f = Foo;
23+
24+
fs::write(Path::new("foo.txt"), "foo"); //~ needless_path_new
25+
26+
fs::copy(
27+
Path::new("foo"), //~ needless_path_new
28+
Path::new("bar"), //~ needless_path_new
29+
);
30+
31+
Foo::takes_path(Path::new("foo"));
32+
33+
f.takes_self_and_path_and_impl_path(
34+
Path::new("foo"),
35+
Path::new("bar"), //~ needless_path_new
36+
);
37+
38+
// We can and should change both independently
39+
takes_two_impl_paths_with_different_generics(
40+
Path::new("foo"), //~ needless_path_new
41+
Path::new("bar"), //~ needless_path_new
42+
);
43+
44+
let a = takes_impl_path;
45+
a(Path::new("foo.txt")); //~ needless_path_new
46+
47+
// Don't lint
48+
49+
takes_path(Path::new("foo"));
50+
51+
// The paramater that _could_ be passed directly, was;
52+
// The parameter that could't, wasn't
53+
takes_path_and_impl_path(Path::new("foo"), "bar");
54+
Foo::takes_path_and_impl_path(Path::new("foo"), "bar");
55+
f.takes_self_and_path_and_impl_path(Path::new("foo"), "bar");
56+
57+
// We are conservative and don't suggest changing a parameter
58+
// if it contains a generic type used elsewhere in the function
59+
takes_two_impl_paths_with_the_same_generic(Path::new("foo"), Path::new("bar"));
60+
61+
// The type ascription specifies `Path`, not just `impl AsRef<Path>`
62+
let _: Option<&Path> = Some(Path::new("foo"));
63+
64+
// The return type requires `Path`, not just `impl AsRef<Path>`
65+
fn foo() -> Option<&'static Path> {
66+
Some(Path::new("foo.txt"))
67+
}
68+
}

tests/ui/needless_path_new.stderr

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
error: the expression enclosed in `Path::new` can be passed directly
2+
--> tests/ui/needless_path_new.rs:24:15
3+
|
4+
LL | fs::write(Path::new("foo.txt"), "foo");
5+
| ^^^^^^^^^^^^^^^^^^^^ help: try: `"foo.txt"`
6+
|
7+
= note: `-D clippy::needless-path-new` implied by `-D warnings`
8+
= help: to override `-D warnings` add `#[allow(clippy::needless_path_new)]`
9+
10+
error: the expression enclosed in `Path::new` can be passed directly
11+
--> tests/ui/needless_path_new.rs:28:9
12+
|
13+
LL | Path::new("bar"),
14+
| ^^^^^^^^^^^^^^^^ help: try: `"bar"`
15+
16+
error: the expression enclosed in `Path::new` can be passed directly
17+
--> tests/ui/needless_path_new.rs:27:9
18+
|
19+
LL | Path::new("foo"),
20+
| ^^^^^^^^^^^^^^^^ help: try: `"foo"`
21+
22+
error: the expression enclosed in `Path::new` can be passed directly
23+
--> tests/ui/needless_path_new.rs:35:9
24+
|
25+
LL | Path::new("bar"),
26+
| ^^^^^^^^^^^^^^^^ help: try: `"bar"`
27+
28+
error: the expression enclosed in `Path::new` can be passed directly
29+
--> tests/ui/needless_path_new.rs:41:9
30+
|
31+
LL | Path::new("bar"),
32+
| ^^^^^^^^^^^^^^^^ help: try: `"bar"`
33+
34+
error: the expression enclosed in `Path::new` can be passed directly
35+
--> tests/ui/needless_path_new.rs:40:9
36+
|
37+
LL | Path::new("foo"),
38+
| ^^^^^^^^^^^^^^^^ help: try: `"foo"`
39+
40+
error: the expression enclosed in `Path::new` can be passed directly
41+
--> tests/ui/needless_path_new.rs:45:7
42+
|
43+
LL | a(Path::new("foo.txt"));
44+
| ^^^^^^^^^^^^^^^^^^^^ help: try: `"foo.txt"`
45+
46+
error: aborting due to 7 previous errors
47+

0 commit comments

Comments
 (0)