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

Author: nyurik

CHANGELOG.md

@@ -5814,6 +5814,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,26 @@ 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.
+    /// See [YouTube video](https://www.youtube.com/watch?v=6c7pZYP_iIE)
+    /// ### Example
+    /// ```no_run
+    /// fn foo(a: &Option<String>) {}
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// fn foo(a: Option<&String>) {}
+    /// ```
+    #[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 +458,7 @@ impl_lint_pass!(Functions => [
     MISNAMED_GETTERS,
     IMPL_TRAIT_IN_PARAMS,
     RENAMED_FUNCTION_PARAMS,
+    REF_OPTION,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Functions {
@@ -455,18 +477,21 @@ 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);
     }
 
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
         must_use::check_item(cx, item);
         result::check_item(cx, item, self.large_error_threshold);
+        ref_option::check_item(cx, item, self.avoid_breaking_exported_api);
     }
 
     fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) {
         must_use::check_impl_item(cx, item);
         result::check_impl_item(cx, item, self.large_error_threshold);
         impl_trait_in_params::check_impl_item(cx, item);
         renamed_function_params::check_impl_item(cx, item, &self.trait_ids);
+        ref_option::check_impl_item(cx, item, self.avoid_breaking_exported_api);
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) {
@@ -475,5 +500,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,108 @@
+use crate::functions::hir::GenericArg;
+use crate::functions::REF_OPTION;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::source::snippet;
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{FnDecl, GenericArgs, ImplItem, MutTy, QPath, TraitItem, Ty, TyKind};
+use rustc_lint::LateContext;
+use rustc_span::{sym, Span};
+
+fn check_ty(cx: &LateContext<'_>, param: &Ty<'_>, fixes: &mut Vec<(Span, String)>) {
+    // TODO: is this the right way to check if ty is an Option?
+    if let TyKind::Ref(lifetime, MutTy { ty, .. }) = param.kind
+        && let TyKind::Path(QPath::Resolved(_, path)) = ty.kind
+        && path.segments.len() == 1
+        && let seg = &path.segments[0]
+        && seg.ident.name == sym::Option
+        // check if option contains a regular type, not a reference
+        // TODO: Should this instead just check that opt_ty is a TyKind::Path?
+        && let Some(GenericArgs { args: [GenericArg::Type(opt_ty)], .. }) = seg.args
+        && !matches!(opt_ty.kind, TyKind::Ref(..))
+    {
+        // FIXME: Should this use the Option path from the original type?
+        // FIXME: Should reference be added in some other way to the snippet?
+        // FIXME: What should the lifetime be of the reference in Option<&T>?
+        let lifetime = snippet(cx, lifetime.ident.span, "..");
+        fixes.push((
+            param.span,
+            format!(
+                "Option<&{}{}{}>",
+                lifetime,
+                if lifetime.is_empty() { "" } else { " " },
+                snippet(cx, opt_ty.span, "..")
+            ),
+        ));
+    }
+}
+
+fn check_fn_sig(cx: &LateContext<'_>, decl: &FnDecl<'_>, span: Span) {
+    // dbg!(snippet(cx, span, ".."));
+    let mut fixes = Vec::new();
+    // Check function arguments' types
+    for param in decl.inputs {
+        check_ty(cx, param, &mut fixes);
+    }
+    // Check return type
+    if let hir::FnRetTy::Return(ty) = &decl.output {
+        check_ty(cx, ty, &mut fixes);
+    }
+    if !fixes.is_empty() {
+        // FIXME: These changes will often result in broken code that will need to be fixed manually
+        //        What should the applicability be?
+        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::HasPlaceholders);
+            },
+        );
+    }
+}
+
+pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>, avoid_breaking_exported_api: bool) {
+    if let hir::ItemKind::Fn(ref sig, _, _) = item.kind
+        && !(avoid_breaking_exported_api && cx.effective_visibilities.is_exported(item.owner_id.def_id))
+    {
+        check_fn_sig(cx, sig.decl, sig.span);
+    }
+}
+
+pub(super) fn check_impl_item(cx: &LateContext<'_>, impl_item: &ImplItem<'_>, avoid_breaking_exported_api: bool) {
+    if let hir::ImplItemKind::Fn(ref sig, _) = impl_item.kind
+        && !(avoid_breaking_exported_api && cx.effective_visibilities.is_exported(impl_item.owner_id.def_id))
+    {
+        check_fn_sig(cx, sig.decl, sig.span);
+    }
+}
+
+pub(super) fn check_trait_item(cx: &LateContext<'_>, trait_item: &TraitItem<'_>, 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))
+    {
+        check_fn_sig(cx, sig.decl, sig.span);
+    }
+}
+
+pub(crate) fn check_fn(cx: &LateContext<'_>, kind: FnKind<'_>, decl: &FnDecl<'_>, span: Span) {
+    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;
+        };
+
+        check_fn_sig(cx, decl, span);
+    }
+}

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,55 @@
+//@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::all)]
+#![warn(clippy::ref_option)]
+
+fn main() {}
+
+// FIXME: Using `&None` instead of `unimplemented!()` is a better test, but it results in an error:
+//        `after rustfix is applied, all errors should be gone, but weren't`
+
+fn opt_u8(a: Option<&u8>) {}
+fn opt_gen<T>(a: Option<&T>) {}
+fn opt_string(a: Option<&String>) {}
+fn ret_string<'a>(p: &'a String) -> Option<&'a u8> {
+    panic!()
+}
+fn ret_string_static() -> Option<&'static u8> {
+    panic!()
+}
+fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
+pub fn pub_mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
+
+pub fn pub_opt_string(a: Option<&String>) {}
+
+pub trait PubTrait {
+    fn trait_opt(&self, a: Option<&Vec<u8>>);
+    fn trait_ret(&self) -> Option<&Vec<u8>>;
+}
+
+trait PrivateTrait {
+    fn private_trait_opt(&self, a: Option<&String>);
+    fn private_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!()
+    }
+}
+
+fn lambdas() {
+    let s = Some("hello".to_string());
+    let x = |a: Option<&String>| {};
+    let x = |a: Option<&String>| -> Option<&String> { panic!() };
+}