From 7065239da55420e26adb7cb647fc7eb4e9c8798e Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Thu, 15 Aug 2019 10:53:11 +0700 Subject: [PATCH 1/3] Add option_and_then_some lint --- CHANGELOG.md | 1 + README.md | 2 +- clippy_dev/src/lib.rs | 6 +- clippy_lints/src/lib.rs | 2 + clippy_lints/src/methods/mod.rs | 129 +++++++++++++++++++++++++-- src/lintlist/mod.rs | 9 +- tests/ui/option_and_then_some.fixed | 21 +++++ tests/ui/option_and_then_some.rs | 21 +++++ tests/ui/option_and_then_some.stderr | 20 +++++ 9 files changed, 201 insertions(+), 10 deletions(-) create mode 100644 tests/ui/option_and_then_some.fixed create mode 100644 tests/ui/option_and_then_some.rs create mode 100644 tests/ui/option_and_then_some.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e5dfa112..59dfde0b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1088,6 +1088,7 @@ Released 2018-09-13 [`not_unsafe_ptr_arg_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref [`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref +[`option_and_then_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_and_then_some [`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none [`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn [`option_map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unwrap_or diff --git a/README.md b/README.md index 389fe316a..e5da76360 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 310 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 311 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_dev/src/lib.rs b/clippy_dev/src/lib.rs index db407565b..b53a55799 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -123,13 +123,13 @@ pub fn gen_deprecated(lints: &[Lint]) -> Vec { lints .iter() .filter_map(|l| { - l.clone().deprecation.and_then(|depr_text| { - Some(vec![ + l.clone().deprecation.map(|depr_text| { + vec![ " store.register_removed(".to_string(), format!(" \"clippy::{}\",", l.name), format!(" \"{}\",", depr_text), " );".to_string(), - ]) + ] }) }) .flatten() diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7e9a251d4..4d568a8a8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -795,6 +795,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { methods::ITER_SKIP_NEXT, methods::NEW_RET_NO_SELF, methods::OK_EXPECT, + methods::OPTION_AND_THEN_SOME, methods::OPTION_MAP_OR_NONE, methods::OR_FUN_CALL, methods::SEARCH_IS_SOME, @@ -1033,6 +1034,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { methods::CLONE_ON_COPY, methods::FILTER_NEXT, methods::FLAT_MAP_IDENTITY, + methods::OPTION_AND_THEN_SOME, methods::SEARCH_IS_SOME, methods::SUSPICIOUS_MAP, methods::UNNECESSARY_FILTER_MAP, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c8e3ca14a..8d062bd89 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -21,11 +21,11 @@ use syntax::symbol::LocalInternedString; use crate::utils::sugg; use crate::utils::usage::mutated_variables; use crate::utils::{ - get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, is_copy, - is_ctor_function, is_expn_of, iter_input_pats, last_path_segment, match_def_path, match_qpath, match_trait_method, - match_type, match_var, method_calls, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, - snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_sugg, - span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq, + get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, in_macro_or_desugar, + is_copy, is_ctor_function, is_expn_of, iter_input_pats, last_path_segment, match_def_path, match_qpath, + match_trait_method, match_type, match_var, method_calls, method_chain_args, remove_blocks, return_ty, same_tys, + single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint, + span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq, }; use crate::utils::{paths, span_help_and_lint}; @@ -264,6 +264,32 @@ declare_clippy_lint! { "using `Option.map_or(None, f)`, which is more succinctly expressed as `and_then(f)`" } +declare_clippy_lint! { + /// **What it does:** Checks for usage of `_.and_then(|x| Some(y))`. + /// + /// **Why is this bad?** Readability, this can be written more concisely as + /// `_.map(|x| y)`. + /// + /// **Known problems:** None + /// + /// **Example:** + /// + /// ```rust + /// let x = Some("foo"); + /// let _ = x.and_then(|s| Some(s.len())); + /// ``` + /// + /// The correct use would be: + /// + /// ```rust + /// let x = Some("foo"); + /// let _ = x.map(|s| s.len()); + /// ``` + pub OPTION_AND_THEN_SOME, + complexity, + "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`" +} + declare_clippy_lint! { /// **What it does:** Checks for usage of `_.filter(_).next()`. /// @@ -918,6 +944,7 @@ declare_lint_pass!(Methods => [ OPTION_MAP_UNWRAP_OR_ELSE, RESULT_MAP_UNWRAP_OR_ELSE, OPTION_MAP_OR_NONE, + OPTION_AND_THEN_SOME, OR_FUN_CALL, EXPECT_FUN_CALL, CHARS_NEXT_CMP, @@ -967,6 +994,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { ["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0]), ["unwrap_or_else", "map"] => lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]), ["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]), ["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]), @@ -2072,6 +2100,97 @@ fn lint_map_or_none<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, } } +/// Lint use of `_.and_then(|x| Some(y))` for `Option`s +fn lint_option_and_then_some(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr]) { + const LINT_MSG: &str = "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`"; + const NO_OP_MSG: &str = "using `Option.and_then(Some)`, which is a no-op"; + + // Searches an return expressions in `y` in `_.and_then(|x| Some(y))`, which we don't lint + struct RetCallFinder { + found: bool, + } + + impl<'tcx> intravisit::Visitor<'tcx> for RetCallFinder { + fn visit_expr(&mut self, expr: &'tcx hir::Expr) { + if self.found { + return; + } + if let hir::ExprKind::Ret(..) = &expr.node { + self.found = true; + } else { + intravisit::walk_expr(self, expr); + } + } + + fn nested_visit_map<'this>(&'this mut self) -> intravisit::NestedVisitorMap<'this, 'tcx> { + intravisit::NestedVisitorMap::None + } + } + + let ty = cx.tables.expr_ty(&args[0]); + if !match_type(cx, ty, &paths::OPTION) { + return; + } + + match args[1].node { + hir::ExprKind::Closure(_, _, body_id, closure_args_span, _) => { + let closure_body = cx.tcx.hir().body(body_id); + let closure_expr = remove_blocks(&closure_body.value); + if_chain! { + if let hir::ExprKind::Call(ref some_expr, ref some_args) = closure_expr.node; + if let hir::ExprKind::Path(ref qpath) = some_expr.node; + if match_qpath(qpath, &paths::OPTION_SOME); + if some_args.len() == 1; + then { + let inner_expr = &some_args[0]; + + let mut finder = RetCallFinder { found: false }; + finder.visit_expr(inner_expr); + if finder.found { + return; + } + + let some_inner_snip = if in_macro_or_desugar(inner_expr.span) { + snippet_with_macro_callsite(cx, inner_expr.span, "_") + } else { + snippet(cx, inner_expr.span, "_") + }; + + let closure_args_snip = snippet(cx, closure_args_span, ".."); + let option_snip = snippet(cx, args[0].span, ".."); + let note = format!("{}.map({} {})", option_snip, closure_args_snip, some_inner_snip); + span_lint_and_sugg( + cx, + OPTION_AND_THEN_SOME, + expr.span, + LINT_MSG, + "try this", + note, + Applicability::MachineApplicable, + ); + } + } + }, + // `_.and_then(Some)` case, which is no-op. + hir::ExprKind::Path(ref qpath) => { + if match_qpath(qpath, &paths::OPTION_SOME) { + let option_snip = snippet(cx, args[0].span, ".."); + let note = format!("{}", option_snip); + span_lint_and_sugg( + cx, + OPTION_AND_THEN_SOME, + expr.span, + NO_OP_MSG, + "use the expression directly", + note, + Applicability::MachineApplicable, + ); + } + }, + _ => {}, + } +} + /// lint use of `filter().next()` for `Iterators` fn lint_filter_next<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, filter_args: &'tcx [hir::Expr]) { // lint if caller of `.filter().next()` is an Iterator diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 485352f5c..9abe4558b 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; 310] = [ +pub const ALL_LINTS: [Lint; 311] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1316,6 +1316,13 @@ pub const ALL_LINTS: [Lint; 310] = [ deprecation: None, module: "eq_op", }, + Lint { + name: "option_and_then_some", + group: "complexity", + desc: "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`", + deprecation: None, + module: "methods", + }, Lint { name: "option_map_or_none", group: "style", diff --git a/tests/ui/option_and_then_some.fixed b/tests/ui/option_and_then_some.fixed new file mode 100644 index 000000000..852f48879 --- /dev/null +++ b/tests/ui/option_and_then_some.fixed @@ -0,0 +1,21 @@ +// run-rustfix +#![deny(clippy::option_and_then_some)] + +// need a main anyway, use it get rid of unused warnings too +pub fn main() { + let x = Some(5); + // the easiest cases + let _ = x; + let _ = x.map(|o| o + 1); + // and an easy counter-example + let _ = x.and_then(|o| if o < 32 { Some(o) } else { None }); + + // Different type + let x: Result = Ok(1); + let _ = x.and_then(Ok); +} + +pub fn foo() -> Option { + let x = Some(String::from("hello")); + Some("hello".to_owned()).and_then(|s| Some(format!("{}{}", s, x?))) +} diff --git a/tests/ui/option_and_then_some.rs b/tests/ui/option_and_then_some.rs new file mode 100644 index 000000000..aebc66374 --- /dev/null +++ b/tests/ui/option_and_then_some.rs @@ -0,0 +1,21 @@ +// run-rustfix +#![deny(clippy::option_and_then_some)] + +// need a main anyway, use it get rid of unused warnings too +pub fn main() { + let x = Some(5); + // the easiest cases + let _ = x.and_then(Some); + let _ = x.and_then(|o| Some(o + 1)); + // and an easy counter-example + let _ = x.and_then(|o| if o < 32 { Some(o) } else { None }); + + // Different type + let x: Result = Ok(1); + let _ = x.and_then(Ok); +} + +pub fn foo() -> Option { + let x = Some(String::from("hello")); + Some("hello".to_owned()).and_then(|s| Some(format!("{}{}", s, x?))) +} diff --git a/tests/ui/option_and_then_some.stderr b/tests/ui/option_and_then_some.stderr new file mode 100644 index 000000000..a3b0a2614 --- /dev/null +++ b/tests/ui/option_and_then_some.stderr @@ -0,0 +1,20 @@ +error: using `Option.and_then(Some)`, which is a no-op + --> $DIR/option_and_then_some.rs:8:13 + | +LL | let _ = x.and_then(Some); + | ^^^^^^^^^^^^^^^^ help: use the expression directly: `x` + | +note: lint level defined here + --> $DIR/option_and_then_some.rs:2:9 + | +LL | #![deny(clippy::option_and_then_some)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)` + --> $DIR/option_and_then_some.rs:9:13 + | +LL | let _ = x.and_then(|o| Some(o + 1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `x.map(|o| o + 1)` + +error: aborting due to 2 previous errors + From 50ecd595a6d8ec66299c4ce2f1804a381458d74f Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 16 Aug 2019 09:42:07 +0700 Subject: [PATCH 2/3] Allow option_and_then_some in option_map_or_none test --- tests/ui/option_map_or_none.fixed | 2 ++ tests/ui/option_map_or_none.rs | 2 ++ tests/ui/option_map_or_none.stderr | 4 ++-- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/ui/option_map_or_none.fixed b/tests/ui/option_map_or_none.fixed index e637f973c..decbae4e5 100644 --- a/tests/ui/option_map_or_none.fixed +++ b/tests/ui/option_map_or_none.fixed @@ -1,5 +1,7 @@ // run-rustfix +#![allow(clippy::option_and_then_some)] + fn main() { let opt = Some(1); diff --git a/tests/ui/option_map_or_none.rs b/tests/ui/option_map_or_none.rs index 4b9b24788..0f1d2218d 100644 --- a/tests/ui/option_map_or_none.rs +++ b/tests/ui/option_map_or_none.rs @@ -1,5 +1,7 @@ // run-rustfix +#![allow(clippy::option_and_then_some)] + fn main() { let opt = Some(1); diff --git a/tests/ui/option_map_or_none.stderr b/tests/ui/option_map_or_none.stderr index 66ed1e9f0..eb5c68ed7 100644 --- a/tests/ui/option_map_or_none.stderr +++ b/tests/ui/option_map_or_none.stderr @@ -1,5 +1,5 @@ error: called `map_or(None, f)` on an Option value. This can be done more directly by calling `and_then(f)` instead - --> $DIR/option_map_or_none.rs:8:13 + --> $DIR/option_map_or_none.rs:10:13 | LL | let _ = opt.map_or(None, |x| Some(x + 1)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using and_then instead: `opt.and_then(|x| Some(x + 1))` @@ -7,7 +7,7 @@ LL | let _ = opt.map_or(None, |x| Some(x + 1)); = note: `-D clippy::option-map-or-none` implied by `-D warnings` error: called `map_or(None, f)` on an Option value. This can be done more directly by calling `and_then(f)` instead - --> $DIR/option_map_or_none.rs:11:13 + --> $DIR/option_map_or_none.rs:13:13 | LL | let _ = opt.map_or(None, |x| { | _____________^ From 41eba2f26aa0bb45148d37e9696b73322f8d6ca0 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Mon, 19 Aug 2019 05:41:47 +0000 Subject: [PATCH 3/3] Add test --- tests/ui/option_and_then_some.fixed | 4 ++++ tests/ui/option_and_then_some.rs | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/tests/ui/option_and_then_some.fixed b/tests/ui/option_and_then_some.fixed index 852f48879..035bc1e54 100644 --- a/tests/ui/option_and_then_some.fixed +++ b/tests/ui/option_and_then_some.fixed @@ -19,3 +19,7 @@ pub fn foo() -> Option { let x = Some(String::from("hello")); Some("hello".to_owned()).and_then(|s| Some(format!("{}{}", s, x?))) } + +pub fn example2(x: bool) -> Option<&'static str> { + Some("a").and_then(|s| Some(if x { s } else { return None })) +} diff --git a/tests/ui/option_and_then_some.rs b/tests/ui/option_and_then_some.rs index aebc66374..d49da7813 100644 --- a/tests/ui/option_and_then_some.rs +++ b/tests/ui/option_and_then_some.rs @@ -19,3 +19,7 @@ pub fn foo() -> Option { let x = Some(String::from("hello")); Some("hello".to_owned()).and_then(|s| Some(format!("{}{}", s, x?))) } + +pub fn example2(x: bool) -> Option<&'static str> { + Some("a").and_then(|s| Some(if x { s } else { return None })) +}