WIP New lint: borrowed_option

CHANGELOG.md

@@ -5981,6 +5981,7 @@ Released 2018-09-13
 [`borrow_deref_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_deref_ref
 [`borrow_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const
 [`borrowed_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box
+[`borrowed_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_option
 [`box_collection`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_collection
 [`box_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_default
 [`box_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_vec

clippy_lints/src/declared_lints.rs

@@ -726,6 +726,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::transmute::WRONG_TRANSMUTE_INFO,
     crate::tuple_array_conversions::TUPLE_ARRAY_CONVERSIONS_INFO,
     crate::types::BORROWED_BOX_INFO,
+    crate::types::BORROWED_OPTION_INFO,
     crate::types::BOX_COLLECTION_INFO,
     crate::types::LINKEDLIST_INFO,
     crate::types::OPTION_OPTION_INFO,

clippy_lints/src/types/borrowed_box.rs

@@ -7,15 +7,16 @@ use rustc_hir::{
 use rustc_lint::LateContext;
 use rustc_span::sym;
 
-use super::BORROWED_BOX;
+use super::{BORROWED_BOX, BORROWED_OPTION};
 
 pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, lt: &Lifetime, mut_ty: &MutTy<'_>) -> bool {
     match mut_ty.ty.kind {
         TyKind::Path(ref qpath) => {
             let hir_id = mut_ty.ty.hir_id;
             let def = cx.qpath_res(qpath, hir_id);
             if let Some(def_id) = def.opt_def_id()
-                && Some(def_id) == cx.tcx.lang_items().owned_box()
+                && (Some(def_id) == cx.tcx.lang_items().owned_box()
+                    || Some(def_id) == cx.tcx.lang_items().option_type())
                 && let QPath::Resolved(None, path) = *qpath
                 && let [ref bx] = *path.segments
                 && let Some(params) = bx.args
@@ -25,7 +26,8 @@ pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, lt: &Lifetime, m
                     _ => None,
                 })
             {
-                if is_any_trait(cx, inner.as_unambig_ty()) {
+                let is_box = Some(def_id) == cx.tcx.lang_items().owned_box();
+                if is_box && is_any_trait(cx, inner.as_unambig_ty()) {
                     // Ignore `Box<Any>` types; see issue #1884 for details.
                     return false;
                 }
@@ -39,6 +41,7 @@ pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, lt: &Lifetime, m
                 if mut_ty.mutbl == Mutability::Mut {
                     // Ignore `&mut Box<T>` types; see issue #2907 for
                     // details.
+                    // Same reasoning applies for `&mut Option<T>` types.
                     return false;
                 }
 
@@ -59,11 +62,19 @@ pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, lt: &Lifetime, m
                 };
                 span_lint_and_sugg(
                     cx,
-                    BORROWED_BOX,
+                    if is_box { BORROWED_BOX } else { BORROWED_OPTION },
                     hir_ty.span,
-                    "you seem to be trying to use `&Box<T>`. Consider using just `&T`",
+                    if is_box {
+                        "you seem to be trying to use `&Box<T>`. Consider using just `&T`"
+                    } else {
+                        "you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead"
+                    },
                     "try",
-                    suggestion,
+                    if is_box {
+                        suggestion
+                    } else {
+                        format!("Option<{suggestion}>")
+                    },
                     // To make this `MachineApplicable`, at least one needs to check if it isn't a trait item
                     // because the trait impls of it will break otherwise;
                     // and there may be other cases that result in invalid code.

clippy_lints/src/types/mod.rs

@@ -186,6 +186,38 @@ declare_clippy_lint! {
     "a borrow of a boxed type"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of [`&Option<T>`](https://doc.rust-lang.org/std/option/index.html) anywhere in the code.
+    ///
+    /// ### Why is this bad?
+    /// An `&Option<T>` parameter prevents calling the function if the caller holds a different type, e.g. `Result<T, E>`.
+    /// Using `Option<&T>` generalizes the function, e.g. allowing to pass `res.ok().as_ref()`.
+    /// Returning `&Option<T>` needlessly exposes implementation details and has no advantage over `Option<&T>`.
+    /// `&Option<T>` requieres a dereferencing to determine the variant (`Some` or `None`), while `Option<&T>` does not.
+    ///
+    /// ### Example
+    /// ```rust,compile_fail
+    /// fn foo(bar: &Option<String>) -> &Option<String> { bar }
+    /// fn call_foo(bar: &Result<String, ()>) {
+    ///     foo(bar.ok()); // does not work
+    /// }
+    /// ```
+    ///
+    /// Use instead:
+    ///
+    /// ```rust
+    /// fn foo(bar: Option<&String>) -> Option<&String> { bar }
+    /// fn call_foo(bar: &Result<String, ()>) {
+    ///     foo(bar.ok().as_ref()); // works!
+    /// }
+    /// ```
+    #[clippy::version = "1.74.0"]
+    pub BORROWED_OPTION,
+    complexity,
+    "`&Option<T>` instead of `Option<&T>`"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for usage of redundant allocations anywhere in the code.
@@ -403,6 +435,7 @@ impl_lint_pass!(Types => [
     OPTION_OPTION,
     LINKEDLIST,
     BORROWED_BOX,
+    BORROWED_OPTION,
     REDUNDANT_ALLOCATION,
     RC_BUFFER,
     RC_MUTEX,

clippy_lints/src/write.rs

@@ -615,7 +615,7 @@ fn relocalize_format_args_indexes(
         // If there are format options, we need to handle them as well
         if *format_options != FormatOptions::default() {
             // lambda to process width and precision format counts and add them to the suggestion
-            let mut process_format_count = |count: &Option<FormatCount>, formatter: &dyn Fn(usize) -> String| {
+            let mut process_format_count = |count: Option<&FormatCount>, formatter: &dyn Fn(usize) -> String| {
                 if let Some(FormatCount::Argument(FormatArgPosition {
                     index: Ok(format_arg_index),
                     kind: FormatArgPositionKind::Number,
@@ -626,8 +626,8 @@ fn relocalize_format_args_indexes(
                 }
             };
 
-            process_format_count(&format_options.width, &|index: usize| format!("{index}$"));
-            process_format_count(&format_options.precision, &|index: usize| format!(".{index}$"));
+            process_format_count(format_options.width.as_ref(), &|index: usize| format!("{index}$"));
+            process_format_count(format_options.precision.as_ref(), &|index: usize| format!(".{index}$"));
         }
     }
 }

clippy_utils/src/ast_utils/mod.rs

@@ -400,7 +400,7 @@ pub fn eq_item_kind(l: &ItemKind, r: &ItemKind) -> bool {
                 && eq_fn_sig(lf, rf)
                 && eq_id(*li, *ri)
                 && eq_generics(lg, rg)
-                && eq_opt_fn_contract(lc, rc)
+                && eq_opt_fn_contract(lc.as_deref(), rc.as_deref())
                 && both(lb.as_ref(), rb.as_ref(), |l, r| eq_block(l, r))
         },
         (Mod(ls, li, lmk), Mod(rs, ri, rmk)) => {
@@ -554,7 +554,7 @@ pub fn eq_foreign_item_kind(l: &ForeignItemKind, r: &ForeignItemKind) -> bool {
                 && eq_fn_sig(lf, rf)
                 && eq_id(*li, *ri)
                 && eq_generics(lg, rg)
-                && eq_opt_fn_contract(lc, rc)
+                && eq_opt_fn_contract(lc.as_deref(), rc.as_deref())
                 && both(lb.as_ref(), rb.as_ref(), |l, r| eq_block(l, r))
         },
         (
@@ -637,7 +637,7 @@ pub fn eq_assoc_item_kind(l: &AssocItemKind, r: &AssocItemKind) -> bool {
                 && eq_fn_sig(lf, rf)
                 && eq_id(*li, *ri)
                 && eq_generics(lg, rg)
-                && eq_opt_fn_contract(lc, rc)
+                && eq_opt_fn_contract(lc.as_deref(), rc.as_deref())
                 && both(lb.as_ref(), rb.as_ref(), |l, r| eq_block(l, r))
         },
         (
@@ -723,8 +723,7 @@ pub fn eq_fn_header(l: &FnHeader, r: &FnHeader) -> bool {
         && eq_ext(&l.ext, &r.ext)
 }
 
-#[expect(clippy::ref_option, reason = "This is the type how it is stored in the AST")]
-pub fn eq_opt_fn_contract(l: &Option<Box<FnContract>>, r: &Option<Box<FnContract>>) -> bool {
+pub fn eq_opt_fn_contract(l: Option<&FnContract>, r: Option<&FnContract>) -> bool {
     match (l, r) {
         (Some(l), Some(r)) => {
             eq_expr_opt(l.requires.as_deref(), r.requires.as_deref())

tests/ui-toml/ref_option/ref_option.all.fixed

@@ -3,7 +3,7 @@
 //@[private] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/ref_option/private
 //@[all] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/ref_option/all
 
-#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box)]
+#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box, clippy::borrowed_option)]
 #![warn(clippy::ref_option)]
 
 fn opt_u8(a: Option<&u8>) {}

tests/ui-toml/ref_option/ref_option.private.fixed

@@ -3,7 +3,7 @@
 //@[private] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/ref_option/private
 //@[all] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/ref_option/all
 
-#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box)]
+#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box, clippy::borrowed_option)]
 #![warn(clippy::ref_option)]
 
 fn opt_u8(a: Option<&u8>) {}

tests/ui-toml/ref_option/ref_option.rs

@@ -3,7 +3,7 @@
 //@[private] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/ref_option/private
 //@[all] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/ref_option/all
 
-#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box)]
+#![allow(unused, clippy::needless_lifetimes, clippy::borrowed_box, clippy::borrowed_option)]
 #![warn(clippy::ref_option)]
 
 fn opt_u8(a: &Option<u8>) {}

tests/ui-toml/ref_option/ref_option_traits.all.stderr

@@ -1,5 +1,5 @@
 error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
-  --> tests/ui-toml/ref_option/ref_option_traits.rs:10:5
+  --> tests/ui-toml/ref_option/ref_option_traits.rs:11:5
    |
 LL |     fn pub_trait_opt(&self, a: &Option<Vec<u8>>);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^----------------^^
@@ -10,23 +10,23 @@ LL |     fn pub_trait_opt(&self, a: &Option<Vec<u8>>);
    = help: to override `-D warnings` add `#[allow(clippy::ref_option)]`
 
 error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
-  --> tests/ui-toml/ref_option/ref_option_traits.rs:12:5
+  --> tests/ui-toml/ref_option/ref_option_traits.rs:13:5
    |
 LL |     fn pub_trait_ret(&self) -> &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-toml/ref_option/ref_option_traits.rs:17:5
+  --> tests/ui-toml/ref_option/ref_option_traits.rs:18:5
    |
 LL |     fn trait_opt(&self, a: &Option<String>);
    |     ^^^^^^^^^^^^^^^^^^^^^^^---------------^^
    |                            |
    |                            help: change this to: `Option<&String>`
 
 error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
-  --> tests/ui-toml/ref_option/ref_option_traits.rs:19:5
+  --> tests/ui-toml/ref_option/ref_option_traits.rs:20:5
    |
 LL |     fn trait_ret(&self) -> &Option<String>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^---------------^

tests/ui-toml/ref_option/ref_option_traits.private.stderr

@@ -1,5 +1,5 @@
 error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
-  --> tests/ui-toml/ref_option/ref_option_traits.rs:17:5
+  --> tests/ui-toml/ref_option/ref_option_traits.rs:18:5
    |
 LL |     fn trait_opt(&self, a: &Option<String>);
    |     ^^^^^^^^^^^^^^^^^^^^^^^---------------^^
@@ -10,7 +10,7 @@ LL |     fn trait_opt(&self, a: &Option<String>);
    = help: to override `-D warnings` add `#[allow(clippy::ref_option)]`
 
 error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
-  --> tests/ui-toml/ref_option/ref_option_traits.rs:19:5
+  --> tests/ui-toml/ref_option/ref_option_traits.rs:20:5
    |
 LL |     fn trait_ret(&self) -> &Option<String>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^---------------^

tests/ui-toml/ref_option/ref_option_traits.rs

@@ -5,6 +5,7 @@
 //@[all] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/ref_option/all
 
 #![warn(clippy::ref_option)]
+#![allow(clippy::borrowed_option)]
 
 pub trait PubTrait {
     fn pub_trait_opt(&self, a: &Option<Vec<u8>>);

tests/ui/borrow_option.fixed

@@ -0,0 +1,59 @@
+#![deny(clippy::borrowed_option)]
+#![allow(dead_code, unused_variables)]
+#![allow(
+    clippy::uninlined_format_args,
+    clippy::disallowed_names,
+    clippy::needless_pass_by_ref_mut,
+    clippy::needless_lifetimes
+)]
+
+use std::fmt::Display;
+
+pub fn test1(foo: &mut Option<bool>) {
+    // Although this function could be changed to "Option<&mut bool>",
+    // avoiding the borrowed option, mutable references to boxes are not
+    // flagged by this lint.
+    //
+    // This omission is intentional: By passing a "&mut Option<T>",
+    // the memory location of the pointed-to object could be
+    // modified. By passing a "Option<&mut T>", the contents
+    // could change, but not the location.
+    println!("{:?}", foo)
+}
+
+pub fn test2() {
+    let foo: Option<&bool>;
+    //~^ borrowed_option
+}
+
+struct Test3<'a> {
+    foo: Option<&'a bool>,
+    //~^ borrowed_option
+}
+
+trait Test4 {
+    fn test4(b: Option<&bool>);
+    //~^ borrowed_option
+}
+
+pub fn test5(_display: Option<&impl Display>) {}
+//~^ borrowed_option
+pub fn test6(_display: Option<&(impl Display + Send)>) {}
+//~^ borrowed_option
+pub fn test7<'a>(_display: Option<&'a (impl Display + 'a)>) {}
+//~^ borrowed_option
+
+#[allow(clippy::borrowed_box, clippy::borrowed_option)]
+trait Trait {
+    fn f(o: &Option<bool>);
+}
+
+// Trait impls are not linted
+impl Trait for () {
+    fn f(_: &Option<bool>) {}
+}
+
+fn main() {
+    test1(&mut Some(false));
+    test2();
+}

tests/ui/borrow_option.rs

@@ -0,0 +1,59 @@
+#![deny(clippy::borrowed_option)]
+#![allow(dead_code, unused_variables)]
+#![allow(
+    clippy::uninlined_format_args,
+    clippy::disallowed_names,
+    clippy::needless_pass_by_ref_mut,
+    clippy::needless_lifetimes
+)]
+
+use std::fmt::Display;
+
+pub fn test1(foo: &mut Option<bool>) {
+    // Although this function could be changed to "Option<&mut bool>",
+    // avoiding the borrowed option, mutable references to boxes are not
+    // flagged by this lint.
+    //
+    // This omission is intentional: By passing a "&mut Option<T>",
+    // the memory location of the pointed-to object could be
+    // modified. By passing a "Option<&mut T>", the contents
+    // could change, but not the location.
+    println!("{:?}", foo)
+}
+
+pub fn test2() {
+    let foo: &Option<bool>;
+    //~^ borrowed_option
+}
+
+struct Test3<'a> {
+    foo: &'a Option<bool>,
+    //~^ borrowed_option
+}
+
+trait Test4 {
+    fn test4(b: &Option<bool>);
+    //~^ borrowed_option
+}
+
+pub fn test5(_display: &Option<impl Display>) {}
+//~^ borrowed_option
+pub fn test6(_display: &Option<impl Display + Send>) {}
+//~^ borrowed_option
+pub fn test7<'a>(_display: &'a Option<impl Display + 'a>) {}
+//~^ borrowed_option
+
+#[allow(clippy::borrowed_box, clippy::borrowed_option)]
+trait Trait {
+    fn f(o: &Option<bool>);
+}
+
+// Trait impls are not linted
+impl Trait for () {
+    fn f(_: &Option<bool>) {}
+}
+
+fn main() {
+    test1(&mut Some(false));
+    test2();
+}