From 35e6057e715b47fa8ceed67fbf53e459c86d571c Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Tue, 11 Mar 2025 20:59:03 +0100 Subject: [PATCH] `needless_pass_by_value`: reference the innermost `Option` content If types such as `Option>` are not used by value, then `Option>` will be suggested, instead of `Option<&Option>`. --- clippy_lints/src/needless_pass_by_value.rs | 15 ++++------- clippy_utils/src/lib.rs | 28 +++++++++++++++++---- tests/ui/needless_pass_by_value.rs | 29 ++++++++++++++++++++++ tests/ui/needless_pass_by_value.stderr | 28 +++++++++++++++++++-- 4 files changed, 83 insertions(+), 17 deletions(-) diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index f5652e7b832d..1acc3ffeb936 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -1,10 +1,10 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::is_self; use clippy_utils::ptr::get_spans; use clippy_utils::source::{SpanRangeExt, snippet}; use clippy_utils::ty::{ implements_trait, implements_trait_with_env_from_iter, is_copy, is_type_diagnostic_item, is_type_lang_item, }; +use clippy_utils::{is_self, peel_hir_ty_options}; use rustc_abi::ExternAbi; use rustc_errors::{Applicability, Diag}; use rustc_hir::intravisit::FnKind; @@ -279,18 +279,13 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue { } } - let suggestion = if is_type_diagnostic_item(cx, ty, sym::Option) - && let snip = snippet(cx, input.span, "_") - && let Some((first, rest)) = snip.split_once('<') - { - format!("{first}<&{rest}") - } else { - format!("&{}", snippet(cx, input.span, "_")) - }; + let inner_input = peel_hir_ty_options(cx, input); + let before_span = input.span.until(inner_input.span); + let after_span = before_span.shrink_to_hi().to(input.span.shrink_to_hi()); diag.span_suggestion( input.span, "consider taking a reference instead", - suggestion, + format!("{}&{}", snippet(cx, before_span, "_"), snippet(cx, after_span, "_")), Applicability::MaybeIncorrect, ); }; diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 2d2a19c7b78f..61b65382bb2e 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -107,10 +107,10 @@ use rustc_hir::hir_id::{HirIdMap, HirIdSet}; use rustc_hir::intravisit::{FnKind, Visitor, walk_expr}; use rustc_hir::{ self as hir, Arm, BindingMode, Block, BlockCheckMode, Body, ByRef, Closure, ConstArgKind, ConstContext, - Destination, Expr, ExprField, ExprKind, FnDecl, FnRetTy, GenericArgs, HirId, Impl, ImplItem, ImplItemKind, - ImplItemRef, Item, ItemKind, LangItem, LetStmt, MatchSource, Mutability, Node, OwnerId, OwnerNode, Param, Pat, - PatExpr, PatExprKind, PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitFn, TraitItem, TraitItemKind, - TraitItemRef, TraitRef, TyKind, UnOp, def, + Destination, Expr, ExprField, ExprKind, FnDecl, FnRetTy, GenericArg, GenericArgs, HirId, Impl, ImplItem, + ImplItemKind, ImplItemRef, Item, ItemKind, LangItem, LetStmt, MatchSource, Mutability, Node, OwnerId, OwnerNode, + Param, Pat, PatExpr, PatExprKind, PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitFn, TraitItem, + TraitItemKind, TraitItemRef, TraitRef, TyKind, UnOp, def, }; use rustc_lexer::{TokenKind, tokenize}; use rustc_lint::{LateContext, Level, Lint, LintContext}; @@ -435,7 +435,7 @@ pub fn qpath_generic_tys<'tcx>(qpath: &QPath<'tcx>) -> impl Iterator Some(ty.as_unambig_ty()), + GenericArg::Type(ty) => Some(ty.as_unambig_ty()), _ => None, }) } @@ -3709,3 +3709,21 @@ pub fn is_mutable(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { true } } + +/// Peel `Option<…>` from `hir_ty` as long as the HIR name is `Option` and it corresponds to the +/// `core::Option<_>` type. +pub fn peel_hir_ty_options<'tcx>(cx: &LateContext<'tcx>, mut hir_ty: &'tcx hir::Ty<'tcx>) -> &'tcx hir::Ty<'tcx> { + let Some(option_def_id) = cx.tcx.get_diagnostic_item(sym::Option) else { + return hir_ty; + }; + while let TyKind::Path(QPath::Resolved(None, path)) = hir_ty.kind + && let Some(segment) = path.segments.last() + && segment.ident.name == sym::Option + && let Res::Def(DefKind::Enum, def_id) = segment.res + && def_id == option_def_id + && let [GenericArg::Type(arg_ty)] = segment.args().args + { + hir_ty = arg_ty.as_unambig_ty(); + } + hir_ty +} diff --git a/tests/ui/needless_pass_by_value.rs b/tests/ui/needless_pass_by_value.rs index adea373fd55a..aef7fff28705 100644 --- a/tests/ui/needless_pass_by_value.rs +++ b/tests/ui/needless_pass_by_value.rs @@ -196,6 +196,35 @@ fn option_inner_ref(x: Option) { assert!(x.is_some()); } +mod non_standard { + #[derive(Debug)] + pub struct Option(T); +} + +fn non_standard_option(x: non_standard::Option) { + //~^ needless_pass_by_value + dbg!(&x); +} + +fn option_by_name(x: Option>>>) { + //~^ needless_pass_by_value + dbg!(&x); +} + +type OptStr = Option; + +fn non_option(x: OptStr) { + //~^ needless_pass_by_value + dbg!(&x); +} + +type Opt = Option; + +fn non_option_either(x: Opt) { + //~^ needless_pass_by_value + dbg!(&x); +} + fn main() { // This should not cause an ICE either // https://github.com/rust-lang/rust-clippy/issues/3144 diff --git a/tests/ui/needless_pass_by_value.stderr b/tests/ui/needless_pass_by_value.stderr index 987bfc4affc5..fc67e06ca42b 100644 --- a/tests/ui/needless_pass_by_value.stderr +++ b/tests/ui/needless_pass_by_value.stderr @@ -29,7 +29,7 @@ error: this argument is passed by value, but not consumed in the function body --> tests/ui/needless_pass_by_value.rs:58:18 | LL | fn test_match(x: Option>, y: Option>) { - | ^^^^^^^^^^^^^^^^^^^^^^ help: consider taking a reference instead: `Option<&Option>` + | ^^^^^^^^^^^^^^^^^^^^^^ help: consider taking a reference instead: `Option>` error: this argument is passed by value, but not consumed in the function body --> tests/ui/needless_pass_by_value.rs:73:24 @@ -185,5 +185,29 @@ error: this argument is passed by value, but not consumed in the function body LL | fn option_inner_ref(x: Option) { | ^^^^^^^^^^^^^^ help: consider taking a reference instead: `Option<&String>` -error: aborting due to 23 previous errors +error: this argument is passed by value, but not consumed in the function body + --> tests/ui/needless_pass_by_value.rs:204:27 + | +LL | fn non_standard_option(x: non_standard::Option) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider taking a reference instead: `&non_standard::Option` + +error: this argument is passed by value, but not consumed in the function body + --> tests/ui/needless_pass_by_value.rs:209:22 + | +LL | fn option_by_name(x: Option>>>) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider taking a reference instead: `Option>>>` + +error: this argument is passed by value, but not consumed in the function body + --> tests/ui/needless_pass_by_value.rs:216:18 + | +LL | fn non_option(x: OptStr) { + | ^^^^^^ help: consider taking a reference instead: `&OptStr` + +error: this argument is passed by value, but not consumed in the function body + --> tests/ui/needless_pass_by_value.rs:223:25 + | +LL | fn non_option_either(x: Opt) { + | ^^^^^^^^^^^ help: consider taking a reference instead: `&Opt` + +error: aborting due to 27 previous errors