Skip to content

[redundant_pattern_matching]: include guard in suggestion #11175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 52 additions & 18 deletions clippy_lints/src/matches/redundant_pattern_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,20 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::{snippet, walk_span_to_context};
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::{is_type_diagnostic_item, needs_ordered_drop};
use clippy_utils::visitors::any_temporaries_need_ordered_drop;
use clippy_utils::visitors::{any_temporaries_need_ordered_drop, for_each_expr};
use clippy_utils::{higher, is_expn_of, is_trait_method};
use if_chain::if_chain;
use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::LangItem::{self, OptionNone, OptionSome, PollPending, PollReady, ResultErr, ResultOk};
use rustc_hir::{Arm, Expr, ExprKind, Node, Pat, PatKind, QPath, UnOp};
use rustc_hir::{Arm, Expr, ExprKind, Guard, Node, Pat, PatKind, QPath, UnOp};
use rustc_lint::LateContext;
use rustc_middle::ty::subst::GenericArgKind;
use rustc_middle::ty::{self, Ty};
use rustc_span::{sym, Symbol};
use std::fmt::Write;
use std::ops::ControlFlow;

pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr) {
Expand Down Expand Up @@ -202,30 +204,58 @@ pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, op
if arms.len() == 2 {
let node_pair = (&arms[0].pat.kind, &arms[1].pat.kind);

if let Some(good_method) = found_good_method(cx, arms, node_pair) {
if let Some((good_method, maybe_guard)) = found_good_method(cx, arms, node_pair) {
let span = is_expn_of(expr.span, "matches").unwrap_or(expr.span.to(op.span));
let result_expr = match &op.kind {
ExprKind::AddrOf(_, _, borrowed) => borrowed,
_ => op,
};
let mut sugg = format!("{}.{good_method}", snippet(cx, result_expr.span, "_"));

if let Some(guard) = maybe_guard {
let Guard::If(guard) = *guard else { return }; // `...is_none() && let ...` is a syntax error

// wow, the HIR for match guards in `PAT if let PAT = expr && expr => ...` is annoying!
// `guard` here is `Guard::If` with the let expression somewhere deep in the tree of exprs,
// counter to the intuition that it should be `Guard::IfLet`, so we need another check
// to see that there aren't any let chains anywhere in the guard, as that would break
// if we suggest `t.is_none() && (let X = y && z)` for:
// `match t { None if let X = y && z => true, _ => false }`
Comment on lines +218 to +223
Copy link
Member Author

@y21 y21 Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this comment might be a bit confusing. If you ( the reviewer(s) ) already understand what this means, feel free to ignore.
Looking at the HIR here: https://godbolt.org/z/8413T1rjP (line 521-522), the guard is not Guard::IfLet, but Guard::If, with the let expression being potentially somewhere deep in the expression tree, so just checking Guard::If is not enough -- it may still be an if let guard despite the guard not being Guard::IfLet

Copy link
Member

@Centri3 Centri3 Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perhaps because it's desugared to something akin a let-chain, or at least something very similar, which is like And<Let, And<Expr, And<Expr, Let>>> or something in the HIR, with Expr being a regular && and Let being && let, so you can probably check the first in Binary::And afaik but perhaps this would have issues on something like if true && let Some(x) = x, I'm not sure. Better safe than sorry though so the current way is fine

Just if let Some(_) = x on its own is IfLet. I suppose it's because IfLet only allows one Let, while it needs a BinaryKind::And for a let chain to be represented

let has_nested_let_chain = for_each_expr(guard, |expr| {
if matches!(expr.kind, ExprKind::Let(..)) {
ControlFlow::Break(())
} else {
ControlFlow::Continue(())
}
})
.is_some();

if has_nested_let_chain {
return;
}

let guard = Sugg::hir(cx, guard, "..");
let _ = write!(sugg, " && {}", guard.maybe_par());
}

span_lint_and_sugg(
cx,
REDUNDANT_PATTERN_MATCHING,
span,
&format!("redundant pattern matching, consider using `{good_method}`"),
"try",
format!("{}.{good_method}", snippet(cx, result_expr.span, "_")),
sugg,
Applicability::MachineApplicable,
);
}
}
}

fn found_good_method<'a>(
fn found_good_method<'tcx>(
cx: &LateContext<'_>,
arms: &[Arm<'_>],
arms: &'tcx [Arm<'tcx>],
node: (&PatKind<'_>, &PatKind<'_>),
) -> Option<&'a str> {
) -> Option<(&'static str, Option<&'tcx Guard<'tcx>>)> {
match node {
(
PatKind::TupleStruct(ref path_left, patterns_left, _),
Expand Down Expand Up @@ -311,7 +341,11 @@ fn get_ident(path: &QPath<'_>) -> Option<rustc_span::symbol::Ident> {
}
}

fn get_good_method<'a>(cx: &LateContext<'_>, arms: &[Arm<'_>], path_left: &QPath<'_>) -> Option<&'a str> {
fn get_good_method<'tcx>(
cx: &LateContext<'_>,
arms: &'tcx [Arm<'tcx>],
path_left: &QPath<'_>,
) -> Option<(&'static str, Option<&'tcx Guard<'tcx>>)> {
if let Some(name) = get_ident(path_left) {
return match name.as_str() {
"Ok" => {
Expand Down Expand Up @@ -377,16 +411,16 @@ fn is_pat_variant(cx: &LateContext<'_>, pat: &Pat<'_>, path: &QPath<'_>, expecte
}

#[expect(clippy::too_many_arguments)]
fn find_good_method_for_match<'a>(
fn find_good_method_for_match<'a, 'tcx>(
cx: &LateContext<'_>,
arms: &[Arm<'_>],
arms: &'tcx [Arm<'tcx>],
path_left: &QPath<'_>,
path_right: &QPath<'_>,
expected_item_left: Item,
expected_item_right: Item,
should_be_left: &'a str,
should_be_right: &'a str,
) -> Option<&'a str> {
) -> Option<(&'a str, Option<&'tcx Guard<'tcx>>)> {
let first_pat = arms[0].pat;
let second_pat = arms[1].pat;

Expand All @@ -404,22 +438,22 @@ fn find_good_method_for_match<'a>(

match body_node_pair {
(ExprKind::Lit(lit_left), ExprKind::Lit(lit_right)) => match (&lit_left.node, &lit_right.node) {
(LitKind::Bool(true), LitKind::Bool(false)) => Some(should_be_left),
(LitKind::Bool(false), LitKind::Bool(true)) => Some(should_be_right),
(LitKind::Bool(true), LitKind::Bool(false)) => Some((should_be_left, arms[0].guard.as_ref())),
(LitKind::Bool(false), LitKind::Bool(true)) => Some((should_be_right, arms[1].guard.as_ref())),
_ => None,
},
_ => None,
}
}

fn find_good_method_for_matches_macro<'a>(
fn find_good_method_for_matches_macro<'a, 'tcx>(
cx: &LateContext<'_>,
arms: &[Arm<'_>],
arms: &'tcx [Arm<'tcx>],
path_left: &QPath<'_>,
expected_item_left: Item,
should_be_left: &'a str,
should_be_right: &'a str,
) -> Option<&'a str> {
) -> Option<(&'a str, Option<&'tcx Guard<'tcx>>)> {
let first_pat = arms[0].pat;

let body_node_pair = if is_pat_variant(cx, first_pat, path_left, expected_item_left) {
Expand All @@ -430,8 +464,8 @@ fn find_good_method_for_matches_macro<'a>(

match body_node_pair {
(ExprKind::Lit(lit_left), ExprKind::Lit(lit_right)) => match (&lit_left.node, &lit_right.node) {
(LitKind::Bool(true), LitKind::Bool(false)) => Some(should_be_left),
(LitKind::Bool(false), LitKind::Bool(true)) => Some(should_be_right),
(LitKind::Bool(true), LitKind::Bool(false)) => Some((should_be_left, arms[0].guard.as_ref())),
(LitKind::Bool(false), LitKind::Bool(true)) => Some((should_be_right, arms[1].guard.as_ref())),
_ => None,
},
_ => None,
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/redundant_pattern_matching_option.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@
clippy::equatable_if_let,
clippy::if_same_then_else
)]
#![feature(let_chains, if_let_guard)]

fn issue_11174<T>(boolean: bool, maybe_some: Option<T>) -> bool {
maybe_some.is_none() && (!boolean)
}

fn issue_11174_edge_cases<T>(boolean: bool, boolean2: bool, maybe_some: Option<T>) {
let _ = maybe_some.is_none() && (boolean || boolean2); // guard needs parentheses
let _ = match maybe_some { // can't use `matches!` here
// because `expr` metavars in macros don't allow let exprs
None if let Some(x) = Some(0) && x > 5 => true,
_ => false
};
}

fn main() {
if None::<()>.is_none() {}
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/redundant_pattern_matching_option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@
clippy::equatable_if_let,
clippy::if_same_then_else
)]
#![feature(let_chains, if_let_guard)]

fn issue_11174<T>(boolean: bool, maybe_some: Option<T>) -> bool {
matches!(maybe_some, None if !boolean)
}

fn issue_11174_edge_cases<T>(boolean: bool, boolean2: bool, maybe_some: Option<T>) {
let _ = matches!(maybe_some, None if boolean || boolean2); // guard needs parentheses
let _ = match maybe_some { // can't use `matches!` here
// because `expr` metavars in macros don't allow let exprs
None if let Some(x) = Some(0) && x > 5 => true,
_ => false
};
}

fn main() {
if let None = None::<()> {}
Expand Down
Loading