Merge pull request #2299 from mikerite/option_option_pr

Implemented option_option lint

clippy_lints/src/lib.rs

@@ -605,6 +605,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         types::IMPLICIT_HASHER,
         types::LET_UNIT_VALUE,
         types::LINKEDLIST,
+        types::OPTION_OPTION,
         types::TYPE_COMPLEXITY,
         types::UNIT_CMP,
         types::UNNECESSARY_CAST,

clippy_lints/src/types.rs

@@ -50,6 +50,26 @@ declare_lint! {
     "usage of `Box<Vec<T>>`, vector elements are already on the heap"
 }
 
+/// **What it does:** Checks for use of `Option<Option<_>>` in function signatures and type
+/// definitions
+///
+/// **Why is this bad?** `Option<_>` represents an optional value. `Option<Option<_>>`
+/// represents an optional optional value which is logically the same thing as an optional
+/// value but has an unneeded extra level of wrapping.
+///
+/// **Known problems:** None.
+///
+/// **Example**
+/// ```rust
+/// fn x() -> Option<Option<u32>> {
+///     None
+/// }
+declare_lint! {
+    pub OPTION_OPTION,
+    Warn,
+    "usage of `Option<Option<T>>`"
+}
+
 /// **What it does:** Checks for usage of any `LinkedList`, suggesting to use a
 /// `Vec` or a `VecDeque` (formerly called `RingBuf`).
 ///
@@ -111,7 +131,7 @@ declare_lint! {
 
 impl LintPass for TypePass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(BOX_VEC, LINKEDLIST, BORROWED_BOX)
+        lint_array!(BOX_VEC, OPTION_OPTION, LINKEDLIST, BORROWED_BOX)
     }
 }
 
@@ -156,6 +176,23 @@ fn check_fn_decl(cx: &LateContext, decl: &FnDecl) {
     }
 }
 
+/// Check if `qpath` has last segment with type parameter matching `path`
+fn match_type_parameter(cx: &LateContext, qpath: &QPath, path: &[&str]) -> bool {
+    let last = last_path_segment(qpath);
+    if_chain! {
+        if let Some(ref params) = last.parameters;
+        if !params.parenthesized;
+        if let Some(ty) = params.types.get(0);
+        if let TyPath(ref qpath) = ty.node;
+        if let Some(did) = opt_def_id(cx.tables.qpath_def(qpath, cx.tcx.hir.node_to_hir_id(ty.id)));
+        if match_def_path(cx.tcx, did, path);
+        then {
+            return true;
+        }
+    }
+    false
+}
+
 /// Recursively check for `TypePass` lints in the given type. Stop at the first
 /// lint found.
 ///
@@ -171,15 +208,7 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty, is_local: bool) {
             let def = cx.tables.qpath_def(qpath, hir_id);
             if let Some(def_id) = opt_def_id(def) {
                 if Some(def_id) == cx.tcx.lang_items().owned_box() {
-                    let last = last_path_segment(qpath);
-                    if_chain! {
-                        if let Some(ref params) = last.parameters;
-                        if !params.parenthesized;
-                        if let Some(vec) = params.types.get(0);
-                        if let TyPath(ref qpath) = vec.node;
-                        if let Some(did) = opt_def_id(cx.tables.qpath_def(qpath, cx.tcx.hir.node_to_hir_id(vec.id)));
-                        if match_def_path(cx.tcx, did, &paths::VEC);
-                        then {
+                    if match_type_parameter(cx, qpath, &paths::VEC) {
                         span_help_and_lint(
                             cx,
                             BOX_VEC,
@@ -189,6 +218,16 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty, is_local: bool) {
                         );
                         return; // don't recurse into the type
                     }
+                } else if match_def_path(cx.tcx, def_id, &paths::OPTION) {
+                    if match_type_parameter(cx, qpath, &paths::OPTION) {
+                        span_lint(
+                            cx,
+                            OPTION_OPTION,
+                            ast_ty.span,
+                            "consider using `Option<T>` instead of `Option<Option<T>>` or a custom \
+                            enum if you need to distinguish all 3 cases",
+                        );
+                        return; // don't recurse into the type
                     }
                 } else if match_def_path(cx.tcx, def_id, &paths::LINKED_LIST) {
                     span_help_and_lint(

tests/ui/needless_pass_by_value.rs

@@ -2,7 +2,7 @@
 
 
 #![warn(needless_pass_by_value)]
-#![allow(dead_code, single_match, if_let_redundant_pattern_matching, many_single_char_names)]
+#![allow(dead_code, single_match, if_let_redundant_pattern_matching, many_single_char_names, option_option)]
 
 #![feature(collections_range)]
 

tests/ui/option_option.rs

@@ -0,0 +1,63 @@
+fn input(_: Option<Option<u8>>) {
+}
+
+fn output() -> Option<Option<u8>> {
+    None
+}
+
+fn output_nested() -> Vec<Option<Option<u8>>> {
+    vec![None]
+}
+
+// The lint only generates one warning for this
+fn output_nested_nested() -> Option<Option<Option<u8>>> {
+    None
+}
+
+struct Struct {
+    x: Option<Option<u8>>,
+}
+
+impl Struct {
+    fn struct_fn() -> Option<Option<u8>> {
+        None
+    }
+}
+
+trait Trait {
+    fn trait_fn() -> Option<Option<u8>>;
+}
+
+enum Enum {
+    Tuple(Option<Option<u8>>),
+    Struct{x: Option<Option<u8>>},
+}
+
+// The lint allows this
+type OptionOption = Option<Option<u32>>;
+
+// The lint allows this
+fn output_type_alias() -> OptionOption {
+    None
+}
+
+// The line allows this
+impl Trait for Struct {
+    fn trait_fn() -> Option<Option<u8>> {
+        None
+    }
+}
+
+fn main() {
+    input(None);
+    output();
+    output_nested();
+
+    // The lint allows this
+    let local: Option<Option<u8>> = None;
+
+    // The lint allows this
+    let expr = Some(Some(true));
+}
+
+

tests/ui/option_option.stderr

@@ -0,0 +1,58 @@
+error: consider using `Option<T>` instead of `Option<Option<T>>` or a custom enum if you need to distinguish all 3 cases
+ --> $DIR/option_option.rs:1:13
+  |
+1 | fn input(_: Option<Option<u8>>) {
+  |             ^^^^^^^^^^^^^^^^^^
+  |
+  = note: `-D option-option` implied by `-D warnings`
+
+error: consider using `Option<T>` instead of `Option<Option<T>>` or a custom enum if you need to distinguish all 3 cases
+ --> $DIR/option_option.rs:4:16
+  |
+4 | fn output() -> Option<Option<u8>> {
+  |                ^^^^^^^^^^^^^^^^^^
+
+error: consider using `Option<T>` instead of `Option<Option<T>>` or a custom enum if you need to distinguish all 3 cases
+ --> $DIR/option_option.rs:8:27
+  |
+8 | fn output_nested() -> Vec<Option<Option<u8>>> {
+  |                           ^^^^^^^^^^^^^^^^^^
+
+error: consider using `Option<T>` instead of `Option<Option<T>>` or a custom enum if you need to distinguish all 3 cases
+  --> $DIR/option_option.rs:13:30
+   |
+13 | fn output_nested_nested() -> Option<Option<Option<u8>>> {
+   |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: consider using `Option<T>` instead of `Option<Option<T>>` or a custom enum if you need to distinguish all 3 cases
+  --> $DIR/option_option.rs:18:8
+   |
+18 |     x: Option<Option<u8>>,
+   |        ^^^^^^^^^^^^^^^^^^
+
+error: consider using `Option<T>` instead of `Option<Option<T>>` or a custom enum if you need to distinguish all 3 cases
+  --> $DIR/option_option.rs:22:23
+   |
+22 |     fn struct_fn() -> Option<Option<u8>> {
+   |                       ^^^^^^^^^^^^^^^^^^
+
+error: consider using `Option<T>` instead of `Option<Option<T>>` or a custom enum if you need to distinguish all 3 cases
+  --> $DIR/option_option.rs:28:22
+   |
+28 |     fn trait_fn() -> Option<Option<u8>>;
+   |                      ^^^^^^^^^^^^^^^^^^
+
+error: consider using `Option<T>` instead of `Option<Option<T>>` or a custom enum if you need to distinguish all 3 cases
+  --> $DIR/option_option.rs:32:11
+   |
+32 |     Tuple(Option<Option<u8>>),
+   |           ^^^^^^^^^^^^^^^^^^
+
+error: consider using `Option<T>` instead of `Option<Option<T>>` or a custom enum if you need to distinguish all 3 cases
+  --> $DIR/option_option.rs:33:15
+   |
+33 |     Struct{x: Option<Option<u8>>},
+   |               ^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 9 previous errors
+