From 6737bae9b117f875907f037df90c66318efd496b Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 26 Dec 2017 07:25:13 +0200 Subject: [PATCH 1/5] Implemented option_option lint --- clippy_lints/src/lib.rs | 1 + clippy_lints/src/types.rs | 77 ++++++++++++++++++++++-------- tests/ui/needless_pass_by_value.rs | 2 +- tests/ui/option_option.rs | 46 ++++++++++++++++++ tests/ui/option_option.stderr | 57 ++++++++++++++++++++++ 5 files changed, 163 insertions(+), 20 deletions(-) create mode 100644 tests/ui/option_option.rs create mode 100644 tests/ui/option_option.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9b598df4fa62..1f81dd0b9ff5 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -599,6 +599,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, diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index c146d306a5a6..2297ccc3f02c 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -42,6 +42,26 @@ declare_lint! { "usage of `Box>`, vector elements are already on the heap" } +/// **What it does:** Checks for use of `Option>` in function signatures and type +/// definitions +/// +/// **Why is this bad?** `Option<_>` represents an optional value. `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> { +/// None +/// } +declare_lint! { + pub OPTION_OPTION, + Warn, + "usage of `Option>`" +} + /// **What it does:** Checks for usage of any `LinkedList`, suggesting to use a /// `Vec` or a `VecDeque` (formerly called `RingBuf`). /// @@ -97,7 +117,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) } } @@ -142,6 +162,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(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, path); + then { + return true; + } + } + false +} + /// Recursively check for `TypePass` lints in the given type. Stop at the first /// lint found. /// @@ -157,24 +194,26 @@ 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 { - span_help_and_lint( - cx, - BOX_VEC, - ast_ty.span, - "you seem to be trying to use `Box>`. Consider using just `Vec`", - "`Vec` is already on the heap, `Box>` makes an extra allocation.", - ); - return; // don't recurse into the type - } + if match_type_parameter(cx, qpath, &paths::VEC) { + span_help_and_lint( + cx, + BOX_VEC, + ast_ty.span, + "you seem to be trying to use `Box>`. Consider using just `Vec`", + "`Vec` is already on the heap, `Box>` makes an extra allocation.", + ); + 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_help_and_lint( + cx, + OPTION_OPTION, + ast_ty.span, + "consider using `Option` instead of `Option>`", + "`Option<_>` is easier to use than `Option`", + ); + return; // don't recurse into the type } } else if match_def_path(cx.tcx, def_id, &paths::LINKED_LIST) { span_help_and_lint( diff --git a/tests/ui/needless_pass_by_value.rs b/tests/ui/needless_pass_by_value.rs index 84c7e8329513..081ff0dc5964 100644 --- a/tests/ui/needless_pass_by_value.rs +++ b/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)] use std::borrow::Borrow; use std::convert::AsRef; diff --git a/tests/ui/option_option.rs b/tests/ui/option_option.rs new file mode 100644 index 000000000000..88232c3b23fe --- /dev/null +++ b/tests/ui/option_option.rs @@ -0,0 +1,46 @@ +fn input(_: Option>) { +} + +fn output() -> Option> { + None +} + +fn output_nested() -> Vec>> { + vec![None] +} + +// The lint only generates one warning for this +fn output_nested_nested() -> Option>> { + None +} + +struct Struct { + x: Option>, +} + +enum Enum { + Tuple(Option>), + Struct{x: Option>}, +} + +// The lint allows this +type OptionOption = Option>; + +// The lint allows this +fn output_type_alias() -> OptionOption { + None +} + +fn main() { + input(None); + output(); + output_nested(); + + // The lint allows this + let local: Option> = None; + + // The lint allows this + let expr = Some(Some(true)); +} + + diff --git a/tests/ui/option_option.stderr b/tests/ui/option_option.stderr new file mode 100644 index 000000000000..514538be167e --- /dev/null +++ b/tests/ui/option_option.stderr @@ -0,0 +1,57 @@ +error: consider using `Option` instead of `Option>` + --> $DIR/option_option.rs:1:13 + | +1 | fn input(_: Option>) { + | ^^^^^^^^^^^^^^^^^^ + | + = note: `-D option-option` implied by `-D warnings` + = help: `Option<_>` is easier to use than `Option` + +error: consider using `Option` instead of `Option>` + --> $DIR/option_option.rs:4:16 + | +4 | fn output() -> Option> { + | ^^^^^^^^^^^^^^^^^^ + | + = help: `Option<_>` is easier to use than `Option` + +error: consider using `Option` instead of `Option>` + --> $DIR/option_option.rs:8:27 + | +8 | fn output_nested() -> Vec>> { + | ^^^^^^^^^^^^^^^^^^ + | + = help: `Option<_>` is easier to use than `Option` + +error: consider using `Option` instead of `Option>` + --> $DIR/option_option.rs:13:30 + | +13 | fn output_nested_nested() -> Option>> { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: `Option<_>` is easier to use than `Option` + +error: consider using `Option` instead of `Option>` + --> $DIR/option_option.rs:18:8 + | +18 | x: Option>, + | ^^^^^^^^^^^^^^^^^^ + | + = help: `Option<_>` is easier to use than `Option` + +error: consider using `Option` instead of `Option>` + --> $DIR/option_option.rs:22:11 + | +22 | Tuple(Option>), + | ^^^^^^^^^^^^^^^^^^ + | + = help: `Option<_>` is easier to use than `Option` + +error: consider using `Option` instead of `Option>` + --> $DIR/option_option.rs:23:15 + | +23 | Struct{x: Option>}, + | ^^^^^^^^^^^^^^^^^^ + | + = help: `Option<_>` is easier to use than `Option` + From e7567f2eac94006ae92fc0c0435630c2eef07c65 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 17 Jan 2018 07:24:33 +0200 Subject: [PATCH 2/5] Made requested changes --- clippy_lints/src/types.rs | 6 +++--- tests/ui/option_option.stderr | 27 +++++++-------------------- 2 files changed, 10 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index c8b564e4228e..e5109cf3ebb3 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -220,12 +220,12 @@ fn check_ty(cx: &LateContext, ast_ty: &hir::Ty, is_local: bool) { } } else if match_def_path(cx.tcx, def_id, &paths::OPTION) { if match_type_parameter(cx, qpath, &paths::OPTION) { - span_help_and_lint( + span_lint( cx, OPTION_OPTION, ast_ty.span, - "consider using `Option` instead of `Option>`", - "`Option<_>` is easier to use than `Option`", + "consider using `Option` instead of `Option>` or a custom \ + enum if you need to distinguish all 3 cases", ); return; // don't recurse into the type } diff --git a/tests/ui/option_option.stderr b/tests/ui/option_option.stderr index 514538be167e..91f686288dd5 100644 --- a/tests/ui/option_option.stderr +++ b/tests/ui/option_option.stderr @@ -1,57 +1,44 @@ -error: consider using `Option` instead of `Option>` +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases --> $DIR/option_option.rs:1:13 | 1 | fn input(_: Option>) { | ^^^^^^^^^^^^^^^^^^ | = note: `-D option-option` implied by `-D warnings` - = help: `Option<_>` is easier to use than `Option` -error: consider using `Option` instead of `Option>` +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases --> $DIR/option_option.rs:4:16 | 4 | fn output() -> Option> { | ^^^^^^^^^^^^^^^^^^ - | - = help: `Option<_>` is easier to use than `Option` -error: consider using `Option` instead of `Option>` +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases --> $DIR/option_option.rs:8:27 | 8 | fn output_nested() -> Vec>> { | ^^^^^^^^^^^^^^^^^^ - | - = help: `Option<_>` is easier to use than `Option` -error: consider using `Option` instead of `Option>` +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases --> $DIR/option_option.rs:13:30 | 13 | fn output_nested_nested() -> Option>> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: `Option<_>` is easier to use than `Option` -error: consider using `Option` instead of `Option>` +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases --> $DIR/option_option.rs:18:8 | 18 | x: Option>, | ^^^^^^^^^^^^^^^^^^ - | - = help: `Option<_>` is easier to use than `Option` -error: consider using `Option` instead of `Option>` +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases --> $DIR/option_option.rs:22:11 | 22 | Tuple(Option>), | ^^^^^^^^^^^^^^^^^^ - | - = help: `Option<_>` is easier to use than `Option` -error: consider using `Option` instead of `Option>` +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases --> $DIR/option_option.rs:23:15 | 23 | Struct{x: Option>}, | ^^^^^^^^^^^^^^^^^^ - | - = help: `Option<_>` is easier to use than `Option` From d13af87d8a5786c26ab8c497079a248115cead95 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 18 Jan 2018 07:48:03 +0200 Subject: [PATCH 3/5] Fixed tests --- tests/ui/option_option.stderr | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ui/option_option.stderr b/tests/ui/option_option.stderr index 91f686288dd5..ebdc4fe92666 100644 --- a/tests/ui/option_option.stderr +++ b/tests/ui/option_option.stderr @@ -42,3 +42,5 @@ error: consider using `Option` instead of `Option>` or a custom enu 23 | Struct{x: Option>}, | ^^^^^^^^^^^^^^^^^^ +error: aborting due to 7 previous errors + From bf7efead17dcb685cff8c7696f1ba6f7c28a82f1 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 18 Jan 2018 07:52:24 +0200 Subject: [PATCH 4/5] Rename variable Rename `vec` to `ty` in `match_type_parameter`. This variable is a type and not a vector. Previously it would only refer to `Vec<_>` so the name used to make sense. --- clippy_lints/src/types.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index e5109cf3ebb3..d10199c992d9 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -182,9 +182,9 @@ fn match_type_parameter(cx: &LateContext, qpath: &QPath, path: &[&str]) -> bool 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 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; From 79c6c60f511da6db0cce1707ef2923ee7038ae95 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Fri, 19 Jan 2018 08:10:09 +0200 Subject: [PATCH 5/5] Added further tests --- tests/ui/option_option.rs | 17 +++++++++++++++++ tests/ui/option_option.stderr | 22 +++++++++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/tests/ui/option_option.rs b/tests/ui/option_option.rs index 88232c3b23fe..249745c6a450 100644 --- a/tests/ui/option_option.rs +++ b/tests/ui/option_option.rs @@ -18,6 +18,16 @@ struct Struct { x: Option>, } +impl Struct { + fn struct_fn() -> Option> { + None + } +} + +trait Trait { + fn trait_fn() -> Option>; +} + enum Enum { Tuple(Option>), Struct{x: Option>}, @@ -31,6 +41,13 @@ fn output_type_alias() -> OptionOption { None } +// The line allows this +impl Trait for Struct { + fn trait_fn() -> Option> { + None + } +} + fn main() { input(None); output(); diff --git a/tests/ui/option_option.stderr b/tests/ui/option_option.stderr index ebdc4fe92666..19e00efae718 100644 --- a/tests/ui/option_option.stderr +++ b/tests/ui/option_option.stderr @@ -31,16 +31,28 @@ error: consider using `Option` instead of `Option>` or a custom enu | ^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:22:11 + --> $DIR/option_option.rs:22:23 | -22 | Tuple(Option>), +22 | fn struct_fn() -> Option> { + | ^^^^^^^^^^^^^^^^^^ + +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases + --> $DIR/option_option.rs:28:22 + | +28 | fn trait_fn() -> Option>; + | ^^^^^^^^^^^^^^^^^^ + +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases + --> $DIR/option_option.rs:32:11 + | +32 | Tuple(Option>), | ^^^^^^^^^^^^^^^^^^ error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases - --> $DIR/option_option.rs:23:15 + --> $DIR/option_option.rs:33:15 | -23 | Struct{x: Option>}, +33 | Struct{x: Option>}, | ^^^^^^^^^^^^^^^^^^ -error: aborting due to 7 previous errors +error: aborting due to 9 previous errors