Auto merge of #10990 - y21:issue8634-partial, r=blyxyas,xFrednet

[`single_match`]: don't lint if block contains comments

Fixes #8634

It now ignores matches with a comment in the "else" arm

changelog: [`single_match`]: don't lint if block contains comments

4 files changed, 105 insertions, 4 deletions

diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs
index 3fbd0867ea9..00fa3eb9bfe 100644
--- a/clippy_lints/src/matches/mod.rs
+++ b/clippy_lints/src/matches/mod.rs
@@ -38,6 +38,11 @@ declare_clippy_lint! {
     /// Checks for matches with a single arm where an `if let`
     /// will usually suffice.
     ///
+    /// This intentionally does not lint if there are comments
+    /// inside of the other arm, so as to allow the user to document
+    /// why having another explicit pattern with an empty body is necessary,
+    /// or because the comments need to be preserved for other reasons.
+    ///
     /// ### Why is this bad?
     /// Just readability – `if let` nests less than a `match`.
     ///
diff --git a/clippy_lints/src/matches/single_match.rs b/clippy_lints/src/matches/single_match.rs
index ad47c13896c..35627d6c649 100644
--- a/clippy_lints/src/matches/single_match.rs
+++ b/clippy_lints/src/matches/single_match.rs
@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::{expr_block, snippet};
+use clippy_utils::source::{expr_block, get_source_text, snippet};
 use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, peel_mid_ty_refs};
 use clippy_utils::{is_lint_allowed, is_unit_expr, is_wild, peel_blocks, peel_hir_pat_refs, peel_n_hir_expr_refs};
 use core::cmp::max;
@@ -7,10 +7,26 @@ use rustc_errors::Applicability;
 use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, Pat, PatKind};
 use rustc_lint::LateContext;
 use rustc_middle::ty::{self, Ty};
-use rustc_span::sym;
+use rustc_span::{sym, Span};
 
 use super::{MATCH_BOOL, SINGLE_MATCH, SINGLE_MATCH_ELSE};
 
+/// Checks if there are comments contained within a span.
+/// This is a very "naive" check, as it just looks for the literal characters // and /* in the
+/// source text. This won't be accurate if there are potentially expressions contained within the
+/// span, e.g. a string literal `"//"`, but we know that this isn't the case for empty
+/// match arms.
+fn empty_arm_has_comment(cx: &LateContext<'_>, span: Span) -> bool {
+    if let Some(ff) = get_source_text(cx, span)
+        && let Some(text) = ff.as_str()
+    {
+        text.as_bytes().windows(2)
+            .any(|w| w == b"//" || w == b"/*")
+    } else {
+        false
+    }
+}
+
 #[rustfmt::skip]
 pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>) {
     if arms.len() == 2 && arms[0].guard.is_none() && arms[1].guard.is_none() {
@@ -25,7 +41,7 @@ pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr:
             return;
         }
         let els = arms[1].body;
-        let els = if is_unit_expr(peel_blocks(els)) {
+        let els = if is_unit_expr(peel_blocks(els)) && !empty_arm_has_comment(cx, els.span) {
             None
         } else if let ExprKind::Block(Block { stmts, expr: block_expr, .. }, _) = els.kind {
             if stmts.len() == 1 && block_expr.is_none() || stmts.is_empty() && block_expr.is_some() {
@@ -35,7 +51,7 @@ pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr:
             // block with 2+ statements or 1 expr and 1+ statement
             Some(els)
         } else {
-            // not a block, don't lint
+            // not a block or an emtpy block w/ comments, don't lint
             return;
         };
 
diff --git a/tests/ui/single_match.fixed b/tests/ui/single_match.fixed
index a54e658bf23..e7b1fd6a85f 100644
--- a/tests/ui/single_match.fixed
+++ b/tests/ui/single_match.fixed
@@ -212,3 +212,43 @@ fn issue_10808(bar: Option<i32>) {
         }
     }
 }
+
+mod issue8634 {
+    struct SomeError(i32, i32);
+
+    fn foo(x: Result<i32, ()>) {
+        match x {
+            Ok(y) => {
+                println!("Yay! {y}");
+            },
+            Err(()) => {
+                // Ignore this error because blah blah blah.
+            },
+        }
+    }
+
+    fn bar(x: Result<i32, SomeError>) {
+        match x {
+            Ok(y) => {
+                println!("Yay! {y}");
+            },
+            Err(_) => {
+                // TODO: Process the error properly.
+            },
+        }
+    }
+
+    fn block_comment(x: Result<i32, SomeError>) {
+        match x {
+            Ok(y) => {
+                println!("Yay! {y}");
+            },
+            Err(_) => {
+                /*
+                let's make sure that this also
+                does not lint block comments.
+                */
+            },
+        }
+    }
+}
diff --git a/tests/ui/single_match.rs b/tests/ui/single_match.rs
index f296c6c4d1c..1515a7053e5 100644
--- a/tests/ui/single_match.rs
+++ b/tests/ui/single_match.rs
@@ -270,3 +270,43 @@ fn issue_10808(bar: Option<i32>) {
         _ => {},
     }
 }
+
+mod issue8634 {
+    struct SomeError(i32, i32);
+
+    fn foo(x: Result<i32, ()>) {
+        match x {
+            Ok(y) => {
+                println!("Yay! {y}");
+            },
+            Err(()) => {
+                // Ignore this error because blah blah blah.
+            },
+        }
+    }
+
+    fn bar(x: Result<i32, SomeError>) {
+        match x {
+            Ok(y) => {
+                println!("Yay! {y}");
+            },
+            Err(_) => {
+                // TODO: Process the error properly.
+            },
+        }
+    }
+
+    fn block_comment(x: Result<i32, SomeError>) {
+        match x {
+            Ok(y) => {
+                println!("Yay! {y}");
+            },
+            Err(_) => {
+                /*
+                let's make sure that this also
+                does not lint block comments.
+                */
+            },
+        }
+    }
+}