needless_pass_by_value: reference the innermost Option content

If types such as `Option<Option<String>>` are not used by value, then
`Option<Option<&String>>` will be suggested, instead of
`Option<&Option<String>>`.

Author: samueltardieu

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,
                     );
                 };

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<Item = &'tc
         .map_or(&[][..], |a| a.args)
         .iter()
         .filter_map(|a| match a {
-            hir::GenericArg::Type(ty) => 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
+}

tests/ui/needless_pass_by_value.rs

@@ -196,6 +196,35 @@ fn option_inner_ref(x: Option<String>) {
     assert!(x.is_some());
 }
 
+mod non_standard {
+    #[derive(Debug)]
+    pub struct Option<T>(T);
+}
+
+fn non_standard_option(x: non_standard::Option<String>) {
+    //~^ needless_pass_by_value
+    dbg!(&x);
+}
+
+fn option_by_name(x: Option<std::option::Option<core::option::Option<non_standard::Option<String>>>>) {
+    //~^ needless_pass_by_value
+    dbg!(&x);
+}
+
+type OptStr = Option<String>;
+
+fn non_option(x: OptStr) {
+    //~^ needless_pass_by_value
+    dbg!(&x);
+}
+
+type Opt<T> = Option<T>;
+
+fn non_option_either(x: Opt<String>) {
+    //~^ needless_pass_by_value
+    dbg!(&x);
+}
+
 fn main() {
     // This should not cause an ICE either
     // https://github.com/rust-lang/rust-clippy/issues/3144

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<Option<String>>, y: Option<Option<String>>) {
-   |                  ^^^^^^^^^^^^^^^^^^^^^^ help: consider taking a reference instead: `Option<&Option<String>>`
+   |                  ^^^^^^^^^^^^^^^^^^^^^^ help: consider taking a reference instead: `Option<Option<&String>>`
 
 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<String>) {
    |                        ^^^^^^^^^^^^^^ 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<String>) {
+   |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider taking a reference instead: `&non_standard::Option<String>`
+
+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<std::option::Option<core::option::Option<non_standard::Option<String>>>>) {
+   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider taking a reference instead: `Option<std::option::Option<core::option::Option<&non_standard::Option<String>>>>`
+
+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<String>) {
+   |                         ^^^^^^^^^^^ help: consider taking a reference instead: `&Opt<String>`
+
+error: aborting due to 27 previous errors