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: 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 71% rename from clippy_lints/src/methods/option_map_unwrap_or.rs rename to clippy_lints/src/methods/map_unwrap_or.rs index 769392a6fd8f..a6b71d1c4960 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,33 @@ 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, suggest) = if unwrap_snippet == "None" { + ("None", "and_then(f)") + } else { + ("a", "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)" + 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() }; - let msg = &format!( - "called `map(f).unwrap_or({})` on an Option value. \ - This can be done more directly by calling `{}` instead", - arg, suggest - ); + + let lint_type = 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; @@ -70,9 +86,9 @@ 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, lint_type, expr.span, &msg, expr.span, ¬e); } else if same_span && multiline { - span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg); + span_lint(cx, lint_type, 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", 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 +