WIP: New lint: needless_path_new

CHANGELOG.md

@@ -6097,6 +6097,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

@@ -550,6 +550,7 @@ pub static LINTS: &[&crate::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

@@ -270,6 +270,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;
@@ -951,5 +952,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     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,108 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::path_res;
+use clippy_utils::source::snippet;
+use clippy_utils::ty::implements_trait;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, QPath};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::{self, List, 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.89.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]);
+
+impl<'tcx> LateLintPass<'tcx> for NeedlessPathNew {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) {
+        match e.kind {
+            ExprKind::Call(fn_expr, args) => {
+                check_arguments(cx, &mut args.iter(), cx.typeck_results().expr_ty(fn_expr));
+            },
+            ExprKind::MethodCall(_, receiver, arguments, _)
+                if let Some(def_id) = cx.typeck_results().type_dependent_def_id(e.hir_id) =>
+            {
+                let args = cx.typeck_results().node_args(e.hir_id);
+                let method_type = cx.tcx.type_of(def_id).instantiate(cx.tcx, args);
+                check_arguments(cx, &mut iter::once(receiver).chain(arguments.iter()), method_type);
+            },
+            _ => (),
+        }
+    }
+}
+
+fn check_arguments<'tcx>(
+    cx: &LateContext<'tcx>,
+    arguments: &mut dyn Iterator<Item = &'tcx Expr<'tcx>>,
+    type_definition: Ty<'tcx>,
+) {
+    let tcx = cx.tcx;
+    // 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 Some(path_def_id) = tcx.get_diagnostic_item(sym::Path) else {
+        return;
+    };
+    let path_ty = Ty::new_adt(tcx, tcx.adt_def(path_def_id), List::empty());
+    let Some(asref_def_id) = tcx.get_diagnostic_item(sym::AsRef) else {
+        return;
+    };
+
+    let implements_asref_path = |arg| implements_trait(cx, arg, asref_def_id, &[path_ty.into()]);
+
+    if let ty::FnDef(..) | ty::FnPtr(..) = type_definition.kind() {
+        let parameters = type_definition.fn_sig(tcx).skip_binder().inputs();
+        for (argument, parameter) in iter::zip(arguments, parameters) {
+            if let ExprKind::Call(func, args) = argument.kind
+                && is_path_new(func)
+                && implements_asref_path(cx.typeck_results().expr_ty(&args[0]))
+                && implements_asref_path(*parameter)
+            {
+                span_lint_and_sugg(
+                    cx,
+                    NEEDLESS_PATH_NEW,
+                    argument.span,
+                    "the expression enclosed in `Path::new` implements `AsRef<Path>`",
+                    "remove the enclosing `Path::new`",
+                    format!("{}", snippet(cx, args[0].span, "..")),
+                    Applicability::MachineApplicable,
+                );
+            }
+        }
+    }
+}

tests/ui/needless_path_new.fixed

@@ -0,0 +1,54 @@
+#![warn(clippy::needless_path_new)]
+
+use std::fs;
+use std::path::Path;
+
+fn takes_path(_: &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) {}
+
+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 at the same time
+    takes_two_impl_paths_with_the_same_generic(
+        "foo", //~ needless_path_new
+        "bar", //~ 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");
+}

tests/ui/needless_path_new.rs

@@ -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) {}
+
+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 at the same time
+    takes_two_impl_paths_with_the_same_generic(
+        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");
+
+    fn foo() -> Option<&'static Path> {
+        // Some(...) is `ExprKind::Call`, but we don't consider it
+        Some(Path::new("foo.txt"))
+    }
+}

tests/ui/needless_path_new.stderr

@@ -0,0 +1,41 @@
+error: the expression enclosed in `Path::new` implements `AsRef<Path>`
+  --> tests/ui/needless_path_new.rs:24: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:27: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:28: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:35: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:40: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:41:9
+   |
+LL |         Path::new("bar"),
+   |         ^^^^^^^^^^^^^^^^ help: remove the enclosing `Path::new`: `"bar"`
+
+error: aborting due to 6 previous errors
+