From 95c369fa9168d2eee08d1a693dc1c63872fe46c0 Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Mon, 20 Jan 2020 10:54:54 +0900 Subject: [PATCH 1/3] Add `skip_while_next` lint --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 3 +++ clippy_lints/src/methods/mod.rs | 39 ++++++++++++++++++++++++++++ src/lintlist/mod.rs | 9 ++++++- tests/ui/auxiliary/option_helpers.rs | 4 +++ tests/ui/skip_while_next.rs | 29 +++++++++++++++++++++ tests/ui/skip_while_next.stderr | 20 ++++++++++++++ 8 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 tests/ui/skip_while_next.rs create mode 100644 tests/ui/skip_while_next.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 11d745a4c23c..f7e8a4534de6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1285,6 +1285,7 @@ Released 2018-09-13 [`single_char_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern [`single_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match [`single_match_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else +[`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next [`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization [`str_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string [`string_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add diff --git a/README.md b/README.md index 7b1d14e9afca..d752e5b4cc15 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 347 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 348 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 497146772a51..962ff38eb6cd 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -645,6 +645,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &methods::SEARCH_IS_SOME, &methods::SHOULD_IMPLEMENT_TRAIT, &methods::SINGLE_CHAR_PATTERN, + &methods::SKIP_WHILE_NEXT, &methods::STRING_EXTEND_CHARS, &methods::SUSPICIOUS_MAP, &methods::TEMPORARY_CSTRING_AS_PTR, @@ -1223,6 +1224,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&methods::SEARCH_IS_SOME), LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT), LintId::of(&methods::SINGLE_CHAR_PATTERN), + LintId::of(&methods::SKIP_WHILE_NEXT), LintId::of(&methods::STRING_EXTEND_CHARS), LintId::of(&methods::SUSPICIOUS_MAP), LintId::of(&methods::TEMPORARY_CSTRING_AS_PTR), @@ -1475,6 +1477,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&methods::FLAT_MAP_IDENTITY), LintId::of(&methods::OPTION_AND_THEN_SOME), LintId::of(&methods::SEARCH_IS_SOME), + LintId::of(&methods::SKIP_WHILE_NEXT), LintId::of(&methods::SUSPICIOUS_MAP), LintId::of(&methods::UNNECESSARY_FILTER_MAP), LintId::of(&methods::USELESS_ASREF), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index ed37d3411b5a..8dfebb56f302 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -375,6 +375,29 @@ declare_clippy_lint! { "using `filter(p).next()`, which is more succinctly expressed as `.find(p)`" } +declare_clippy_lint! { + /// **What it does:** Checks for usage of `_.skip_while(condition).next()`. + /// + /// **Why is this bad?** Readability, this can be written more concisely as + /// `_.find(!condition)`. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// # let vec = vec![1]; + /// vec.iter().skip_while(|x| **x == 0).next(); + /// ``` + /// Could be written as + /// ```rust + /// # let vec = vec![1]; + /// vec.iter().find(|x| **x != 0); + /// ``` + pub SKIP_WHILE_NEXT, + complexity, + "using `skip_while(p).next()`, which is more succinctly expressed as `.find(!p)`" +} + declare_clippy_lint! { /// **What it does:** Checks for usage of `_.map(_).flatten(_)`, /// @@ -1192,6 +1215,7 @@ declare_lint_pass!(Methods => [ SEARCH_IS_SOME, TEMPORARY_CSTRING_AS_PTR, FILTER_NEXT, + SKIP_WHILE_NEXT, FILTER_MAP, FILTER_MAP_NEXT, FLAT_MAP_IDENTITY, @@ -1237,6 +1261,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { ["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]), ["and_then", ..] => lint_option_and_then_some(cx, expr, arg_lists[0]), ["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]), + ["next", "skip_while"] => lint_skip_while_next(cx, expr, arg_lists[1]), ["map", "filter"] => lint_filter_map(cx, expr, arg_lists[1], arg_lists[0]), ["map", "filter_map"] => lint_filter_map_map(cx, expr, arg_lists[1], arg_lists[0]), ["next", "filter_map"] => lint_filter_map_next(cx, expr, arg_lists[1]), @@ -2530,6 +2555,20 @@ fn lint_filter_next<'a, 'tcx>( } } +/// lint use of `skip_while().next()` for `Iterators` +fn lint_skip_while_next<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + expr: &'tcx hir::Expr<'_>, + _skip_while_args: &'tcx [hir::Expr<'_>], +) { + // lint if caller of `.skip_while().next()` is an Iterator + if match_trait_method(cx, expr, &paths::ITERATOR) { + let msg = "called `skip_while(p).next()` on an `Iterator`. \ + This is more succinctly expressed by calling `.find(!p)` instead."; + span_lint(cx, SKIP_WHILE_NEXT, expr.span, msg); + } +} + /// lint use of `filter().map()` for `Iterators` fn lint_filter_map<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index c6339daf2ebe..dcdfd015393b 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 347] = [ +pub const ALL_LINTS: [Lint; 348] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1862,6 +1862,13 @@ pub const ALL_LINTS: [Lint; 347] = [ deprecation: None, module: "matches", }, + Lint { + name: "skip_while_next", + group: "complexity", + desc: "using `skip_while(p).next()`, which is more succinctly expressed as `.find(!p)`", + deprecation: None, + module: "methods", + }, Lint { name: "slow_vector_initialization", group: "perf", diff --git a/tests/ui/auxiliary/option_helpers.rs b/tests/ui/auxiliary/option_helpers.rs index 331952119689..ed11c41e21c1 100644 --- a/tests/ui/auxiliary/option_helpers.rs +++ b/tests/ui/auxiliary/option_helpers.rs @@ -44,4 +44,8 @@ impl IteratorFalsePositives { pub fn skip(self, _: usize) -> IteratorFalsePositives { self } + + pub fn skip_while(self) -> IteratorFalsePositives { + self + } } diff --git a/tests/ui/skip_while_next.rs b/tests/ui/skip_while_next.rs new file mode 100644 index 000000000000..a522c0f08b20 --- /dev/null +++ b/tests/ui/skip_while_next.rs @@ -0,0 +1,29 @@ +// aux-build:option_helpers.rs + +#![warn(clippy::skip_while_next)] +#![allow(clippy::blacklisted_name)] + +extern crate option_helpers; +use option_helpers::IteratorFalsePositives; + +#[rustfmt::skip] +fn skip_while_next() { + let v = vec![3, 2, 1, 0, -1, -2, -3]; + + // Single-line case. + let _ = v.iter().skip_while(|&x| *x < 0).next(); + + // Multi-line case. + let _ = v.iter().skip_while(|&x| { + *x < 0 + } + ).next(); + + // Check that hat we don't lint if the caller is not an `Iterator`. + let foo = IteratorFalsePositives { foo: 0 }; + let _ = foo.skip_while().next(); +} + +fn main() { + skip_while_next(); +} diff --git a/tests/ui/skip_while_next.stderr b/tests/ui/skip_while_next.stderr new file mode 100644 index 000000000000..2ce88ac23a56 --- /dev/null +++ b/tests/ui/skip_while_next.stderr @@ -0,0 +1,20 @@ +error: called `skip_while(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(!p)` instead. + --> $DIR/skip_while_next.rs:14:13 + | +LL | let _ = v.iter().skip_while(|&x| *x < 0).next(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::skip-while-next` implied by `-D warnings` + +error: called `skip_while(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(!p)` instead. + --> $DIR/skip_while_next.rs:17:13 + | +LL | let _ = v.iter().skip_while(|&x| { + | _____________^ +LL | | *x < 0 +LL | | } +LL | | ).next(); + | |___________________________^ + +error: aborting due to 2 previous errors + From 84e4bc54e0042d2db086cc5b385372a7ae4c49f3 Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Mon, 20 Jan 2020 10:55:23 +0900 Subject: [PATCH 2/3] Remove trailing whitespaces --- doc/adding_lints.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/doc/adding_lints.md b/doc/adding_lints.md index 6fd052893bfc..deecbf80a228 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -55,7 +55,7 @@ we want to check. The output of Clippy is compared against a `.stderr` file. Note that you don't have to create this file yourself, we'll get to generating the `.stderr` files further down. -We start by opening the test file created at `tests/ui/foo_functions.rs`. +We start by opening the test file created at `tests/ui/foo_functions.rs`. Update the file with some examples to get started: @@ -102,7 +102,7 @@ Once we are satisfied with the output, we need to run `tests/ui/update-all-references.sh` to update the `.stderr` file for our lint. Please note that, we should run `TESTNAME=foo_functions cargo uitest` every time before running `tests/ui/update-all-references.sh`. -Running `TESTNAME=foo_functions cargo uitest` should pass then. When we commit +Running `TESTNAME=foo_functions cargo uitest` should pass then. When we commit our lint, we need to commit the generated `.stderr` files, too. ### Rustfix tests @@ -133,7 +133,7 @@ With tests in place, let's have a look at implementing our lint now. ### Lint declaration -Let's start by opening the new file created in the `clippy_lints` crate +Let's start by opening the new file created in the `clippy_lints` crate at `clippy_lints/src/foo_functions.rs`. That's the crate where all the lint code is. This file has already imported some initial things we will need: @@ -178,7 +178,7 @@ state the thing that is being checked for and read well when used with * The last part should be a text that explains what exactly is wrong with the code -The rest of this file contains an empty implementation for our lint pass, +The rest of this file contains an empty implementation for our lint pass, which in this case is `EarlyLintPass` and should look like this: ```rust @@ -194,7 +194,7 @@ impl EarlyLintPass for FooFunctions {} Don't worry about the `name` method here. As long as it includes the name of the lint pass it should be fine. -The new lint automation runs `update_lints`, which automates some things, but it +The new lint automation runs `update_lints`, which automates some things, but it doesn't automate everything. We will have to register our lint pass manually in the `register_plugins` function in `clippy_lints/src/lib.rs`: From bec5b69e4550f14310ee8b5cf9c6ac35222d48eb Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Mon, 20 Jan 2020 21:07:31 +0900 Subject: [PATCH 3/3] Apply review comment --- clippy_lints/src/methods/mod.rs | 10 +++++++--- tests/ui/skip_while_next.stderr | 7 +++++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8dfebb56f302..013c9deb5094 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2563,9 +2563,13 @@ fn lint_skip_while_next<'a, 'tcx>( ) { // lint if caller of `.skip_while().next()` is an Iterator if match_trait_method(cx, expr, &paths::ITERATOR) { - let msg = "called `skip_while(p).next()` on an `Iterator`. \ - This is more succinctly expressed by calling `.find(!p)` instead."; - span_lint(cx, SKIP_WHILE_NEXT, expr.span, msg); + span_help_and_lint( + cx, + SKIP_WHILE_NEXT, + expr.span, + "called `skip_while(p).next()` on an `Iterator`", + "this is more succinctly expressed by calling `.find(!p)` instead", + ); } } diff --git a/tests/ui/skip_while_next.stderr b/tests/ui/skip_while_next.stderr index 2ce88ac23a56..a6b7bcd63ff3 100644 --- a/tests/ui/skip_while_next.stderr +++ b/tests/ui/skip_while_next.stderr @@ -1,12 +1,13 @@ -error: called `skip_while(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(!p)` instead. +error: called `skip_while(p).next()` on an `Iterator` --> $DIR/skip_while_next.rs:14:13 | LL | let _ = v.iter().skip_while(|&x| *x < 0).next(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::skip-while-next` implied by `-D warnings` + = help: this is more succinctly expressed by calling `.find(!p)` instead -error: called `skip_while(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(!p)` instead. +error: called `skip_while(p).next()` on an `Iterator` --> $DIR/skip_while_next.rs:17:13 | LL | let _ = v.iter().skip_while(|&x| { @@ -15,6 +16,8 @@ LL | | *x < 0 LL | | } LL | | ).next(); | |___________________________^ + | + = help: this is more succinctly expressed by calling `.find(!p)` instead error: aborting due to 2 previous errors