New lint: borrowed_option

rust-lang · Mar 29, 2024 · 09929e1 · 09929e1
1 parent 124e68b
commit 09929e1
Show file tree

Hide file tree

Showing 22 changed files with 188 additions and 93 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5121,6 +5121,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

diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -697,6 +697,7 @@ pub(crate) static LINTS: &[&crate::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,

diff --git a/clippy_lints/src/types/borrowed_box.rs b/clippy_lints/src/types/borrowed_box.rs
@@ -7,15 +7,15 @@ 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 +25,8 @@ pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, lt: &Lifetime, m
                     _ => None,
                 })
             {
-                if is_any_trait(cx, inner) {
+                let is_box = Some(def_id) == cx.tcx.lang_items().owned_box();
+                if is_box && is_any_trait(cx, inner) {
                     // Ignore `Box<Any>` types; see issue #1884 for details.
                     return false;
                 }
@@ -39,6 +40,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;
                 }
 
@@ -60,11 +62,15 @@ 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.

diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs
@@ -188,6 +188,37 @@ 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>`.
+    ///
+    /// ### Example
+    /// ```rust,compile_fail
+    /// fn foo(bar: &Option<i32>) -> &Option<i32> { bar }
+    /// fn call_foo(bar: &Result<i32, ()>) {
+    ///     foo(bar.ok()); // does not work
+    /// }
+    /// ```
+    ///
+    /// Use instead:
+    ///
+    /// ```rust
+    /// fn foo(bar: Option<&i32>) -> Option<&i32> { bar }
+    /// fn call_foo(bar: &Result<i32, ()>) {
+    ///     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.
@@ -309,7 +340,7 @@ pub struct Types {
     avoid_breaking_exported_api: bool,
 }
 
-impl_lint_pass!(Types => [BOX_COLLECTION, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]);
+impl_lint_pass!(Types => [BOX_COLLECTION, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, BORROWED_OPTION, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]);
 
 impl<'tcx> LateLintPass<'tcx> for Types {
     fn check_fn(

diff --git a/tests/ui/borrow_box.rs b/tests/ui/borrow_box.rs
@@ -9,7 +9,7 @@
 
 use std::fmt::Display;
 
-pub fn test1(foo: &mut Box<bool>) {
+pub fn test1(foo: &mut Box<bool>, bar: &mut Option<bool>) {
     // Although this function could be changed to "&mut bool",
     // avoiding the Box, mutable references to boxes are not
     // flagged by this lint.
@@ -18,26 +18,33 @@ pub fn test1(foo: &mut Box<bool>) {
     // the memory location of the pointed-to object could be
     // modified. By passing a mutable reference, the contents
     // could change, but not the location.
-    println!("{:?}", foo)
+    //
+    // The same reasoning applies to ignoring &mut Option
+    println!("{:?} {:?}", foo, bar)
 }
 
 pub fn test2() {
     let foo: &Box<bool>;
     //~^ ERROR: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+    let bar: &Option<bool>;
+    //~^ ERROR: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
 }
 
 struct Test3<'a> {
     foo: &'a Box<bool>,
     //~^ ERROR: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+    bar: &'a Option<bool>,
+    //~^ ERROR: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
 }
 
 trait Test4 {
-    fn test4(a: &Box<bool>);
+    fn test4(a: &Box<bool>, b: &Option<bool>);
     //~^ ERROR: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+    //~| ERROR: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
 }
 
 impl<'a> Test4 for Test3<'a> {
-    fn test4(a: &Box<bool>) {
+    fn test4(a: &Box<bool>, b: &Option<bool>) {
         unimplemented!();
     }
 }
@@ -106,12 +113,15 @@ pub fn test15(_display: &Box<dyn Display + Send>) {}
 pub fn test16<'a>(_display: &'a Box<dyn Display + 'a>) {}
 //~^ ERROR: you seem to be trying to use `&Box<T>`. Consider using just `&T`
 
-pub fn test17(_display: &Box<impl Display>) {}
+pub fn test17(_display: &Box<impl Display>, _display2: &Option<impl Display>) {}
 //~^ ERROR: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-pub fn test18(_display: &Box<impl Display + Send>) {}
+//~| ERROR: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
+pub fn test18(_display: &Box<impl Display + Send>, _display2: &Option<impl Display + Send>) {}
 //~^ ERROR: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-pub fn test19<'a>(_display: &'a Box<impl Display + 'a>) {}
+//~| ERROR: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
+pub fn test19<'a>(_display: &'a Box<impl Display + 'a>, _display2: &'a Option<impl Display + 'a>) {}
 //~^ ERROR: you seem to be trying to use `&Box<T>`. Consider using just `&T`
+//~| ERROR: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
 
 // This exists only to check what happens when parentheses are already present.
 // Even though the current implementation doesn't put extra parentheses,
@@ -120,7 +130,7 @@ pub fn test20(_display: &Box<(dyn Display + Send)>) {}
 //~^ ERROR: you seem to be trying to use `&Box<T>`. Consider using just `&T`
 
 fn main() {
-    test1(&mut Box::new(false));
+    test1(&mut Box::new(false), &mut Some(false));
     test2();
     test5(&mut (Box::new(false) as Box<dyn Any>));
     test6();

diff --git a/tests/ui/borrow_box.stderr b/tests/ui/borrow_box.stderr
@@ -1,5 +1,5 @@
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> tests/ui/borrow_box.rs:25:14
+  --> tests/ui/borrow_box.rs:27:14
    |
 LL |     let foo: &Box<bool>;
    |              ^^^^^^^^^^ help: try: `&bool`
@@ -10,59 +10,98 @@ note: the lint level is defined here
 LL | #![deny(clippy::borrowed_box)]
    |         ^^^^^^^^^^^^^^^^^^^^
 
+error: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
+  --> tests/ui/borrow_box.rs:29:14
+   |
+LL |     let bar: &Option<bool>;
+   |              ^^^^^^^^^^^^^ help: try: `Option<&bool>`
+   |
+   = note: `-D clippy::borrowed-option` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::borrowed_option)]`
+
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> tests/ui/borrow_box.rs:30:10
+  --> tests/ui/borrow_box.rs:34:10
    |
 LL |     foo: &'a Box<bool>,
    |          ^^^^^^^^^^^^^ help: try: `&'a bool`
 
+error: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
+  --> tests/ui/borrow_box.rs:36:10
+   |
+LL |     bar: &'a Option<bool>,
+   |          ^^^^^^^^^^^^^^^^ help: try: `Option<&'a bool>`
+
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> tests/ui/borrow_box.rs:35:17
+  --> tests/ui/borrow_box.rs:41:17
    |
-LL |     fn test4(a: &Box<bool>);
+LL |     fn test4(a: &Box<bool>, b: &Option<bool>);
    |                 ^^^^^^^^^^ help: try: `&bool`
 
+error: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
+  --> tests/ui/borrow_box.rs:41:32
+   |
+LL |     fn test4(a: &Box<bool>, b: &Option<bool>);
+   |                                ^^^^^^^^^^^^^ help: try: `Option<&bool>`
+
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> tests/ui/borrow_box.rs:102:25
+  --> tests/ui/borrow_box.rs:109:25
    |
 LL | pub fn test14(_display: &Box<dyn Display>) {}
    |                         ^^^^^^^^^^^^^^^^^ help: try: `&dyn Display`
 
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> tests/ui/borrow_box.rs:104:25
+  --> tests/ui/borrow_box.rs:111:25
    |
 LL | pub fn test15(_display: &Box<dyn Display + Send>) {}
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&(dyn Display + Send)`
 
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> tests/ui/borrow_box.rs:106:29
+  --> tests/ui/borrow_box.rs:113:29
    |
 LL | pub fn test16<'a>(_display: &'a Box<dyn Display + 'a>) {}
    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&'a (dyn Display + 'a)`
 
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> tests/ui/borrow_box.rs:109:25
+  --> tests/ui/borrow_box.rs:116:25
    |
-LL | pub fn test17(_display: &Box<impl Display>) {}
+LL | pub fn test17(_display: &Box<impl Display>, _display2: &Option<impl Display>) {}
    |                         ^^^^^^^^^^^^^^^^^^ help: try: `&impl Display`
 
+error: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
+  --> tests/ui/borrow_box.rs:116:56
+   |
+LL | pub fn test17(_display: &Box<impl Display>, _display2: &Option<impl Display>) {}
+   |                                                        ^^^^^^^^^^^^^^^^^^^^^ help: try: `Option<&impl Display>`
+
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> tests/ui/borrow_box.rs:111:25
+  --> tests/ui/borrow_box.rs:119:25
    |
-LL | pub fn test18(_display: &Box<impl Display + Send>) {}
+LL | pub fn test18(_display: &Box<impl Display + Send>, _display2: &Option<impl Display + Send>) {}
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&(impl Display + Send)`
 
+error: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
+  --> tests/ui/borrow_box.rs:119:63
+   |
+LL | pub fn test18(_display: &Box<impl Display + Send>, _display2: &Option<impl Display + Send>) {}
+   |                                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Option<&(impl Display + Send)>`
+
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> tests/ui/borrow_box.rs:113:29
+  --> tests/ui/borrow_box.rs:122:29
    |
-LL | pub fn test19<'a>(_display: &'a Box<impl Display + 'a>) {}
+LL | pub fn test19<'a>(_display: &'a Box<impl Display + 'a>, _display2: &'a Option<impl Display + 'a>) {}
    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&'a (impl Display + 'a)`
 
+error: you seem to be trying to use `&Option<T>`. Consider using `Option<&T>` instead
+  --> tests/ui/borrow_box.rs:122:68
+   |
+LL | pub fn test19<'a>(_display: &'a Box<impl Display + 'a>, _display2: &'a Option<impl Display + 'a>) {}
+   |                                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'a (impl Display + 'a)>`
+
 error: you seem to be trying to use `&Box<T>`. Consider using just `&T`
-  --> tests/ui/borrow_box.rs:119:25
+  --> tests/ui/borrow_box.rs:129:25
    |
 LL | pub fn test20(_display: &Box<(dyn Display + Send)>) {}
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&(dyn Display + Send)`
 
-error: aborting due to 10 previous errors
+error: aborting due to 16 previous errors
 
diff --git a/tests/ui/clone_on_copy.fixed b/tests/ui/clone_on_copy.fixed
@@ -6,7 +6,8 @@
     clippy::unnecessary_operation,
     clippy::vec_init_then_push,
     clippy::toplevel_ref_arg,
-    clippy::needless_borrow
+    clippy::needless_borrow,
+    clippy::borrowed_option
 )]
 
 use std::cell::RefCell;

diff --git a/tests/ui/clone_on_copy.rs b/tests/ui/clone_on_copy.rs
@@ -6,7 +6,8 @@
     clippy::unnecessary_operation,
     clippy::vec_init_then_push,
     clippy::toplevel_ref_arg,
-    clippy::needless_borrow
+    clippy::needless_borrow,
+    clippy::borrowed_option
 )]
 
 use std::cell::RefCell;

diff --git a/tests/ui/clone_on_copy.stderr b/tests/ui/clone_on_copy.stderr
@@ -1,5 +1,5 @@
 error: using `clone` on type `i32` which implements the `Copy` trait
-  --> tests/ui/clone_on_copy.rs:23:5
+  --> tests/ui/clone_on_copy.rs:24:5
    |
 LL |     42.clone();
    |     ^^^^^^^^^^ help: try removing the `clone` call: `42`
@@ -8,49 +8,49 @@ LL |     42.clone();
    = help: to override `-D warnings` add `#[allow(clippy::clone_on_copy)]`
 
 error: using `clone` on type `i32` which implements the `Copy` trait
-  --> tests/ui/clone_on_copy.rs:27:5
+  --> tests/ui/clone_on_copy.rs:28:5
    |
 LL |     (&42).clone();
    |     ^^^^^^^^^^^^^ help: try dereferencing it: `*(&42)`
 
 error: using `clone` on type `i32` which implements the `Copy` trait
-  --> tests/ui/clone_on_copy.rs:30:5
+  --> tests/ui/clone_on_copy.rs:31:5
    |
 LL |     rc.borrow().clone();
    |     ^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: `*rc.borrow()`
 
 error: using `clone` on type `u32` which implements the `Copy` trait
-  --> tests/ui/clone_on_copy.rs:33:5
+  --> tests/ui/clone_on_copy.rs:34:5
    |
 LL |     x.clone().rotate_left(1);
    |     ^^^^^^^^^ help: try removing the `clone` call: `x`
 
 error: using `clone` on type `i32` which implements the `Copy` trait
-  --> tests/ui/clone_on_copy.rs:47:5
+  --> tests/ui/clone_on_copy.rs:48:5
    |
 LL |     m!(42).clone();
    |     ^^^^^^^^^^^^^^ help: try removing the `clone` call: `m!(42)`
 
 error: using `clone` on type `[u32; 2]` which implements the `Copy` trait
-  --> tests/ui/clone_on_copy.rs:57:5
+  --> tests/ui/clone_on_copy.rs:58:5
    |
 LL |     x.clone()[0];
    |     ^^^^^^^^^ help: try dereferencing it: `(*x)`
 
 error: using `clone` on type `char` which implements the `Copy` trait
-  --> tests/ui/clone_on_copy.rs:67:14
+  --> tests/ui/clone_on_copy.rs:68:14
    |
 LL |     is_ascii('z'.clone());
    |              ^^^^^^^^^^^ help: try removing the `clone` call: `'z'`
 
 error: using `clone` on type `i32` which implements the `Copy` trait
-  --> tests/ui/clone_on_copy.rs:71:14
+  --> tests/ui/clone_on_copy.rs:72:14
    |
 LL |     vec.push(42.clone());
    |              ^^^^^^^^^^ help: try removing the `clone` call: `42`
 
 error: using `clone` on type `Option<i32>` which implements the `Copy` trait
-  --> tests/ui/clone_on_copy.rs:75:17
+  --> tests/ui/clone_on_copy.rs:76:17
    |
 LL |     let value = opt.clone()?; // operator precedence needed (*opt)?
    |                 ^^^^^^^^^^^ help: try dereferencing it: `(*opt)`