From e7fd44f213df3d8a8d7efcacc8e694a58046b824 Mon Sep 17 00:00:00 2001
From: y21 <30553356+y21@users.noreply.github.com>
Date: Mon, 17 Jul 2023 21:18:11 +0200
Subject: [PATCH] add guard to suggestion instead of not linting
---
.../src/matches/redundant_pattern_match.rs | 77 +++++++++++++------
.../redundant_pattern_matching_option.fixed | 12 ++-
tests/ui/redundant_pattern_matching_option.rs | 10 +++
.../redundant_pattern_matching_option.stderr | 74 ++++++++++--------
4 files changed, 118 insertions(+), 55 deletions(-)
diff --git a/clippy_lints/src/matches/redundant_pattern_match.rs b/clippy_lints/src/matches/redundant_pattern_match.rs
index a7c799f2de5..3994533322d 100644
--- a/clippy_lints/src/matches/redundant_pattern_match.rs
+++ b/clippy_lints/src/matches/redundant_pattern_match.rs
@@ -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) {
@@ -199,36 +201,61 @@ fn find_sugg_for_if_let<'tcx>(
}
pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, op: &Expr<'_>, arms: &[Arm<'_>]) {
- if let [arm1, arm2] = arms
- && arm1.guard.is_none()
- && arm2.guard.is_none()
- {
- let node_pair = (&arm1.pat.kind, &arm2.pat.kind);
+ 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 }`
+ 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, _),
@@ -314,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" => {
@@ -380,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;
@@ -407,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) {
@@ -433,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,
diff --git a/tests/ui/redundant_pattern_matching_option.fixed b/tests/ui/redundant_pattern_matching_option.fixed
index d50c204875f..d9fcd98c56e 100644
--- a/tests/ui/redundant_pattern_matching_option.fixed
+++ b/tests/ui/redundant_pattern_matching_option.fixed
@@ -10,9 +10,19 @@
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)
+ 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() {
diff --git a/tests/ui/redundant_pattern_matching_option.rs b/tests/ui/redundant_pattern_matching_option.rs
index ce365a87aa7..cbd9494f15a 100644
--- a/tests/ui/redundant_pattern_matching_option.rs
+++ b/tests/ui/redundant_pattern_matching_option.rs
@@ -10,11 +10,21 @@
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::<()> {}
diff --git a/tests/ui/redundant_pattern_matching_option.stderr b/tests/ui/redundant_pattern_matching_option.stderr
index 2d5e1a74e5e..b0e43924dbc 100644
--- a/tests/ui/redundant_pattern_matching_option.stderr
+++ b/tests/ui/redundant_pattern_matching_option.stderr
@@ -1,49 +1,61 @@
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:19:12
+ --> $DIR/redundant_pattern_matching_option.rs:16:5
|
-LL | if let None = None::<()> {}
- | -------^^^^------------- help: try: `if None::<()>.is_none()`
+LL | matches!(maybe_some, None if !boolean)
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `maybe_some.is_none() && (!boolean)`
|
= note: `-D clippy::redundant-pattern-matching` implied by `-D warnings`
+error: redundant pattern matching, consider using `is_none()`
+ --> $DIR/redundant_pattern_matching_option.rs:20:13
+ |
+LL | let _ = matches!(maybe_some, None if boolean || boolean2); // guard needs parentheses
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `maybe_some.is_none() && (boolean || boolean2)`
+
+error: redundant pattern matching, consider using `is_none()`
+ --> $DIR/redundant_pattern_matching_option.rs:29:12
+ |
+LL | if let None = None::<()> {}
+ | -------^^^^------------- help: try: `if None::<()>.is_none()`
+
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:21:12
+ --> $DIR/redundant_pattern_matching_option.rs:31:12
|
LL | if let Some(_) = Some(42) {}
| -------^^^^^^^----------- help: try: `if Some(42).is_some()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:23:12
+ --> $DIR/redundant_pattern_matching_option.rs:33:12
|
LL | if let Some(_) = Some(42) {
| -------^^^^^^^----------- help: try: `if Some(42).is_some()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:29:15
+ --> $DIR/redundant_pattern_matching_option.rs:39:15
|
LL | while let Some(_) = Some(42) {}
| ----------^^^^^^^----------- help: try: `while Some(42).is_some()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:31:15
+ --> $DIR/redundant_pattern_matching_option.rs:41:15
|
LL | while let None = Some(42) {}
| ----------^^^^----------- help: try: `while Some(42).is_none()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:33:15
+ --> $DIR/redundant_pattern_matching_option.rs:43:15
|
LL | while let None = None::<()> {}
| ----------^^^^------------- help: try: `while None::<()>.is_none()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:36:15
+ --> $DIR/redundant_pattern_matching_option.rs:46:15
|
LL | while let Some(_) = v.pop() {
| ----------^^^^^^^---------- help: try: `while v.pop().is_some()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:44:5
+ --> $DIR/redundant_pattern_matching_option.rs:54:5
|
LL | / match Some(42) {
LL | | Some(_) => true,
@@ -52,7 +64,7 @@ LL | | };
| |_____^ help: try: `Some(42).is_some()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:49:5
+ --> $DIR/redundant_pattern_matching_option.rs:59:5
|
LL | / match None::<()> {
LL | | Some(_) => false,
@@ -61,7 +73,7 @@ LL | | };
| |_____^ help: try: `None::<()>.is_none()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:54:13
+ --> $DIR/redundant_pattern_matching_option.rs:64:13
|
LL | let _ = match None::<()> {
| _____________^
@@ -71,55 +83,55 @@ LL | | };
| |_____^ help: try: `None::<()>.is_none()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:60:20
+ --> $DIR/redundant_pattern_matching_option.rs:70:20
|
LL | let _ = if let Some(_) = opt { true } else { false };
| -------^^^^^^^------ help: try: `if opt.is_some()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:66:20
+ --> $DIR/redundant_pattern_matching_option.rs:76:20
|
LL | let _ = if let Some(_) = gen_opt() {
| -------^^^^^^^------------ help: try: `if gen_opt().is_some()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:68:19
+ --> $DIR/redundant_pattern_matching_option.rs:78:19
|
LL | } else if let None = gen_opt() {
| -------^^^^------------ help: try: `if gen_opt().is_none()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:74:12
+ --> $DIR/redundant_pattern_matching_option.rs:84:12
|
LL | if let Some(..) = gen_opt() {}
| -------^^^^^^^^------------ help: try: `if gen_opt().is_some()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:89:12
+ --> $DIR/redundant_pattern_matching_option.rs:99:12
|
LL | if let Some(_) = Some(42) {}
| -------^^^^^^^----------- help: try: `if Some(42).is_some()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:91:12
+ --> $DIR/redundant_pattern_matching_option.rs:101:12
|
LL | if let None = None::<()> {}
| -------^^^^------------- help: try: `if None::<()>.is_none()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:93:15
+ --> $DIR/redundant_pattern_matching_option.rs:103:15
|
LL | while let Some(_) = Some(42) {}
| ----------^^^^^^^----------- help: try: `while Some(42).is_some()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:95:15
+ --> $DIR/redundant_pattern_matching_option.rs:105:15
|
LL | while let None = None::<()> {}
| ----------^^^^------------- help: try: `while None::<()>.is_none()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:97:5
+ --> $DIR/redundant_pattern_matching_option.rs:107:5
|
LL | / match Some(42) {
LL | | Some(_) => true,
@@ -128,7 +140,7 @@ LL | | };
| |_____^ help: try: `Some(42).is_some()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:102:5
+ --> $DIR/redundant_pattern_matching_option.rs:112:5
|
LL | / match None::<()> {
LL | | Some(_) => false,
@@ -137,19 +149,19 @@ LL | | };
| |_____^ help: try: `None::<()>.is_none()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:110:12
+ --> $DIR/redundant_pattern_matching_option.rs:120:12
|
LL | if let None = *(&None::<()>) {}
| -------^^^^----------------- help: try: `if (&None::<()>).is_none()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:111:12
+ --> $DIR/redundant_pattern_matching_option.rs:121:12
|
LL | if let None = *&None::<()> {}
| -------^^^^--------------- help: try: `if (&None::<()>).is_none()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:117:5
+ --> $DIR/redundant_pattern_matching_option.rs:127:5
|
LL | / match x {
LL | | Some(_) => true,
@@ -158,7 +170,7 @@ LL | | };
| |_____^ help: try: `x.is_some()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:122:5
+ --> $DIR/redundant_pattern_matching_option.rs:132:5
|
LL | / match x {
LL | | None => true,
@@ -167,7 +179,7 @@ LL | | };
| |_____^ help: try: `x.is_none()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:127:5
+ --> $DIR/redundant_pattern_matching_option.rs:137:5
|
LL | / match x {
LL | | Some(_) => false,
@@ -176,7 +188,7 @@ LL | | };
| |_____^ help: try: `x.is_none()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:132:5
+ --> $DIR/redundant_pattern_matching_option.rs:142:5
|
LL | / match x {
LL | | None => false,
@@ -185,16 +197,16 @@ LL | | };
| |_____^ help: try: `x.is_some()`
error: redundant pattern matching, consider using `is_some()`
- --> $DIR/redundant_pattern_matching_option.rs:147:13
+ --> $DIR/redundant_pattern_matching_option.rs:157:13
|
LL | let _ = matches!(x, Some(_));
| ^^^^^^^^^^^^^^^^^^^^ help: try: `x.is_some()`
error: redundant pattern matching, consider using `is_none()`
- --> $DIR/redundant_pattern_matching_option.rs:149:13
+ --> $DIR/redundant_pattern_matching_option.rs:159:13
|
LL | let _ = matches!(x, None);
| ^^^^^^^^^^^^^^^^^ help: try: `x.is_none()`
-error: aborting due to 28 previous errors
+error: aborting due to 30 previous errors