Skip to content

Commit

Permalink
Auto merge of #7971 - togami2864:fix/option-map-or-none, r=llogiq
Browse files Browse the repository at this point in the history 
fix suggestion in option_map_or_none

fix: #7960
changelog: change suggestion in the lint rule `option_map_or_none`
  • Loading branch information
@bors
bors committed Nov 17, 2021
2 parents d550e5f + 8e317f5 commit 6ac42fe
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 69 deletions.
139 changes: 89 additions & 50 deletions clippy_lints/src/methods/option_map_or_none.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::is_lang_ctor;
use clippy_utils::source::snippet;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{is_lang_ctor, single_segment_path};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::LangItem::{OptionNone, OptionSome};
Expand All @@ -11,6 +11,28 @@ use rustc_span::symbol::sym;
use super::OPTION_MAP_OR_NONE;
use super::RESULT_MAP_OR_INTO_OPTION;

// The expression inside a closure may or may not have surrounding braces
// which causes problems when generating a suggestion.
fn reduce_unit_expression<'a>(
cx: &LateContext<'_>,
expr: &'a hir::Expr<'_>,
) -> Option<(&'a hir::Expr<'a>, &'a [hir::Expr<'a>])> {
match expr.kind {
hir::ExprKind::Call(func, arg_char) => Some((func, arg_char)),
hir::ExprKind::Block(block, _) => {
match (block.stmts, block.expr) {
(&[], Some(inner_expr)) => {
// If block only contains an expression,
// reduce `|x| { x + 1 }` to `|x| x + 1`
reduce_unit_expression(cx, inner_expr)
},
_ => None,
}
},
_ => None,
}
}

/// lint use of `_.map_or(None, _)` for `Option`s and `Result`s
pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
Expand All @@ -31,58 +53,75 @@ pub(super) fn check<'tcx>(
return;
}

let (lint_name, msg, instead, hint) = {
let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = def_arg.kind {
is_lang_ctor(cx, qpath, OptionNone)
} else {
return;
};
let default_arg_is_none = if let hir::ExprKind::Path(ref qpath) = def_arg.kind {
is_lang_ctor(cx, qpath, OptionNone)
} else {
return;
};

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

let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_arg.kind {
is_lang_ctor(cx, qpath, OptionSome)
} else {
false
};
let f_arg_is_some = if let hir::ExprKind::Path(ref qpath) = map_arg.kind {
is_lang_ctor(cx, qpath, OptionSome)
} else {
false
};

if is_option {
let self_snippet = snippet(cx, recv.span, "..");
if_chain! {
if let hir::ExprKind::Closure(_, _, id, span, _) = map_arg.kind;
let arg_snippet = snippet(cx, span, "..");
let body = cx.tcx.hir().body(id);
if let Some((func, arg_char)) = reduce_unit_expression(cx, &body.value);
if arg_char.len() == 1;
if let hir::ExprKind::Path(ref qpath) = func.kind;
if let Some(segment) = single_segment_path(qpath);
if segment.ident.name == sym::Some;
then {
let func_snippet = snippet(cx, arg_char[0].span, "..");
let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
`map(..)` instead";
return span_lint_and_sugg(
cx,
OPTION_MAP_OR_NONE,
expr.span,
msg,
"try using `map` instead",
format!("{0}.map({1} {2})", self_snippet, arg_snippet,func_snippet),
Applicability::MachineApplicable,
);
}

if is_option {
let self_snippet = snippet(cx, recv.span, "..");
let func_snippet = snippet(cx, map_arg.span, "..");
let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
`and_then(..)` instead";
(
OPTION_MAP_OR_NONE,
msg,
"try using `and_then` instead",
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, recv.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,
);
let func_snippet = snippet(cx, map_arg.span, "..");
let msg = "called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling \
`and_then(..)` instead";
return span_lint_and_sugg(
cx,
OPTION_MAP_OR_NONE,
expr.span,
msg,
"try using `and_then` instead",
format!("{0}.and_then({1})", self_snippet, func_snippet),
Applicability::MachineApplicable,
);
} 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, recv.span, "..");
return span_lint_and_sugg(
cx,
RESULT_MAP_OR_INTO_OPTION,
expr.span,
msg,
"try using `ok` instead",
format!("{0}.ok()", self_snippet),
Applicability::MachineApplicable,
);
}
}
14 changes: 10 additions & 4 deletions tests/ui/option_map_or_none.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,19 @@

fn main() {
let opt = Some(1);
let bar = |_| Some(1);

// Check `OPTION_MAP_OR_NONE`.
// Single line case.
let _ = opt.and_then(|x| Some(x + 1));
let _: Option<i32> = opt.map(|x| x + 1);
// Multi-line case.
#[rustfmt::skip]
let _ = opt.and_then(|x| {
Some(x + 1)
});
let _: Option<i32> = opt.map(|x| x + 1);
// function returning `Option`
let _: Option<i32> = opt.and_then(bar);
let _: Option<i32> = opt.and_then(|x| {
let offset = 0;
let height = x;
Some(offset + height)
});
}
12 changes: 10 additions & 2 deletions tests/ui/option_map_or_none.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,21 @@

fn main() {
let opt = Some(1);
let bar = |_| Some(1);

// Check `OPTION_MAP_OR_NONE`.
// Single line case.
let _ = opt.map_or(None, |x| Some(x + 1));
let _: Option<i32> = opt.map_or(None, |x| Some(x + 1));
// Multi-line case.
#[rustfmt::skip]
let _ = opt.map_or(None, |x| {
let _: Option<i32> = opt.map_or(None, |x| {
Some(x + 1)
});
// function returning `Option`
let _: Option<i32> = opt.map_or(None, bar);
let _: Option<i32> = opt.map_or(None, |x| {
let offset = 0;
let height = x;
Some(offset + height)
});
}
45 changes: 32 additions & 13 deletions tests/ui/option_map_or_none.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,26 +1,45 @@
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
--> $DIR/option_map_or_none.rs:10:13
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead
--> $DIR/option_map_or_none.rs:11:26
|
LL | let _ = opt.map_or(None, |x| Some(x + 1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(|x| Some(x + 1))`
LL | let _: Option<i32> = opt.map_or(None, |x| Some(x + 1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `map` instead: `opt.map(|x| x + 1)`
|
= note: `-D clippy::option-map-or-none` implied by `-D warnings`

error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
--> $DIR/option_map_or_none.rs:13:13
error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `map(..)` instead
--> $DIR/option_map_or_none.rs:14:26
|
LL | let _ = opt.map_or(None, |x| {
| _____________^
LL | let _: Option<i32> = opt.map_or(None, |x| {
| __________________________^
LL | | Some(x + 1)
LL | | });
| |_________________________^
| |_________________________^ help: try using `map` instead: `opt.map(|x| x + 1)`

error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
--> $DIR/option_map_or_none.rs:18:26
|
LL | let _: Option<i32> = opt.map_or(None, bar);
| ^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `opt.and_then(bar)`

error: called `map_or(None, ..)` on an `Option` value. This can be done more directly by calling `and_then(..)` instead
--> $DIR/option_map_or_none.rs:19:26
|
LL | let _: Option<i32> = opt.map_or(None, |x| {
| __________________________^
LL | | let offset = 0;
LL | | let height = x;
LL | | Some(offset + height)
LL | | });
| |______^
|
help: try using `and_then` instead
|
LL ~ let _ = opt.and_then(|x| {
LL + Some(x + 1)
LL ~ });
LL ~ let _: Option<i32> = opt.and_then(|x| {
LL + let offset = 0;
LL + let height = x;
LL + Some(offset + height)
LL ~ });
|

error: aborting due to 2 previous errors
error: aborting due to 4 previous errors

0 comments on commit 6ac42fe

Please sign in to comment.