Rollup merge of rust-lang#137409 - estebank:match-arm, r=compiler-errors

Tweak comma handling of "missing match arm" suggestion and fix "remove this arm" suggestion, and make suggestion verbose

Better track trailing commas in match arms. Do not suggest adding trailing comma to match arm with block body. Better heuristic for "is this match in one line".

When encountering an unreachable match arm, (correctly) suggest removing the entire arm:

```
error: a never pattern is always unreachable
  --> $DIR/ICE-130779-never-arm-no-oatherwise-block.rs:10:20
   |
LL |         ! if true => {}
   |                      ^^ this will never be executed
   |
help: remove the unreachable match arm
   |
LL -         ! if true => {}
   |
```

Noticed in rust-lang#137343 (comment).

r? `@compiler-errors`

The first commit is independent of the second, but to make the second one produce accurate suggestions the span needs to include the trailing comma, hence the grouping of both changes in this PR.

Author: matthiaskrgr

compiler/rustc_ast_lowering/messages.ftl

@@ -133,11 +133,11 @@ ast_lowering_misplaced_relax_trait_bound =
 ast_lowering_never_pattern_with_body =
     a never pattern is always unreachable
     .label = this will never be executed
-    .suggestion = remove this expression
+    .suggestion = remove the match arm expression
 
 ast_lowering_never_pattern_with_guard =
     a guard on a never pattern will never be run
-    .suggestion = remove this guard
+    .suggestion = remove the match arm guard
 
 ast_lowering_no_precise_captures_on_apit = `use<...>` precise capturing syntax not allowed in argument-position `impl Trait`
 

compiler/rustc_ast_lowering/src/errors.rs

@@ -335,7 +335,7 @@ pub(crate) struct MisplacedRelaxTraitBound {
 pub(crate) struct MatchArmWithNoBody {
     #[primary_span]
     pub span: Span,
-    #[suggestion(code = " => todo!(),", applicability = "has-placeholders")]
+    #[suggestion(code = " => todo!(),", applicability = "has-placeholders", style = "verbose")]
     pub suggestion: Span,
 }
 
@@ -344,16 +344,18 @@ pub(crate) struct MatchArmWithNoBody {
 pub(crate) struct NeverPatternWithBody {
     #[primary_span]
     #[label]
-    #[suggestion(code = "", applicability = "maybe-incorrect")]
     pub span: Span,
+    #[suggestion(code = ",", applicability = "maybe-incorrect", style = "verbose")]
+    pub removal_span: Span,
 }
 
 #[derive(Diagnostic)]
 #[diag(ast_lowering_never_pattern_with_guard)]
 pub(crate) struct NeverPatternWithGuard {
     #[primary_span]
-    #[suggestion(code = "", applicability = "maybe-incorrect")]
     pub span: Span,
+    #[suggestion(code = ",", applicability = "maybe-incorrect", style = "verbose")]
+    pub removal_span: Span,
 }
 
 #[derive(Diagnostic)]

compiler/rustc_ast_lowering/src/expr.rs

@@ -676,6 +676,16 @@ impl<'hir> LoweringContext<'_, 'hir> {
         {
             self.lower_expr(body)
         } else {
+            let removal_span = |removal_span: Span| {
+                // Seek upwards in the macro call sites to see if we find the place where
+                // `pat!()` was called so that we can get the right span to remove.
+                let Some(pat_span) = pat.span.find_ancestor_in_same_ctxt(arm.span) else {
+                    return removal_span;
+                };
+                // - pat!() => {}
+                // + pat!(),
+                pat_span.shrink_to_hi().with_hi(arm.span.hi())
+            };
             // Either `body.is_none()` or `is_never_pattern` here.
             if !is_never_pattern {
                 if self.tcx.features().never_patterns() {
@@ -684,9 +694,15 @@ impl<'hir> LoweringContext<'_, 'hir> {
                     self.dcx().emit_err(MatchArmWithNoBody { span, suggestion });
                 }
             } else if let Some(body) = &arm.body {
-                self.dcx().emit_err(NeverPatternWithBody { span: body.span });
+                self.dcx().emit_err(NeverPatternWithBody {
+                    span: body.span,
+                    removal_span: removal_span(body.span),
+                });
             } else if let Some(g) = &arm.guard {
-                self.dcx().emit_err(NeverPatternWithGuard { span: g.span });
+                self.dcx().emit_err(NeverPatternWithGuard {
+                    span: g.span,
+                    removal_span: removal_span(g.span),
+                });
             }
 
             // We add a fake `loop {}` arm body so that it typecks to `!`. The mir lowering of never

compiler/rustc_ast_lowering/src/pat.rs

@@ -20,7 +20,8 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         self.arena.alloc(self.lower_pat_mut(pattern))
     }
 
-    fn lower_pat_mut(&mut self, mut pattern: &Pat) -> hir::Pat<'hir> {
+    fn lower_pat_mut(&mut self, outer_pattern: &Pat) -> hir::Pat<'hir> {
+        let mut pattern = outer_pattern;
         ensure_sufficient_stack(|| {
             // loop here to avoid recursion
             let pat_hir_id = self.lower_node_id(pattern.id);
@@ -147,7 +148,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                 }
             };
 
-            self.pat_with_node_id_of(pattern, node, pat_hir_id)
+            self.pat_with_node_id_of(outer_pattern, node, pat_hir_id)
         })
     }
 

compiler/rustc_mir_build/src/thir/pattern/check_match.rs

@@ -24,7 +24,7 @@ use rustc_session::lint::builtin::{
 };
 use rustc_span::edit_distance::find_best_match_for_name;
 use rustc_span::hygiene::DesugaringKind;
-use rustc_span::{Ident, Span};
+use rustc_span::{ExpnKind, Ident, Span};
 use rustc_trait_selection::infer::InferCtxtExt;
 use tracing::instrument;
 
@@ -543,6 +543,7 @@ impl<'p, 'tcx> MatchVisitor<'p, 'tcx> {
                     witnesses,
                     arms,
                     braces_span,
+                    expr_span,
                 ));
             }
         }
@@ -1210,6 +1211,7 @@ fn report_non_exhaustive_match<'p, 'tcx>(
     witnesses: Vec<WitnessPat<'p, 'tcx>>,
     arms: &[ArmId],
     braces_span: Option<Span>,
+    expr_span: Span,
 ) -> ErrorGuaranteed {
     let is_empty_match = arms.is_empty();
     let non_empty_enum = match scrut_ty.kind() {
@@ -1350,43 +1352,59 @@ fn report_non_exhaustive_match<'p, 'tcx>(
                 format!(" {{{indentation}{more}{suggested_arm},{indentation}}}",),
             ));
         }
-        [only] => {
+        [only] if let Some(braces_span) = braces_span => {
             let only = &thir[*only];
-            let (pre_indentation, is_multiline) = if let Some(snippet) =
-                sm.indentation_before(only.span)
-                && let Ok(with_trailing) =
-                    sm.span_extend_while(only.span, |c| c.is_whitespace() || c == ',')
-                && sm.is_multiline(with_trailing)
-            {
-                (format!("\n{snippet}"), true)
-            } else {
-                (" ".to_string(), false)
-            };
             let only_body = &thir[only.body];
-            let comma = if matches!(only_body.kind, ExprKind::Block { .. })
-                && only.span.eq_ctxt(only_body.span)
-                && is_multiline
+            let pre_indentation = if let Some(snippet) = sm.indentation_before(only.span)
+                && sm.is_multiline(braces_span)
             {
-                ""
+                format!("\n{snippet}")
             } else {
-                ","
+                " ".to_string()
+            };
+            let comma = match only_body.kind {
+                ExprKind::Block { .. } if only_body.span.eq_ctxt(sp) => "",
+                ExprKind::Scope { value, .. }
+                    if let expr = &thir[value]
+                        && let ExprKind::Block { .. } = expr.kind
+                        && expr.span.eq_ctxt(sp) =>
+                {
+                    ""
+                }
+                _ if sm
+                    .span_to_snippet(only.span)
+                    .map_or(false, |snippet| snippet.ends_with(",")) =>
+                {
+                    ""
+                }
+                _ => ",",
             };
             suggestion = Some((
                 only.span.shrink_to_hi(),
                 format!("{comma}{pre_indentation}{suggested_arm}"),
             ));
         }
-        [.., prev, last] => {
+        [.., prev, last] if braces_span.is_some() => {
             let prev = &thir[*prev];
             let last = &thir[*last];
             if prev.span.eq_ctxt(last.span) {
                 let last_body = &thir[last.body];
-                let comma = if matches!(last_body.kind, ExprKind::Block { .. })
-                    && last.span.eq_ctxt(last_body.span)
-                {
-                    ""
-                } else {
-                    ","
+                let comma = match last_body.kind {
+                    ExprKind::Block { .. } if last_body.span.eq_ctxt(sp) => "",
+                    ExprKind::Scope { value, .. }
+                        if let expr = &thir[value]
+                            && let ExprKind::Block { .. } = expr.kind
+                            && expr.span.eq_ctxt(sp) =>
+                    {
+                        ""
+                    }
+                    _ if sm
+                        .span_to_snippet(last.span)
+                        .map_or(false, |snippet| snippet.ends_with(",")) =>
+                    {
+                        ""
+                    }
+                    _ => ",",
                 };
                 let spacing = if sm.is_multiline(prev.span.between(last.span)) {
                     sm.indentation_before(last.span).map(|indent| format!("\n{indent}"))
@@ -1401,7 +1419,25 @@ fn report_non_exhaustive_match<'p, 'tcx>(
                 }
             }
         }
-        _ => {}
+        _ => {
+            if let Some(data) = expr_span.macro_backtrace().next()
+                && let ExpnKind::Macro(macro_kind, name) = data.kind
+            {
+                let macro_kind = macro_kind.descr();
+                // We don't want to point at the macro invocation place as that is already shown
+                // or talk about macro-backtrace and the macro's name, as we are already doing
+                // that as part of this note.
+                let mut span: MultiSpan = expr_span.with_ctxt(data.call_site.ctxt()).into();
+                span.push_span_label(data.def_site, "");
+                err.span_note(
+                    span,
+                    format!(
+                        "within {macro_kind} `{name}`, this `match` expression doesn't expand to \
+                         cover all patterns",
+                    ),
+                );
+            }
+        }
     }
 
     let msg = format!(

compiler/rustc_parse/src/parser/expr.rs

@@ -3074,6 +3074,7 @@ impl<'a> Parser<'a> {
             let (pat, guard) = this.parse_match_arm_pat_and_guard()?;
 
             let span_before_body = this.prev_token.span;
+            let mut comma = None;
             let arm_body;
             let is_fat_arrow = this.check(exp!(FatArrow));
             let is_almost_fat_arrow =
@@ -3143,6 +3144,7 @@ impl<'a> Parser<'a> {
                 {
                     let body = this.mk_expr_err(span, guar);
                     arm_body = Some(body);
+                    let _ = this.eat(exp!(Comma));
                     Ok(Recovered::Yes(guar))
                 } else {
                     let expr_span = expr.span;
@@ -3183,8 +3185,12 @@ impl<'a> Parser<'a> {
                     })
                 }
             };
+            if let TokenKind::Comma = this.prev_token.kind {
+                comma = Some(this.prev_token.span);
+            }
 
-            let hi_span = arm_body.as_ref().map_or(span_before_body, |body| body.span);
+            let hi_span =
+                comma.unwrap_or(arm_body.as_ref().map_or(span_before_body, |body| body.span));
             let arm_span = lo.to(hi_span);
 
             // We want to recover:

src/tools/clippy/clippy_lints/src/loops/single_element_loop.rs

@@ -71,7 +71,7 @@ pub(super) fn check<'tcx>(
     {
         let mut applicability = Applicability::MachineApplicable;
         let mut pat_snip = snippet_with_applicability(cx, pat.span, "..", &mut applicability);
-        if matches!(pat.kind, PatKind::Or(..)) {
+        if matches!(pat.kind, PatKind::Or(..)) && !pat_snip.starts_with("(") {
             pat_snip = format!("({pat_snip})").into();
         }
         let mut arg_snip = snippet_with_applicability(cx, arg_expression.span, "..", &mut applicability);

src/tools/clippy/tests/ui/match_same_arms.stderr

@@ -9,15 +9,15 @@ note: `_` wildcard arm here
   --> tests/ui/match_same_arms.rs:14:9
    |
 LL |         _ => 0,
-   |         ^^^^^^
+   |         ^^^^^^^
    = note: `-D clippy::match-same-arms` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::match_same_arms)]`
 
 error: this match arm has an identical body to another arm
   --> tests/ui/match_same_arms.rs:18:9
    |
 LL |         (1, .., 3) => 42,
-   |         ^^^^^^^^^^^^^^^^
+   |         ^^^^^^^^^^^^^^^^^
    |
    = help: try changing either arm body
 help: or try merging the arm patterns and removing the obsolete arm
@@ -30,7 +30,7 @@ error: this match arm has an identical body to another arm
   --> tests/ui/match_same_arms.rs:25:9
    |
 LL |         51 => 1,
-   |         ^^^^^^^
+   |         ^^^^^^^^
    |
    = help: try changing either arm body
 help: or try merging the arm patterns and removing the obsolete arm
@@ -44,7 +44,7 @@ error: this match arm has an identical body to another arm
   --> tests/ui/match_same_arms.rs:26:9
    |
 LL |         41 => 2,
-   |         ^^^^^^^
+   |         ^^^^^^^^
    |
    = help: try changing either arm body
 help: or try merging the arm patterns and removing the obsolete arm
@@ -57,7 +57,7 @@ error: this match arm has an identical body to another arm
   --> tests/ui/match_same_arms.rs:33:9
    |
 LL |         2 => 2,
-   |         ^^^^^^
+   |         ^^^^^^^
    |
    = help: try changing either arm body
 help: or try merging the arm patterns and removing the obsolete arm
@@ -71,7 +71,7 @@ error: this match arm has an identical body to another arm
   --> tests/ui/match_same_arms.rs:35:9
    |
 LL |         3 => 2,
-   |         ^^^^^^
+   |         ^^^^^^^
    |
    = help: try changing either arm body
 help: or try merging the arm patterns and removing the obsolete arm
@@ -85,7 +85,7 @@ error: this match arm has an identical body to another arm
   --> tests/ui/match_same_arms.rs:33:9
    |
 LL |         2 => 2,
-   |         ^^^^^^
+   |         ^^^^^^^
    |
    = help: try changing either arm body
 help: or try merging the arm patterns and removing the obsolete arm
@@ -99,7 +99,7 @@ error: this match arm has an identical body to another arm
   --> tests/ui/match_same_arms.rs:52:17
    |
 LL |                 CommandInfo::External { name, .. } => name.to_string(),
-   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: try changing either arm body
 help: or try merging the arm patterns and removing the obsolete arm