diff --git a/README.md b/README.md index 2c8b20f83..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 86 lints included in this crate: +There are 88 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -28,6 +28,7 @@ name [eq_op](https://github.com/Manishearth/rust-clippy/wiki#eq_op) | warn | equal operands on both sides of a comparison or bitwise combination (e.g. `x == x`) [explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop) | warn | for-looping with an explicit counter when `_.enumerate()` would do [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop) | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do +[filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next) | warn | using `filter(p).next()`, which is more succinctly expressed as `.find(p)` [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp) | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds) [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op) | warn | using identity operations, e.g. `x + 0` or `y / 1` [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask) | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2` @@ -55,8 +56,8 @@ name [non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal) | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead [nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options) | warn | nonsensical combination of options for opening a file [ok_expect](https://github.com/Manishearth/rust-clippy/wiki#ok_expect) | warn | using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result -[option_map_unwrap_or](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or) | warn | using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`) -[option_map_unwrap_or_else](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else) | warn | using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)`) +[option_map_unwrap_or](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or) | warn | using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)` +[option_map_unwrap_or_else](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else) | warn | using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)` [option_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#option_unwrap_used) | allow | using `Option.unwrap()`, which should at least get a better message using `expect()` [out_of_bounds_indexing](https://github.com/Manishearth/rust-clippy/wiki#out_of_bounds_indexing) | deny | out of bound constant indexing [panic_params](https://github.com/Manishearth/rust-clippy/wiki#panic_params) | warn | missing parameters in `panic!` @@ -68,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 adc1d402e..6d39cad20 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -176,9 +176,11 @@ pub fn plugin_registrar(reg: &mut Registry) { matches::MATCH_BOOL, matches::MATCH_REF_PATS, matches::SINGLE_MATCH, + methods::FILTER_NEXT, 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 d4809d374..a6200534e 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -6,7 +6,7 @@ use std::iter; use std::borrow::Cow; use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, method_chain_args, - walk_ptrs_ty_depth, walk_ptrs_ty}; + match_trait_method, walk_ptrs_ty_depth, walk_ptrs_ty}; use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH}; use utils::MethodArgs; @@ -135,7 +135,7 @@ declare_lint!(pub OK_EXPECT, Warn, /// **Example:** `x.map(|a| a + 1).unwrap_or(0)` declare_lint!(pub OPTION_MAP_UNWRAP_OR, Warn, "using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as \ - `map_or(a, f)`)"); + `map_or(a, f)`"); /// **What it does:** This lint `Warn`s on `_.map(_).unwrap_or_else(_)`. /// @@ -146,7 +146,29 @@ declare_lint!(pub OPTION_MAP_UNWRAP_OR, Warn, /// **Example:** `x.map(|a| a + 1).unwrap_or_else(some_function)` declare_lint!(pub OPTION_MAP_UNWRAP_OR_ELSE, Warn, "using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as \ - `map_or_else(g, f)`)"); + `map_or_else(g, f)`"); + +/// **What it does:** This lint `Warn`s on `_.filter(_).next()`. +/// +/// **Why is this bad?** Readability, this can be written more concisely as `_.find(_)`. +/// +/// **Known problems:** None. +/// +/// **Example:** `iter.filter(|x| x == 0).next()` +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 { @@ -174,6 +196,18 @@ impl LateLintPass for MethodsPass { else if let Some(arglists) = method_chain_args(expr, &["map", "unwrap_or_else"]) { lint_map_unwrap_or_else(cx, expr, arglists[0], arglists[1]); } + 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]); + } } } @@ -275,8 +309,8 @@ fn lint_ok_expect(cx: &LateContext, expr: &Expr, ok_args: &MethodArgs) { #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `map().unwrap_or()` for `Option`s -fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs, - map_args: &MethodArgs) { +fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, map_args: &MethodArgs, + unwrap_args: &MethodArgs) { // lint if the caller of `map()` is an `Option` if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) { // lint message @@ -293,7 +327,8 @@ fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs, if same_span && !multiline { span_note_and_lint( cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span, - &format!("replace this with map_or({1}, {0})", map_snippet, unwrap_snippet) + &format!("replace `map({0}).unwrap_or({1})` with `map_or({1}, {0})`", map_snippet, + unwrap_snippet) ); } else if same_span && multiline { @@ -304,8 +339,8 @@ fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs, #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `map().unwrap_or_else()` for `Option`s -fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs, - map_args: &MethodArgs) { +fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, map_args: &MethodArgs, + unwrap_args: &MethodArgs) { // lint if the caller of `map()` is an `Option` if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) { // lint message @@ -322,7 +357,8 @@ fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, unwrap_args: &MethodAr if same_span && !multiline { span_note_and_lint( cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg, expr.span, - &format!("replace this with map_or_else({1}, {0})", map_snippet, unwrap_snippet) + &format!("replace `map({0}).unwrap_or_else({1})` with `with map_or_else({1}, {0})`", + map_snippet, unwrap_snippet) ); } else if same_span && multiline { @@ -331,6 +367,44 @@ fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, unwrap_args: &MethodAr } } +#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec +/// lint use of `filter().next() for Iterators` +fn lint_filter_next(cx: &LateContext, expr: &Expr, filter_args: &MethodArgs) { + // lint if caller of `.filter().next()` is an Iterator + if match_trait_method(cx, expr, &["core", "iter", "Iterator"]) { + let msg = "called `filter(p).next()` on an Iterator. This is more succinctly expressed by \ + calling `.find(p)` instead."; + let filter_snippet = snippet(cx, filter_args[1].span, ".."); + if filter_snippet.lines().count() <= 1 { // add note if not multi-line + span_note_and_lint(cx, FILTER_NEXT, expr.span, msg, expr.span, + &format!("replace `filter({0}).next()` with `find({0})`", filter_snippet)); + } + else { + span_lint(cx, FILTER_NEXT, expr.span, msg); + } + } +} + +#[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 `{0}({1}).is_some()` with `any({1})`", search_method, + 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 9078a78d6..b41b28dc1 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -50,7 +50,7 @@ fn option_methods() { // Check OPTION_MAP_UNWRAP_OR // single line case let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or(a)` - //~| NOTE replace this + //~| NOTE replace `map(|x| x + 1).unwrap_or(0)` .unwrap_or(0); // should lint even though this call is on a separate line // multi line cases let _ = opt.map(|x| { //~ ERROR called `map(f).unwrap_or(a)` @@ -67,7 +67,7 @@ fn option_methods() { // Check OPTION_MAP_UNWRAP_OR_ELSE // single line case let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or_else(g)` - //~| NOTE replace this + //~| NOTE replace `map(|x| x + 1).unwrap_or_else(|| 0)` .unwrap_or_else(|| 0); // should lint even though this call is on a separate line // multi line cases let _ = opt.map(|x| { //~ ERROR called `map(f).unwrap_or_else(g)` @@ -83,6 +83,98 @@ fn option_methods() { } +/// Struct to generate false positive for Iterator-based lints +#[derive(Copy, Clone)] +struct IteratorFalsePositives { + foo: u32, +} + +impl IteratorFalsePositives { + fn filter(self) -> IteratorFalsePositives { + self + } + + 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 +fn filter_next() { + let v = vec![3, 2, 1, 0, -1, -2, -3]; + + // check single-line case + let _ = v.iter().filter(|&x| *x < 0).next(); + //~^ ERROR called `filter(p).next()` on an Iterator. + //~| NOTE replace `filter(|&x| *x < 0).next()` + + // check multi-line case + let _ = v.iter().filter(|&x| { //~ERROR called `filter(p).next()` on an Iterator. + *x < 0 + } + ).next(); + + // check that we don't lint if the caller is not an Iterator + 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 `find(|&x| *x < 0).is_some()` + + // 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 `position(|&x| x < 0).is_some()` + + // 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 `rposition(|&x| x < 0).is_some()` + + // 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;