result_map_or_into_option: add lint to catch manually adpating Result -> Option

Result<T, E> has an `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](https://github.com/rust-lang/rust-clippy/pull/2128)
This commit is contained in:
Nick Torres 2020-04-03 23:59:52 -07:00
parent 7907abea27
commit 91d8a804d3
7 changed files with 131 additions and 25 deletions

View file

@ -330,6 +330,24 @@ 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:**
///
/// **Example:**
/// ```rust
/// # let opt = Some(1);
/// # let r = opt.map_or(None, Some);
/// ```
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))`.
///
@ -1248,6 +1266,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,
@ -2517,37 +2536,73 @@ 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 {
match_qpath(qpath, &paths::OPTION_NONE)
} else {
false
};
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);
if map_or_arg_is_none {
// lint message
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,
);
}
// 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 default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = map_or_args[1].kind {
match_qpath(qpath, &paths::OPTION_NONE)
} else {
false
};
// This is really only needed if `is_result` holds. Computing it here
// makes `mess`'s assignment a bit easier, so just compute it here.
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
};
let mess = if is_option && default_arg_is_none {
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";
Some((
OPTION_MAP_OR_NONE,
msg,
"try using `and_then` instead",
format!("{0}.and_then({1})", self_snippet, func_snippet),
))
} else if is_result && 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, "..");
Some((
RESULT_MAP_OR_INTO_OPTION,
msg,
"try using `ok` instead",
format!("{0}.ok()", self_snippet),
))
} else {
None
};
if let Some((lint, msg, instead, hint)) = mess {
span_lint_and_sugg(
cx,
lint,
expr.span,
msg,
instead,
hint,
Applicability::MachineApplicable,
);
}
}