diff --git a/CHANGELOG.md b/CHANGELOG.md index bc60b1c57f7d..66767ce9db72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6284,6 +6284,7 @@ Released 2018-09-13 [`needless_parens_on_range_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_parens_on_range_literals [`needless_pass_by_ref_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_ref_mut [`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value +[`needless_path_new`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_path_new [`needless_pub_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pub_self [`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark [`needless_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index e67e8d9070f2..f86da63840eb 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -552,6 +552,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::needless_parens_on_range_literals::NEEDLESS_PARENS_ON_RANGE_LITERALS_INFO, crate::needless_pass_by_ref_mut::NEEDLESS_PASS_BY_REF_MUT_INFO, crate::needless_pass_by_value::NEEDLESS_PASS_BY_VALUE_INFO, + crate::needless_path_new::NEEDLESS_PATH_NEW_INFO, crate::needless_question_mark::NEEDLESS_QUESTION_MARK_INFO, crate::needless_update::NEEDLESS_UPDATE_INFO, crate::neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d468993e7444..05ca2aa4d226 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -268,6 +268,7 @@ mod needless_maybe_sized; mod needless_parens_on_range_literals; mod needless_pass_by_ref_mut; mod needless_pass_by_value; +mod needless_path_new; mod needless_question_mark; mod needless_update; mod neg_cmp_op_on_partial_ord; @@ -830,5 +831,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(cloned_ref_to_slice_refs::ClonedRefToSliceRefs::new(conf))); store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom)); store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny)); + store.register_late_pass(|_| Box::new(needless_path_new::NeedlessPathNew)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/needless_path_new.rs b/clippy_lints/src/needless_path_new.rs new file mode 100644 index 000000000000..20c9b6ac9df7 --- /dev/null +++ b/clippy_lints/src/needless_path_new.rs @@ -0,0 +1,129 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet; +use clippy_utils::{expr_or_init, path_res}; +use rustc_errors::Applicability; +use rustc_hir::def::{CtorKind, DefKind, Res}; +use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self, GenericPredicates, ParamTy, Ty}; +use rustc_session::declare_lint_pass; +use rustc_span::sym; +use std::iter; + +declare_clippy_lint! { + /// ### What it does + /// Detects expressions being enclosed in `Path::new` when passed to a function that accepts + /// `impl AsRef`, when the enclosed expression could be used. + /// + /// ### Why is this bad? + /// It is unnecessarily verbose + /// + /// ### Example + /// ```no_run + /// # use std::{fs, path::Path}; + /// fs::write(Path::new("foo.txt"), "foo"); + /// ``` + /// Use instead: + /// ```no_run + /// # use std::{fs, path::Path}; + /// fs::write("foo.txt", "foo"); + /// ``` + #[clippy::version = "1.90.0"] + pub NEEDLESS_PATH_NEW, + nursery, + "an argument passed to a function that accepts `impl AsRef` \ + being enclosed in `Path::new` when the argument implements the trait" +} + +declare_lint_pass!(NeedlessPathNew => [NEEDLESS_PATH_NEW]); + +fn is_used_anywhere_else<'a>(param_ty: &'_ ParamTy, mut other_sig_tys: impl Iterator>) -> bool { + other_sig_tys.any(|sig_ty| { + sig_ty.walk().any(|generic_arg| { + if let Some(ty) = generic_arg.as_type() + && let ty::Param(pt) = ty.kind() + && pt == param_ty + { + true + } else { + false + } + }) + }) +} + +impl<'tcx> LateLintPass<'tcx> for NeedlessPathNew { + fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) { + let tcx = cx.tcx; + + let (fn_did, args) = match e.kind { + ExprKind::Call(callee, args) + if let Res::Def(DefKind::Fn | DefKind::AssocFn | DefKind::Ctor(_, CtorKind::Fn), did) = + // re: `expr_or_init`: `callee` might be a variable storing a fn ptr, for example, + // so we need to get to the actual initializer + path_res(cx, expr_or_init(cx, callee)) => + { + (did, args) + }, + ExprKind::MethodCall(_, _, args, _) + if let Some(did) = cx.typeck_results().type_dependent_def_id(e.hir_id) => + { + (did, args) + }, + _ => return, + }; + + let sig = tcx.fn_sig(fn_did).skip_binder().skip_binder(); + + // whether `func` is `Path::new` + let is_path_new = |func: &Expr<'_>| { + if let ExprKind::Path(ref qpath) = func.kind + && let QPath::TypeRelative(ty, path) = qpath + && let Some(did) = path_res(cx, *ty).opt_def_id() + && tcx.is_diagnostic_item(sym::Path, did) + && path.ident.name == sym::new + { + true + } else { + false + } + }; + + let has_required_preds = |_param_ty: &ParamTy, _preds: GenericPredicates<'_>| -> bool { true }; + + // as far as I understand, `ExprKind::MethodCall` doesn't include the receiver in `args`, + // but does in `sig.inputs()` -- so we iterate over both in `rev`erse in order to line + // them up starting from the _end_ + // + // and for `ExprKind::Call` this is basically a no-op + iter::zip(sig.inputs().iter().rev(), args.iter().rev()) + .enumerate() + .for_each(|(arg_idx, (arg_ty, arg))| { + // we want `argument` to be `Path::new(x)` + if let ExprKind::Call(path_new, [x]) = arg.kind + && is_path_new(path_new) + && let ty::Param(arg_param_ty) = arg_ty.kind() + && !is_used_anywhere_else( + arg_param_ty, + sig.inputs() + .iter() + // `arg_idx` is based on the reversed order, so we need to reverse as well + .rev() + .enumerate() + .filter_map(|(i, input)| (i != arg_idx).then_some(*input)), + ) + && has_required_preds(arg_param_ty, cx.tcx.predicates_of(fn_did)) + { + span_lint_and_sugg( + cx, + NEEDLESS_PATH_NEW, + arg.span, + "the expression enclosed in `Path::new` implements `AsRef`", + "remove the enclosing `Path::new`", + format!("{}", snippet(cx, x.span, "..")), + Applicability::MachineApplicable, + ); + } + }) + } +} diff --git a/tests/ui/needless_path_ne2.fixed b/tests/ui/needless_path_ne2.fixed new file mode 100644 index 000000000000..07b0b1d9505c --- /dev/null +++ b/tests/ui/needless_path_ne2.fixed @@ -0,0 +1,16 @@ +#![warn(clippy::needless_path_new)] + +use std::fs; +use std::path::Path; + +// fn foo() -> Option<&'static Path> { +// // Some(...) is `ExprKind::Call`, but we don't consider it +// Some(Path::new("foo.txt")) +// } +// +fn main() { + // let _: Option<&Path> = Some(Path::new("foo")); + fn foo() -> Option> { + Some("bar.txt") //~ needless_path_new + } +} diff --git a/tests/ui/needless_path_ne2.rs b/tests/ui/needless_path_ne2.rs new file mode 100644 index 000000000000..db3948db4575 --- /dev/null +++ b/tests/ui/needless_path_ne2.rs @@ -0,0 +1,16 @@ +#![warn(clippy::needless_path_new)] + +use std::fs; +use std::path::Path; + +fn foo() -> Option<&'static Path> { + // Some(...) is `ExprKind::Call`, but we don't consider it + Some(Path::new("foo.txt")) +} + +fn main() { + let _: Option<&Path> = Some(Path::new("foo")); + fn foo() -> Option> { + Some(Path::new("bar.txt")) //~ needless_path_new + } +} diff --git a/tests/ui/needless_path_ne2.stderr b/tests/ui/needless_path_ne2.stderr new file mode 100644 index 000000000000..450d2bbf56a1 --- /dev/null +++ b/tests/ui/needless_path_ne2.stderr @@ -0,0 +1,11 @@ +error: the expression enclosed in `Path::new` implements `AsRef` + --> tests/ui/needless_path_new_some.rs:14:14 + | +LL | Some(Path::new("bar.txt")) + | ^^^^^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"bar.txt"` + | + = note: `-D clippy::needless-path-new` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::needless_path_new)]` + +error: aborting due to 1 previous error + diff --git a/tests/ui/needless_path_new.fixed b/tests/ui/needless_path_new.fixed new file mode 100644 index 000000000000..f6ada65f7881 --- /dev/null +++ b/tests/ui/needless_path_new.fixed @@ -0,0 +1,65 @@ +#![warn(clippy::needless_path_new)] + +use std::fs; +use std::path::Path; + +fn takes_path(_: &Path) {} + +fn takes_impl_path(_: impl AsRef) {} + +fn takes_path_and_impl_path(_: &Path, _: impl AsRef) {} + +fn takes_two_impl_paths_with_the_same_generic>(_: P, _: P) {} +fn takes_two_impl_paths_with_different_generics, Q: AsRef>(_: P, _: Q) {} + +struct Foo; + +impl Foo { + fn takes_path(_: &Path) {} + fn takes_self_and_path(&self, _: &Path) {} + fn takes_path_and_impl_path(_: &Path, _: impl AsRef) {} + fn takes_self_and_path_and_impl_path(&self, _: &Path, _: impl AsRef) {} +} + +fn main() { + let f = Foo; + + fs::write("foo.txt", "foo"); //~ needless_path_new + + fs::copy( + "foo", //~ needless_path_new + "bar", //~ needless_path_new + ); + + Foo::takes_path(Path::new("foo")); + + f.takes_self_and_path_and_impl_path( + Path::new("foo"), + "bar", //~ needless_path_new + ); + + // we can and should change both independently + takes_two_impl_paths_with_different_generics( + "foo", //~ needless_path_new + "bar", //~ needless_path_new + ); + + let a = takes_impl_path; + + a("foo.txt"); //~ needless_path_new + + // no warning + takes_path(Path::new("foo")); + + // the paramater that _could_ be passed directly, was + // the parameter that could't, wasn't + takes_path_and_impl_path(Path::new("foo"), "bar"); + + // same but as a method + Foo::takes_path_and_impl_path(Path::new("foo"), "bar"); + f.takes_self_and_path_and_impl_path(Path::new("foo"), "bar"); + + // we are conservative and don't suggest changing a parameter + // if it contains a generic type used elsewhere in the function + takes_two_impl_paths_with_the_same_generic(Path::new("foo"), Path::new("bar")); +} diff --git a/tests/ui/needless_path_new.rs b/tests/ui/needless_path_new.rs new file mode 100644 index 000000000000..efefda9e768c --- /dev/null +++ b/tests/ui/needless_path_new.rs @@ -0,0 +1,73 @@ +#![warn(clippy::needless_path_new)] + +use std::fs; +use std::path::Path; + +fn takes_path(_: &Path) {} + +fn takes_impl_path(_: impl AsRef) {} + +fn takes_path_and_impl_path(_: &Path, _: impl AsRef) {} + +fn takes_two_impl_paths_with_the_same_generic>(_: P, _: P) {} +fn takes_two_impl_paths_with_different_generics, Q: AsRef>(_: P, _: Q) {} + +struct Foo; + +impl Foo { + fn takes_path(_: &Path) {} + fn takes_self_and_path(&self, _: &Path) {} + fn takes_path_and_impl_path(_: &Path, _: impl AsRef) {} + fn takes_self_and_path_and_impl_path(&self, _: &Path, _: impl AsRef) {} +} + +fn main() { + let f = Foo; + + fs::write(Path::new("foo.txt"), "foo"); //~ needless_path_new + + fs::copy( + Path::new("foo"), //~ needless_path_new + Path::new("bar"), //~ needless_path_new + ); + + Foo::takes_path(Path::new("foo")); + + f.takes_self_and_path_and_impl_path( + Path::new("foo"), + Path::new("bar"), //~ needless_path_new + ); + + // we can and should change both independently + takes_two_impl_paths_with_different_generics( + Path::new("foo"), //~ needless_path_new + Path::new("bar"), //~ needless_path_new + ); + + let a = takes_impl_path; + + a(Path::new("foo.txt")); //~ needless_path_new + + // no warning + takes_path(Path::new("foo")); + + // the paramater that _could_ be passed directly, was + // the parameter that could't, wasn't + takes_path_and_impl_path(Path::new("foo"), "bar"); + + // same but as a method + Foo::takes_path_and_impl_path(Path::new("foo"), "bar"); + f.takes_self_and_path_and_impl_path(Path::new("foo"), "bar"); + + // we are conservative and don't suggest changing a parameter + // if it contains a generic type used elsewhere in the function + takes_two_impl_paths_with_the_same_generic(Path::new("foo"), Path::new("bar")); + + // the type ascription specifies `Path`, not just `impl AsRef` + let _: Option<&Path> = Some(Path::new("foo")); //~ needless_path_new + + // the return type requires `Path`, not just `impl AsRef` + fn foo() -> Option<&'static Path> { + Some(Path::new("foo.txt")) + } +} diff --git a/tests/ui/needless_path_new.stderr b/tests/ui/needless_path_new.stderr new file mode 100644 index 000000000000..bd3c615328ba --- /dev/null +++ b/tests/ui/needless_path_new.stderr @@ -0,0 +1,47 @@ +error: the expression enclosed in `Path::new` implements `AsRef` + --> tests/ui/needless_path_new.rs:27:15 + | +LL | fs::write(Path::new("foo.txt"), "foo"); + | ^^^^^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"foo.txt"` + | + = note: `-D clippy::needless-path-new` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::needless_path_new)]` + +error: the expression enclosed in `Path::new` implements `AsRef` + --> tests/ui/needless_path_new.rs:31:9 + | +LL | Path::new("bar"), + | ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"bar"` + +error: the expression enclosed in `Path::new` implements `AsRef` + --> tests/ui/needless_path_new.rs:30:9 + | +LL | Path::new("foo"), + | ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"foo"` + +error: the expression enclosed in `Path::new` implements `AsRef` + --> tests/ui/needless_path_new.rs:38:9 + | +LL | Path::new("bar"), + | ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"bar"` + +error: the expression enclosed in `Path::new` implements `AsRef` + --> tests/ui/needless_path_new.rs:44:9 + | +LL | Path::new("bar"), + | ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"bar"` + +error: the expression enclosed in `Path::new` implements `AsRef` + --> tests/ui/needless_path_new.rs:43:9 + | +LL | Path::new("foo"), + | ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"foo"` + +error: the expression enclosed in `Path::new` implements `AsRef` + --> tests/ui/needless_path_new.rs:49:7 + | +LL | a(Path::new("foo.txt")); + | ^^^^^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"foo.txt"` + +error: aborting due to 7 previous errors +