diff options
| author | llogiq <bogusandre@gmail.com> | 2024-12-13 16:52:31 +0000 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2024-12-13 16:52:31 +0000 |
| commit | c607408df56146d875e292966a2ea1488bed4981 (patch) | |
| tree | 8c201d839868685df2bdcaec2a4a91ae17ab4fa0 | |
| parent | f2aed50873e8e908119044c283c33cf6ef0b66e0 (diff) | |
| parent | efe3fe9b8c5f16bbf39af7415bbde9441bc54dbb (diff) | |
| download | rust-c607408df56146d875e292966a2ea1488bed4981.tar.gz rust-c607408df56146d875e292966a2ea1488bed4981.zip | |
Fix `single_match` lint being emitted when it should not (#13765)
We realized when running `clippy --fix` on rustdoc (PR comment [here](https://github.com/rust-lang/rust/pull/133537/files#r1861377721)) that some comments were removed, which is problematic. This PR checks that comments outside of `match` arms are taken into account before emitting the lint. changelog: Fix `single_match` lint being emitted when it should not
| -rw-r--r-- | clippy_lints/src/matches/mod.rs | 27 | ||||
| -rw-r--r-- | clippy_utils/src/lib.rs | 14 | ||||
| -rw-r--r-- | tests/ui/single_match.fixed | 30 | ||||
| -rw-r--r-- | tests/ui/single_match.rs | 20 | ||||
| -rw-r--r-- | tests/ui/single_match.stderr | 16 |
5 files changed, 85 insertions, 22 deletions
diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 64969271764..1fd2ebcb54a 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -27,7 +27,9 @@ mod wild_in_or_pats; use clippy_config::Conf; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::source::walk_span_to_context; -use clippy_utils::{higher, is_direct_expn_of, is_in_const_context, is_span_match, span_contains_cfg}; +use clippy_utils::{ + higher, is_direct_expn_of, is_in_const_context, is_span_match, span_contains_cfg, span_extract_comments, +}; use rustc_hir::{Arm, Expr, ExprKind, LetStmt, MatchSource, Pat, PatKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; @@ -1059,7 +1061,28 @@ impl<'tcx> LateLintPass<'tcx> for Matches { } redundant_pattern_match::check_match(cx, expr, ex, arms); - single_match::check(cx, ex, arms, expr); + let source_map = cx.tcx.sess.source_map(); + let mut match_comments = span_extract_comments(source_map, expr.span); + // We remove comments from inside arms block. + if !match_comments.is_empty() { + for arm in arms { + for comment in span_extract_comments(source_map, arm.body.span) { + if let Some(index) = match_comments + .iter() + .enumerate() + .find(|(_, cm)| **cm == comment) + .map(|(index, _)| index) + { + match_comments.remove(index); + } + } + } + } + // 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); + } match_bool::check(cx, ex, arms, expr); overlapping_arms::check(cx, ex, arms); match_wild_enum::check(cx, ex, arms); diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 434c26d687d..d57b872c759 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -2977,12 +2977,18 @@ pub fn span_contains_comment(sm: &SourceMap, span: Span) -> bool { /// /// Comments are returned wrapped with their relevant delimiters pub fn span_extract_comment(sm: &SourceMap, span: Span) -> String { + span_extract_comments(sm, span).join("\n") +} + +/// Returns all the comments a given span contains. +/// +/// Comments are returned wrapped with their relevant delimiters. +pub fn span_extract_comments(sm: &SourceMap, span: Span) -> Vec<String> { let snippet = sm.span_to_snippet(span).unwrap_or_default(); - let res = tokenize_with_text(&snippet) + tokenize_with_text(&snippet) .filter(|(t, ..)| matches!(t, TokenKind::BlockComment { .. } | TokenKind::LineComment { .. })) - .map(|(_, s, _)| s) - .join("\n"); - res + .map(|(_, s, _)| s.to_string()) + .collect::<Vec<_>>() } pub fn span_find_starting_semi(sm: &SourceMap, span: Span) -> Span { diff --git a/tests/ui/single_match.fixed b/tests/ui/single_match.fixed index 4016b2699d6..3fb9afa86a5 100644 --- a/tests/ui/single_match.fixed +++ b/tests/ui/single_match.fixed @@ -17,7 +17,13 @@ fn single_match() { }; let x = Some(1u8); - if let Some(y) = x { println!("{:?}", y) } + 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), + _ => (), + } let z = (1u8, 1u8); if let (2..=3, 7..=9) = z { dummy() }; @@ -318,5 +324,25 @@ fn irrefutable_match() { - println!() + println!(); + + 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}"); + }, + // bla + None => {}, + } } diff --git a/tests/ui/single_match.rs b/tests/ui/single_match.rs index 75edaa60605..b7d2e011151 100644 --- a/tests/ui/single_match.rs +++ b/tests/ui/single_match.rs @@ -401,4 +401,24 @@ fn irrefutable_match() { CONST_I32 => println!(), _ => {}, } + + 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}"); + }, + // bla + None => {}, + } } diff --git a/tests/ui/single_match.stderr b/tests/ui/single_match.stderr index 9240b09c50a..d691b627abe 100644 --- a/tests/ui/single_match.stderr +++ b/tests/ui/single_match.stderr @@ -19,18 +19,6 @@ LL ~ }; | error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` - --> tests/ui/single_match.rs:23:5 - | -LL | / match x { -LL | | // Note the missing block braces. -LL | | // We suggest `if let Some(y) = x { .. }` because the macro -LL | | // is expanded before we can do anything. -LL | | Some(y) => println!("{:?}", y), -LL | | _ => (), -LL | | } - | |_____^ help: try: `if let Some(y) = x { println!("{:?}", y) }` - -error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` --> tests/ui/single_match.rs:32:5 | LL | / match z { @@ -279,7 +267,7 @@ LL | / match CONST_I32 { LL | | CONST_I32 => println!(), LL | | _ => {}, LL | | } - | |_____^ help: try: `println!()` + | |_____^ help: try: `println!();` -error: aborting due to 26 previous errors +error: aborting due to 25 previous errors |
