needless_pass_by_value: reference the innermost Option content (rust-lang#14392)

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

changelog: [`needless_pass_by_value`]: suggest using a reference on the
innermost `Option` content

fix rust-lang#14375

Author: flip1995

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,10 @@ 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, "_"))
-                    };
-                    diag.span_suggestion(
-                        input.span,
+                    diag.span_suggestion_verbose(
+                        peel_hir_ty_options(cx, input).span.shrink_to_lo(),
                         "consider taking a reference instead",
-                        suggestion,
+                        '&',
                         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/crashes/needless_pass_by_value-w-late-bound.stderr

@@ -2,7 +2,7 @@ error: this argument is passed by value, but not consumed in the function body
   --> tests/ui/crashes/needless_pass_by_value-w-late-bound.rs:7:12
    |
 LL | fn test(x: Foo<'_>) {}
-   |            ^^^^^^^ help: consider taking a reference instead: `&Foo<'_>`
+   |            ^^^^^^^
    |
 help: or consider marking this type as `Copy`
   --> tests/ui/crashes/needless_pass_by_value-w-late-bound.rs:5:1
@@ -11,6 +11,10 @@ LL | struct Foo<'a>(&'a [(); 100]);
    | ^^^^^^^^^^^^^^
    = note: `-D clippy::needless-pass-by-value` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_pass_by_value)]`
+help: consider taking a reference instead
+   |
+LL | fn test(x: &Foo<'_>) {}
+   |            +
 
 error: aborting due to 1 previous error
 

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