OpenE2K/rust
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Auto merge of #5529 - alex-700:improve-option-and-then-some-lint, r=phansch

Browse Source 
Improve `option_and_then_some` lint

fixed #5492

changelog: Improve and generalize `option_and_then_some` and rename it to `bind_instead_of_map`.
This commit is contained in:
bors 2020-05-17 10:58:56 +00:00
commit 6ae0643d1a
20 changed files with 523 additions and 125 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
CHANGELOG.md
View File

@ -315,7 +315,7 @@ Released 2019-11-07
* [`missing_safety_doc`] [#4535](https://github.com/rust-lang/rust-clippy/pull/4535)
* [`mem_replace_with_uninit`] [#4511](https://github.com/rust-lang/rust-clippy/pull/4511)
* [`suspicious_map`] [#4394](https://github.com/rust-lang/rust-clippy/pull/4394)
* [`option_and_then_some`] [#4386](https://github.com/rust-lang/rust-clippy/pull/4386)
* `option_and_then_some` [#4386](https://github.com/rust-lang/rust-clippy/pull/4386)
* [`manual_saturating_arithmetic`] [#4498](https://github.com/rust-lang/rust-clippy/pull/4498)
* Deprecate `unused_collect` lint. This is fully covered by rustc's `#[must_use]` on `collect` [#4348](https://github.com/rust-lang/rust-clippy/pull/4348)
* Move `type_repetition_in_bounds` to pedantic group [#4403](https://github.com/rust-lang/rust-clippy/pull/4403)
@ -1273,6 +1273,7 @@ Released 2018-09-13
[`assign_ops`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_ops
[`await_holding_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_lock
[`bad_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#bad_bit_mask
[`bind_instead_of_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#bind_instead_of_map
[`blacklisted_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name
[`blocks_in_if_conditions`]: https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_if_conditions
[`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison
@ -1494,7 +1495,6 @@ Released 2018-09-13
[`not_unsafe_ptr_arg_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref
[`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect
[`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
[`option_and_then_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_and_then_some
[`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref
[`option_env_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_env_unwrap
[`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none

2
clippy_lints/src/eq_op.rs
View File

@ -115,7 +115,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for EqOp {
let rsnip = snippet(cx, r.span, "...").to_string();
multispan_sugg(
diag,
"use the values directly".to_string(),
"use the values directly",
vec![(left.span, lsnip), (right.span, rsnip)],
);
},

7
clippy_lints/src/lib.rs
View File

@ -650,6 +650,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&mem_replace::MEM_REPLACE_OPTION_WITH_NONE,
&mem_replace::MEM_REPLACE_WITH_DEFAULT,
&mem_replace::MEM_REPLACE_WITH_UNINIT,
&methods::BIND_INSTEAD_OF_MAP,
&methods::CHARS_LAST_CMP,
&methods::CHARS_NEXT_CMP,
&methods::CLONE_DOUBLE_REF,
@ -676,7 +677,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&methods::MAP_UNWRAP_OR,
&methods::NEW_RET_NO_SELF,
&methods::OK_EXPECT,
&methods::OPTION_AND_THEN_SOME,
&methods::OPTION_AS_REF_DEREF,
&methods::OPTION_MAP_OR_NONE,
&methods::OR_FUN_CALL,
@ -1291,6 +1291,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&mem_replace::MEM_REPLACE_OPTION_WITH_NONE),
LintId::of(&mem_replace::MEM_REPLACE_WITH_DEFAULT),
LintId::of(&mem_replace::MEM_REPLACE_WITH_UNINIT),
LintId::of(&methods::BIND_INSTEAD_OF_MAP),
LintId::of(&methods::CHARS_LAST_CMP),
LintId::of(&methods::CHARS_NEXT_CMP),
LintId::of(&methods::CLONE_DOUBLE_REF),
@ -1307,7 +1308,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::MANUAL_SATURATING_ARITHMETIC),
LintId::of(&methods::NEW_RET_NO_SELF),
LintId::of(&methods::OK_EXPECT),
LintId::of(&methods::OPTION_AND_THEN_SOME),
LintId::of(&methods::OPTION_AS_REF_DEREF),
LintId::of(&methods::OPTION_MAP_OR_NONE),
LintId::of(&methods::OR_FUN_CALL),
@ -1559,10 +1559,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&matches::MATCH_AS_REF),
LintId::of(&matches::MATCH_SINGLE_BINDING),
LintId::of(&matches::WILDCARD_IN_OR_PATTERNS),
LintId::of(&methods::BIND_INSTEAD_OF_MAP),
LintId::of(&methods::CLONE_ON_COPY),
LintId::of(&methods::FILTER_NEXT),
LintId::of(&methods::FLAT_MAP_IDENTITY),
LintId::of(&methods::OPTION_AND_THEN_SOME),
LintId::of(&methods::OPTION_AS_REF_DEREF),
LintId::of(&methods::SEARCH_IS_SOME),
LintId::of(&methods::SKIP_WHILE_NEXT),
@ -1784,6 +1784,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) {
ls.register_renamed("clippy::new_without_default_derive", "clippy::new_without_default");
ls.register_renamed("clippy::cyclomatic_complexity", "clippy::cognitive_complexity");
ls.register_renamed("clippy::const_static_lifetime", "clippy::redundant_static_lifetimes");
ls.register_renamed("clippy::option_and_then_some", "clippy::bind_instead_of_map");
ls.register_renamed("clippy::block_in_if_condition_expr", "clippy::blocks_in_if_conditions");
ls.register_renamed("clippy::block_in_if_condition_stmt", "clippy::blocks_in_if_conditions");
ls.register_renamed("clippy::option_map_unwrap_or", "clippy::map_unwrap_or");

6
clippy_lints/src/loops.rs
View File

@ -1134,7 +1134,7 @@ fn check_for_loop_range<'a, 'tcx>(
|diag| {
multispan_sugg(
diag,
"consider using an iterator".to_string(),
"consider using an iterator",
vec![
(pat.span, format!("({}, <item>)", ident.name)),
(
@ -1163,7 +1163,7 @@ fn check_for_loop_range<'a, 'tcx>(
|diag| {
multispan_sugg(
diag,
"consider using an iterator".to_string(),
"consider using an iterator",
vec![(pat.span, "<item>".to_string()), (arg.span, repl)],
);
},
@ -1462,7 +1462,7 @@ fn check_for_loop_over_map_kv<'a, 'tcx>(
let map = sugg::Sugg::hir(cx, arg, "map");
multispan_sugg(
diag,
"use the corresponding method".into(),
"use the corresponding method",
vec![
(pat_span, snippet(cx, new_pat_span, kind).into_owned()),
(arg_span, format!("{}.{}s{}()", map.maybe_par(), kind, mutbl)),

2
clippy_lints/src/matches.rs
View File

@ -820,7 +820,7 @@ fn check_match_ref_pats(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>
span_lint_and_then(cx, MATCH_REF_PATS, expr.span, title, |diag| {
if !expr.span.from_expansion() {
multispan_sugg(diag, msg.to_owned(), suggs);
multispan_sugg(diag, msg, suggs);
}
});
}

309
clippy_lints/src/methods/bind_instead_of_map.rs Normal file
View File

@ -0,0 +1,309 @@
use super::{contains_return, BIND_INSTEAD_OF_MAP};
use crate::utils::{
in_macro, match_qpath, match_type, method_calls, multispan_sugg_with_applicability, paths, remove_blocks, snippet,
snippet_with_macro_callsite, span_lint_and_sugg, span_lint_and_then,
};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::intravisit::{self, Visitor};
use rustc_lint::LateContext;
use rustc_middle::hir::map::Map;
use rustc_span::Span;
pub(crate) struct OptionAndThenSome;
impl BindInsteadOfMap for OptionAndThenSome {
const TYPE_NAME: &'static str = "Option";
const TYPE_QPATH: &'static [&'static str] = &paths::OPTION;
const BAD_METHOD_NAME: &'static str = "and_then";
const BAD_VARIANT_NAME: &'static str = "Some";
const BAD_VARIANT_QPATH: &'static [&'static str] = &paths::OPTION_SOME;
const GOOD_METHOD_NAME: &'static str = "map";
}
pub(crate) struct ResultAndThenOk;
impl BindInsteadOfMap for ResultAndThenOk {
const TYPE_NAME: &'static str = "Result";
const TYPE_QPATH: &'static [&'static str] = &paths::RESULT;
const BAD_METHOD_NAME: &'static str = "and_then";
const BAD_VARIANT_NAME: &'static str = "Ok";
const BAD_VARIANT_QPATH: &'static [&'static str] = &paths::RESULT_OK;
const GOOD_METHOD_NAME: &'static str = "map";
}
pub(crate) struct ResultOrElseErrInfo;
impl BindInsteadOfMap for ResultOrElseErrInfo {
const TYPE_NAME: &'static str = "Result";
const TYPE_QPATH: &'static [&'static str] = &paths::RESULT;
const BAD_METHOD_NAME: &'static str = "or_else";
const BAD_VARIANT_NAME: &'static str = "Err";
const BAD_VARIANT_QPATH: &'static [&'static str] = &paths::RESULT_ERR;
const GOOD_METHOD_NAME: &'static str = "map_err";
}
pub(crate) trait BindInsteadOfMap {
const TYPE_NAME: &'static str;
const TYPE_QPATH: &'static [&'static str];
const BAD_METHOD_NAME: &'static str;
const BAD_VARIANT_NAME: &'static str;
const BAD_VARIANT_QPATH: &'static [&'static str];
const GOOD_METHOD_NAME: &'static str;
fn no_op_msg() -> String {
format!(
"using `{}.{}({})`, which is a no-op",
Self::TYPE_NAME,
Self::BAD_METHOD_NAME,
Self::BAD_VARIANT_NAME
)
}
fn lint_msg() -> String {
format!(
"using `{}.{}(|x| {}(y))`, which is more succinctly expressed as `{}(|x| y)`",
Self::TYPE_NAME,
Self::BAD_METHOD_NAME,
Self::BAD_VARIANT_NAME,
Self::GOOD_METHOD_NAME
)
}
fn lint_closure_autofixable(
cx: &LateContext<'_, '_>,
expr: &hir::Expr<'_>,
args: &[hir::Expr<'_>],
closure_expr: &hir::Expr<'_>,
closure_args_span: Span,
) -> bool {
if_chain! {
if let hir::ExprKind::Call(ref some_expr, ref some_args) = closure_expr.kind;
if let hir::ExprKind::Path(ref qpath) = some_expr.kind;
if match_qpath(qpath, Self::BAD_VARIANT_QPATH);
if some_args.len() == 1;
then {
let inner_expr = &some_args[0];
if contains_return(inner_expr) {
return false;
}
let some_inner_snip = if inner_expr.span.from_expansion() {
snippet_with_macro_callsite(cx, inner_expr.span, "_")
} else {
snippet(cx, inner_expr.span, "_")
};
let closure_args_snip = snippet(cx, closure_args_span, "..");
let option_snip = snippet(cx, args[0].span, "..");
let note = format!("{}.{}({} {})", option_snip, Self::GOOD_METHOD_NAME, closure_args_snip, some_inner_snip);
span_lint_and_sugg(
cx,
BIND_INSTEAD_OF_MAP,
expr.span,
Self::lint_msg().as_ref(),
"try this",
note,
Applicability::MachineApplicable,
);
true
} else {
false
}
}
}
fn lint_closure(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, closure_expr: &hir::Expr<'_>) {
let mut suggs = Vec::new();
let can_sugg = find_all_ret_expressions(cx, closure_expr, |ret_expr| {
if_chain! {
if !in_macro(ret_expr.span);
if let hir::ExprKind::Call(ref func_path, ref args) = ret_expr.kind;
if let hir::ExprKind::Path(ref qpath) = func_path.kind;
if match_qpath(qpath, Self::BAD_VARIANT_QPATH);
if args.len() == 1;
if !contains_return(&args[0]);
then {
suggs.push((ret_expr.span, args[0].span.source_callsite()));
true
} else {
false
}
}
});
if can_sugg {
span_lint_and_then(cx, BIND_INSTEAD_OF_MAP, expr.span, Self::lint_msg().as_ref(), |diag| {
multispan_sugg_with_applicability(
diag,
"try this",
Applicability::MachineApplicable,
std::iter::once((*method_calls(expr, 1).2.get(0).unwrap(), Self::GOOD_METHOD_NAME.into())).chain(
suggs
.into_iter()
.map(|(span1, span2)| (span1, snippet(cx, span2, "_").into())),
),
)
});
}
}
/// Lint use of `_.and_then(|x| Some(y))` for `Option`s
fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
if !match_type(cx, cx.tables.expr_ty(&args[0]), Self::TYPE_QPATH) {
return;
}
match args[1].kind {
hir::ExprKind::Closure(_, _, body_id, closure_args_span, _) => {
let closure_body = cx.tcx.hir().body(body_id);
let closure_expr = remove_blocks(&closure_body.value);
if !Self::lint_closure_autofixable(cx, expr, args, closure_expr, closure_args_span) {
Self::lint_closure(cx, expr, closure_expr);
}
},
// `_.and_then(Some)` case, which is no-op.
hir::ExprKind::Path(ref qpath) if match_qpath(qpath, Self::BAD_VARIANT_QPATH) => {
span_lint_and_sugg(
cx,
BIND_INSTEAD_OF_MAP,
expr.span,
Self::no_op_msg().as_ref(),
"use the expression directly",
snippet(cx, args[0].span, "..").into(),
Applicability::MachineApplicable,
);
},
_ => {},
}
}
}
/// returns `true` if expr contains match expr desugared from try
fn contains_try(expr: &hir::Expr<'_>) -> bool {
struct TryFinder {
found: bool,
}
impl<'hir> intravisit::Visitor<'hir> for TryFinder {
type Map = Map<'hir>;
fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> {
intravisit::NestedVisitorMap::None
}
fn visit_expr(&mut self, expr: &'hir hir::Expr<'hir>) {
if self.found {
return;
}
match expr.kind {
hir::ExprKind::Match(_, _, hir::MatchSource::TryDesugar) => self.found = true,
_ => intravisit::walk_expr(self, expr),
}
}
}
let mut visitor = TryFinder { found: false };
visitor.visit_expr(expr);
visitor.found
}
fn find_all_ret_expressions<'hir, F>(_cx: &LateContext<'_, '_>, expr: &'hir hir::Expr<'hir>, callback: F) -> bool
where
F: FnMut(&'hir hir::Expr<'hir>) -> bool,
{
struct RetFinder<F> {
in_stmt: bool,
failed: bool,
cb: F,
}
struct WithStmtGuarg<'a, F> {
val: &'a mut RetFinder<F>,
prev_in_stmt: bool,
}
impl<F> RetFinder<F> {
fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuarg<'_, F> {
let prev_in_stmt = std::mem::replace(&mut self.in_stmt, in_stmt);
WithStmtGuarg {
val: self,
prev_in_stmt,
}
}
}
impl<F> std::ops::Deref for WithStmtGuarg<'_, F> {
type Target = RetFinder<F>;
fn deref(&self) -> &Self::Target {
self.val
}
}
impl<F> std::ops::DerefMut for WithStmtGuarg<'_, F> {
fn deref_mut(&mut self) -> &mut Self::Target {
self.val
}
}
impl<F> Drop for WithStmtGuarg<'_, F> {
fn drop(&mut self) {
self.val.in_stmt = self.prev_in_stmt;
}
}
impl<'hir, F: FnMut(&'hir hir::Expr<'hir>) -> bool> intravisit::Visitor<'hir> for RetFinder<F> {
type Map = Map<'hir>;
fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> {
intravisit::NestedVisitorMap::None
}
fn visit_stmt(&mut self, stmt: &'hir hir::Stmt<'_>) {
intravisit::walk_stmt(&mut *self.inside_stmt(true), stmt)
}
fn visit_expr(&mut self, expr: &'hir hir::Expr<'_>) {
if self.failed {
return;
}
if self.in_stmt {
match expr.kind {
hir::ExprKind::Ret(Some(expr)) => self.inside_stmt(false).visit_expr(expr),
_ => intravisit::walk_expr(self, expr),
}
} else {
match expr.kind {
hir::ExprKind::Match(cond, arms, _) => {
self.inside_stmt(true).visit_expr(cond);
for arm in arms {
self.visit_expr(arm.body);
}
},
hir::ExprKind::Block(..) => intravisit::walk_expr(self, expr),
hir::ExprKind::Ret(Some(expr)) => self.visit_expr(expr),
_ => self.failed |= !(self.cb)(expr),
}
}
}
}
!contains_try(expr) && {
let mut ret_finder = RetFinder {
in_stmt: false,
failed: false,
cb: callback,
};
ret_finder.visit_expr(expr);
!ret_finder.failed
}
}

100
clippy_lints/src/methods/mod.rs
View File

@ -1,3 +1,4 @@
mod bind_instead_of_map;
mod inefficient_to_string;
mod manual_saturating_arithmetic;
mod option_map_unwrap_or;
@ -7,6 +8,7 @@ use std::borrow::Cow;
use std::fmt;
use std::iter;
use bind_instead_of_map::BindInsteadOfMap;
use if_chain::if_chain;
use rustc_ast::ast;
use rustc_errors::Applicability;
@ -306,27 +308,34 @@ declare_clippy_lint! {
}
declare_clippy_lint! {
/// **What it does:** Checks for usage of `_.and_then(|x| Some(y))`.
/// **What it does:** Checks for usage of `_.and_then(|x| Some(y))`, `_.and_then(|x| Ok(y))` or
/// `_.or_else(|x| Err(y))`.
///
/// **Why is this bad?** Readability, this can be written more concisely as
/// `_.map(|x| y)`.
/// `_.map(|x| y)` or `_.map_err(|x| y)`.
///
/// **Known problems:** None
///
/// **Example:**
///
/// ```rust
/// let x = Some("foo");
/// let _ = x.and_then(|s| Some(s.len()));
/// # fn opt() -> Option<&'static str> { Some("42") }
/// # fn res() -> Result<&'static str, &'static str> { Ok("42") }
/// let _ = opt().and_then(|s| Some(s.len()));
/// let _ = res().and_then(|s| if s.len() == 42 { Ok(10) } else { Ok(20) });
/// let _ = res().or_else(|s| if s.len() == 42 { Err(10) } else { Err(20) });
/// ```
///
/// The correct use would be:
///
/// ```rust
/// let x = Some("foo");
/// let _ = x.map(|s| s.len());
/// # fn opt() -> Option<&'static str> { Some("42") }
/// # fn res() -> Result<&'static str, &'static str> { Ok("42") }
/// let _ = opt().map(|s| s.len());
/// let _ = res().map(|s| if s.len() == 42 { 10 } else { 20 });
/// let _ = res().map_err(|s| if s.len() == 42 { 10 } else { 20 });
/// ```
pub OPTION_AND_THEN_SOME,
pub BIND_INSTEAD_OF_MAP,
complexity,
"using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`"
}
@ -1243,7 +1252,7 @@ declare_lint_pass!(Methods => [
MAP_UNWRAP_OR,
RESULT_MAP_OR_INTO_OPTION,
OPTION_MAP_OR_NONE,
OPTION_AND_THEN_SOME,
BIND_INSTEAD_OF_MAP,
OR_FUN_CALL,
EXPECT_FUN_CALL,
CHARS_NEXT_CMP,
@ -1302,7 +1311,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0], method_spans[1]),
["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]),
["and_then", ..] => {
bind_instead_of_map::OptionAndThenSome::lint(cx, expr, arg_lists[0]);
bind_instead_of_map::ResultAndThenOk::lint(cx, expr, arg_lists[0]);
},
["or_else", ..] => {
bind_instead_of_map::ResultOrElseErrInfo::lint(cx, expr, arg_lists[0]);
},
["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
["next", "skip_while"] => lint_skip_while_next(cx, expr, arg_lists[1]),
["map", "filter"] => lint_filter_map(cx, expr, arg_lists[1], arg_lists[0]),
@ -2601,73 +2616,6 @@ fn lint_map_or_none<'a, 'tcx>(
);
}
/// Lint use of `_.and_then(|x| Some(y))` for `Option`s
fn lint_option_and_then_some(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
const LINT_MSG: &str = "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`";
const NO_OP_MSG: &str = "using `Option.and_then(Some)`, which is a no-op";
let ty = cx.tables.expr_ty(&args[0]);
if !is_type_diagnostic_item(cx, ty, sym!(option_type)) {
return;
}
match args[1].kind {
hir::ExprKind::Closure(_, _, body_id, closure_args_span, _) => {
let closure_body = cx.tcx.hir().body(body_id);
let closure_expr = remove_blocks(&closure_body.value);
if_chain! {
if let hir::ExprKind::Call(ref some_expr, ref some_args) = closure_expr.kind;
if let hir::ExprKind::Path(ref qpath) = some_expr.kind;
if match_qpath(qpath, &paths::OPTION_SOME);
if some_args.len() == 1;
then {
let inner_expr = &some_args[0];
if contains_return(inner_expr) {
return;
}
let some_inner_snip = if inner_expr.span.from_expansion() {
snippet_with_macro_callsite(cx, inner_expr.span, "_")
} else {
snippet(cx, inner_expr.span, "_")
};
let closure_args_snip = snippet(cx, closure_args_span, "..");
let option_snip = snippet(cx, args[0].span, "..");
let note = format!("{}.map({} {})", option_snip, closure_args_snip, some_inner_snip);
span_lint_and_sugg(
cx,
OPTION_AND_THEN_SOME,
expr.span,
LINT_MSG,
"try this",
note,
Applicability::MachineApplicable,
);
}
}
},
// `_.and_then(Some)` case, which is no-op.
hir::ExprKind::Path(ref qpath) => {
if match_qpath(qpath, &paths::OPTION_SOME) {
let option_snip = snippet(cx, args[0].span, "..");
let note = format!("{}", option_snip);
span_lint_and_sugg(
cx,
OPTION_AND_THEN_SOME,
expr.span,
NO_OP_MSG,
"use the expression directly",
note,
Applicability::MachineApplicable,
);
}
},
_ => {},
}
}
/// lint use of `filter().next()` for `Iterators`
fn lint_filter_next<'a, 'tcx>(
cx: &LateContext<'a, 'tcx>,

2
clippy_lints/src/needless_pass_by_value.rs
View File

@ -293,7 +293,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
);
spans.sort_by_key(|&(span, _)| span);
}
multispan_sugg(diag, "consider taking a reference instead".to_string(), spans);
multispan_sugg(diag, "consider taking a reference instead", spans);
};
span_lint_and_then(

4
clippy_lints/src/types.rs
View File

@ -2206,7 +2206,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitHasher {
multispan_sugg(
diag,
"consider adding a type parameter".to_string(),
"consider adding a type parameter",
vec![
(
generics_suggestion_span,
@ -2230,7 +2230,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitHasher {
);
if !vis.suggestions.is_empty() {
multispan_sugg(diag, "...and use generic constructor".into(), vis.suggestions);
multispan_sugg(diag, "...and use generic constructor", vis.suggestions);
}
}

28
clippy_lints/src/utils/diagnostics.rs
View File

@ -1,6 +1,6 @@
//! Clippy wrappers around rustc's diagnostic functions.
use rustc_errors::{Applicability, CodeSuggestion, DiagnosticBuilder, Substitution, SubstitutionPart, SuggestionStyle};
use rustc_errors::{Applicability, DiagnosticBuilder};
use rustc_hir::HirId;
use rustc_lint::{LateContext, Lint, LintContext};
use rustc_span::source_map::{MultiSpan, Span};
@ -198,20 +198,20 @@ pub fn span_lint_and_sugg<'a, T: LintContext>(
/// appear once per
/// replacement. In human-readable format though, it only appears once before
/// the whole suggestion.
pub fn multispan_sugg<I>(diag: &mut DiagnosticBuilder<'_>, help_msg: String, sugg: I)
pub fn multispan_sugg<I>(diag: &mut DiagnosticBuilder<'_>, help_msg: &str, sugg: I)
where
I: IntoIterator<Item = (Span, String)>,
{
let sugg = CodeSuggestion {
substitutions: vec![Substitution {
parts: sugg
.into_iter()
.map(|(span, snippet)| SubstitutionPart { snippet, span })
.collect(),
}],
msg: help_msg,
style: SuggestionStyle::ShowCode,
applicability: Applicability::Unspecified,
};
diag.suggestions.push(sugg);
multispan_sugg_with_applicability(diag, help_msg, Applicability::Unspecified, sugg)
}
pub fn multispan_sugg_with_applicability<I>(
diag: &mut DiagnosticBuilder<'_>,
help_msg: &str,
applicability: Applicability,
sugg: I,
) where
I: IntoIterator<Item = (Span, String)>,
{
diag.multipart_suggestion(help_msg, sugg.into_iter().collect(), applicability);
}

14
src/lintlist/mod.rs
View File

@ -66,6 +66,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "bit_mask",
},
Lint {
name: "bind_instead_of_map",
group: "complexity",
desc: "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`",
deprecation: None,
module: "methods",
},
Lint {
name: "blacklisted_name",
group: "style",
@ -1578,13 +1585,6 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "eq_op",
},
Lint {
name: "option_and_then_some",
group: "complexity",
desc: "using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`",
deprecation: None,
module: "methods",
},
Lint {
name: "option_as_ref_deref",
group: "complexity",

4
tests/ui/option_and_then_some.fixed → tests/ui/bind_instead_of_map.fixed
View File

@ -1,5 +1,5 @@
// run-rustfix
#![deny(clippy::option_and_then_some)]
#![deny(clippy::bind_instead_of_map)]
// need a main anyway, use it get rid of unused warnings too
pub fn main() {
@ -12,7 +12,7 @@ pub fn main() {
// Different type
let x: Result<u32, &str> = Ok(1);
let _ = x.and_then(Ok);
let _ = x;
}
pub fn foo() -> Option<String> {

2
tests/ui/option_and_then_some.rs → tests/ui/bind_instead_of_map.rs
View File

@ -1,5 +1,5 @@
// run-rustfix
#![deny(clippy::option_and_then_some)]
#![deny(clippy::bind_instead_of_map)]
// need a main anyway, use it get rid of unused warnings too
pub fn main() {

18
tests/ui/option_and_then_some.stderr → tests/ui/bind_instead_of_map.stderr
View File

@ -1,20 +1,26 @@
error: using `Option.and_then(Some)`, which is a no-op
--> $DIR/option_and_then_some.rs:8:13
--> $DIR/bind_instead_of_map.rs:8:13
|
LL | let _ = x.and_then(Some);
| ^^^^^^^^^^^^^^^^ help: use the expression directly: `x`
|
note: the lint level is defined here
--> $DIR/option_and_then_some.rs:2:9
--> $DIR/bind_instead_of_map.rs:2:9
|
LL | #![deny(clippy::option_and_then_some)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | #![deny(clippy::bind_instead_of_map)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
--> $DIR/option_and_then_some.rs:9:13
--> $DIR/bind_instead_of_map.rs:9:13
|
LL | let _ = x.and_then(|o| Some(o + 1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `x.map(|o| o + 1)`
error: aborting due to 2 previous errors
error: using `Result.and_then(Ok)`, which is a no-op
--> $DIR/bind_instead_of_map.rs:15:13
|
LL | let _ = x.and_then(Ok);
| ^^^^^^^^^^^^^^ help: use the expression directly: `x`
error: aborting due to 3 previous errors

61
tests/ui/bind_instead_of_map_multipart.rs Normal file
View File

@ -0,0 +1,61 @@
#![deny(clippy::bind_instead_of_map)]
#![allow(clippy::blocks_in_if_conditions)]
pub fn main() {
let _ = Some("42").and_then(|s| if s.len() < 42 { Some(0) } else { Some(s.len()) });
let _ = Some("42").and_then(|s| if s.len() < 42 { None } else { Some(s.len()) });
let _ = Ok::<_, ()>("42").and_then(|s| if s.len() < 42 { Ok(0) } else { Ok(s.len()) });
let _ = Ok::<_, ()>("42").and_then(|s| if s.len() < 42 { Err(()) } else { Ok(s.len()) });
let _ = Err::<(), _>("42").or_else(|s| if s.len() < 42 { Err(s.len() + 20) } else { Err(s.len()) });
let _ = Err::<(), _>("42").or_else(|s| if s.len() < 42 { Ok(()) } else { Err(s.len()) });
hard_example();
macro_example();
}
fn hard_example() {
Some("42").and_then(|s| {
if {
if s == "43" {
return Some(43);
}
s == "42"
} {
return Some(45);
}
match s.len() {
10 => Some(2),
20 => {
if foo() {
return {
if foo() {
return Some(20);
}
println!("foo");
Some(3)
};
}
Some(20)
},
40 => Some(30),
_ => Some(1),
}
});
}
fn foo() -> bool {
true
}
macro_rules! m {
() => {
Some(10)
};
}
fn macro_example() {
let _ = Some("").and_then(|s| if s.len() == 20 { m!() } else { Some(20) });
let _ = Some("").and_then(|s| if s.len() == 20 { Some(m!()) } else { Some(Some(20)) });
}

73
tests/ui/bind_instead_of_map_multipart.stderr Normal file
View File

@ -0,0 +1,73 @@
error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
--> $DIR/bind_instead_of_map_multipart.rs:5:13
|
LL | let _ = Some("42").and_then(|s| if s.len() < 42 { Some(0) } else { Some(s.len()) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/bind_instead_of_map_multipart.rs:1:9
|
LL | #![deny(clippy::bind_instead_of_map)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: try this
|
LL | let _ = Some("42").map(|s| if s.len() < 42 { 0 } else { s.len() });
| ^^^ ^ ^^^^^^^
error: using `Result.and_then(|x| Ok(y))`, which is more succinctly expressed as `map(|x| y)`
--> $DIR/bind_instead_of_map_multipart.rs:8:13
|
LL | let _ = Ok::<_, ()>("42").and_then(|s| if s.len() < 42 { Ok(0) } else { Ok(s.len()) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try this
|
LL | let _ = Ok::<_, ()>("42").map(|s| if s.len() < 42 { 0 } else { s.len() });
| ^^^ ^ ^^^^^^^
error: using `Result.or_else(|x| Err(y))`, which is more succinctly expressed as `map_err(|x| y)`
--> $DIR/bind_instead_of_map_multipart.rs:11:13
|
LL | let _ = Err::<(), _>("42").or_else(|s| if s.len() < 42 { Err(s.len() + 20) } else { Err(s.len()) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try this
|
LL | let _ = Err::<(), _>("42").map_err(|s| if s.len() < 42 { s.len() + 20 } else { s.len() });
| ^^^^^^^ ^^^^^^^^^^^^ ^^^^^^^
error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
--> $DIR/bind_instead_of_map_multipart.rs:19:5
|
LL | / Some("42").and_then(|s| {
LL | | if {
LL | | if s == "43" {
LL | | return Some(43);
... |
LL | | }
LL | | });
| |______^
|
help: try this
|
LL | Some("42").map(|s| {
LL | if {
LL | if s == "43" {
LL | return 43;
LL | }
LL | s == "42"
...
error: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
--> $DIR/bind_instead_of_map_multipart.rs:60:13
|
LL | let _ = Some("").and_then(|s| if s.len() == 20 { Some(m!()) } else { Some(Some(20)) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try this
|
LL | let _ = Some("").map(|s| if s.len() == 20 { m!() } else { Some(20) });
| ^^^ ^^^^ ^^^^^^^^
error: aborting due to 5 previous errors

4
tests/ui/blocks_in_if_conditions.fixed
View File

@ -63,10 +63,10 @@ fn block_in_assert() {
let opt = Some(42);
assert!(opt
.as_ref()
.and_then(|val| {
.map(|val| {
let mut v = val * 2;
v -= 1;
Some(v * 3)
v * 3
})
.is_some());
}

4
tests/ui/blocks_in_if_conditions.rs
View File

@ -63,10 +63,10 @@ fn block_in_assert() {
let opt = Some(42);
assert!(opt
.as_ref()
.and_then(|val| {
.map(|val| {
let mut v = val * 2;
v -= 1;
Some(v * 3)
v * 3
})
.is_some());
}

2
tests/ui/option_map_or_none.fixed
View File

@ -1,6 +1,6 @@
// run-rustfix
#![allow(clippy::option_and_then_some)]
#![allow(clippy::bind_instead_of_map)]
fn main() {
let opt = Some(1);

2
tests/ui/option_map_or_none.rs
View File

@ -1,6 +1,6 @@
// run-rustfix
#![allow(clippy::option_and_then_some)]
#![allow(clippy::bind_instead_of_map)]
fn main() {
let opt = Some(1);