dnrops
/
rust-clippy
mirror of https://github.com/rust-lang/rust-clippy.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Auto merge of #9451 - kraktus:manual_filter2, r=dswij 

Add `manual_filter` lint for `Option`

Share much of its implementation with `manual_map` and should greatly benefit from its previous feedback.
I'm sure it's possible to even more refactor both and would gladly take input on that as well as any clippy idiomatic usage, since this is my first lint addition.

I've added the lint to the complexity section for now, I don't know if every new lint needs to go in nursery first.

The matching could be expanded to more than `Some(<value>)` to lint on arbitrary struct matching inside the `Some` but I've left it like it was for `manual_map` for now. `needless_match::pat_same_as_expr` provides a more generic match example.

close https://github.com/rust-lang/rust-clippy/issues/8822

changelog: Add lint [`manual_filter`] for `Option`
This commit is contained in:
bors 2022-10-08 15:58:51 +00:00
parent 2c8e473ffe 830fdf2b56
commit 292e313259
14 changed files with 1120 additions and 258 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -3988,6 +3988,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

@ -137,6 +137,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

@ -259,6 +259,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,

153
clippy_lints/src/matches/manual_filter.rs Normal file
View File

@ -0,0 +1,153 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::visitors::contains_unsafe_block;
use clippy_utils::{is_res_lang_ctor, path_res, path_to_local_id};
use rustc_hir::LangItem::OptionSome;
use rustc_hir::{Arm, Expr, ExprKind, HirId, Pat, PatKind};
use rustc_lint::LateContext;
use rustc_span::{sym, SyntaxContext};
use super::manual_utils::{check_with, SomeExpr};
use super::MANUAL_FILTER;
// Function called on the <expr> of `[&+]Some((ref | ref mut) x) => <expr>`
// Need to check if it's of the form `<expr>=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 the `cond` expression if so.
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)
= (is_some_expr(cx, target, ctxt, then_expr),
is_some_expr(cx, target, ctxt, else_expr));
if then_visitor != else_visitor; // check that one expr resolves to `Some(x)`, the other to `None`
then {
return Some(SomeExpr {
expr: peels_blocks_incl_unsafe(cond.peel_drop_temps()),
needs_unsafe_block: contains_unsafe_block(cx, expr),
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 `contains_unsafe_block`
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 <expr> expression:
// Some(x) => if <cond> {
// <expr>
// } else {
// <expr>
// }
// Returns true if <expr> resolves to `Some(x)`, `false` otherwise
fn is_some_expr<'tcx>(cx: &LateContext<'_>, target: HirId, ctxt: SyntaxContext, expr: &'tcx Expr<'_>) -> bool {
if let Some(inner_expr) = peels_blocks_incl_unsafe_opt(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 == 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 is_type_diagnostic_item(cx, ty, sym::Option)
&& let [first_arm, second_arm] = arms
&& first_arm.guard.is_none()
&& second_arm.guard.is_none()
{
check(cx, expr, scrutinee, first_arm.pat, first_arm.body, Some(second_arm.pat), second_arm.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,
);
}
}

322
clippy_lints/src/matches/manual_map.rs
View File

@ -1,22 +1,13 @@
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::{
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,
};
use rustc_ast::util::parser::PREC_POSTFIX;
use rustc_errors::Applicability;
use rustc_hir::LangItem::{OptionNone, OptionSome};
use rustc_hir::{
def::Res, Arm, BindingAnnotation, Block, BlockCheckMode, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path,
QPath, UnsafeSource,
};
use rustc_lint::LateContext;
use rustc_span::{sym, SyntaxContext};
use super::manual_utils::{check_with, SomeExpr};
use super::MANUAL_MAP;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::{is_res_lang_ctor, path_res};
use rustc_hir::LangItem::OptionSome;
use rustc_hir::{Arm, Block, BlockCheckMode, Expr, ExprKind, Pat, UnsafeSource};
use rustc_lint::LateContext;
use rustc_span::SyntaxContext;
pub(super) fn check_match<'tcx>(
cx: &LateContext<'tcx>,
@ -43,7 +34,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,254 +43,74 @@ fn check<'tcx>(
else_pat: Option<&'tcx Pat<'_>>,
else_body: &'tcx Expr<'_>,
) {
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;
}
let expr_ctxt = expr.span.ctxt();
let (some_expr, some_pat, pat_ref_count, is_wild_none) = match (
try_parse_pattern(cx, then_pat, expr_ctxt),
else_pat.map_or(Some(OptionPat::Wild), |p| try_parse_pattern(cx, p, expr_ctxt)),
) {
(Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => {
(else_body, pattern, ref_count, true)
},
(Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => {
(else_body, pattern, ref_count, false)
},
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild)) if is_none_expr(cx, else_body) => {
(then_body, pattern, ref_count, true)
},
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None)) if is_none_expr(cx, else_body) => {
(then_body, pattern, ref_count, false)
},
_ => return,
};
// Top level or patterns aren't allowed in closures.
if matches!(some_pat.kind, PatKind::Or(_)) {
return;
}
let some_expr = match get_some_expr(cx, some_expr, false, expr_ctxt) {
Some(expr) => expr,
None => return,
};
// 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;
}
// `map` won't perform any adjustments.
if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() {
return;
}
// Determine which binding mode to use.
let explicit_ref = some_pat.contains_explicit_ref_binding();
let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then_some(ty_mutability));
let as_ref_str = match binding_ref {
Some(Mutability::Mut) => ".as_mut()",
Some(Mutability::Not) => ".as_ref()",
None => "",
};
match can_move_expr_to_closure(cx, some_expr.expr) {
Some(captures) => {
// Check if captures the closure will need conflict with borrows made in the scrutinee.
// TODO: check all the references made in the scrutinee expression. This will require interacting
// with the borrow checker. Currently only `<local>[.<field>]*` is checked for.
if let Some(binding_ref_mutability) = binding_ref {
let e = peel_hir_expr_while(scrutinee, |e| match e.kind {
ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e),
_ => None,
});
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::Ref(Mutability::Not)) if binding_ref_mutability == Mutability::Mut => {
return;
},
Some(CaptureKind::Ref(Mutability::Not)) | None => (),
}
}
}
},
None => return,
};
let mut app = Applicability::MachineApplicable;
// Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or
// it's being passed by value.
let scrutinee = peel_hir_expr_refs(scrutinee).0;
let (scrutinee_str, _) = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app);
let scrutinee_str = if scrutinee.span.ctxt() == expr.span.ctxt() && scrutinee.precedence().order() < PREC_POSTFIX {
format!("({scrutinee_str})")
} else {
scrutinee_str.into()
};
let body_str = if let PatKind::Binding(annotation, id, some_binding, None) = some_pat.kind {
if_chain! {
if !some_expr.needs_unsafe_block;
if let Some(func) = can_pass_as_func(cx, id, some_expr.expr);
if func.span.ctxt() == some_expr.expr.span.ctxt();
then {
snippet_with_applicability(cx, func.span, "..", &mut app).into_owned()
} else {
if path_to_local_id(some_expr.expr, id)
&& !is_lint_allowed(cx, MATCH_AS_REF, expr.hir_id)
&& binding_ref.is_some()
{
return;
}
// `ref` and `ref mut` annotations were handled earlier.
let annotation = if matches!(annotation, BindingAnnotation::MUT) {
"mut "
} 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} }}")
} else {
format!("|{annotation}{some_binding}| {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} }}")
} else {
format!("|{pat_snip}| {expr_snip}")
}
} else {
// Refutable bindings and mixed reference annotations can't be handled by `map`.
return;
};
span_lint_and_sugg(
if let Some(sugg_info) = check_with(
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})")
},
app,
);
}
// Checks whether the expression could be passed as a function, or whether a closure is needed.
// Returns the function to be passed to `map` if it exists.
fn can_pass_as_func<'tcx>(cx: &LateContext<'tcx>, binding: HirId, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
match expr.kind {
ExprKind::Call(func, [arg])
if path_to_local_id(arg, binding)
&& cx.typeck_results().expr_adjustments(arg).is_empty()
&& !type_is_unsafe_function(cx, cx.typeck_results().expr_ty(func).peel_refs()) =>
{
Some(func)
},
_ => None,
}
}
enum OptionPat<'a> {
Wild,
None,
Some {
// The pattern contained in the `Some` tuple.
pattern: &'a Pat<'a>,
// The number of references before the `Some` tuple.
// e.g. `&&Some(_)` has a ref count of 2.
ref_count: usize,
},
}
struct SomeExpr<'tcx> {
expr: &'tcx Expr<'tcx>,
needs_unsafe_block: bool,
}
// 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>> {
fn f<'tcx>(
cx: &LateContext<'tcx>,
pat: &'tcx Pat<'_>,
ref_count: usize,
ctxt: SyntaxContext,
) -> Option<OptionPat<'tcx>> {
match pat.kind {
PatKind::Wild => Some(OptionPat::Wild),
PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1, ctxt),
PatKind::Path(ref qpath) if is_res_lang_ctor(cx, cx.qpath_res(qpath, pat.hir_id), OptionNone) => {
Some(OptionPat::None)
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
)
},
PatKind::TupleStruct(ref qpath, [pattern], _)
if is_res_lang_ctor(cx, cx.qpath_res(qpath, pat.hir_id), OptionSome) && pat.span.ctxt() == ctxt =>
{
Some(OptionPat::Some { pattern, ref_count })
},
_ => None,
}
sugg_info.app,
);
}
f(cx, pat, 0, ctxt)
}
// 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,
}
}
}
// Checks for the `None` value.
fn is_none_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
is_res_lang_ctor(cx, path_res(cx, peel_blocks(expr)), OptionNone)
get_some_expr_internal(cx, expr, false, ctxt)
}

278
clippy_lints/src/matches/manual_utils.rs Normal file
View File

@ -0,0 +1,278 @@
use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF};
use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
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, sugg::Sugg, CaptureKind,
};
use rustc_ast::util::parser::PREC_POSTFIX;
use rustc_errors::Applicability;
use rustc_hir::LangItem::{OptionNone, OptionSome};
use rustc_hir::{def::Res, BindingAnnotation, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, QPath};
use rustc_lint::LateContext;
use rustc_span::{sym, SyntaxContext};
#[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 None;
}
let expr_ctxt = expr.span.ctxt();
let (some_expr, some_pat, pat_ref_count, is_wild_none) = match (
try_parse_pattern(cx, then_pat, expr_ctxt),
else_pat.map_or(Some(OptionPat::Wild), |p| try_parse_pattern(cx, p, expr_ctxt)),
) {
(Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => {
(else_body, pattern, ref_count, true)
},
(Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => {
(else_body, pattern, ref_count, false)
},
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild)) if is_none_expr(cx, else_body) => {
(then_body, pattern, ref_count, true)
},
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None)) if is_none_expr(cx, else_body) => {
(then_body, pattern, ref_count, false)
},
_ => return None,
};
// Top level or patterns aren't allowed in closures.
if matches!(some_pat.kind, PatKind::Or(_)) {
return None;
}
let some_expr = match get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt) {
Some(expr) => expr,
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 None;
}
// `map` won't perform any adjustments.
if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() {
return None;
}
// Determine which binding mode to use.
let explicit_ref = some_pat.contains_explicit_ref_binding();
let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then_some(ty_mutability));
let as_ref_str = match binding_ref {
Some(Mutability::Mut) => ".as_mut()",
Some(Mutability::Not) => ".as_ref()",
None => "",
};
match can_move_expr_to_closure(cx, some_expr.expr) {
Some(captures) => {
// Check if captures the closure will need conflict with borrows made in the scrutinee.
// TODO: check all the references made in the scrutinee expression. This will require interacting
// with the borrow checker. Currently only `<local>[.<field>]*` is checked for.
if let Some(binding_ref_mutability) = binding_ref {
let e = peel_hir_expr_while(scrutinee, |e| match e.kind {
ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e),
_ => None,
});
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 None,
Some(CaptureKind::Ref(Mutability::Not)) if binding_ref_mutability == Mutability::Mut => {
return None;
},
Some(CaptureKind::Ref(Mutability::Not)) | None => (),
}
}
}
},
None => return None,
};
let mut app = Applicability::MachineApplicable;
// Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or
// it's being passed by value.
let scrutinee = peel_hir_expr_refs(scrutinee).0;
let (scrutinee_str, _) = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app);
let scrutinee_str = if scrutinee.span.ctxt() == expr.span.ctxt() && scrutinee.precedence().order() < PREC_POSTFIX {
format!("({scrutinee_str})")
} else {
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;
if let Some(func) = can_pass_as_func(cx, id, some_expr.expr);
if func.span.ctxt() == some_expr.expr.span.ctxt();
then {
snippet_with_applicability(cx, func.span, "..", &mut app).into_owned()
} else {
if path_to_local_id(some_expr.expr, id)
&& !is_lint_allowed(cx, MATCH_AS_REF, expr.hir_id)
&& binding_ref.is_some()
{
return None;
}
// `ref` and `ref mut` annotations were handled earlier.
let annotation = if matches!(annotation, BindingAnnotation::MUT) {
"mut "
} else {
""
};
if some_expr.needs_unsafe_block {
format!("|{annotation}{some_binding}| unsafe {{ {closure_expr_snip} }}")
} else {
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;
if some_expr.needs_unsafe_block {
format!("|{pat_snip}| unsafe {{ {closure_expr_snip} }}")
} else {
format!("|{pat_snip}| {closure_expr_snip}")
}
} else {
// Refutable bindings and mixed reference annotations can't be handled by `map`.
return None;
};
// 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.
// Returns the function to be passed to `map` if it exists.
fn can_pass_as_func<'tcx>(cx: &LateContext<'tcx>, binding: HirId, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
match expr.kind {
ExprKind::Call(func, [arg])
if path_to_local_id(arg, binding)
&& cx.typeck_results().expr_adjustments(arg).is_empty()
&& !type_is_unsafe_function(cx, cx.typeck_results().expr_ty(func).peel_refs()) =>
{
Some(func)
},
_ => None,
}
}
#[derive(Debug)]
pub(super) enum OptionPat<'a> {
Wild,
None,
Some {
// The pattern contained in the `Some` tuple.
pattern: &'a Pat<'a>,
// The number of references before the `Some` tuple.
// e.g. `&&Some(_)` has a ref count of 2.
ref_count: usize,
},
}
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.
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<'_>,
ref_count: usize,
ctxt: SyntaxContext,
) -> Option<OptionPat<'tcx>> {
match pat.kind {
PatKind::Wild => Some(OptionPat::Wild),
PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1, ctxt),
PatKind::Path(ref qpath) if is_res_lang_ctor(cx, cx.qpath_res(qpath, pat.hir_id), OptionNone) => {
Some(OptionPat::None)
},
PatKind::TupleStruct(ref qpath, [pattern], _)
if is_res_lang_ctor(cx, cx.qpath_res(qpath, pat.hir_id), OptionSome) && pat.span.ctxt() == ctxt =>
{
Some(OptionPat::Some { pattern, ref_count })
},
_ => None,
}
}
f(cx, pat, 0, ctxt)
}
// Checks for the `None` value.
fn is_none_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
is_res_lang_ctor(cx, path_res(cx, peel_blocks(expr)), OptionNone)
}

40
clippy_lints/src/matches/mod.rs
View File

@ -1,7 +1,9 @@
mod collapsible_match;
mod infallible_destructuring_match;
mod manual_filter;
mod manual_map;
mod manual_unwrap_or;
mod manual_utils;
mod match_as_ref;
mod match_bool;
mod match_like_matches;
@ -898,6 +900,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.66.0"]
pub MANUAL_FILTER,
complexity,
"reimplentation of `filter`"
}
#[derive(Default)]
pub struct Matches {
msrv: Option<RustcVersion>,
@ -939,6 +969,7 @@ impl_lint_pass!(Matches => [
SIGNIFICANT_DROP_IN_SCRUTINEE,
TRY_ERR,
MANUAL_MAP,
MANUAL_FILTER,
]);
impl<'tcx> LateLintPass<'tcx> for Matches {
@ -988,6 +1019,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 +1046,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);
Sugg::NonParen(snip)
}
}

1
src/docs.rs
View File

@ -258,6 +258,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