From 4fcaab3d62f378e99b29455ef1bce54b28bf612b Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 16 May 2019 08:25:39 +0200 Subject: [PATCH] Split redundant_closure lint Move the method checking into a new lint called `redundant_closures_for_method_calls` and put it in the pedantic group. This aspect of the lint seems more controversial than the rest. cc #3942 --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/eta_reduction.rs | 23 +++++++++++++++++++++-- clippy_lints/src/lib.rs | 1 + tests/ui/eta.fixed | 6 +++++- tests/ui/eta.rs | 6 +++++- tests/ui/eta.stderr | 30 ++++++++++++++++-------------- tests/ui/map_clone.fixed | 2 +- tests/ui/map_clone.rs | 2 +- 9 files changed, 52 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 397916fe4..b22fd876c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1035,6 +1035,7 @@ All notable changes to this project will be documented in this file. [`redundant_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone [`redundant_closure`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure [`redundant_closure_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_call +[`redundant_closures_for_method_calls`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closures_for_method_calls [`redundant_field_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names [`redundant_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern [`redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching diff --git a/README.md b/README.md index 4ed388db1..e670924de 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 301 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 302 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/eta_reduction.rs b/clippy_lints/src/eta_reduction.rs index 60e2ab8b9..8334faa6e 100644 --- a/clippy_lints/src/eta_reduction.rs +++ b/clippy_lints/src/eta_reduction.rs @@ -32,7 +32,26 @@ declare_clippy_lint! { "redundant closures, i.e., `|a| foo(a)` (which can be written as just `foo`)" } -declare_lint_pass!(EtaReduction => [REDUNDANT_CLOSURE]); +declare_clippy_lint! { + /// **What it does:** Checks for closures which only invoke a method on the closure + /// argument and can be replaced by referencing the method directly. + /// + /// **Why is this bad?** It's unnecessary to create the closure. + /// + /// **Example:** + /// ```rust,ignore + /// Some('a').map(|s| s.to_uppercase()); + /// ``` + /// may be rewritten as + /// ```rust,ignore + /// Some('a').map(char::to_uppercase); + /// ``` + pub REDUNDANT_CLOSURES_FOR_METHOD_CALLS, + pedantic, + "redundant closures for method calls" +} + +declare_lint_pass!(EtaReduction => [REDUNDANT_CLOSURE, REDUNDANT_CLOSURES_FOR_METHOD_CALLS]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for EtaReduction { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { @@ -104,7 +123,7 @@ fn check_closure(cx: &LateContext<'_, '_>, expr: &Expr) { if let Some(name) = get_ufcs_type_name(cx, method_def_id, &args[0]); then { - span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure found", |db| { + span_lint_and_then(cx, REDUNDANT_CLOSURES_FOR_METHOD_CALLS, expr.span, "redundant closure found", |db| { db.span_suggestion( expr.span, "remove closure as shown", diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index da413cd12..e9c750553 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -614,6 +614,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { enum_glob_use::ENUM_GLOB_USE, enum_variants::MODULE_NAME_REPETITIONS, enum_variants::PUB_ENUM_VARIANT_NAMES, + eta_reduction::REDUNDANT_CLOSURES_FOR_METHOD_CALLS, functions::TOO_MANY_LINES, if_not_else::IF_NOT_ELSE, infinite_iter::MAYBE_INFINITE_ITER, diff --git a/tests/ui/eta.fixed b/tests/ui/eta.fixed index cef4bda24..e829f6848 100644 --- a/tests/ui/eta.fixed +++ b/tests/ui/eta.fixed @@ -9,7 +9,11 @@ clippy::option_map_unit_fn, clippy::trivially_copy_pass_by_ref )] -#![warn(clippy::redundant_closure, clippy::needless_borrow)] +#![warn( + clippy::redundant_closure, + clippy::redundant_closures_for_method_calls, + clippy::needless_borrow +)] use std::path::PathBuf; diff --git a/tests/ui/eta.rs b/tests/ui/eta.rs index 5f029eac4..273706aaf 100644 --- a/tests/ui/eta.rs +++ b/tests/ui/eta.rs @@ -9,7 +9,11 @@ clippy::option_map_unit_fn, clippy::trivially_copy_pass_by_ref )] -#![warn(clippy::redundant_closure, clippy::needless_borrow)] +#![warn( + clippy::redundant_closure, + clippy::redundant_closures_for_method_calls, + clippy::needless_borrow +)] use std::path::PathBuf; diff --git a/tests/ui/eta.stderr b/tests/ui/eta.stderr index afcafc46b..d8c6e3cdb 100644 --- a/tests/ui/eta.stderr +++ b/tests/ui/eta.stderr @@ -1,5 +1,5 @@ error: redundant closure found - --> $DIR/eta.rs:17:27 + --> $DIR/eta.rs:21:27 | LL | let a = Some(1u8).map(|a| foo(a)); | ^^^^^^^^^^ help: remove closure as shown: `foo` @@ -7,19 +7,19 @@ LL | let a = Some(1u8).map(|a| foo(a)); = note: `-D clippy::redundant-closure` implied by `-D warnings` error: redundant closure found - --> $DIR/eta.rs:18:10 + --> $DIR/eta.rs:22:10 | LL | meta(|a| foo(a)); | ^^^^^^^^^^ help: remove closure as shown: `foo` error: redundant closure found - --> $DIR/eta.rs:19:27 + --> $DIR/eta.rs:23:27 | LL | let c = Some(1u8).map(|a| {1+2; foo}(a)); | ^^^^^^^^^^^^^^^^^ help: remove closure as shown: `{1+2; foo}` error: this expression borrows a reference that is immediately dereferenced by the compiler - --> $DIR/eta.rs:21:21 + --> $DIR/eta.rs:25:21 | LL | all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted | ^^^ help: change this to: `&2` @@ -27,61 +27,63 @@ LL | all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted = note: `-D clippy::needless-borrow` implied by `-D warnings` error: redundant closure found - --> $DIR/eta.rs:28:27 + --> $DIR/eta.rs:32:27 | LL | let e = Some(1u8).map(|a| generic(a)); | ^^^^^^^^^^^^^^ help: remove closure as shown: `generic` error: redundant closure found - --> $DIR/eta.rs:71:51 + --> $DIR/eta.rs:75:51 | LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.foo()); | ^^^^^^^^^^^ help: remove closure as shown: `TestStruct::foo` + | + = note: `-D clippy::redundant-closures-for-method-calls` implied by `-D warnings` error: redundant closure found - --> $DIR/eta.rs:73:51 + --> $DIR/eta.rs:77:51 | LL | let e = Some(TestStruct { some_ref: &i }).map(|a| a.trait_foo()); | ^^^^^^^^^^^^^^^^^ help: remove closure as shown: `TestTrait::trait_foo` error: redundant closure found - --> $DIR/eta.rs:76:42 + --> $DIR/eta.rs:80:42 | LL | let e = Some(&mut vec![1, 2, 3]).map(|v| v.clear()); | ^^^^^^^^^^^^^ help: remove closure as shown: `std::vec::Vec::clear` error: redundant closure found - --> $DIR/eta.rs:81:29 + --> $DIR/eta.rs:85:29 | LL | let e = Some("str").map(|s| s.to_string()); | ^^^^^^^^^^^^^^^^^ help: remove closure as shown: `std::string::ToString::to_string` error: redundant closure found - --> $DIR/eta.rs:83:27 + --> $DIR/eta.rs:87:27 | LL | let e = Some('a').map(|s| s.to_uppercase()); | ^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `char::to_uppercase` error: redundant closure found - --> $DIR/eta.rs:86:65 + --> $DIR/eta.rs:90:65 | LL | let e: std::vec::Vec = vec!['a', 'b', 'c'].iter().map(|c| c.to_ascii_uppercase()).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `char::to_ascii_uppercase` error: redundant closure found - --> $DIR/eta.rs:104:50 + --> $DIR/eta.rs:108:50 | LL | let _: Vec<_> = arr.iter().map(|x| x.map_err(|e| some.take().unwrap()(e))).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `some.take().unwrap()` error: redundant closure found - --> $DIR/eta.rs:169:27 + --> $DIR/eta.rs:173:27 | LL | let a = Some(1u8).map(|a| foo_ptr(a)); | ^^^^^^^^^^^^^^ help: remove closure as shown: `foo_ptr` error: redundant closure found - --> $DIR/eta.rs:174:27 + --> $DIR/eta.rs:178:27 | LL | let a = Some(1u8).map(|a| closure(a)); | ^^^^^^^^^^^^^^ help: remove closure as shown: `closure` diff --git a/tests/ui/map_clone.fixed b/tests/ui/map_clone.fixed index 228671ca2..fc25ab13e 100644 --- a/tests/ui/map_clone.fixed +++ b/tests/ui/map_clone.fixed @@ -3,7 +3,7 @@ #![allow(clippy::iter_cloned_collect)] #![allow(clippy::clone_on_copy)] #![allow(clippy::missing_docs_in_private_items)] -#![allow(clippy::redundant_closure)] +#![allow(clippy::redundant_closures_for_method_calls)] fn main() { let _: Vec = vec![5_i8; 6].iter().copied().collect(); diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs index 9a7be3da2..2826f10f4 100644 --- a/tests/ui/map_clone.rs +++ b/tests/ui/map_clone.rs @@ -3,7 +3,7 @@ #![allow(clippy::iter_cloned_collect)] #![allow(clippy::clone_on_copy)] #![allow(clippy::missing_docs_in_private_items)] -#![allow(clippy::redundant_closure)] +#![allow(clippy::redundant_closures_for_method_calls)] fn main() { let _: Vec = vec![5_i8; 6].iter().map(|x| *x).collect();