WIP: New lint: needless_path_new

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

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,

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`
 }

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<Path>`, 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<Path>` \
+    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<Item = Ty<'a>>) -> 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<Path>`",
+                        "remove the enclosing `Path::new`",
+                        format!("{}", snippet(cx, x.span, "..")),
+                        Applicability::MachineApplicable,
+                    );
+                }
+            })
+    }
+}

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<impl AsRef<Path>> {
+        Some("bar.txt") //~ needless_path_new
+    }
+}

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<impl AsRef<Path>> {
+        Some(Path::new("bar.txt")) //~ needless_path_new
+    }
+}

tests/ui/needless_path_ne2.stderr

@@ -0,0 +1,11 @@
+error: the expression enclosed in `Path::new` implements `AsRef<Path>`
+  --> 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
+

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<Path>) {}
+
+fn takes_path_and_impl_path(_: &Path, _: impl AsRef<Path>) {}
+
+fn takes_two_impl_paths_with_the_same_generic<P: AsRef<Path>>(_: P, _: P) {}
+fn takes_two_impl_paths_with_different_generics<P: AsRef<Path>, Q: AsRef<Path>>(_: 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<Path>) {}
+    fn takes_self_and_path_and_impl_path(&self, _: &Path, _: impl AsRef<Path>) {}
+}
+
+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"));
+}

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<Path>) {}
+
+fn takes_path_and_impl_path(_: &Path, _: impl AsRef<Path>) {}
+
+fn takes_two_impl_paths_with_the_same_generic<P: AsRef<Path>>(_: P, _: P) {}
+fn takes_two_impl_paths_with_different_generics<P: AsRef<Path>, Q: AsRef<Path>>(_: 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<Path>) {}
+    fn takes_self_and_path_and_impl_path(&self, _: &Path, _: impl AsRef<Path>) {}
+}
+
+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<Path>`
+    let _: Option<&Path> = Some(Path::new("foo")); //~ needless_path_new
+
+    // the return type requires `Path`, not just `impl AsRef<Path>`
+    fn foo() -> Option<&'static Path> {
+        Some(Path::new("foo.txt"))
+    }
+}

tests/ui/needless_path_new.stderr

@@ -0,0 +1,47 @@
+error: the expression enclosed in `Path::new` implements `AsRef<Path>`
+  --> 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<Path>`
+  --> 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<Path>`
+  --> 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<Path>`
+  --> 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<Path>`
+  --> 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<Path>`
+  --> 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<Path>`
+  --> 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
+