Merge commit 'aa0d551351' into clippy-subtree-update

clippy_lints/src/functions/mod.rs

@@ -2,6 +2,7 @@ mod impl_trait_in_params;
 mod misnamed_getters;
 mod must_use;
 mod not_unsafe_ptr_arg_deref;
+mod ref_option;
 mod renamed_function_params;
 mod result;
 mod too_many_arguments;
@@ -399,6 +400,53 @@ declare_clippy_lint! {
     "renamed function parameters in trait implementation"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Warns when a function signature uses `&Option<T>` instead of `Option<&T>`.
+    ///
+    /// ### Why is this bad?
+    /// More flexibility, better memory optimization, and more idiomatic Rust code.
+    ///
+    /// `&Option<T>` in a function signature breaks encapsulation because the caller must own T
+    /// and move it into an Option to call with it. When returned, the owner must internally store
+    /// it as `Option<T>` in order to return it.
+    /// At a lower level, `&Option<T>` points to memory with the `presence` bit flag plus the `T` value,
+    /// whereas `Option<&T>` is usually [optimized](https://doc.rust-lang.org/1.81.0/std/option/index.html#representation)
+    /// to a single pointer, so it may be more optimal.
+    ///
+    /// See this [YouTube video](https://www.youtube.com/watch?v=6c7pZYP_iIE) by
+    /// Logan Smith for an in-depth explanation of why this is important.
+    ///
+    /// ### Known problems
+    /// This lint recommends changing the function signatures, but it cannot
+    /// automatically change the function calls or the function implementations.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// // caller uses  foo(&opt)
+    /// fn foo(a: &Option<String>) {}
+    /// # struct Unit {}
+    /// # impl Unit {
+    /// fn bar(&self) -> &Option<String> { &None }
+    /// # }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// // caller should use  `foo1(opt.as_ref())`
+    /// fn foo1(a: Option<&String>) {}
+    /// // better yet, use string slice  `foo2(opt.as_deref())`
+    /// fn foo2(a: Option<&str>) {}
+    /// # struct Unit {}
+    /// # impl Unit {
+    /// fn bar(&self) -> Option<&String> { None }
+    /// # }
+    /// ```
+    #[clippy::version = "1.82.0"]
+    pub REF_OPTION,
+    pedantic,
+    "function signature uses `&Option<T>` instead of `Option<&T>`"
+}
+
 pub struct Functions {
     too_many_arguments_threshold: u64,
     too_many_lines_threshold: u64,
@@ -437,6 +485,7 @@ impl_lint_pass!(Functions => [
     MISNAMED_GETTERS,
     IMPL_TRAIT_IN_PARAMS,
     RENAMED_FUNCTION_PARAMS,
+    REF_OPTION,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Functions {
@@ -455,6 +504,16 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
         not_unsafe_ptr_arg_deref::check_fn(cx, kind, decl, body, def_id);
         misnamed_getters::check_fn(cx, kind, decl, body, span);
         impl_trait_in_params::check_fn(cx, &kind, body, hir_id);
+        ref_option::check_fn(
+            cx,
+            kind,
+            decl,
+            span,
+            hir_id,
+            def_id,
+            body,
+            self.avoid_breaking_exported_api,
+        );
     }
 
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
@@ -475,5 +534,6 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
         must_use::check_trait_item(cx, item);
         result::check_trait_item(cx, item, self.large_error_threshold);
         impl_trait_in_params::check_trait_item(cx, item, self.avoid_breaking_exported_api);
+        ref_option::check_trait_item(cx, item, self.avoid_breaking_exported_api);
     }
 }

clippy_lints/src/functions/ref_option.rs

@@ -0,0 +1,122 @@
+use crate::functions::REF_OPTION;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::is_trait_impl_item;
+use clippy_utils::source::snippet;
+use clippy_utils::ty::is_type_diagnostic_item;
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{FnDecl, HirId};
+use rustc_lint::LateContext;
+use rustc_middle::ty::{self, GenericArgKind, Mutability, Ty};
+use rustc_span::def_id::LocalDefId;
+use rustc_span::{Span, sym};
+
+fn check_ty<'a>(cx: &LateContext<'a>, param: &rustc_hir::Ty<'a>, param_ty: Ty<'a>, fixes: &mut Vec<(Span, String)>) {
+    if let ty::Ref(_, opt_ty, Mutability::Not) = param_ty.kind()
+        && is_type_diagnostic_item(cx, *opt_ty, sym::Option)
+        && let ty::Adt(_, opt_gen) = opt_ty.kind()
+        && let [gen] = opt_gen.as_slice()
+        && let GenericArgKind::Type(gen_ty) = gen.unpack()
+        && !gen_ty.is_ref()
+        // Need to gen the original spans, so first parsing mid, and hir parsing afterward
+        && let hir::TyKind::Ref(lifetime, hir::MutTy { ty, .. }) = param.kind
+        && let hir::TyKind::Path(hir::QPath::Resolved(_, path)) = ty.kind
+        && let (Some(first), Some(last)) = (path.segments.first(), path.segments.last())
+        && let Some(hir::GenericArgs {
+            args: [hir::GenericArg::Type(opt_ty)],
+            ..
+        }) = last.args
+    {
+        let lifetime = snippet(cx, lifetime.ident.span, "..");
+        fixes.push((
+            param.span,
+            format!(
+                "{}<&{lifetime}{}{}>",
+                snippet(cx, first.ident.span.to(last.ident.span), ".."),
+                if lifetime.is_empty() { "" } else { " " },
+                snippet(cx, opt_ty.span, "..")
+            ),
+        ));
+    }
+}
+
+fn check_fn_sig<'a>(cx: &LateContext<'a>, decl: &FnDecl<'a>, span: Span, sig: ty::FnSig<'a>) {
+    let mut fixes = Vec::new();
+    // Check function arguments' types
+    for (param, param_ty) in decl.inputs.iter().zip(sig.inputs()) {
+        check_ty(cx, param, *param_ty, &mut fixes);
+    }
+    // Check return type
+    if let hir::FnRetTy::Return(ty) = &decl.output {
+        check_ty(cx, ty, sig.output(), &mut fixes);
+    }
+    if !fixes.is_empty() {
+        span_lint_and_then(
+            cx,
+            REF_OPTION,
+            span,
+            "it is more idiomatic to use `Option<&T>` instead of `&Option<T>`",
+            |diag| {
+                diag.multipart_suggestion("change this to", fixes, Applicability::Unspecified);
+            },
+        );
+    }
+}
+
+#[allow(clippy::too_many_arguments)]
+pub(crate) fn check_fn<'a>(
+    cx: &LateContext<'a>,
+    kind: FnKind<'_>,
+    decl: &FnDecl<'a>,
+    span: Span,
+    hir_id: HirId,
+    def_id: LocalDefId,
+    body: &hir::Body<'_>,
+    avoid_breaking_exported_api: bool,
+) {
+    if avoid_breaking_exported_api && cx.effective_visibilities.is_exported(def_id) {
+        return;
+    }
+
+    if let FnKind::Closure = kind {
+        // Compute the span of the closure parameters + return type if set
+        let span = if let hir::FnRetTy::Return(out_ty) = &decl.output {
+            if decl.inputs.is_empty() {
+                out_ty.span
+            } else {
+                span.with_hi(out_ty.span.hi())
+            }
+        } else if let (Some(first), Some(last)) = (decl.inputs.first(), decl.inputs.last()) {
+            first.span.to(last.span)
+        } else {
+            // No parameters - no point in checking
+            return;
+        };
+
+        // Figure out the signature of the closure
+        let ty::Closure(_, args) = cx.typeck_results().expr_ty(body.value).kind() else {
+            return;
+        };
+        let sig = args.as_closure().sig().skip_binder();
+
+        check_fn_sig(cx, decl, span, sig);
+    } else if !is_trait_impl_item(cx, hir_id) {
+        let sig = cx.tcx.fn_sig(def_id).instantiate_identity().skip_binder();
+        check_fn_sig(cx, decl, span, sig);
+    }
+}
+
+pub(super) fn check_trait_item<'a>(
+    cx: &LateContext<'a>,
+    trait_item: &hir::TraitItem<'a>,
+    avoid_breaking_exported_api: bool,
+) {
+    if let hir::TraitItemKind::Fn(ref sig, _) = trait_item.kind
+        && !(avoid_breaking_exported_api && cx.effective_visibilities.is_exported(trait_item.owner_id.def_id))
+    {
+        let def_id = trait_item.owner_id.def_id;
+        let ty_sig = cx.tcx.fn_sig(def_id).instantiate_identity().skip_binder();
+        check_fn_sig(cx, sig.decl, sig.span, ty_sig);
+    }
+}