From 0e883479f134b83c6ea18b3dca4a51d8583444e5 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Wed, 23 Apr 2025 00:33:43 +0200 Subject: [PATCH 1/2] new lint: `needless_path_new` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/needless_path_new.rs | 169 ++++++++++++++++++++++++++ tests/ui/needless_path_new.fixed | 68 +++++++++++ tests/ui/needless_path_new.rs | 68 +++++++++++ tests/ui/needless_path_new.stderr | 47 +++++++ 7 files changed, 356 insertions(+) create mode 100644 clippy_lints/src/needless_path_new.rs create mode 100644 tests/ui/needless_path_new.fixed create mode 100644 tests/ui/needless_path_new.rs create mode 100644 tests/ui/needless_path_new.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b950e562c41..5b4ccc5ad133 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6434,6 +6434,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 2a4bedc18455..5c06a8e102b4 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -553,6 +553,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 c56fa257b068..89a278aacdd4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -269,6 +269,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; @@ -831,5 +832,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co 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(toplevel_ref_arg::ToplevelRefArg)); + 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..bb9cd26f0159 --- /dev/null +++ b/clippy_lints/src/needless_path_new.rs @@ -0,0 +1,169 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::sugg::Sugg; +use clippy_utils::{expr_or_init, is_path_diagnostic_item, path_res}; +use rustc_errors::Applicability; +use rustc_hir::def::{CtorKind, DefKind, Res}; +use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_infer::infer::InferCtxt; +use rustc_infer::traits::{Obligation, ObligationCause}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{self, GenericPredicates, ParamTy, PredicatePolarity, Ty}; +use rustc_session::declare_lint_pass; +use rustc_span::sym; +use rustc_trait_selection::infer::TyCtxtInferExt; +use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt; +use std::iter; + +declare_clippy_lint! { + /// ### What it does + /// Detects expressions being enclosed in `Path::new` when passed to a function which would + /// accept the enclosed expression directly. + /// + /// ### 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.92.0"] + pub NEEDLESS_PATH_NEW, + nursery, + "`Path::new(x)` passed as an argument where `x` would suffice" +} + +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>) { + 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(); + + let infcx = cx.tcx.infer_ctxt().build(cx.typing_mode()); + + // `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 `arg` to be `Path::new(x)` + if let ExprKind::Call(path_new, [x]) = arg.kind + && let ExprKind::Path(QPath::TypeRelative(path, new)) = path_new.kind + && is_path_diagnostic_item(cx, path, sym::Path) + && new.ident.name == sym::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 for the + // `enumerate` indices to work + .rev() + .enumerate() + .filter_map(|(i, input)| (i != arg_idx).then_some(*input)), + ) + && let x_ty = cx.typeck_results().expr_ty(x) + && has_required_preds(cx, &infcx, *arg_ty, x_ty, cx.tcx.predicates_of(fn_did)) + { + let mut applicability = Applicability::MachineApplicable; + let sugg = Sugg::hir_with_applicability(cx, x, "_", &mut applicability); + span_lint_and_sugg( + cx, + NEEDLESS_PATH_NEW, + arg.span, + "the expression enclosed in `Path::new` can be passed directly", + "try", + sugg.to_string(), + applicability, + ); + } + }); + } +} + +fn is_used_anywhere_else<'tcx>(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 + } + }) + }) +} + +fn has_required_preds<'tcx>( + cx: &LateContext<'tcx>, + infcx: &InferCtxt<'tcx>, + param_ty: Ty<'tcx>, + x_ty: Ty<'tcx>, + preds: GenericPredicates<'tcx>, +) -> bool { + let mut has_preds = false; + + let has_required_preds = preds + .predicates + .iter() + .filter_map(|(clause, _)| clause.as_trait_clause()) + .map(|pred| pred.skip_binder()) + .filter(|pred| { + // dbg!(pred.self_ty(), param_ty); + pred.self_ty() == param_ty + }) + .all(|pred| { + has_preds = true; + + if pred.polarity != PredicatePolarity::Positive { + return false; + } + + let new_pred = pred.with_replaced_self_ty(cx.tcx, x_ty); + let obligation = Obligation::new(cx.tcx, ObligationCause::dummy(), cx.param_env, new_pred); + infcx.predicate_must_hold_modulo_regions(&obligation) + // match cx.tcx.get_diagnostic_name(pred.def_id()) { + // Some(sym::AsRef) => { + // // TODO: check if it's `AsRef` in paricular + // }, + // Some(sym::Sized) => todo!(), + // _ => return false, + // }; + }); + + if !has_preds { + // There were no trait clauses -- this means that the type just needs to be `Path`, so the + // lint is not applicable + return false; + } + + has_required_preds +} diff --git a/tests/ui/needless_path_new.fixed b/tests/ui/needless_path_new.fixed new file mode 100644 index 000000000000..074436e8ba5d --- /dev/null +++ b/tests/ui/needless_path_new.fixed @@ -0,0 +1,68 @@ +#![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 + + // Don't lint + + 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"); + 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")); + + // 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.rs b/tests/ui/needless_path_new.rs new file mode 100644 index 000000000000..f060cee10859 --- /dev/null +++ b/tests/ui/needless_path_new.rs @@ -0,0 +1,68 @@ +#![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 + + // Don't lint + + 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"); + 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")); + + // 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..2da16b6170e0 --- /dev/null +++ b/tests/ui/needless_path_new.stderr @@ -0,0 +1,47 @@ +error: the expression enclosed in `Path::new` can be passed directly + --> tests/ui/needless_path_new.rs:24:15 + | +LL | fs::write(Path::new("foo.txt"), "foo"); + | ^^^^^^^^^^^^^^^^^^^^ help: try: `"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` can be passed directly + --> tests/ui/needless_path_new.rs:28:9 + | +LL | Path::new("bar"), + | ^^^^^^^^^^^^^^^^ help: try: `"bar"` + +error: the expression enclosed in `Path::new` can be passed directly + --> tests/ui/needless_path_new.rs:27:9 + | +LL | Path::new("foo"), + | ^^^^^^^^^^^^^^^^ help: try: `"foo"` + +error: the expression enclosed in `Path::new` can be passed directly + --> tests/ui/needless_path_new.rs:35:9 + | +LL | Path::new("bar"), + | ^^^^^^^^^^^^^^^^ help: try: `"bar"` + +error: the expression enclosed in `Path::new` can be passed directly + --> tests/ui/needless_path_new.rs:41:9 + | +LL | Path::new("bar"), + | ^^^^^^^^^^^^^^^^ help: try: `"bar"` + +error: the expression enclosed in `Path::new` can be passed directly + --> tests/ui/needless_path_new.rs:40:9 + | +LL | Path::new("foo"), + | ^^^^^^^^^^^^^^^^ help: try: `"foo"` + +error: the expression enclosed in `Path::new` can be passed directly + --> tests/ui/needless_path_new.rs:45:7 + | +LL | a(Path::new("foo.txt")); + | ^^^^^^^^^^^^^^^^^^^^ help: try: `"foo.txt"` + +error: aborting due to 7 previous errors + From 945d5e4bea0f8180b26f19227621754f64ed8d08 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Fri, 19 Sep 2025 13:14:51 +0200 Subject: [PATCH 2/2] has_required_preds: make the `Clause::as_trait_clause` more explicit should I keep this? --- clippy_lints/src/needless_path_new.rs | 29 ++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/needless_path_new.rs b/clippy_lints/src/needless_path_new.rs index bb9cd26f0159..df28cc631b23 100644 --- a/clippy_lints/src/needless_path_new.rs +++ b/clippy_lints/src/needless_path_new.rs @@ -7,7 +7,7 @@ use rustc_hir::{Expr, ExprKind, QPath}; use rustc_infer::infer::InferCtxt; use rustc_infer::traits::{Obligation, ObligationCause}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::{self, GenericPredicates, ParamTy, PredicatePolarity, Ty}; +use rustc_middle::ty::{self, ClauseKind, GenericPredicates, ParamTy, PredicatePolarity, Ty}; use rustc_session::declare_lint_pass; use rustc_span::sym; use rustc_trait_selection::infer::TyCtxtInferExt; @@ -134,8 +134,31 @@ fn has_required_preds<'tcx>( let has_required_preds = preds .predicates .iter() - .filter_map(|(clause, _)| clause.as_trait_clause()) - .map(|pred| pred.skip_binder()) + .filter_map(|(clause, _)| { + let clause = clause.kind(); + #[expect(clippy::match_same_arms, reason = "branches have different reasons to be `None`")] + match clause.skip_binder() { + // This is what we analyze + // NOTE: repeats the contents of `Clause::as_trait_clause`, + // except we don't `Binder::rebind` as we don't care about the binder + ClauseKind::Trait(trait_clause) => Some(trait_clause), + + // Trivially holds for `P`: `Path::new` has signature `&S -> &Path`, so any "outlives" that holds for + // `P` does so for `S` as well + ClauseKind::TypeOutlives(_) | ClauseKind::RegionOutlives(_) => None, + + // Irrelevant to us: neither `AsRef` nor `Sized` have associated types + ClauseKind::Projection(_) => None, + + // Irrelevant: we don't have anything to do with consts + ClauseKind::ConstArgHasType(..) | ClauseKind::ConstEvaluatable(_) | ClauseKind::HostEffect(_) => None, + + // Irrelevant?: we don't deal with unstable impls + ClauseKind::UnstableFeature(_) => None, + + ClauseKind::WellFormed(_) => None, + } + }) .filter(|pred| { // dbg!(pred.self_ty(), param_ty); pred.self_ty() == param_ty