Skip to content
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.

Sign up for GitHub

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

Reinstate single_match/single_match_else lints with comments #14420

Merged
merged 1 commit into from
Mar 18, 2025
+175 −75
Merged

Reinstate single_match/single_match_else lints with comments #14420

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 3 additions & 5 deletions clippy_lints/src/matches/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -1110,11 +1110,9 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
}
}
}
// If there are still comments, it means they are outside of the arms, therefore
// we should not lint.
if match_comments.is_empty() {
single_match::check(cx, ex, arms, expr);
}
// If there are still comments, it means they are outside of the arms. Tell the lint
// code about it.
single_match::check(cx, ex, arms, expr, !match_comments.is_empty());
match_bool::check(cx, ex, arms, expr);
overlapping_arms::check(cx, ex, arms);
match_wild_enum::check(cx, ex, arms);
38 changes: 30 additions & 8 deletions clippy_lints/src/matches/single_match.rs
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::{SpanRangeExt, expr_block, snippet, snippet_block_with_context};
use clippy_utils::ty::implements_trait;
use clippy_utils::{
is_lint_allowed, is_unit_expr, peel_blocks, peel_hir_pat_refs, peel_middle_ty_refs, peel_n_hir_expr_refs,
};
use core::ops::ControlFlow;
use rustc_arena::DroplessArena;
use rustc_errors::Applicability;
use rustc_errors::{Applicability, Diag};
use rustc_hir::def::{DefKind, Res};
use rustc_hir::intravisit::{Visitor, walk_pat};
use rustc_hir::{Arm, Expr, ExprKind, HirId, Node, Pat, PatExpr, PatExprKind, PatKind, QPath, StmtKind};
@@ -32,7 +32,7 @@ fn empty_arm_has_comment(cx: &LateContext<'_>, span: Span) -> bool {
}

#[rustfmt::skip]
pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, ex: &'tcx Expr<'_>, arms: &'tcx [Arm<'_>], expr: &'tcx Expr<'_>) {
pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, ex: &'tcx Expr<'_>, arms: &'tcx [Arm<'_>], expr: &'tcx Expr<'_>, contains_comments: bool) {
if let [arm1, arm2] = arms
&& arm1.guard.is_none()
&& arm2.guard.is_none()
@@ -77,15 +77,31 @@ pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, ex: &'tcx Expr<'_>, arms: &'tc
}
}

report_single_pattern(cx, ex, arm1, expr, els);
report_single_pattern(cx, ex, arm1, expr, els, contains_comments);
}
}
}

fn report_single_pattern(cx: &LateContext<'_>, ex: &Expr<'_>, arm: &Arm<'_>, expr: &Expr<'_>, els: Option<&Expr<'_>>) {
fn report_single_pattern(
cx: &LateContext<'_>,
ex: &Expr<'_>,
arm: &Arm<'_>,
expr: &Expr<'_>,
els: Option<&Expr<'_>>,
contains_comments: bool,
) {
let lint = if els.is_some() { SINGLE_MATCH_ELSE } else { SINGLE_MATCH };
let ctxt = expr.span.ctxt();
let mut app = Applicability::MachineApplicable;
let note = |diag: &mut Diag<'_, ()>| {
if contains_comments {
diag.note("you might want to preserve the comments from inside the `match`");
}
};
let mut app = if contains_comments {
Applicability::MaybeIncorrect
} else {
Applicability::MachineApplicable
};
let els_str = els.map_or(String::new(), |els| {
format!(" else {}", expr_block(cx, els, ctxt, "..", Some(expr.span), &mut app))
});
@@ -109,7 +125,10 @@ fn report_single_pattern(cx: &LateContext<'_>, ex: &Expr<'_>, arm: &Arm<'_>, exp
}
(sugg, "try")
};
span_lint_and_sugg(cx, lint, expr.span, msg, help, sugg.to_string(), app);
span_lint_and_then(cx, lint, expr.span, msg, |diag| {
diag.span_suggestion(expr.span, help, sugg.to_string(), app);
note(diag);
});
return;
}

@@ -162,7 +181,10 @@ fn report_single_pattern(cx: &LateContext<'_>, ex: &Expr<'_>, arm: &Arm<'_>, exp
(msg, sugg)
};

span_lint_and_sugg(cx, lint, expr.span, msg, "try", sugg, app);
span_lint_and_then(cx, lint, expr.span, msg, |diag| {
diag.span_suggestion(expr.span, "try", sugg.to_string(), app);
note(diag);
});
}

struct PatVisitor<'tcx> {
34 changes: 12 additions & 22 deletions tests/ui/single_match.fixed
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//@require-annotations-for-level: WARN
#![warn(clippy::single_match)]
#![allow(
unused,
@@ -18,13 +19,9 @@ fn single_match() {
//~^^^^^^ single_match

let x = Some(1u8);
match x {
// Note the missing block braces.
// We suggest `if let Some(y) = x { .. }` because the macro
// is expanded before we can do anything.
Some(y) => println!("{:?}", y),
_ => (),
}
if let Some(y) = x { println!("{:?}", y) }
//~^^^^^^^ single_match
//~| NOTE: you might want to preserve the comments from inside the `match`

let z = (1u8, 1u8);
if let (2..=3, 7..=9) = z { dummy() };
@@ -358,21 +355,14 @@ fn irrefutable_match() {

let mut x = vec![1i8];

// Should not lint.
match x.pop() {
// bla
Some(u) => println!("{u}"),
// more comments!
None => {},
}
// Should not lint.
match x.pop() {
// bla
Some(u) => {
// bla
println!("{u}");
},
if let Some(u) = x.pop() { println!("{u}") }
//~^^^^^^ single_match
//~| NOTE: you might want to preserve the comments from inside the `match`

if let Some(u) = x.pop() {
// bla
None => {},
println!("{u}");
}
//~^^^^^^^^^ single_match
//~| NOTE: you might want to preserve the comments from inside the `match`
}
10 changes: 8 additions & 2 deletions tests/ui/single_match.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//@require-annotations-for-level: WARN
#![warn(clippy::single_match)]
#![allow(
unused,
@@ -28,6 +29,8 @@ fn single_match() {
Some(y) => println!("{:?}", y),
_ => (),
}
//~^^^^^^^ single_match
//~| NOTE: you might want to preserve the comments from inside the `match`

let z = (1u8, 1u8);
match z {
@@ -437,14 +440,15 @@ fn irrefutable_match() {

let mut x = vec![1i8];

// Should not lint.
match x.pop() {
// bla
Some(u) => println!("{u}"),
// more comments!
None => {},
}
// Should not lint.
//~^^^^^^ single_match
//~| NOTE: you might want to preserve the comments from inside the `match`

match x.pop() {
// bla
Some(u) => {
@@ -454,4 +458,6 @@ fn irrefutable_match() {
// bla
None => {},
}
//~^^^^^^^^^ single_match
//~| NOTE: you might want to preserve the comments from inside the `match`
}
97 changes: 70 additions & 27 deletions tests/ui/single_match.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:15:5
--> tests/ui/single_match.rs:16:5
|
LL | / match x {
LL | | Some(y) => {
@@ -19,7 +19,18 @@ LL ~ };
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:33:5
--> tests/ui/single_match.rs:25:5
|
LL | / match x {
... |
LL | | _ => (),
LL | | }
| |_____^ help: try: `if let Some(y) = x { println!("{:?}", y) }`
|
= note: you might want to preserve the comments from inside the `match`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:36:5
|
LL | / match z {
LL | | (2..=3, 7..=9) => dummy(),
@@ -28,7 +39,7 @@ LL | | };
| |_____^ help: try: `if let (2..=3, 7..=9) = z { dummy() }`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:63:5
--> tests/ui/single_match.rs:66:5
|
LL | / match x {
LL | | Some(y) => dummy(),
@@ -37,7 +48,7 @@ LL | | };
| |_____^ help: try: `if let Some(y) = x { dummy() }`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:69:5
--> tests/ui/single_match.rs:72:5
|
LL | / match y {
LL | | Ok(y) => dummy(),
@@ -46,7 +57,7 @@ LL | | };
| |_____^ help: try: `if let Ok(y) = y { dummy() }`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:77:5
--> tests/ui/single_match.rs:80:5
|
LL | / match c {
LL | | Cow::Borrowed(..) => dummy(),
@@ -55,7 +66,7 @@ LL | | };
| |_____^ help: try: `if let Cow::Borrowed(..) = c { dummy() }`

error: you seem to be trying to use `match` for an equality check. Consider using `if`
--> tests/ui/single_match.rs:99:5
--> tests/ui/single_match.rs:102:5
|
LL | / match x {
LL | | "test" => println!(),
@@ -64,7 +75,7 @@ LL | | }
| |_____^ help: try: `if x == "test" { println!() }`

error: you seem to be trying to use `match` for an equality check. Consider using `if`
--> tests/ui/single_match.rs:113:5
--> tests/ui/single_match.rs:116:5
|
LL | / match x {
LL | | Foo::A => println!(),
@@ -73,7 +84,7 @@ LL | | }
| |_____^ help: try: `if x == Foo::A { println!() }`

error: you seem to be trying to use `match` for an equality check. Consider using `if`
--> tests/ui/single_match.rs:120:5
--> tests/ui/single_match.rs:123:5
|
LL | / match x {
LL | | FOO_C => println!(),
@@ -82,7 +93,7 @@ LL | | }
| |_____^ help: try: `if x == FOO_C { println!() }`

error: you seem to be trying to use `match` for an equality check. Consider using `if`
--> tests/ui/single_match.rs:126:5
--> tests/ui/single_match.rs:129:5
|
LL | / match &&x {
LL | | Foo::A => println!(),
@@ -91,7 +102,7 @@ LL | | }
| |_____^ help: try: `if x == Foo::A { println!() }`

error: you seem to be trying to use `match` for an equality check. Consider using `if`
--> tests/ui/single_match.rs:133:5
--> tests/ui/single_match.rs:136:5
|
LL | / match &x {
LL | | Foo::A => println!(),
@@ -100,7 +111,7 @@ LL | | }
| |_____^ help: try: `if x == &Foo::A { println!() }`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:151:5
--> tests/ui/single_match.rs:154:5
|
LL | / match x {
LL | | Bar::A => println!(),
@@ -109,7 +120,7 @@ LL | | }
| |_____^ help: try: `if let Bar::A = x { println!() }`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:160:5
--> tests/ui/single_match.rs:163:5
|
LL | / match x {
LL | | None => println!(),
@@ -118,7 +129,7 @@ LL | | };
| |_____^ help: try: `if let None = x { println!() }`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:183:5
--> tests/ui/single_match.rs:186:5
|
LL | / match x {
LL | | (Some(_), _) => {},
@@ -127,7 +138,7 @@ LL | | }
| |_____^ help: try: `if let (Some(_), _) = x {}`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:190:5
--> tests/ui/single_match.rs:193:5
|
LL | / match x {
LL | | (Some(E::V), _) => todo!(),
@@ -136,7 +147,7 @@ LL | | }
| |_____^ help: try: `if let (Some(E::V), _) = x { todo!() }`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:197:5
--> tests/ui/single_match.rs:200:5
|
LL | / match (Some(42), Some(E::V), Some(42)) {
LL | | (.., Some(E::V), _) => {},
@@ -145,7 +156,7 @@ LL | | }
| |_____^ help: try: `if let (.., Some(E::V), _) = (Some(42), Some(E::V), Some(42)) {}`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:270:5
--> tests/ui/single_match.rs:273:5
|
LL | / match bar {
LL | | Some(v) => unsafe {
@@ -165,7 +176,7 @@ LL + } }
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:279:5
--> tests/ui/single_match.rs:282:5
|
LL | / match bar {
LL | | #[rustfmt::skip]
@@ -187,7 +198,7 @@ LL + }
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:360:5
--> tests/ui/single_match.rs:363:5
|
LL | / match Ok::<_, u32>(Some(A)) {
LL | | Ok(Some(A)) => println!(),
@@ -196,7 +207,7 @@ LL | | }
| |_____^ help: try: `if let Ok(Some(A)) = Ok::<_, u32>(Some(A)) { println!() }`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:376:5
--> tests/ui/single_match.rs:379:5
|
LL | / match &Some(A) {
LL | | Some(A | B) => println!(),
@@ -205,7 +216,7 @@ LL | | }
| |_____^ help: try: `if let Some(A | B) = &Some(A) { println!() }`

error: you seem to be trying to use `match` for an equality check. Consider using `if`
--> tests/ui/single_match.rs:384:5
--> tests/ui/single_match.rs:387:5
|
LL | / match &s[0..3] {
LL | | b"foo" => println!(),
@@ -214,7 +225,7 @@ LL | | }
| |_____^ help: try: `if &s[0..3] == b"foo" { println!() }`

error: this pattern is irrefutable, `match` is useless
--> tests/ui/single_match.rs:398:5
--> tests/ui/single_match.rs:401:5
|
LL | / match DATA {
LL | | DATA => println!(),
@@ -223,7 +234,7 @@ LL | | }
| |_____^ help: try: `println!();`

error: this pattern is irrefutable, `match` is useless
--> tests/ui/single_match.rs:404:5
--> tests/ui/single_match.rs:407:5
|
LL | / match CONST_I32 {
LL | | CONST_I32 => println!(),
@@ -232,7 +243,7 @@ LL | | }
| |_____^ help: try: `println!();`

error: this pattern is irrefutable, `match` is useless
--> tests/ui/single_match.rs:411:5
--> tests/ui/single_match.rs:414:5
|
LL | / match i {
LL | | i => {
@@ -252,7 +263,7 @@ LL + }
|

error: this pattern is irrefutable, `match` is useless
--> tests/ui/single_match.rs:420:5
--> tests/ui/single_match.rs:423:5
|
LL | / match i {
LL | | i => {},
@@ -261,7 +272,7 @@ LL | | }
| |_____^ help: `match` expression can be removed

error: this pattern is irrefutable, `match` is useless
--> tests/ui/single_match.rs:426:5
--> tests/ui/single_match.rs:429:5
|
LL | / match i {
LL | | i => (),
@@ -270,13 +281,45 @@ LL | | }
| |_____^ help: `match` expression can be removed

error: this pattern is irrefutable, `match` is useless
--> tests/ui/single_match.rs:432:5
--> tests/ui/single_match.rs:435:5
|
LL | / match CONST_I32 {
LL | | CONST_I32 => println!(),
LL | | _ => {},
LL | | }
| |_____^ help: try: `println!();`

error: aborting due to 26 previous errors
error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:443:5
|
LL | / match x.pop() {
LL | | // bla
LL | | Some(u) => println!("{u}"),
... |
LL | | }
| |_____^ help: try: `if let Some(u) = x.pop() { println!("{u}") }`
|
= note: you might want to preserve the comments from inside the `match`

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match.rs:452:5
|
LL | / match x.pop() {
LL | | // bla
LL | | Some(u) => {
... |
LL | | None => {},
LL | | }
| |_____^
|
= note: you might want to preserve the comments from inside the `match`
help: try
|
LL ~ if let Some(u) = x.pop() {
LL + // bla
LL + println!("{u}");
LL + }
|

error: aborting due to 29 previous errors

8 changes: 8 additions & 0 deletions tests/ui/single_match_else.fixed
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
//@aux-build: proc_macros.rs
//@require-annotations-for-level: WARN

#![warn(clippy::single_match_else)]
#![allow(unused, clippy::needless_return, clippy::no_effect, clippy::uninlined_format_args)]
@@ -90,6 +91,13 @@ fn main() {
}
//~^^^^^^^ single_match_else

if let Some(a) = Some(1) { println!("${:?}", a) } else {
println!("else block");
return;
}
//~^^^^^^^^ single_match_else
//~| NOTE: you might want to preserve the comments from inside the `match`

// lint here
use std::convert::Infallible;
if let Ok(a) = Result::<i32, &Infallible>::Ok(1) { println!("${:?}", a) } else {
12 changes: 12 additions & 0 deletions tests/ui/single_match_else.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
//@aux-build: proc_macros.rs
//@require-annotations-for-level: WARN

#![warn(clippy::single_match_else)]
#![allow(unused, clippy::needless_return, clippy::no_effect, clippy::uninlined_format_args)]
@@ -99,6 +100,17 @@ fn main() {
}
//~^^^^^^^ single_match_else

match Some(1) {
Some(a) => println!("${:?}", a),
// This is an inner comment
None => {
println!("else block");
return;
},
}
//~^^^^^^^^ single_match_else
//~| NOTE: you might want to preserve the comments from inside the `match`

// lint here
use std::convert::Infallible;
match Result::<i32, &Infallible>::Ok(1) {
43 changes: 32 additions & 11 deletions tests/ui/single_match_else.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match_else.rs:17:13
--> tests/ui/single_match_else.rs:18:13
|
LL | let _ = match ExprNode::Butterflies {
| _____________^
@@ -22,7 +22,7 @@ LL ~ };
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match_else.rs:83:5
--> tests/ui/single_match_else.rs:84:5
|
LL | / match Some(1) {
LL | | Some(a) => println!("${:?}", a),
@@ -42,7 +42,7 @@ LL + }
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match_else.rs:93:5
--> tests/ui/single_match_else.rs:94:5
|
LL | / match Some(1) {
LL | | Some(a) => println!("${:?}", a),
@@ -62,7 +62,28 @@ LL + }
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match_else.rs:104:5
--> tests/ui/single_match_else.rs:103:5
|
LL | / match Some(1) {
LL | | Some(a) => println!("${:?}", a),
LL | | // This is an inner comment
LL | | None => {
... |
LL | | },
LL | | }
| |_____^
|
= note: you might want to preserve the comments from inside the `match`
help: try
|
LL ~ if let Some(a) = Some(1) { println!("${:?}", a) } else {
LL + println!("else block");
LL + return;
LL + }
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match_else.rs:116:5
|
LL | / match Result::<i32, &Infallible>::Ok(1) {
LL | | Ok(a) => println!("${:?}", a),
@@ -81,7 +102,7 @@ LL + }
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match_else.rs:114:5
--> tests/ui/single_match_else.rs:126:5
|
LL | / match Cow::from("moo") {
LL | | Cow::Owned(a) => println!("${:?}", a),
@@ -100,7 +121,7 @@ LL + }
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match_else.rs:125:5
--> tests/ui/single_match_else.rs:137:5
|
LL | / match bar {
LL | | Some(v) => unsafe {
@@ -123,7 +144,7 @@ LL + }
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match_else.rs:137:5
--> tests/ui/single_match_else.rs:149:5
|
LL | / match bar {
LL | | Some(v) => {
@@ -147,7 +168,7 @@ LL + } }
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match_else.rs:150:5
--> tests/ui/single_match_else.rs:162:5
|
LL | / match bar {
LL | | Some(v) => unsafe {
@@ -171,7 +192,7 @@ LL + } }
|

error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
--> tests/ui/single_match_else.rs:163:5
--> tests/ui/single_match_else.rs:175:5
|
LL | / match bar {
LL | | #[rustfmt::skip]
@@ -196,7 +217,7 @@ LL + }
|

error: this pattern is irrefutable, `match` is useless
--> tests/ui/single_match_else.rs:213:5
--> tests/ui/single_match_else.rs:225:5
|
LL | / match ExprNode::Butterflies {
LL | | ExprNode::Butterflies => Some(&NODE),
@@ -207,5 +228,5 @@ LL | | },
LL | | }
| |_____^ help: try: `Some(&NODE)`

error: aborting due to 10 previous errors
error: aborting due to 11 previous errors