From a6bd2d06227fa315c7a404d61dee9ded4a1e45bc Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Wed, 30 Dec 2015 00:38:03 -0800 Subject: [PATCH] Add SEARCH_IS_SOME lint --- README.md | 3 +- src/lib.rs | 1 + src/methods.rs | 40 +++++++++++++++++++++ tests/compile-fail/methods.rs | 66 +++++++++++++++++++++++++++++++---- 4 files changed, 102 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 9840eda8d..9fe59d4fd 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 87 lints included in this crate: +There are 88 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -69,6 +69,7 @@ name [redundant_pattern](https://github.com/Manishearth/rust-clippy/wiki#redundant_pattern) | warn | using `name @ _` in a pattern [result_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#result_unwrap_used) | allow | using `Result.unwrap()`, which might be better handled [reverse_range_loop](https://github.com/Manishearth/rust-clippy/wiki#reverse_range_loop) | warn | Iterating over an empty range, such as `10..0` or `5..5` +[search_is_some](https://github.com/Manishearth/rust-clippy/wiki#search_is_some) | warn | using an iterator search followed by `is_some()`, which is more succinctly expressed as a call to `any()` [shadow_reuse](https://github.com/Manishearth/rust-clippy/wiki#shadow_reuse) | allow | rebinding a name to an expression that re-uses the original value, e.g. `let x = x + 1` [shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same) | allow | rebinding a name to itself, e.g. `let mut x = &mut x` [shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | allow | The name is re-bound without even using the original value diff --git a/src/lib.rs b/src/lib.rs index 4caf008e3..6d39cad20 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -180,6 +180,7 @@ pub fn plugin_registrar(reg: &mut Registry) { methods::OK_EXPECT, methods::OPTION_MAP_UNWRAP_OR, methods::OPTION_MAP_UNWRAP_OR_ELSE, + methods::SEARCH_IS_SOME, methods::SHOULD_IMPLEMENT_TRAIT, methods::STR_TO_STRING, methods::STRING_TO_STRING, diff --git a/src/methods.rs b/src/methods.rs index f2f7dbdea..081e64b53 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -158,6 +158,18 @@ declare_lint!(pub OPTION_MAP_UNWRAP_OR_ELSE, Warn, declare_lint!(pub FILTER_NEXT, Warn, "using `filter(p).next()`, which is more succinctly expressed as `.find(p)`"); +/// **What it does:** This lint `Warn`s on an iterator search (such as `find()`, `position()`, or +/// `rposition()`) followed by a call to `is_some()`. +/// +/// **Why is this bad?** Readability, this can be written more concisely as `_.any(_)`. +/// +/// **Known problems:** None. +/// +/// **Example:** `iter.find(|x| x == 0).is_some()` +declare_lint!(pub SEARCH_IS_SOME, Warn, + "using an iterator search followed by `is_some()`, which is more succinctly \ + expressed as a call to `any()`"); + impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING, @@ -187,6 +199,15 @@ impl LateLintPass for MethodsPass { else if let Some(arglists) = method_chain_args(expr, &["filter", "next"]) { lint_filter_next(cx, expr, arglists[0]); } + else if let Some(arglists) = method_chain_args(expr, &["find", "is_some"]) { + lint_search_is_some(cx, expr, "find", arglists[0], arglists[1]); + } + else if let Some(arglists) = method_chain_args(expr, &["position", "is_some"]) { + lint_search_is_some(cx, expr, "position", arglists[0], arglists[1]); + } + else if let Some(arglists) = method_chain_args(expr, &["rposition", "is_some"]) { + lint_search_is_some(cx, expr, "rposition", arglists[0], arglists[1]); + } } } @@ -362,6 +383,25 @@ fn lint_filter_next(cx: &LateContext, expr: &Expr, filter_args: &MethodArgs) { } } +#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec +/// lint searching an Iterator followed by `is_some()` +fn lint_search_is_some(cx: &LateContext, expr: &Expr, search_method: &str, search_args: &MethodArgs, + is_some_args: &MethodArgs) { + // lint if caller of search is an Iterator + if match_trait_method(cx, &*is_some_args[0], &["core", "iter", "Iterator"]) { + let msg = format!("called `is_some()` after searching an iterator with {}. This is more \ + succinctly expressed by calling `any()`.", search_method); + let search_snippet = snippet(cx, search_args[1].span, ".."); + if search_snippet.lines().count() <= 1 { // add note if not multi-line + span_note_and_lint(cx, SEARCH_IS_SOME, expr.span, &msg, expr.span, + &format!("replace this with `any({})`)", search_snippet)); + } + else { + span_lint(cx, SEARCH_IS_SOME, expr.span, &msg); + } + } +} + // Given a `Result` type, return its error type (`E`) fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option> { if !match_type(cx, ty, &RESULT_PATH) { diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index d2fed0f90..1878ae15b 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -83,18 +83,32 @@ fn option_methods() { } -/// Struct to generate false positive for FILTER_NEXT lint -struct FilterNextTest { - _foo: u32, +/// Struct to generate false positive for Iterator-based lints +#[derive(Copy, Clone)] +struct IteratorFalsePositives { + foo: u32, } -impl FilterNextTest { - fn filter(self) -> FilterNextTest { +impl IteratorFalsePositives { + fn filter(self) -> IteratorFalsePositives { self } - fn next(self) -> FilterNextTest { + + fn next(self) -> IteratorFalsePositives { self } + + fn find(self) -> Option { + Some(self.foo) + } + + fn position(self) -> Option { + Some(self.foo) + } + + fn rposition(self) -> Option { + Some(self.foo) + } } /// Checks implementation of FILTER_NEXT lint @@ -112,10 +126,48 @@ fn filter_next() { ).next(); // check that we don't lint if the caller is not an Iterator - let foo = FilterNextTest { _foo: 0 }; + let foo = IteratorFalsePositives { foo: 0 }; let _ = foo.filter().next(); } +/// Checks implementation of SEARCH_IS_SOME lint +fn search_is_some() { + let v = vec![3, 2, 1, 0, -1, -2, -3]; + + // check `find().is_some()`, single-line + let _ = v.iter().find(|&x| *x < 0).is_some(); //~ERROR called `is_some()` after searching + //~| NOTE replace this + // check `find().is_some()`, multi-line + let _ = v.iter().find(|&x| { //~ERROR called `is_some()` after searching + *x < 0 + } + ).is_some(); + + // check `position().is_some()`, single-line + let _ = v.iter().position(|&x| x < 0).is_some(); //~ERROR called `is_some()` after searching + //~| NOTE replace this + // check `position().is_some()`, multi-line + let _ = v.iter().position(|&x| { //~ERROR called `is_some()` after searching + x < 0 + } + ).is_some(); + + // check `rposition().is_some()`, single-line + let _ = v.iter().rposition(|&x| x < 0).is_some(); //~ERROR called `is_some()` after searching + //~| NOTE replace this + // check `rposition().is_some()`, multi-line + let _ = v.iter().rposition(|&x| { //~ERROR called `is_some()` after searching + x < 0 + } + ).is_some(); + + // check that we don't lint if the caller is not an Iterator + let foo = IteratorFalsePositives { foo: 0 }; + let _ = foo.find().is_some(); + let _ = foo.position().is_some(); + let _ = foo.rposition().is_some(); +} + fn main() { use std::io;