From 9c4c360a538bbdfc231fcbddab713b40477ccccd Mon Sep 17 00:00:00 2001 From: Guanqun Lu Date: Sat, 16 Nov 2019 01:12:11 +0800 Subject: [PATCH 1/6] lint: add map(f).unwrap_or(a) for Result --- clippy_lints/src/lib.rs | 2 + ...tion_map_unwrap_or.rs => map_unwrap_or.rs} | 64 ++++++++++++++----- clippy_lints/src/methods/mod.rs | 25 +++++++- src/lintlist/mod.rs | 9 ++- 4 files changed, 79 insertions(+), 21 deletions(-) rename clippy_lints/src/methods/{option_map_unwrap_or.rs => map_unwrap_or.rs} (66%) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index bae4eebf8508..16155c4dba91 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -621,6 +621,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf &methods::OPTION_UNWRAP_USED, &methods::OR_FUN_CALL, &methods::RESULT_EXPECT_USED, + &methods::RESULT_MAP_UNWRAP_OR, &methods::RESULT_MAP_UNWRAP_OR_ELSE, &methods::RESULT_UNWRAP_USED, &methods::SEARCH_IS_SOME, @@ -1036,6 +1037,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf LintId::of(&methods::MAP_FLATTEN), LintId::of(&methods::OPTION_MAP_UNWRAP_OR), LintId::of(&methods::OPTION_MAP_UNWRAP_OR_ELSE), + LintId::of(&methods::RESULT_MAP_UNWRAP_OR), LintId::of(&methods::RESULT_MAP_UNWRAP_OR_ELSE), LintId::of(&misc::USED_UNDERSCORE_BINDING), LintId::of(&misc_early::UNSEPARATED_LITERAL_SUFFIX), diff --git a/clippy_lints/src/methods/option_map_unwrap_or.rs b/clippy_lints/src/methods/map_unwrap_or.rs similarity index 66% rename from clippy_lints/src/methods/option_map_unwrap_or.rs rename to clippy_lints/src/methods/map_unwrap_or.rs index 769392a6fd8f..7a19ac42e5ed 100644 --- a/clippy_lints/src/methods/option_map_unwrap_or.rs +++ b/clippy_lints/src/methods/map_unwrap_or.rs @@ -6,17 +6,19 @@ use rustc::lint::LateContext; use rustc_data_structures::fx::FxHashSet; use syntax_pos::symbol::Symbol; -use super::OPTION_MAP_UNWRAP_OR; +use super::{OPTION_MAP_UNWRAP_OR, RESULT_MAP_UNWRAP_OR}; -/// lint use of `map().unwrap_or()` for `Option`s +/// lint use of `map().unwrap_or()` for `Option`s and `Result`s pub(super) fn lint<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, map_args: &'tcx [hir::Expr], unwrap_args: &'tcx [hir::Expr], ) { - // lint if the caller of `map()` is an `Option` - if match_type(cx, cx.tables.expr_ty(&map_args[0]), &paths::OPTION) { + let is_option = match_type(cx, cx.tables.expr_ty(&map_args[0]), &paths::OPTION); + let is_result = match_type(cx, cx.tables.expr_ty(&map_args[0]), &paths::RESULT); + + if is_option || is_result { if !is_copy(cx, cx.tables.expr_ty(&unwrap_args[1])) { // Do not lint if the `map` argument uses identifiers in the `map` // argument that are also used in the `unwrap_or` argument @@ -43,19 +45,27 @@ pub(super) fn lint<'a, 'tcx>( let map_snippet = snippet(cx, map_args[1].span, ".."); let unwrap_snippet = snippet(cx, unwrap_args[1].span, ".."); // lint message - // comparing the snippet from source to raw text ("None") below is safe - // because we already have checked the type. - let arg = if unwrap_snippet == "None" { "None" } else { "a" }; - let suggest = if unwrap_snippet == "None" { - "and_then(f)" + let msg = if is_option { + // comparing the snippet from source to raw text ("None") below is safe + // because we already have checked the type. + let arg = if unwrap_snippet == "None" { "None" } else { "a" }; + let suggest = if unwrap_snippet == "None" { + "and_then(f)" + } else { + "map_or(a, f)" + }; + + format!( + "called `map(f).unwrap_or({})` on an Option value. \ + This can be done more directly by calling `{}` instead", + arg, suggest + ) } else { - "map_or(a, f)" + "called `map(f).unwrap_or(a)` on a Result value. \ + This can be done more directly by calling `map_or(a, f)` instead" + .to_string() }; - let msg = &format!( - "called `map(f).unwrap_or({})` on an Option value. \ - This can be done more directly by calling `{}` instead", - arg, suggest - ); + // lint, with note if neither arg is > 1 line and both map() and // unwrap_or() have the same span let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1; @@ -70,9 +80,29 @@ pub(super) fn lint<'a, 'tcx>( "replace `map({}).unwrap_or({})` with `{}`", map_snippet, unwrap_snippet, suggest ); - span_note_and_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span, ¬e); + span_note_and_lint( + cx, + if is_option { + OPTION_MAP_UNWRAP_OR + } else { + RESULT_MAP_UNWRAP_OR + }, + expr.span, + &msg, + expr.span, + ¬e, + ); } else if same_span && multiline { - span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg); + span_lint( + cx, + if is_option { + OPTION_MAP_UNWRAP_OR + } else { + RESULT_MAP_UNWRAP_OR + }, + expr.span, + &msg, + ); }; } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index ca62e7ea9d2b..3851b0dcb035 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1,6 +1,6 @@ mod inefficient_to_string; mod manual_saturating_arithmetic; -mod option_map_unwrap_or; +mod map_unwrap_or; mod unnecessary_filter_map; use std::borrow::Cow; @@ -252,7 +252,7 @@ declare_clippy_lint! { } declare_clippy_lint! { - /// **What it does:** Checks for usage of `_.map(_).unwrap_or(_)`. + /// **What it does:** Checks for usage of `_.map(_).unwrap_or(_)` for `Option`. /// /// **Why is this bad?** Readability, this can be written more concisely as /// `_.map_or(_, _)`. @@ -269,6 +269,24 @@ declare_clippy_lint! { "using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`" } +declare_clippy_lint! { + /// **What it does:** Checks for usage of `_.map(_).unwrap_or(_)` for `Result`. + /// + /// **Why is this bad?** Readability, this can be written more concisely as + /// `_.map_or(_, _)`. + /// + /// **Known problems:** The order of the arguments is not in execution order + /// + /// **Example:** + /// ```rust + /// # let x: Result = Ok(1); + /// x.map(|a| a + 1).unwrap_or(0); + /// ``` + pub RESULT_MAP_UNWRAP_OR, + pedantic, + "using `Result.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`" +} + declare_clippy_lint! { /// **What it does:** Checks for usage of `_.map(_).unwrap_or_else(_)`. /// @@ -1093,6 +1111,7 @@ declare_lint_pass!(Methods => [ WRONG_PUB_SELF_CONVENTION, OK_EXPECT, OPTION_MAP_UNWRAP_OR, + RESULT_MAP_UNWRAP_OR, OPTION_MAP_UNWRAP_OR_ELSE, RESULT_MAP_UNWRAP_OR_ELSE, OPTION_MAP_OR_NONE, @@ -1147,7 +1166,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { ["unwrap", ..] => lint_unwrap(cx, expr, arg_lists[0]), ["expect", "ok"] => lint_ok_expect(cx, expr, arg_lists[1]), ["expect", ..] => lint_expect(cx, expr, arg_lists[0]), - ["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0]), + ["unwrap_or", "map"] => 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]), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index f4ebf6cbd918..1bb70f6ecfaf 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; 340] = [ +pub const ALL_LINTS: [Lint; 341] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1708,6 +1708,13 @@ pub const ALL_LINTS: [Lint; 340] = [ deprecation: None, module: "map_unit_fn", }, + Lint { + name: "result_map_unwrap_or", + group: "pedantic", + desc: "using `Result.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`", + deprecation: None, + module: "methods", + }, Lint { name: "result_map_unwrap_or_else", group: "pedantic", From 11f5d3de47a55ea93fb0f16414b08b7ce41fa321 Mon Sep 17 00:00:00 2001 From: Guanqun Lu Date: Sat, 16 Nov 2019 17:35:36 +0800 Subject: [PATCH 2/6] add unit test for result_map_unwrap_or --- tests/ui/result_map_unwrap_or.rs | 14 ++++++++++++++ tests/ui/result_map_unwrap_or.stderr | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 tests/ui/result_map_unwrap_or.rs create mode 100644 tests/ui/result_map_unwrap_or.stderr diff --git a/tests/ui/result_map_unwrap_or.rs b/tests/ui/result_map_unwrap_or.rs new file mode 100644 index 000000000000..beb0c0997a13 --- /dev/null +++ b/tests/ui/result_map_unwrap_or.rs @@ -0,0 +1,14 @@ +#![warn(clippy::result_map_unwrap_or)] + +fn main() { + let res: Result = Ok(1); + + // Check `RESULT_MAP_OR_NONE`. + // Single line case. + let _ = res.map(|x| x + 1).unwrap_or(0); + // Multi-line case. + #[rustfmt::skip] + let _ = res.map(|x| { + x + 1 + }).unwrap_or(0); +} diff --git a/tests/ui/result_map_unwrap_or.stderr b/tests/ui/result_map_unwrap_or.stderr new file mode 100644 index 000000000000..a7733a43aaff --- /dev/null +++ b/tests/ui/result_map_unwrap_or.stderr @@ -0,0 +1,20 @@ +error: called `map(f).unwrap_or(a)` on a Result value. This can be done more directly by calling `map_or(a, f)` instead + --> $DIR/result_map_unwrap_or.rs:8:13 + | +LL | let _ = res.map(|x| x + 1).unwrap_or(0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::result-map-unwrap-or` implied by `-D warnings` + = note: replace `map(|x| x + 1).unwrap_or(0)` with `map_or(0, |x| x + 1)` + +error: called `map(f).unwrap_or(a)` on a Result value. This can be done more directly by calling `map_or(a, f)` instead + --> $DIR/result_map_unwrap_or.rs:11:13 + | +LL | let _ = res.map(|x| { + | _____________^ +LL | | x + 1 +LL | | }).unwrap_or(0); + | |______________________________________^ + +error: aborting due to 2 previous errors + From b55ead5d2d435a3571e216c350b3a6ba67ebf3e0 Mon Sep 17 00:00:00 2001 From: Guanqun Lu Date: Sat, 16 Nov 2019 17:52:10 +0800 Subject: [PATCH 3/6] add documentation --- CHANGELOG.md | 1 + README.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 962f9067a4e4..0bed419aa139 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1176,6 +1176,7 @@ Released 2018-09-13 [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts [`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn +[`result_map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unwrap_or [`result_map_unwrap_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unwrap_or_else [`result_unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unwrap_used [`reverse_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#reverse_range_loop diff --git a/README.md b/README.md index 6133fa4c3a57..4106ee278fa6 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 340 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 341 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: From 0d4c7ec0721734bfa532007faaef9ca357eea371 Mon Sep 17 00:00:00 2001 From: Guanqun Lu Date: Tue, 17 Dec 2019 14:20:08 +0800 Subject: [PATCH 4/6] add a debug_assert according to the comment --- clippy_lints/src/methods/map_unwrap_or.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/clippy_lints/src/methods/map_unwrap_or.rs b/clippy_lints/src/methods/map_unwrap_or.rs index 7a19ac42e5ed..c90fba815e71 100644 --- a/clippy_lints/src/methods/map_unwrap_or.rs +++ b/clippy_lints/src/methods/map_unwrap_or.rs @@ -61,6 +61,7 @@ pub(super) fn lint<'a, 'tcx>( arg, suggest ) } else { + debug_assert!(is_result); "called `map(f).unwrap_or(a)` on a Result value. \ This can be done more directly by calling `map_or(a, f)` instead" .to_string() From 2ecaf8fb84b1cf4207a9b36c6ff4b6f4f5c6cf84 Mon Sep 17 00:00:00 2001 From: Guanqun Lu Date: Wed, 18 Dec 2019 14:01:35 +0800 Subject: [PATCH 5/6] fix code comments --- clippy_lints/src/methods/map_unwrap_or.rs | 37 +++++++---------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/methods/map_unwrap_or.rs b/clippy_lints/src/methods/map_unwrap_or.rs index c90fba815e71..c36c2acc5c53 100644 --- a/clippy_lints/src/methods/map_unwrap_or.rs +++ b/clippy_lints/src/methods/map_unwrap_or.rs @@ -48,11 +48,10 @@ pub(super) fn lint<'a, 'tcx>( let msg = if is_option { // comparing the snippet from source to raw text ("None") below is safe // because we already have checked the type. - let arg = if unwrap_snippet == "None" { "None" } else { "a" }; - let suggest = if unwrap_snippet == "None" { - "and_then(f)" + let (arg, suggest) = if unwrap_snippet == "None" { + ("None", "and_then(f)") } else { - "map_or(a, f)" + ("a", "map_or(a, f)") }; format!( @@ -67,6 +66,12 @@ pub(super) fn lint<'a, 'tcx>( .to_string() }; + let lint = if is_option { + OPTION_MAP_UNWRAP_OR + } else { + RESULT_MAP_UNWRAP_OR + }; + // lint, with note if neither arg is > 1 line and both map() and // unwrap_or() have the same span let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1; @@ -81,29 +86,9 @@ pub(super) fn lint<'a, 'tcx>( "replace `map({}).unwrap_or({})` with `{}`", map_snippet, unwrap_snippet, suggest ); - span_note_and_lint( - cx, - if is_option { - OPTION_MAP_UNWRAP_OR - } else { - RESULT_MAP_UNWRAP_OR - }, - expr.span, - &msg, - expr.span, - ¬e, - ); + span_note_and_lint(cx, lint, expr.span, &msg, expr.span, ¬e); } else if same_span && multiline { - span_lint( - cx, - if is_option { - OPTION_MAP_UNWRAP_OR - } else { - RESULT_MAP_UNWRAP_OR - }, - expr.span, - &msg, - ); + span_lint(cx, lint, expr.span, &msg); }; } } From 756e78f88795f5dee32bcb8034ed84c345ec7421 Mon Sep 17 00:00:00 2001 From: Guanqun Lu Date: Wed, 18 Dec 2019 16:49:30 +0800 Subject: [PATCH 6/6] use another name 'lint_type' to avoid the same name as the function --- clippy_lints/src/methods/map_unwrap_or.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/map_unwrap_or.rs b/clippy_lints/src/methods/map_unwrap_or.rs index c36c2acc5c53..a6b71d1c4960 100644 --- a/clippy_lints/src/methods/map_unwrap_or.rs +++ b/clippy_lints/src/methods/map_unwrap_or.rs @@ -66,7 +66,7 @@ pub(super) fn lint<'a, 'tcx>( .to_string() }; - let lint = if is_option { + let lint_type = if is_option { OPTION_MAP_UNWRAP_OR } else { RESULT_MAP_UNWRAP_OR @@ -86,9 +86,9 @@ pub(super) fn lint<'a, 'tcx>( "replace `map({}).unwrap_or({})` with `{}`", map_snippet, unwrap_snippet, suggest ); - span_note_and_lint(cx, lint, expr.span, &msg, expr.span, ¬e); + span_note_and_lint(cx, lint_type, expr.span, &msg, expr.span, ¬e); } else if same_span && multiline { - span_lint(cx, lint, expr.span, &msg); + span_lint(cx, lint_type, expr.span, &msg); }; } }