Auto merge of rust-lang#13336 - nyurik:ref-option-sig, r=llogiq

Suggest `Option<&T>` instead of `&Option<T>`

closes rust-lang#13054

```rust
// bad code
fn foo(a: &Option<T>) {}
fn bar(&self) -> &Option<T> {}

// Use instead
fn foo(a: Option<&T>) {}
fn bar(&self) -> Option<&T> {}
```

Handles argument types and return types in functions, methods, and closures with explicit types.  Honors `avoid_breaking_exported_api` parameter.

See this great [YouTube video](https://www.youtube.com/watch?v=6c7pZYP_iIE) with the in-depth explanation.

### Open Questions
These are not blocking, and could be done in separate PRs if needed.
* [ ] Should `&Option<Box<T>>` be suggested as `Option<&T>` -- without the box?  Handled by [clippy::borrowed_box](https://rust-lang.github.io/rust-clippy/master/index.html#/borrowed_box)
* [ ] Should `&Option<String>` be suggested as `Option<&str>` -- using de-refed type?

### Possible Future Improvements
These cases might also be good to handle, probably in a separate PR.

```rust
fn lambdas() {
  let x = |a: &Option<String>| {};
  let x = |a: &Option<String>| -> &Option<String> { todo!() };
}

fn mut_ref_to_ref(a: &mut &Option<u8>) {}
```

changelog: [`ref_option`]: Suggest `Option<&T>` instead of `&Option<T>`

Author: bors

CHANGELOG.md

@@ -5871,6 +5871,7 @@ Released 2018-09-13
 [`ref_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr
 [`ref_binding_to_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_binding_to_reference
 [`ref_in_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_in_deref
+[`ref_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option
 [`ref_option_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref
 [`ref_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_patterns
 [`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro

book/src/lint_configuration.md

@@ -353,6 +353,7 @@ Suppress lints whenever the suggested change would cause breakage for other crat
 * [`rc_buffer`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer)
 * [`rc_mutex`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex)
 * [`redundant_allocation`](https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation)
+* [`ref_option`](https://rust-lang.github.io/rust-clippy/master/index.html#ref_option)
 * [`single_call_fn`](https://rust-lang.github.io/rust-clippy/master/index.html#single_call_fn)
 * [`trivially_copy_pass_by_ref`](https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref)
 * [`unnecessary_box_returns`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns)

clippy_config/src/conf.rs

@@ -378,6 +378,7 @@ define_Conf! {
         rc_buffer,
         rc_mutex,
         redundant_allocation,
+        ref_option,
         single_call_fn,
         trivially_copy_pass_by_ref,
         unnecessary_box_returns,

clippy_lints/src/declared_lints.rs

@@ -202,6 +202,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::functions::MUST_USE_CANDIDATE_INFO,
     crate::functions::MUST_USE_UNIT_INFO,
     crate::functions::NOT_UNSAFE_PTR_ARG_DEREF_INFO,
+    crate::functions::REF_OPTION_INFO,
     crate::functions::RENAMED_FUNCTION_PARAMS_INFO,
     crate::functions::RESULT_LARGE_ERR_INFO,
     crate::functions::RESULT_UNIT_ERR_INFO,

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

tests/ui/ref_option/all/clippy.toml

@@ -0,0 +1 @@
+avoid-breaking-exported-api = false

tests/ui/ref_option/private/clippy.toml

@@ -0,0 +1 @@
+avoid-breaking-exported-api = true

tests/ui/ref_option/ref_option.all.fixed

@@ -0,0 +1,62 @@
+//@revisions: private all
+//@[private] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/private
+//@[all] rustc-env:CLIPPY_CONF_DIR=tests/ui/ref_option/all
+
+#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box)]
+#![warn(clippy::ref_option)]
+
+fn opt_u8(a: Option<&u8>) {}
+fn opt_gen<T>(a: Option<&T>) {}
+fn opt_string(a: std::option::Option<&String>) {}
+fn ret_string<'a>(p: &'a str) -> Option<&'a u8> {
+    panic!()
+}
+fn ret_string_static() -> Option<&'static u8> {
+    panic!()
+}
+fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
+fn ret_box<'a>() -> Option<&'a Box<u8>> {
+    panic!()
+}
+
+pub fn pub_opt_string(a: Option<&String>) {}
+pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
+
+pub trait PubTrait {
+    fn pub_trait_opt(&self, a: Option<&Vec<u8>>);
+    fn pub_trait_ret(&self) -> Option<&Vec<u8>>;
+}
+
+trait PrivateTrait {
+    fn trait_opt(&self, a: Option<&String>);
+    fn trait_ret(&self) -> Option<&String>;
+}
+
+pub struct PubStruct;
+
+impl PubStruct {
+    pub fn pub_opt_params(&self, a: Option<&()>) {}
+    pub fn pub_opt_ret(&self) -> Option<&String> {
+        panic!()
+    }
+
+    fn private_opt_params(&self, a: Option<&()>) {}
+    fn private_opt_ret(&self) -> Option<&String> {
+        panic!()
+    }
+}
+
+// valid, don't change
+fn mut_u8(a: &mut Option<u8>) {}
+pub fn pub_mut_u8(a: &mut Option<String>) {}
+
+// might be good to catch in the future
+fn mut_u8_ref(a: &mut &Option<u8>) {}
+pub fn pub_mut_u8_ref(a: &mut &Option<String>) {}
+fn lambdas() {
+    // Not handled for now, not sure if we should
+    let x = |a: &Option<String>| {};
+    let x = |a: &Option<String>| -> &Option<String> { panic!() };
+}
+
+fn main() {}