Skip to content

Commit

Permalink
Rollup merge of rust-lang#5415 - nickrtorres:master, r=flip1995
Browse files Browse the repository at this point in the history
Add new lint for `Result<T, E>.map_or(None, Some(T))`

Fixes rust-lang#5414

PR Checklist
---
- [x] Followed lint naming conventions (the name is a bit awkward, but it seems to conform)
- [x] Added passing UI tests (including committed .stderr file)
- [x] cargo test passes locally
- [x] Executed cargo dev update_lints
- [x] Added lint documentation
- [x] Run cargo dev fmt

`Result<T, E>` has an [`ok()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.ok) method that adapts a `Result<T,E>` into an `Option<T>`.
It's possible to get around this adapter by writing `Result<T,E>.map_or(None, Some)`.

This lint is implemented as a new variant of the existing [`option_map_none` lint](rust-lang/rust-clippy#2128)
  • Loading branch information
flip1995 authored Apr 8, 2020
2 parents 5ea4771 + 5d54fbb commit a1e49f9
Show file tree
Hide file tree
Showing 7 changed files with 142 additions and 16 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1448,6 +1448,7 @@ Released 2018-09-13
[`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts
[`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs
[`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used
[`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option
[`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn
[`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
Expand Down
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&methods::OPTION_UNWRAP_USED,
&methods::OR_FUN_CALL,
&methods::RESULT_EXPECT_USED,
&methods::RESULT_MAP_OR_INTO_OPTION,
&methods::RESULT_MAP_UNWRAP_OR_ELSE,
&methods::RESULT_UNWRAP_USED,
&methods::SEARCH_IS_SOME,
Expand Down Expand Up @@ -1281,6 +1282,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::OPTION_AS_REF_DEREF),
LintId::of(&methods::OPTION_MAP_OR_NONE),
LintId::of(&methods::OR_FUN_CALL),
LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION),
LintId::of(&methods::SEARCH_IS_SOME),
LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
LintId::of(&methods::SINGLE_CHAR_PATTERN),
Expand Down Expand Up @@ -1459,6 +1461,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::NEW_RET_NO_SELF),
LintId::of(&methods::OK_EXPECT),
LintId::of(&methods::OPTION_MAP_OR_NONE),
LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION),
LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
LintId::of(&methods::STRING_EXTEND_CHARS),
LintId::of(&methods::UNNECESSARY_FOLD),
Expand Down
99 changes: 83 additions & 16 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,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 `_.map_or(None, Some)`.
///
/// **Why is this bad?** Readability, this can be written more concisely as
/// `_.ok()`.
///
/// **Known problems:** None.
///
/// **Example:**
///
/// Bad:
/// ```rust
/// # let r: Result<u32, &str> = Ok(1);
/// assert_eq!(Some(1), r.map_or(None, Some));
/// ```
///
/// Good:
/// ```rust
/// # let r: Result<u32, &str> = Ok(1);
/// assert_eq!(Some(1), r.ok());
/// ```
pub RESULT_MAP_OR_INTO_OPTION,
style,
"using `Result.map_or(None, Some)`, which is more succinctly expressed as `ok()`"
}

declare_clippy_lint! {
/// **What it does:** Checks for usage of `_.and_then(|x| Some(y))`.
///
Expand Down Expand Up @@ -1249,6 +1275,7 @@ declare_lint_pass!(Methods => [
OPTION_MAP_UNWRAP_OR,
OPTION_MAP_UNWRAP_OR_ELSE,
RESULT_MAP_UNWRAP_OR_ELSE,
RESULT_MAP_OR_INTO_OPTION,
OPTION_MAP_OR_NONE,
OPTION_AND_THEN_SOME,
OR_FUN_CALL,
Expand Down Expand Up @@ -2524,38 +2551,78 @@ fn lint_map_unwrap_or_else<'a, 'tcx>(
}
}

/// lint use of `_.map_or(None, _)` for `Option`s
/// lint use of `_.map_or(None, _)` for `Option`s and `Result`s
fn lint_map_or_none<'a, 'tcx>(
cx: &LateContext<'a, 'tcx>,
expr: &'tcx hir::Expr<'_>,
map_or_args: &'tcx [hir::Expr<'_>],
) {
if match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::OPTION) {
// check if the first non-self argument to map_or() is None
let map_or_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind {
let is_option = match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::OPTION);
let is_result = match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::RESULT);

// There are two variants of this `map_or` lint:
// (1) using `map_or` as an adapter from `Result<T,E>` to `Option<T>`
// (2) using `map_or` as a combinator instead of `and_then`
//
// (For this lint) we don't care if any other type calls `map_or`
if !is_option && !is_result {
return;
}

let (lint_name, msg, instead, hint) = {
let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind {
match_qpath(qpath, &paths::OPTION_NONE)
} else {
return;
};

if !default_arg_is_none {
// nothing to lint!
return;
}

let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_or_args[2].kind {
match_qpath(qpath, &paths::OPTION_SOME)
} else {
false
};

if map_or_arg_is_none {
// lint message
if is_option {
let self_snippet = snippet(cx, map_or_args[0].span, "..");
let func_snippet = snippet(cx, map_or_args[2].span, "..");
let msg = "called `map_or(None, f)` on an `Option` value. This can be done more directly by calling \
`and_then(f)` instead";
let map_or_self_snippet = snippet(cx, map_or_args[0].span, "..");
let map_or_func_snippet = snippet(cx, map_or_args[2].span, "..");
let hint = format!("{0}.and_then({1})", map_or_self_snippet, map_or_func_snippet);
span_lint_and_sugg(
cx,
(
OPTION_MAP_OR_NONE,
expr.span,
msg,
"try using `and_then` instead",
hint,
Applicability::MachineApplicable,
);
format!("{0}.and_then({1})", self_snippet, func_snippet),
)
} else if f_arg_is_some {
let msg = "called `map_or(None, Some)` on a `Result` value. This can be done more directly by calling \
`ok()` instead";
let self_snippet = snippet(cx, map_or_args[0].span, "..");
(
RESULT_MAP_OR_INTO_OPTION,
msg,
"try using `ok` instead",
format!("{0}.ok()", self_snippet),
)
} else {
// nothing to lint!
return;
}
}
};

span_lint_and_sugg(
cx,
lint_name,
expr.span,
msg,
instead,
hint,
Applicability::MachineApplicable,
);
}

/// Lint use of `_.and_then(|x| Some(y))` for `Option`s
Expand Down
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1823,6 +1823,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "methods",
},
Lint {
name: "result_map_or_into_option",
group: "style",
desc: "using `Result.map_or(None, Some)`, which is more succinctly expressed as `ok()`",
deprecation: None,
module: "methods",
},
Lint {
name: "result_map_unit_fn",
group: "complexity",
Expand Down
19 changes: 19 additions & 0 deletions tests/ui/result_map_or_into_option.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// run-rustfix

#![warn(clippy::result_map_or_into_option)]

fn main() {
let opt: Result<u32, &str> = Ok(1);
let _ = opt.ok();

let rewrap = |s: u32| -> Option<u32> { Some(s) };

// A non-Some `f` arg should not emit the lint
let opt: Result<u32, &str> = Ok(1);
let _ = opt.map_or(None, rewrap);

// A non-Some `f` closure where the argument is not used as the
// return should not emit the lint
let opt: Result<u32, &str> = Ok(1);
opt.map_or(None, |_x| Some(1));
}
19 changes: 19 additions & 0 deletions tests/ui/result_map_or_into_option.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// run-rustfix

#![warn(clippy::result_map_or_into_option)]

fn main() {
let opt: Result<u32, &str> = Ok(1);
let _ = opt.map_or(None, Some);

let rewrap = |s: u32| -> Option<u32> { Some(s) };

// A non-Some `f` arg should not emit the lint
let opt: Result<u32, &str> = Ok(1);
let _ = opt.map_or(None, rewrap);

// A non-Some `f` closure where the argument is not used as the
// return should not emit the lint
let opt: Result<u32, &str> = Ok(1);
opt.map_or(None, |_x| Some(1));
}
10 changes: 10 additions & 0 deletions tests/ui/result_map_or_into_option.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: called `map_or(None, Some)` on a `Result` value. This can be done more directly by calling `ok()` instead
--> $DIR/result_map_or_into_option.rs:7:13
|
LL | let _ = opt.map_or(None, Some);
| ^^^^^^^^^^^^^^^^^^^^^^ help: try using `ok` instead: `opt.ok()`
|
= note: `-D clippy::result-map-or-into-option` implied by `-D warnings`

error: aborting due to previous error

0 comments on commit a1e49f9

Please sign in to comment.