Auto merge of #6165 - dvermd:ref_option_ref, r=flip1995

Add lint 'ref_option_ref' #1377

This lint checks for usage of `&Option<&T>` which can be simplified as `Option<&T>` as suggested in #1377.

This WIP PR is here to get feedback on the lint as there's more cases to be handled:
* statics/consts,
* associated types,
* type alias,
* function/method parameter/return,
* ADT definitions (struct/tuple struct fields, enum variants)

changelog: Add 'ref_option_ref' lint

CHANGELOG.md

@@ -1920,6 +1920,7 @@ Released 2018-09-13
 [`redundant_pub_crate`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pub_crate
 [`redundant_static_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes
 [`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
 [`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro
 [`repeat_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_once
 [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts

clippy_lints/src/lib.rs

@@ -294,6 +294,7 @@ mod redundant_closure_call;
 mod redundant_field_names;
 mod redundant_pub_crate;
 mod redundant_static_lifetimes;
+mod ref_option_ref;
 mod reference;
 mod regex;
 mod repeat_once;
@@ -810,6 +811,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &redundant_field_names::REDUNDANT_FIELD_NAMES,
         &redundant_pub_crate::REDUNDANT_PUB_CRATE,
         &redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES,
+        &ref_option_ref::REF_OPTION_REF,
         &reference::DEREF_ADDROF,
         &reference::REF_IN_DEREF,
         &regex::INVALID_REGEX,
@@ -1033,6 +1035,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &sess.target,
     );
     store.register_late_pass(move || box pass_by_ref_or_value);
+    store.register_late_pass(|| box ref_option_ref::RefOptionRef);
     store.register_late_pass(|| box try_err::TryErr);
     store.register_late_pass(|| box use_self::UseSelf);
     store.register_late_pass(|| box bytecount::ByteCount);
@@ -1261,6 +1264,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&pass_by_ref_or_value::TRIVIALLY_COPY_PASS_BY_REF),
         LintId::of(&ranges::RANGE_MINUS_ONE),
         LintId::of(&ranges::RANGE_PLUS_ONE),
+        LintId::of(&ref_option_ref::REF_OPTION_REF),
         LintId::of(&shadow::SHADOW_UNRELATED),
         LintId::of(&strings::STRING_ADD_ASSIGN),
         LintId::of(&trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS),

clippy_lints/src/ref_option_ref.rs

@@ -0,0 +1,66 @@
+use crate::utils::{last_path_segment, snippet, span_lint_and_sugg};
+use rustc_hir::{GenericArg, Mutability, Ty, TyKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `&Option<&T>`.
+    ///
+    /// **Why is this bad?** Since `&` is Copy, it's useless to have a
+    /// reference on `Option<&T>`.
+    ///
+    /// **Known problems:** It may be irrevelent to use this lint on
+    /// public API code as it will make a breaking change to apply it.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust,ignore
+    /// let x: &Option<&u32> = &Some(&0u32);
+    /// ```
+    /// Use instead:
+    /// ```rust,ignore
+    /// let x: Option<&u32> = Some(&0u32);
+    /// ```
+    pub REF_OPTION_REF,
+    pedantic,
+    "use `Option<&T>` instead of `&Option<&T>`"
+}
+
+declare_lint_pass!(RefOptionRef => [REF_OPTION_REF]);
+
+impl<'tcx> LateLintPass<'tcx> for RefOptionRef {
+    fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx Ty<'tcx>) {
+        if_chain! {
+            if let TyKind::Rptr(_, ref mut_ty) = ty.kind;
+            if mut_ty.mutbl == Mutability::Not;
+            if let TyKind::Path(ref qpath) = &mut_ty.ty.kind;
+            let last = last_path_segment(qpath);
+            if let Some(res) = last.res;
+            if let Some(def_id) = res.opt_def_id();
+
+            if cx.tcx.is_diagnostic_item(sym!(option_type), def_id);
+            if let Some(ref params) = last_path_segment(qpath).args ;
+            if !params.parenthesized;
+            if let Some(inner_ty) = params.args.iter().find_map(|arg| match arg {
+                GenericArg::Type(inner_ty) => Some(inner_ty),
+                _ => None,
+            });
+            if let TyKind::Rptr(_, _) = inner_ty.kind;
+
+            then {
+                span_lint_and_sugg(
+                    cx,
+                    REF_OPTION_REF,
+                    ty.span,
+                    "since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`",
+                    "try",
+                    format!("Option<{}>", &snippet(cx, inner_ty.span, "..")),
+                    Applicability::MaybeIncorrect,
+                );
+            }
+        }
+    }
+}

src/lintlist/mod.rs

@@ -2034,6 +2034,13 @@ vec![
         deprecation: None,
         module: "reference",
     },
+    Lint {
+        name: "ref_option_ref",
+        group: "pedantic",
+        desc: "use `Option<&T>` instead of `&Option<&T>`",
+        deprecation: None,
+        module: "ref_option_ref",
+    },
     Lint {
         name: "repeat_once",
         group: "complexity",

tests/ui/option_if_let_else.fixed

@@ -1,6 +1,7 @@
 // run-rustfix
 #![warn(clippy::option_if_let_else)]
 #![allow(clippy::redundant_closure)]
+#![allow(clippy::ref_option_ref)]
 
 fn bad1(string: Option<&str>) -> (bool, &str) {
     string.map_or((false, "hello"), |x| (true, x))

tests/ui/option_if_let_else.rs

@@ -1,6 +1,7 @@
 // run-rustfix
 #![warn(clippy::option_if_let_else)]
 #![allow(clippy::redundant_closure)]
+#![allow(clippy::ref_option_ref)]
 
 fn bad1(string: Option<&str>) -> (bool, &str) {
     if let Some(x) = string {

tests/ui/option_if_let_else.stderr

@@ -1,5 +1,5 @@
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:6:5
+  --> $DIR/option_if_let_else.rs:7:5
    |
 LL | /     if let Some(x) = string {
 LL | |         (true, x)
@@ -11,7 +11,7 @@ LL | |     }
    = note: `-D clippy::option-if-let-else` implied by `-D warnings`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:16:12
+  --> $DIR/option_if_let_else.rs:17:12
    |
 LL |       } else if let Some(x) = string {
    |  ____________^
@@ -22,19 +22,19 @@ LL | |     }
    | |_____^ help: try: `{ string.map_or(Some((false, "")), |x| Some((true, x))) }`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:24:13
+  --> $DIR/option_if_let_else.rs:25:13
    |
 LL |     let _ = if let Some(s) = *string { s.len() } else { 0 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:25:13
+  --> $DIR/option_if_let_else.rs:26:13
    |
 LL |     let _ = if let Some(s) = &num { s } else { &0 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:26:13
+  --> $DIR/option_if_let_else.rs:27:13
    |
 LL |       let _ = if let Some(s) = &mut num {
    |  _____________^
@@ -54,13 +54,13 @@ LL |     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:32:13
+  --> $DIR/option_if_let_else.rs:33:13
    |
 LL |     let _ = if let Some(ref s) = num { s } else { &0 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:33:13
+  --> $DIR/option_if_let_else.rs:34:13
    |
 LL |       let _ = if let Some(mut s) = num {
    |  _____________^
@@ -80,7 +80,7 @@ LL |     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:39:13
+  --> $DIR/option_if_let_else.rs:40:13
    |
 LL |       let _ = if let Some(ref mut s) = num {
    |  _____________^
@@ -100,7 +100,7 @@ LL |     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:48:5
+  --> $DIR/option_if_let_else.rs:49:5
    |
 LL | /     if let Some(x) = arg {
 LL | |         let y = x * x;
@@ -119,7 +119,7 @@ LL |     })
    |
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:61:13
+  --> $DIR/option_if_let_else.rs:62:13
    |
 LL |       let _ = if let Some(x) = arg {
    |  _____________^
@@ -131,7 +131,7 @@ LL | |     };
    | |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)`
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:70:13
+  --> $DIR/option_if_let_else.rs:71:13
    |
 LL |       let _ = if let Some(x) = arg {
    |  _____________^
@@ -154,7 +154,7 @@ LL |     }, |x| x * x * x * x);
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:99:13
+  --> $DIR/option_if_let_else.rs:100:13
    |
 LL |     let _ = if let Some(x) = optional { x + 2 } else { 5 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`

tests/ui/ref_option_ref.rs

@@ -0,0 +1,47 @@
+#![allow(unused)]
+#![warn(clippy::ref_option_ref)]
+
+// This lint is not tagged as run-rustfix because automatically
+// changing the type of a variable would also means changing
+// all usages of this variable to match and This is not handled
+// by this lint.
+
+static THRESHOLD: i32 = 10;
+static REF_THRESHOLD: &Option<&i32> = &Some(&THRESHOLD);
+const CONST_THRESHOLD: &i32 = &10;
+const REF_CONST: &Option<&i32> = &Some(&CONST_THRESHOLD);
+
+type RefOptRefU32<'a> = &'a Option<&'a u32>;
+type RefOptRef<'a, T> = &'a Option<&'a T>;
+
+fn foo(data: &Option<&u32>) {}
+
+fn bar(data: &u32) -> &Option<&u32> {
+    &None
+}
+
+struct StructRef<'a> {
+    data: &'a Option<&'a u32>,
+}
+
+struct StructTupleRef<'a>(u32, &'a Option<&'a u32>);
+
+enum EnumRef<'a> {
+    Variant1(u32),
+    Variant2(&'a Option<&'a u32>),
+}
+
+trait RefOptTrait {
+    type A;
+    fn foo(&self, _: Self::A);
+}
+
+impl RefOptTrait for u32 {
+    type A = &'static Option<&'static Self>;
+
+    fn foo(&self, _: Self::A) {}
+}
+
+fn main() {
+    let x: &Option<&u32> = &None;
+}

tests/ui/ref_option_ref.stderr

@@ -0,0 +1,70 @@
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:10:23
+   |
+LL | static REF_THRESHOLD: &Option<&i32> = &Some(&THRESHOLD);
+   |                       ^^^^^^^^^^^^^ help: try: `Option<&i32>`
+   |
+   = note: `-D clippy::ref-option-ref` implied by `-D warnings`
+
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:12:18
+   |
+LL | const REF_CONST: &Option<&i32> = &Some(&CONST_THRESHOLD);
+   |                  ^^^^^^^^^^^^^ help: try: `Option<&i32>`
+
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:14:25
+   |
+LL | type RefOptRefU32<'a> = &'a Option<&'a u32>;
+   |                         ^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'a u32>`
+
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:15:25
+   |
+LL | type RefOptRef<'a, T> = &'a Option<&'a T>;
+   |                         ^^^^^^^^^^^^^^^^^ help: try: `Option<&'a T>`
+
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:17:14
+   |
+LL | fn foo(data: &Option<&u32>) {}
+   |              ^^^^^^^^^^^^^ help: try: `Option<&u32>`
+
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:19:23
+   |
+LL | fn bar(data: &u32) -> &Option<&u32> {
+   |                       ^^^^^^^^^^^^^ help: try: `Option<&u32>`
+
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:24:11
+   |
+LL |     data: &'a Option<&'a u32>,
+   |           ^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'a u32>`
+
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:27:32
+   |
+LL | struct StructTupleRef<'a>(u32, &'a Option<&'a u32>);
+   |                                ^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'a u32>`
+
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:31:14
+   |
+LL |     Variant2(&'a Option<&'a u32>),
+   |              ^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'a u32>`
+
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:40:14
+   |
+LL |     type A = &'static Option<&'static Self>;
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Option<&'static Self>`
+
+error: since `&` implements the `Copy` trait, `&Option<&T>` can be simplified to `Option<&T>`
+  --> $DIR/ref_option_ref.rs:46:12
+   |
+LL |     let x: &Option<&u32> = &None;
+   |            ^^^^^^^^^^^^^ help: try: `Option<&u32>`
+
+error: aborting due to 11 previous errors
+