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

Author: y21

clippy_lints/src/matches/single_match.rs

@@ -1,16 +1,32 @@
 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;
 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;
         };
 

tests/ui/single_match.fixed

@@ -212,3 +212,29 @@ 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.
+            },
+        }
+    }
+}

tests/ui/single_match.rs

@@ -270,3 +270,29 @@ 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.
+            },
+        }
+    }
+}