user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Reinstate single_match/single_match_else lints with comments (#14420)

Browse source 
Commit efe3fe9b8c removed the ability for
`single_match` and `single_match_else` to trigger if comments were
present outside of the arms, as those comments would be lost while
rewriting the `match` expression.

This reinstates the lint, but prevents the suggestion from being applied
automatically in the presence of comments by using the `MaybeIncorrect`
applicability. Also, a note is added to the lint message to warn the
user about the need to preserve the comments if acting upon the
suggestion.

changelog: [`single_match`, `single_match_else`]: reinstate lint when
comments are inside the `match` but do not autofix the code

Fix #14418
This commit is contained in:
Alex Macleod 2025-03-18 12:45:19 +00:00 • committed by GitHub
commit d443f38f73
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 175 additions and 75 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -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
clippy_lints/src/matches/single_match.rs
View file

@ -1,4 +1,4 @@
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::{
@ -6,7 +6,7 @@ use clippy_utils::{
};
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
tests/ui/single_match.fixed
View file

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

10
tests/ui/single_match.rs
View file

@ -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
tests/ui/single_match.stderr
View file

@ -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,7 +281,7 @@ 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!(),
@ -278,5 +289,37 @@ 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
tests/ui/single_match_else.fixed
View file

@ -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
tests/ui/single_match_else.rs
View file

@ -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
tests/ui/single_match_else.stderr
View file

@ -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