diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 11afefd6d4be..a6ffc17bff2c 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index c0c72408c7da..d10199c992d9 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -50,6 +50,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`). /// @@ -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,24 +208,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_lint( + cx, + OPTION_OPTION, + ast_ty.span, + "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 } } 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 6e87fcc7b44f..3459d3820b71 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)] #![feature(collections_range)] diff --git a/tests/ui/option_option.rs b/tests/ui/option_option.rs new file mode 100644 index 000000000000..249745c6a450 --- /dev/null +++ b/tests/ui/option_option.rs @@ -0,0 +1,63 @@ +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>, +} + +impl Struct { + fn struct_fn() -> Option> { + None + } +} + +trait Trait { + fn trait_fn() -> 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 +} + +// The line allows this +impl Trait for Struct { + fn trait_fn() -> Option> { + 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..19e00efae718 --- /dev/null +++ b/tests/ui/option_option.stderr @@ -0,0 +1,58 @@ +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` + +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> { + | ^^^^^^^^^^^^^^^^^^ + +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>> { + | ^^^^^^^^^^^^^^^^^^ + +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>> { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + +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>, + | ^^^^^^^^^^^^^^^^^^ + +error: consider using `Option` instead of `Option>` or a custom enum if you need to distinguish all 3 cases + --> $DIR/option_option.rs:22:23 + | +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:33:15 + | +33 | Struct{x: Option>}, + | ^^^^^^^^^^^^^^^^^^ + +error: aborting due to 9 previous errors +