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

Author: nyurik

CHANGELOG.md

@@ -5814,6 +5814,7 @@ Released 2018-09-13
 [`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_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref
+[`ref_option_sig`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_sig
 [`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
 [`renamed_function_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#renamed_function_params

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_SIG_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_sig;
 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_SIG,
+    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_SIG,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Functions {
@@ -460,13 +482,15 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
     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_sig::check_item(cx, item);
     }
 
     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_sig::check_impl_item(cx, item);
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) {
@@ -475,5 +499,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_sig::check_trait_item(cx, item);
     }
 }

clippy_lints/src/functions/ref_option_sig.rs

@@ -0,0 +1,60 @@
+use crate::functions::hir::GenericArg;
+use crate::functions::REF_OPTION_SIG;
+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::{GenericArgs, ImplItem, MutTy, QPath, TraitItem, TyKind};
+use rustc_lint::LateContext;
+use rustc_span::{sym, Span};
+
+fn check_fn_sig(cx: &LateContext<'_>, decl: &hir::FnDecl<'_>, sig_span: Span) {
+    let mut fixes = Vec::new();
+    for param in decl.inputs {
+        if let TyKind::Ref(_, MutTy { ty, .. }) = param.kind
+            // TODO: is this the right way to check if ty is an Option?
+            && 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?
+            fixes.push((param.span, format!("Option<&{}>", snippet(cx, opt_ty.span, ".."))));
+        }
+    }
+
+    if !fixes.is_empty() {
+        span_lint_and_then(
+            cx,
+            REF_OPTION_SIG,
+            sig_span,
+            "it is more idiomatic to use `Option<&T>` instead of `&Option<T>`",
+            |diag| {
+                diag.multipart_suggestion("change this to", fixes, Applicability::Unspecified);
+            },
+        );
+    }
+}
+
+pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
+    if let hir::ItemKind::Fn(ref sig, _, _) = item.kind {
+        check_fn_sig(cx, sig.decl, sig.span);
+    }
+}
+
+pub(super) fn check_impl_item(cx: &LateContext<'_>, impl_item: &ImplItem<'_>) {
+    if let hir::ImplItemKind::Fn(ref sig, _) = impl_item.kind {
+        check_fn_sig(cx, sig.decl, sig.span);
+    }
+}
+
+pub(super) fn check_trait_item(cx: &LateContext<'_>, trait_item: &TraitItem<'_>) {
+    if let hir::TraitItemKind::Fn(ref sig, _) = trait_item.kind {
+        check_fn_sig(cx, sig.decl, sig.span);
+    }
+}

tests/ui/ref_option_sig.fixed

@@ -0,0 +1,25 @@
+#![allow(unused, clippy::all)]
+#![warn(clippy::ref_option_sig)]
+
+fn main() {}
+
+fn opt_u8(a: Option<&u8>) {}
+fn opt_gen<T>(a: Option<&T>) {}
+fn opt_string(a: Option<&String>) {}
+fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
+
+pub fn pub_opt_string(a: Option<&String>) {}
+
+pub trait HasOpt {
+    fn trait_opt(&self, a: Option<&Vec<u8>>);
+}
+
+trait PrivateHasOpt {
+    fn private_trait_opt(&self, a: Option<&Option<String>>);
+}
+
+pub struct UnitOpt;
+
+impl UnitOpt {
+    pub fn opt_params(&self, a: Option<&()>) {}
+}

tests/ui/ref_option_sig.rs

@@ -0,0 +1,25 @@
+#![allow(unused, clippy::all)]
+#![warn(clippy::ref_option_sig)]
+
+fn main() {}
+
+fn opt_u8(a: &Option<u8>) {}
+fn opt_gen<T>(a: &Option<T>) {}
+fn opt_string(a: &Option<String>) {}
+fn mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {}
+
+pub fn pub_opt_string(a: &Option<String>) {}
+
+pub trait HasOpt {
+    fn trait_opt(&self, a: &Option<Vec<u8>>);
+}
+
+trait PrivateHasOpt {
+    fn private_trait_opt(&self, a: &Option<Option<String>>);
+}
+
+pub struct UnitOpt;
+
+impl UnitOpt {
+    pub fn opt_params(&self, a: &Option<()>) {}
+}

tests/ui/ref_option_sig.stderr

@@ -0,0 +1,72 @@
+error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
+  --> tests/ui/ref_option_sig.rs:6:1
+   |
+LL | fn opt_u8(a: &Option<u8>) {}
+   | ^^^^^^^^^^^^^-----------^
+   |              |
+   |              help: change this to: `Option<&u8>`
+   |
+   = note: `-D clippy::ref-option-sig` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::ref_option_sig)]`
+
+error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
+  --> tests/ui/ref_option_sig.rs:7:1
+   |
+LL | fn opt_gen<T>(a: &Option<T>) {}
+   | ^^^^^^^^^^^^^^^^^----------^
+   |                  |
+   |                  help: change this to: `Option<&T>`
+
+error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
+  --> tests/ui/ref_option_sig.rs:8:1
+   |
+LL | fn opt_string(a: &Option<String>) {}
+   | ^^^^^^^^^^^^^^^^^---------------^
+   |                  |
+   |                  help: change this to: `Option<&String>`
+
+error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
+  --> tests/ui/ref_option_sig.rs:9:1
+   |
+LL | fn mult_string(a: &Option<String>, b: &Option<Vec<u8>>) {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: change this to
+   |
+LL | fn mult_string(a: Option<&String>, b: Option<&Vec<u8>>) {}
+   |                   ~~~~~~~~~~~~~~~     ~~~~~~~~~~~~~~~~
+
+error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
+  --> tests/ui/ref_option_sig.rs:11:1
+   |
+LL | pub fn pub_opt_string(a: &Option<String>) {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^---------------^
+   |                          |
+   |                          help: change this to: `Option<&String>`
+
+error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
+  --> tests/ui/ref_option_sig.rs:14:5
+   |
+LL |     fn trait_opt(&self, a: &Option<Vec<u8>>);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^----------------^^
+   |                            |
+   |                            help: change this to: `Option<&Vec<u8>>`
+
+error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
+  --> tests/ui/ref_option_sig.rs:18:5
+   |
+LL |     fn private_trait_opt(&self, a: &Option<Option<String>>);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------------------^^
+   |                                    |
+   |                                    help: change this to: `Option<&Option<String>>`
+
+error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
+  --> tests/ui/ref_option_sig.rs:24:5
+   |
+LL |     pub fn opt_params(&self, a: &Option<()>) {}
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^
+   |                                 |
+   |                                 help: change this to: `Option<&()>`
+
+error: aborting due to 8 previous errors
+