Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-23 21:23:56 +00:00
Code Issues Projects Releases Packages Wiki Activity

Add manual_filter lint for Option

Browse source 
Share much of its implementation with `manual_map` and should greatly benefit from its previous feedback.
This commit is contained in:
kraktus 2022-10-03 13:57:42 +02:00
parent 18e10ca290
commit 0958f9486b
13 changed files with 951 additions and 67 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG.md
View file

@ -3986,6 +3986,7 @@ Released 2018-09-13
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
[`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits
[`manual_clamp`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp
[`manual_filter`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter
[`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map
[`manual_find`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find
[`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map

1
clippy_lints/src/lib.register_all.rs
View file

@ -135,6 +135,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
LintId::of(match_result_ok::MATCH_RESULT_OK),
LintId::of(matches::COLLAPSIBLE_MATCH),
LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH),
LintId::of(matches::MANUAL_FILTER),
LintId::of(matches::MANUAL_MAP),
LintId::of(matches::MANUAL_UNWRAP_OR),
LintId::of(matches::MATCH_AS_REF),

1
clippy_lints/src/lib.register_complexity.rs
View file

@ -27,6 +27,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
LintId::of(manual_strip::MANUAL_STRIP),
LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN),
LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN),
LintId::of(matches::MANUAL_FILTER),
LintId::of(matches::MANUAL_UNWRAP_OR),
LintId::of(matches::MATCH_AS_REF),
LintId::of(matches::MATCH_SINGLE_BINDING),

1
clippy_lints/src/lib.register_lints.rs
View file

@ -257,6 +257,7 @@ store.register_lints(&[
match_result_ok::MATCH_RESULT_OK,
matches::COLLAPSIBLE_MATCH,
matches::INFALLIBLE_DESTRUCTURING_MATCH,
matches::MANUAL_FILTER,
matches::MANUAL_MAP,
matches::MANUAL_UNWRAP_OR,
matches::MATCH_AS_REF,

181
clippy_lints/src/matches/manual_filter.rs Normal file
View file

@ -0,0 +1,181 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{is_res_lang_ctor, path_res, path_to_local_id};
use rustc_hir::intravisit::{walk_expr, Visitor};
use rustc_hir::LangItem::OptionSome;
use rustc_hir::{Arm, Block, BlockCheckMode, Expr, ExprKind, HirId, Pat, PatKind, UnsafeSource};
use rustc_lint::LateContext;
use rustc_span::{sym, SyntaxContext};
use super::manual_map::{check_with, SomeExpr};
use super::MANUAL_FILTER;
#[derive(Default)]
struct NeedsUnsafeBlock(pub bool);
impl<'tcx> Visitor<'tcx> for NeedsUnsafeBlock {
fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
match expr.kind {
ExprKind::Block(
Block {
rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
..
},
_,
) => {
self.0 = true;
},
_ => walk_expr(self, expr),
}
}
}
// Function called on the `expr` of `[&+]Some((ref | ref mut) x) => <expr>`
// Need to check if it's of the `if <cond> {<then_expr>} else {<else_expr>}`
// AND that only one `then/else_expr` resolves to `Some(x)` while the other resolves to `None`
// return `cond` if
fn get_cond_expr<'tcx>(
cx: &LateContext<'tcx>,
pat: &Pat<'_>,
expr: &'tcx Expr<'_>,
ctxt: SyntaxContext,
) -> Option<SomeExpr<'tcx>> {
if_chain! {
if let Some(block_expr) = peels_blocks_incl_unsafe_opt(expr);
if let ExprKind::If(cond, then_expr, Some(else_expr)) = block_expr.kind;
if let PatKind::Binding(_,target, ..) = pat.kind;
if let (then_visitor, else_visitor)
= (handle_if_or_else_expr(cx, target, ctxt, then_expr),
handle_if_or_else_expr(cx, target, ctxt, else_expr));
if then_visitor != else_visitor; // check that one expr resolves to `Some(x)`, the other to `None`
then {
let mut needs_unsafe_block = NeedsUnsafeBlock::default();
needs_unsafe_block.visit_expr(expr);
return Some(SomeExpr {
expr: peels_blocks_incl_unsafe(cond.peel_drop_temps()),
needs_unsafe_block: needs_unsafe_block.0,
needs_negated: !then_visitor // if the `then_expr` resolves to `None`, need to negate the cond
})
}
};
None
}
fn peels_blocks_incl_unsafe_opt<'a>(expr: &'a Expr<'a>) -> Option<&'a Expr<'a>> {
// we don't want to use `peel_blocks` here because we don't care if the block is unsafe, it's
// checked by `NeedsUnsafeBlock`
if let ExprKind::Block(block, None) = expr.kind {
if block.stmts.is_empty() {
return block.expr;
}
};
None
}
fn peels_blocks_incl_unsafe<'a>(expr: &'a Expr<'a>) -> &'a Expr<'a> {
peels_blocks_incl_unsafe_opt(expr).unwrap_or(expr)
}
// function called for each <ifelse> expression:
// Some(x) => if <cond> {
// <ifelse>
// } else {
// <ifelse>
// }
// Returns true if <ifelse> resolves to `Some(x)`, `false` otherwise
fn handle_if_or_else_expr<'tcx>(
cx: &LateContext<'_>,
target: HirId,
ctxt: SyntaxContext,
if_or_else_expr: &'tcx Expr<'_>,
) -> bool {
if let Some(inner_expr) = peels_blocks_incl_unsafe_opt(if_or_else_expr) {
// there can be not statements in the block as they would be removed when switching to `.filter`
if let ExprKind::Call(callee, [arg]) = inner_expr.kind {
return ctxt == if_or_else_expr.span.ctxt()
&& is_res_lang_ctor(cx, path_res(cx, callee), OptionSome)
&& path_to_local_id(arg, target);
}
};
false
}
// given the closure: `|<pattern>| <expr>`
// returns `|&<pattern>| <expr>`
fn add_ampersand_if_copy(body_str: String, has_copy_trait: bool) -> String {
if has_copy_trait {
let mut with_ampersand = body_str;
with_ampersand.insert(1, '&');
with_ampersand
} else {
body_str
}
}
pub(super) fn check_match<'tcx>(
cx: &LateContext<'tcx>,
scrutinee: &'tcx Expr<'_>,
arms: &'tcx [Arm<'_>],
expr: &'tcx Expr<'_>,
) {
let ty = cx.typeck_results().expr_ty(expr);
if_chain! {
if is_type_diagnostic_item(cx, ty, sym::Option);
if arms.len() == 2;
if arms[0].guard.is_none();
if arms[1].guard.is_none();
then {
check(cx, expr, scrutinee, arms[0].pat, arms[0].body, Some(arms[1].pat), arms[1].body)
}
}
}
pub(super) fn check_if_let<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
let_pat: &'tcx Pat<'_>,
let_expr: &'tcx Expr<'_>,
then_expr: &'tcx Expr<'_>,
else_expr: &'tcx Expr<'_>,
) {
check(cx, expr, let_expr, let_pat, then_expr, None, else_expr);
}
fn check<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
scrutinee: &'tcx Expr<'_>,
then_pat: &'tcx Pat<'_>,
then_body: &'tcx Expr<'_>,
else_pat: Option<&'tcx Pat<'_>>,
else_body: &'tcx Expr<'_>,
) {
if let Some(sugg_info) = check_with(
cx,
expr,
scrutinee,
then_pat,
then_body,
else_pat,
else_body,
get_cond_expr,
) {
let body_str = add_ampersand_if_copy(sugg_info.body_str, sugg_info.scrutinee_impl_copy);
span_lint_and_sugg(
cx,
MANUAL_FILTER,
expr.span,
"manual implementation of `Option::filter`",
"try this",
if sugg_info.needs_brackets {
format!(
"{{ {}{}.filter({body_str}) }}",
sugg_info.scrutinee_str, sugg_info.as_ref_str
)
} else {
format!("{}{}.filter({body_str})", sugg_info.scrutinee_str, sugg_info.as_ref_str)
},
sugg_info.app,
);
}
}

213
clippy_lints/src/matches/manual_map.rs
View file

@ -1,10 +1,11 @@
use super::MANUAL_MAP;
use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
use clippy_utils::ty::{is_type_diagnostic_item, peel_mid_ty_refs_is_mutable, type_is_unsafe_function};
use clippy_utils::ty::{is_copy, is_type_diagnostic_item, peel_mid_ty_refs_is_mutable, type_is_unsafe_function};
use clippy_utils::{
can_move_expr_to_closure, is_else_clause, is_lint_allowed, is_res_lang_ctor, path_res, path_to_local_id,
peel_blocks, peel_hir_expr_refs, peel_hir_expr_while, CaptureKind,
peel_blocks, peel_hir_expr_refs, peel_hir_expr_while, sugg::Sugg, CaptureKind,
};
use rustc_ast::util::parser::PREC_POSTFIX;
use rustc_errors::Applicability;
@ -16,8 +17,6 @@ use rustc_hir::{
use rustc_lint::LateContext;
use rustc_span::{sym, SyntaxContext};
use super::MANUAL_MAP;
pub(super) fn check_match<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
@ -43,7 +42,6 @@ pub(super) fn check_if_let<'tcx>(
check(cx, expr, let_expr, let_pat, then_expr, None, else_expr);
}
#[expect(clippy::too_many_lines)]
fn check<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
@ -53,12 +51,59 @@ fn check<'tcx>(
else_pat: Option<&'tcx Pat<'_>>,
else_body: &'tcx Expr<'_>,
) {
if let Some(sugg_info) = check_with(
cx,
expr,
scrutinee,
then_pat,
then_body,
else_pat,
else_body,
get_some_expr,
) {
span_lint_and_sugg(
cx,
MANUAL_MAP,
expr.span,
"manual implementation of `Option::map`",
"try this",
if sugg_info.needs_brackets {
format!(
"{{ {}{}.map({}) }}",
sugg_info.scrutinee_str, sugg_info.as_ref_str, sugg_info.body_str
)
} else {
format!(
"{}{}.map({})",
sugg_info.scrutinee_str, sugg_info.as_ref_str, sugg_info.body_str
)
},
sugg_info.app,
);
}
}
#[expect(clippy::too_many_arguments)]
#[expect(clippy::too_many_lines)]
pub(super) fn check_with<'tcx, F>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
scrutinee: &'tcx Expr<'_>,
then_pat: &'tcx Pat<'_>,
then_body: &'tcx Expr<'_>,
else_pat: Option<&'tcx Pat<'_>>,
else_body: &'tcx Expr<'_>,
get_some_expr_fn: F,
) -> Option<SuggInfo<'tcx>>
where
F: Fn(&LateContext<'tcx>, &'tcx Pat<'_>, &'tcx Expr<'_>, SyntaxContext) -> Option<SomeExpr<'tcx>>,
{
let (scrutinee_ty, ty_ref_count, ty_mutability) =
peel_mid_ty_refs_is_mutable(cx.typeck_results().expr_ty(scrutinee));
if !(is_type_diagnostic_item(cx, scrutinee_ty, sym::Option)
&& is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::Option))
{
return;
return None;
}
let expr_ctxt = expr.span.ctxt();
@ -78,29 +123,29 @@ fn check<'tcx>(
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None)) if is_none_expr(cx, else_body) => {
(then_body, pattern, ref_count, false)
},
_ => return,
_ => return None,
};
// Top level or patterns aren't allowed in closures.
if matches!(some_pat.kind, PatKind::Or(_)) {
return;
return None;
}
let some_expr = match get_some_expr(cx, some_expr, false, expr_ctxt) {
let some_expr = match get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt) {
Some(expr) => expr,
None => return,
None => return None,
};
// These two lints will go back and forth with each other.
if cx.typeck_results().expr_ty(some_expr.expr) == cx.tcx.types.unit
&& !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, expr.hir_id)
{
return;
return None;
}
// `map` won't perform any adjustments.
if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() {
return;
return None;
}
// Determine which binding mode to use.
@ -125,16 +170,16 @@ fn check<'tcx>(
});
if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(l), .. })) = e.kind {
match captures.get(l) {
Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return,
Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return None,
Some(CaptureKind::Ref(Mutability::Not)) if binding_ref_mutability == Mutability::Mut => {
return;
return None;
},
Some(CaptureKind::Ref(Mutability::Not)) | None => (),
}
}
}
},
None => return,
None => return None,
};
let mut app = Applicability::MachineApplicable;
@ -149,6 +194,7 @@ fn check<'tcx>(
scrutinee_str.into()
};
let closure_expr_snip = some_expr.to_snippet_with_context(cx, expr_ctxt, &mut app);
let body_str = if let PatKind::Binding(annotation, id, some_binding, None) = some_pat.kind {
if_chain! {
if !some_expr.needs_unsafe_block;
@ -161,7 +207,7 @@ fn check<'tcx>(
&& !is_lint_allowed(cx, MATCH_AS_REF, expr.hir_id)
&& binding_ref.is_some()
{
return;
return None;
}
// `ref` and `ref mut` annotations were handled earlier.
@ -170,41 +216,47 @@ fn check<'tcx>(
} else {
""
};
let expr_snip = snippet_with_context(cx, some_expr.expr.span, expr_ctxt, "..", &mut app).0;
if some_expr.needs_unsafe_block {
format!("|{annotation}{some_binding}| unsafe {{ {expr_snip} }}")
format!("|{annotation}{some_binding}| unsafe {{ {closure_expr_snip} }}")
} else {
format!("|{annotation}{some_binding}| {expr_snip}")
format!("|{annotation}{some_binding}| {closure_expr_snip}")
}
}
}
} else if !is_wild_none && explicit_ref.is_none() {
// TODO: handle explicit reference annotations.
let pat_snip = snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app).0;
let expr_snip = snippet_with_context(cx, some_expr.expr.span, expr_ctxt, "..", &mut app).0;
if some_expr.needs_unsafe_block {
format!("|{pat_snip}| unsafe {{ {expr_snip} }}")
format!("|{pat_snip}| unsafe {{ {closure_expr_snip} }}")
} else {
format!("|{pat_snip}| {expr_snip}")
format!("|{pat_snip}| {closure_expr_snip}")
}
} else {
// Refutable bindings and mixed reference annotations can't be handled by `map`.
return;
return None;
};
span_lint_and_sugg(
cx,
MANUAL_MAP,
expr.span,
"manual implementation of `Option::map`",
"try this",
if else_pat.is_none() && is_else_clause(cx.tcx, expr) {
format!("{{ {scrutinee_str}{as_ref_str}.map({body_str}) }}")
} else {
format!("{scrutinee_str}{as_ref_str}.map({body_str})")
},
// relies on the fact that Option<T>: Copy where T: copy
let scrutinee_impl_copy = is_copy(cx, scrutinee_ty);
Some(SuggInfo {
needs_brackets: else_pat.is_none() && is_else_clause(cx.tcx, expr),
scrutinee_impl_copy,
scrutinee_str,
as_ref_str,
body_str,
app,
);
})
}
pub struct SuggInfo<'a> {
pub needs_brackets: bool,
pub scrutinee_impl_copy: bool,
pub scrutinee_str: String,
pub as_ref_str: &'a str,
pub body_str: String,
pub app: Applicability,
}
// Checks whether the expression could be passed as a function, or whether a closure is needed.
@ -222,7 +274,8 @@ fn can_pass_as_func<'tcx>(cx: &LateContext<'tcx>, binding: HirId, expr: &'tcx Ex
}
}
enum OptionPat<'a> {
#[derive(Debug)]
pub(super) enum OptionPat<'a> {
Wild,
None,
Some {
@ -234,14 +287,39 @@ enum OptionPat<'a> {
},
}
struct SomeExpr<'tcx> {
expr: &'tcx Expr<'tcx>,
needs_unsafe_block: bool,
pub(super) struct SomeExpr<'tcx> {
pub expr: &'tcx Expr<'tcx>,
pub needs_unsafe_block: bool,
pub needs_negated: bool, // for `manual_filter` lint
}
impl<'tcx> SomeExpr<'tcx> {
pub fn new_no_negated(expr: &'tcx Expr<'tcx>, needs_unsafe_block: bool) -> Self {
Self {
expr,
needs_unsafe_block,
needs_negated: false,
}
}
pub fn to_snippet_with_context(
&self,
cx: &LateContext<'tcx>,
ctxt: SyntaxContext,
app: &mut Applicability,
) -> Sugg<'tcx> {
let sugg = Sugg::hir_with_context(cx, self.expr, ctxt, "..", app);
if self.needs_negated { !sugg } else { sugg }
}
}
// Try to parse into a recognized `Option` pattern.
// i.e. `_`, `None`, `Some(..)`, or a reference to any of those.
fn try_parse_pattern<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ctxt: SyntaxContext) -> Option<OptionPat<'tcx>> {
pub(super) fn try_parse_pattern<'tcx>(
cx: &LateContext<'tcx>,
pat: &'tcx Pat<'_>,
ctxt: SyntaxContext,
) -> Option<OptionPat<'tcx>> {
fn f<'tcx>(
cx: &LateContext<'tcx>,
pat: &'tcx Pat<'_>,
@ -268,36 +346,41 @@ fn try_parse_pattern<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ctxt: Syn
// Checks for an expression wrapped by the `Some` constructor. Returns the contained expression.
fn get_some_expr<'tcx>(
cx: &LateContext<'tcx>,
_: &'tcx Pat<'_>,
expr: &'tcx Expr<'_>,
needs_unsafe_block: bool,
ctxt: SyntaxContext,
) -> Option<SomeExpr<'tcx>> {
// TODO: Allow more complex expressions.
match expr.kind {
ExprKind::Call(callee, [arg])
if ctxt == expr.span.ctxt() && is_res_lang_ctor(cx, path_res(cx, callee), OptionSome) =>
{
Some(SomeExpr {
expr: arg,
needs_unsafe_block,
})
},
ExprKind::Block(
Block {
stmts: [],
expr: Some(expr),
rules,
..
fn get_some_expr_internal<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
needs_unsafe_block: bool,
ctxt: SyntaxContext,
) -> Option<SomeExpr<'tcx>> {
// TODO: Allow more complex expressions.
match expr.kind {
ExprKind::Call(callee, [arg])
if ctxt == expr.span.ctxt() && is_res_lang_ctor(cx, path_res(cx, callee), OptionSome) =>
{
Some(SomeExpr::new_no_negated(arg, needs_unsafe_block))
},
_,
) => get_some_expr(
cx,
expr,
needs_unsafe_block || *rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
ctxt,
),
_ => None,
ExprKind::Block(
Block {
stmts: [],
expr: Some(expr),
rules,
..
},
_,
) => get_some_expr_internal(
cx,
expr,
needs_unsafe_block || *rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
ctxt,
),
_ => None,
}
}
get_some_expr_internal(cx, expr, false, ctxt)
}
// Checks for the `None` value.

39
clippy_lints/src/matches/mod.rs
View file

@ -1,5 +1,6 @@
mod collapsible_match;
mod infallible_destructuring_match;
mod manual_filter;
mod manual_map;
mod manual_unwrap_or;
mod match_as_ref;
@ -898,6 +899,34 @@ declare_clippy_lint! {
"reimplementation of `map`"
}
declare_clippy_lint! {
/// ### What it does
/// Checks for usages of `match` which could be implemented using `filter`
///
/// ### Why is this bad?
/// Using the `filter` method is clearer and more concise.
///
/// ### Example
/// ```rust
/// match Some(0) {
/// Some(x) => if x % 2 == 0 {
/// Some(x)
/// } else {
/// None
/// },
/// None => None,
/// };
/// ```
/// Use instead:
/// ```rust
/// Some(0).filter(|&x| x % 2 == 0);
/// ```
#[clippy::version = "1.65.0"]
pub MANUAL_FILTER,
complexity,
"reimplentation of `filter`"
}
#[derive(Default)]
pub struct Matches {
msrv: Option<RustcVersion>,
@ -939,6 +968,7 @@ impl_lint_pass!(Matches => [
SIGNIFICANT_DROP_IN_SCRUTINEE,
TRY_ERR,
MANUAL_MAP,
MANUAL_FILTER,
]);
impl<'tcx> LateLintPass<'tcx> for Matches {
@ -988,6 +1018,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
if !in_constant(cx, expr.hir_id) {
manual_unwrap_or::check(cx, expr, ex, arms);
manual_map::check_match(cx, expr, ex, arms);
manual_filter::check_match(cx, ex, arms, expr);
}
if self.infallible_destructuring_match_linted {
@ -1014,6 +1045,14 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
}
if !in_constant(cx, expr.hir_id) {
manual_map::check_if_let(cx, expr, if_let.let_pat, if_let.let_expr, if_let.if_then, else_expr);
manual_filter::check_if_let(
cx,
expr,
if_let.let_pat,
if_let.let_expr,
if_let.if_then,
else_expr,
);
}
}
redundant_pattern_match::check_if_let(

6
clippy_utils/src/sugg.rs
View file

@ -1,7 +1,9 @@
//! Contains utility functions to generate suggestions.
#![deny(clippy::missing_docs_in_private_items)]
use crate::source::{snippet, snippet_opt, snippet_with_applicability, snippet_with_macro_callsite};
use crate::source::{
snippet, snippet_opt, snippet_with_applicability, snippet_with_context, snippet_with_macro_callsite,
};
use crate::ty::expr_sig;
use crate::{get_parent_expr_for_hir, higher};
use rustc_ast::util::parser::AssocOp;
@ -110,7 +112,7 @@ impl<'a> Sugg<'a> {
if expr.span.ctxt() == ctxt {
Self::hir_from_snippet(expr, |span| snippet(cx, span, default))
} else {
let snip = snippet_with_applicability(cx, expr.span, default, applicability);
let snip = snippet_with_context(cx, expr.span, ctxt, default, applicability).0;
Sugg::NonParen(snip)
}
}

1
src/docs.rs
View file

@ -256,6 +256,7 @@ docs! {
"manual_async_fn",
"manual_bits",
"manual_clamp",
"manual_filter",
"manual_filter_map",
"manual_find",
"manual_find_map",

21
src/docs/manual_filter.txt Normal file
View file

@ -0,0 +1,21 @@
### What it does
Checks for usages of `match` which could be implemented using `filter`
### Why is this bad?
Using the `filter` method is clearer and more concise.
### Example
```
match Some(0) {
Some(x) => if x % 2 == 0 {
Some(x)
} else {
None
},
None => None,
};
```
Use instead:
```
Some(0).filter(|&x| x % 2 == 0);
```

119
tests/ui/manual_filter.fixed Normal file
View file

@ -0,0 +1,119 @@
// run-rustfix
#![warn(clippy::manual_filter)]
#![allow(unused_variables, dead_code)]
fn main() {
Some(0).filter(|&x| x <= 0);
Some(1).filter(|&x| x <= 0);
Some(2).filter(|&x| x <= 0);
Some(3).filter(|&x| x > 0);
let y = Some(4);
y.filter(|&x| x <= 0);
Some(5).filter(|&x| x > 0);
Some(6).as_ref().filter(|&x| x > &0);
let external_cond = true;
Some(String::new()).filter(|x| external_cond);
Some(7).filter(|&x| external_cond);
Some(8).filter(|&x| x != 0);
Some(9).filter(|&x| x > 10 && x < 100);
const fn f1() {
// Don't lint, `.filter` is not const
match Some(10) {
Some(x) => {
if x > 10 && x < 100 {
Some(x)
} else {
None
}
},
None => None,
};
}
#[allow(clippy::blocks_in_if_conditions)]
Some(11).filter(|&x| {
println!("foo");
x > 10 && x < 100
});
match Some(12) {
// Don't Lint, statement is lost by `.filter`
Some(x) => {
if x > 10 && x < 100 {
println!("foo");
Some(x)
} else {
None
}
},
None => None,
};
match Some(13) {
// Don't Lint, because of `None => Some(1)`
Some(x) => {
if x > 10 && x < 100 {
println!("foo");
Some(x)
} else {
None
}
},
None => Some(1),
};
unsafe fn f(x: u32) -> bool {
true
}
let _ = Some(14).filter(|&x| unsafe { f(x) });
let _ = Some(15).filter(|&x| unsafe { f(x) });
#[allow(clippy::redundant_pattern_matching)]
if let Some(_) = Some(16) {
Some(16)
} else { Some(16).filter(|&x| x % 2 == 0) };
match Some((17, 17)) {
// Not linted for now could be
Some((x, y)) => {
if y != x {
Some((x, y))
} else {
None
}
},
None => None,
};
struct NamedTuple {
pub x: u8,
pub y: (i32, u32),
}
match Some(NamedTuple {
// Not linted for now could be
x: 17,
y: (18, 19),
}) {
Some(NamedTuple { x, y }) => {
if y.1 != x as u32 {
Some(NamedTuple { x, y })
} else {
None
}
},
None => None,
};
}

243
tests/ui/manual_filter.rs Normal file
View file

@ -0,0 +1,243 @@
// run-rustfix
#![warn(clippy::manual_filter)]
#![allow(unused_variables, dead_code)]
fn main() {
match Some(0) {
None => None,
Some(x) => {
if x > 0 {
None
} else {
Some(x)
}
},
};
match Some(1) {
Some(x) => {
if x > 0 {
None
} else {
Some(x)
}
},
None => None,
};
match Some(2) {
Some(x) => {
if x > 0 {
None
} else {
Some(x)
}
},
_ => None,
};
match Some(3) {
Some(x) => {
if x > 0 {
Some(x)
} else {
None
}
},
None => None,
};
let y = Some(4);
match y {
// Some(4)
None => None,
Some(x) => {
if x > 0 {
None
} else {
Some(x)
}
},
};
match Some(5) {
Some(x) => {
if x > 0 {
Some(x)
} else {
None
}
},
_ => None,
};
match Some(6) {
Some(ref x) => {
if x > &0 {
Some(x)
} else {
None
}
},
_ => None,
};
let external_cond = true;
match Some(String::new()) {
Some(x) => {
if external_cond {
Some(x)
} else {
None
}
},
_ => None,
};
if let Some(x) = Some(7) {
if external_cond { Some(x) } else { None }
} else {
None
};
match &Some(8) {
&Some(x) => {
if x != 0 {
Some(x)
} else {
None
}
},
_ => None,
};
match Some(9) {
Some(x) => {
if x > 10 && x < 100 {
Some(x)
} else {
None
}
},
None => None,
};
const fn f1() {
// Don't lint, `.filter` is not const
match Some(10) {
Some(x) => {
if x > 10 && x < 100 {
Some(x)
} else {
None
}
},
None => None,
};
}
#[allow(clippy::blocks_in_if_conditions)]
match Some(11) {
// Lint, statement is preserved by `.filter`
Some(x) => {
if {
println!("foo");
x > 10 && x < 100
} {
Some(x)
} else {
None
}
},
None => None,
};
match Some(12) {
// Don't Lint, statement is lost by `.filter`
Some(x) => {
if x > 10 && x < 100 {
println!("foo");
Some(x)
} else {
None
}
},
None => None,
};
match Some(13) {
// Don't Lint, because of `None => Some(1)`
Some(x) => {
if x > 10 && x < 100 {
println!("foo");
Some(x)
} else {
None
}
},
None => Some(1),
};
unsafe fn f(x: u32) -> bool {
true
}
let _ = match Some(14) {
Some(x) => {
if unsafe { f(x) } {
Some(x)
} else {
None
}
},
None => None,
};
let _ = match Some(15) {
Some(x) => unsafe {
if f(x) { Some(x) } else { None }
},
None => None,
};
#[allow(clippy::redundant_pattern_matching)]
if let Some(_) = Some(16) {
Some(16)
} else if let Some(x) = Some(16) {
// Lint starting from here
if x % 2 == 0 { Some(x) } else { None }
} else {
None
};
match Some((17, 17)) {
// Not linted for now could be
Some((x, y)) => {
if y != x {
Some((x, y))
} else {
None
}
},
None => None,
};
struct NamedTuple {
pub x: u8,
pub y: (i32, u32),
}
match Some(NamedTuple {
// Not linted for now could be
x: 17,
y: (18, 19),
}) {
Some(NamedTuple { x, y }) => {
if y.1 != x as u32 {
Some(NamedTuple { x, y })
} else {
None
}
},
None => None,
};
}

191
tests/ui/manual_filter.stderr Normal file
View file

@ -0,0 +1,191 @@
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:7:5
|
LL | / match Some(0) {
LL | | None => None,
LL | | Some(x) => {
LL | | if x > 0 {
... |
LL | | },
LL | | };
| |_____^ help: try this: `Some(0).filter(|&x| x <= 0)`
|
= note: `-D clippy::manual-filter` implied by `-D warnings`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:18:5
|
LL | / match Some(1) {
LL | | Some(x) => {
LL | | if x > 0 {
LL | | None
... |
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some(1).filter(|&x| x <= 0)`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:29:5
|
LL | / match Some(2) {
LL | | Some(x) => {
LL | | if x > 0 {
LL | | None
... |
LL | | _ => None,
LL | | };
| |_____^ help: try this: `Some(2).filter(|&x| x <= 0)`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:40:5
|
LL | / match Some(3) {
LL | | Some(x) => {
LL | | if x > 0 {
LL | | Some(x)
... |
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some(3).filter(|&x| x > 0)`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:52:5
|
LL | / match y {
LL | | // Some(4)
LL | | None => None,
LL | | Some(x) => {
... |
LL | | },
LL | | };
| |_____^ help: try this: `y.filter(|&x| x <= 0)`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:64:5
|
LL | / match Some(5) {
LL | | Some(x) => {
LL | | if x > 0 {
LL | | Some(x)
... |
LL | | _ => None,
LL | | };
| |_____^ help: try this: `Some(5).filter(|&x| x > 0)`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:75:5
|
LL | / match Some(6) {
LL | | Some(ref x) => {
LL | | if x > &0 {
LL | | Some(x)
... |
LL | | _ => None,
LL | | };
| |_____^ help: try this: `Some(6).as_ref().filter(|&x| x > &0)`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:87:5
|
LL | / match Some(String::new()) {
LL | | Some(x) => {
LL | | if external_cond {
LL | | Some(x)
... |
LL | | _ => None,
LL | | };
| |_____^ help: try this: `Some(String::new()).filter(|x| external_cond)`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:98:5
|
LL | / if let Some(x) = Some(7) {
LL | | if external_cond { Some(x) } else { None }
LL | | } else {
LL | | None
LL | | };
| |_____^ help: try this: `Some(7).filter(|&x| external_cond)`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:104:5
|
LL | / match &Some(8) {
LL | | &Some(x) => {
LL | | if x != 0 {
LL | | Some(x)
... |
LL | | _ => None,
LL | | };
| |_____^ help: try this: `Some(8).filter(|&x| x != 0)`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:115:5
|
LL | / match Some(9) {
LL | | Some(x) => {
LL | | if x > 10 && x < 100 {
LL | | Some(x)
... |
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some(9).filter(|&x| x > 10 && x < 100)`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:141:5
|
LL | / match Some(11) {
LL | | // Lint, statement is preserved by `.filter`
LL | | Some(x) => {
LL | | if {
... |
LL | | None => None,
LL | | };
| |_____^
|
help: try this
|
LL ~ Some(11).filter(|&x| {
LL + println!("foo");
LL + x > 10 && x < 100
LL ~ });
|
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:185:13
|
LL | let _ = match Some(14) {
| _____________^
LL | | Some(x) => {
LL | | if unsafe { f(x) } {
LL | | Some(x)
... |
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some(14).filter(|&x| unsafe { f(x) })`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:195:13
|
LL | let _ = match Some(15) {
| _____________^
LL | | Some(x) => unsafe {
LL | | if f(x) { Some(x) } else { None }
LL | | },
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some(15).filter(|&x| unsafe { f(x) })`
error: manual implementation of `Option::filter`
--> $DIR/manual_filter.rs:205:12
|
LL | } else if let Some(x) = Some(16) {
| ____________^
LL | | // Lint starting from here
LL | | if x % 2 == 0 { Some(x) } else { None }
LL | | } else {
LL | | None
LL | | };
| |_____^ help: try this: `{ Some(16).filter(|&x| x % 2 == 0) }`
error: aborting due to 15 previous errors