Skip to content

Commit 2feab73

Browse files
ada4aada4a
committed
new lint: needless_path_new 
1 parent 368b235 commit 2feab73

File tree

7 files changed

+310
-0
lines changed

7 files changed

+310
-0
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6289,6 +6289,7 @@ Released 2018-09-13
62896289
[`needless_parens_on_range_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_parens_on_range_literals
62906290
[`needless_pass_by_ref_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_ref_mut
62916291
[`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value
6292+
[`needless_path_new`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_path_new
62926293
[`needless_pub_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pub_self
62936294
[`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark
62946295
[`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
@@ -551,6 +551,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
551551
crate::needless_parens_on_range_literals::NEEDLESS_PARENS_ON_RANGE_LITERALS_INFO,
552552
crate::needless_pass_by_ref_mut::NEEDLESS_PASS_BY_REF_MUT_INFO,
553553
crate::needless_pass_by_value::NEEDLESS_PASS_BY_VALUE_INFO,
554+
crate::needless_path_new::NEEDLESS_PATH_NEW_INFO,
554555
crate::needless_question_mark::NEEDLESS_QUESTION_MARK_INFO,
555556
crate::needless_update::NEEDLESS_UPDATE_INFO,
556557
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;
@@ -833,5 +834,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
833834
store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom));
834835
store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny));
835836
store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg));
837+
store.register_late_pass(|_| Box::new(needless_path_new::NeedlessPathNew));
836838
// add lints here, do not remove this comment, it's used in `new_lint`
837839
}

‎clippy_lints/src/needless_path_new.rs‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
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_lint::{LateContext, LateLintPass};
8+
use rustc_middle::ty::{self, GenericPredicates, ParamTy, Ty};
9+
use rustc_session::declare_lint_pass;
10+
use rustc_span::sym;
11+
use std::iter;
12+
13+
declare_clippy_lint! {
14+
/// ### What it does
15+
/// Detects expressions being enclosed in `Path::new` when passed to a function that accepts
16+
/// `impl AsRef<Path>`, when the enclosed expression could be used.
17+
///
18+
/// ### Why is this bad?
19+
/// It is unnecessarily verbose
20+
///
21+
/// ### Example
22+
/// ```no_run
23+
/// # use std::{fs, path::Path};
24+
/// fs::write(Path::new("foo.txt"), "foo");
25+
/// ```
26+
/// Use instead:
27+
/// ```no_run
28+
/// # use std::{fs, path::Path};
29+
/// fs::write("foo.txt", "foo");
30+
/// ```
31+
#[clippy::version = "1.90.0"]
32+
pub NEEDLESS_PATH_NEW,
33+
nursery,
34+
"an argument passed to a function that accepts `impl AsRef<Path>` \
35+
being enclosed in `Path::new` when the argument implements the trait"
36+
}
37+
38+
declare_lint_pass!(NeedlessPathNew => [NEEDLESS_PATH_NEW]);
39+
40+
impl<'tcx> LateLintPass<'tcx> for NeedlessPathNew {
41+
fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) {
42+
let tcx = cx.tcx;
43+
44+
let (fn_did, args) = match e.kind {
45+
ExprKind::Call(callee, args)
46+
if let Res::Def(DefKind::Fn | DefKind::AssocFn | DefKind::Ctor(_, CtorKind::Fn), did) =
47+
// re: `expr_or_init`: `callee` might be a variable storing a fn ptr, for example,
48+
// so we need to get to the actual initializer
49+
path_res(cx, expr_or_init(cx, callee)) =>
50+
{
51+
(did, args)
52+
},
53+
ExprKind::MethodCall(_, _, args, _)
54+
if let Some(did) = cx.typeck_results().type_dependent_def_id(e.hir_id) =>
55+
{
56+
(did, args)
57+
},
58+
_ => return,
59+
};
60+
61+
let sig = tcx.fn_sig(fn_did).skip_binder().skip_binder();
62+
63+
let has_required_preds = |_param_ty: &ParamTy, _preds: GenericPredicates<'_>| -> bool {
64+
// TODO
65+
true
66+
};
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
86+
.rev()
87+
.enumerate()
88+
.filter_map(|(i, input)| (i != arg_idx).then_some(*input)),
89+
)
90+
&& has_required_preds(arg_param_ty, cx.tcx.predicates_of(fn_did))
91+
{
92+
let mut applicability = Applicability::MachineApplicable;
93+
let sugg = Sugg::hir_with_applicability(cx, x, "_", &mut applicability);
94+
span_lint_and_sugg(
95+
cx,
96+
NEEDLESS_PATH_NEW,
97+
arg.span,
98+
"the expression enclosed in `Path::new` implements `AsRef<Path>`",
99+
"remove the enclosing `Path::new`",
100+
sugg.to_string(),
101+
applicability,
102+
);
103+
}
104+
})
105+
}
106+
}
107+
108+
fn is_used_anywhere_else<'a>(param_ty: &'_ ParamTy, mut other_sig_tys: impl Iterator<Item = Ty<'a>>) -> bool {
109+
other_sig_tys.any(|sig_ty| {
110+
sig_ty.walk().any(|generic_arg| {
111+
if let Some(ty) = generic_arg.as_type()
112+
&& let ty::Param(pt) = ty.kind()
113+
&& pt == param_ty
114+
{
115+
true
116+
} else {
117+
false
118+
}
119+
})
120+
})
121+
}

‎tests/ui/needless_path_new.fixed‎

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

‎tests/ui/needless_path_new.rs‎

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

‎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` implements `AsRef<Path>`
2+
--> tests/ui/needless_path_new.rs:27:15
3+
|
4+
LL | fs::write(Path::new("foo.txt"), "foo");
5+
| ^^^^^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"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` implements `AsRef<Path>`
11+
--> tests/ui/needless_path_new.rs:31:9
12+
|
13+
LL | Path::new("bar"),
14+
| ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"bar"`
15+
16+
error: the expression enclosed in `Path::new` implements `AsRef<Path>`
17+
--> tests/ui/needless_path_new.rs:30:9
18+
|
19+
LL | Path::new("foo"),
20+
| ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"foo"`
21+
22+
error: the expression enclosed in `Path::new` implements `AsRef<Path>`
23+
--> tests/ui/needless_path_new.rs:38:9
24+
|
25+
LL | Path::new("bar"),
26+
| ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"bar"`
27+
28+
error: the expression enclosed in `Path::new` implements `AsRef<Path>`
29+
--> tests/ui/needless_path_new.rs:44:9
30+
|
31+
LL | Path::new("bar"),
32+
| ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"bar"`
33+
34+
error: the expression enclosed in `Path::new` implements `AsRef<Path>`
35+
--> tests/ui/needless_path_new.rs:43:9
36+
|
37+
LL | Path::new("foo"),
38+
| ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"foo"`
39+
40+
error: the expression enclosed in `Path::new` implements `AsRef<Path>`
41+
--> tests/ui/needless_path_new.rs:49:7
42+
|
43+
LL | a(Path::new("foo.txt"));
44+
| ^^^^^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"foo.txt"`
45+
46+
error: aborting due to 7 previous errors
47+

0 commit comments

Comments
 (0)