Rollup merge of rust-lang#139628 - makai410:suggest-vis, r=compiler-errors

Suggest remove redundant `$()?` around `vis`

Resolves: rust-lang#139480 .

Author: jhpratt

compiler/rustc_expand/src/mbe/macro_rules.rs

@@ -12,7 +12,7 @@ use rustc_ast::{self as ast, DUMMY_NODE_ID, NodeId};
 use rustc_ast_pretty::pprust;
 use rustc_attr_parsing::{AttributeKind, find_attr};
 use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
-use rustc_errors::{Applicability, ErrorGuaranteed};
+use rustc_errors::{Applicability, Diag, ErrorGuaranteed};
 use rustc_feature::Features;
 use rustc_hir as hir;
 use rustc_lint_defs::BuiltinLintDiag;
@@ -27,19 +27,18 @@ use rustc_span::hygiene::Transparency;
 use rustc_span::{Ident, MacroRulesNormalizedIdent, Span, kw, sym};
 use tracing::{debug, instrument, trace, trace_span};
 
-use super::diagnostics;
 use super::macro_parser::{NamedMatches, NamedParseResult};
+use super::{SequenceRepetition, diagnostics};
 use crate::base::{
     DummyResult, ExpandResult, ExtCtxt, MacResult, MacroExpanderResult, SyntaxExtension,
     SyntaxExtensionKind, TTMacroExpander,
 };
 use crate::expand::{AstFragment, AstFragmentKind, ensure_complete_parse, parse_ast_fragment};
-use crate::mbe;
 use crate::mbe::diagnostics::{annotate_doc_comment, parse_failure_msg};
-use crate::mbe::macro_check;
 use crate::mbe::macro_parser::NamedMatch::*;
 use crate::mbe::macro_parser::{Error, ErrorReported, Failure, MatcherLoc, Success, TtParser};
 use crate::mbe::transcribe::transcribe;
+use crate::mbe::{self, KleeneOp, macro_check};
 
 pub(crate) struct ParserAnyMacro<'a> {
     parser: Parser<'a>,
@@ -640,6 +639,37 @@ fn is_empty_token_tree(sess: &Session, seq: &mbe::SequenceRepetition) -> bool {
     }
 }
 
+/// Checks if a `vis` nonterminal fragment is unnecessarily wrapped in an optional repetition.
+///
+/// When a `vis` fragment (which can already be empty) is wrapped in `$(...)?`,
+/// this suggests removing the redundant repetition syntax since it provides no additional benefit.
+fn check_redundant_vis_repetition(
+    err: &mut Diag<'_>,
+    sess: &Session,
+    seq: &SequenceRepetition,
+    span: &DelimSpan,
+) {
+    let is_zero_or_one: bool = seq.kleene.op == KleeneOp::ZeroOrOne;
+    let is_vis = seq.tts.first().map_or(false, |tt| {
+        matches!(tt, mbe::TokenTree::MetaVarDecl(_, _, Some(NonterminalKind::Vis)))
+    });
+
+    if is_vis && is_zero_or_one {
+        err.note("a `vis` fragment can already be empty");
+        err.multipart_suggestion(
+            "remove the `$(` and `)?`",
+            vec![
+                (
+                    sess.source_map().span_extend_to_prev_char_before(span.open, '$', true),
+                    "".to_string(),
+                ),
+                (span.close.with_hi(seq.kleene.span.hi()), "".to_string()),
+            ],
+            Applicability::MaybeIncorrect,
+        );
+    }
+}
+
 /// Checks that the lhs contains no repetition which could match an empty token
 /// tree, because then the matcher would hang indefinitely.
 fn check_lhs_no_empty_seq(sess: &Session, tts: &[mbe::TokenTree]) -> Result<(), ErrorGuaranteed> {
@@ -654,8 +684,10 @@ fn check_lhs_no_empty_seq(sess: &Session, tts: &[mbe::TokenTree]) -> Result<(),
             TokenTree::Sequence(span, seq) => {
                 if is_empty_token_tree(sess, seq) {
                     let sp = span.entire();
-                    let guar = sess.dcx().span_err(sp, "repetition matches empty token tree");
-                    return Err(guar);
+                    let mut err =
+                        sess.dcx().struct_span_err(sp, "repetition matches empty token tree");
+                    check_redundant_vis_repetition(&mut err, sess, seq, span);
+                    return Err(err.emit());
                 }
                 check_lhs_no_empty_seq(sess, &seq.tts)?
             }

compiler/rustc_span/src/source_map.rs

@@ -633,6 +633,24 @@ impl SourceMap {
         sp
     }
 
+    /// Extends the given `Span` to just before the previous occurrence of `c`. Return the same span
+    /// if an error occurred while retrieving the code snippet.
+    pub fn span_extend_to_prev_char_before(
+        &self,
+        sp: Span,
+        c: char,
+        accept_newlines: bool,
+    ) -> Span {
+        if let Ok(prev_source) = self.span_to_prev_source(sp) {
+            let prev_source = prev_source.rsplit(c).next().unwrap_or("");
+            if accept_newlines || !prev_source.contains('\n') {
+                return sp.with_lo(BytePos(sp.lo().0 - prev_source.len() as u32 - 1_u32));
+            }
+        }
+
+        sp
+    }
+
     /// Extends the given `Span` to just after the previous occurrence of `pat` when surrounded by
     /// whitespace. Returns None if the pattern could not be found or if an error occurred while
     /// retrieving the code snippet.

tests/ui/macros/remove-repetition-issue-139480.rs

@@ -0,0 +1,28 @@
+macro_rules! ciallo {
+    ($($v: vis)? $name: ident) => {
+    //~^ error: repetition matches empty token tree
+    };
+}
+
+macro_rules! meow {
+    ($name: ident $($v: vis)?) => {
+    //~^ error: repetition matches empty token tree
+    };
+}
+
+macro_rules! gbc {
+    ($name: ident $/*
+        this comment gets removed by the suggestion
+        */
+        ($v: vis)?) => {
+    //~^ error: repetition matches empty token tree
+    };
+}
+
+ciallo!(hello);
+
+meow!(miaow, pub);
+
+gbc!(mygo,);
+
+fn main() {}

tests/ui/macros/remove-repetition-issue-139480.stderr

@@ -0,0 +1,44 @@
+error: repetition matches empty token tree
+  --> $DIR/remove-repetition-issue-139480.rs:2:7
+   |
+LL |     ($($v: vis)? $name: ident) => {
+   |       ^^^^^^^^^
+   |
+   = note: a `vis` fragment can already be empty
+help: remove the `$(` and `)?`
+   |
+LL -     ($($v: vis)? $name: ident) => {
+LL +     ($v: vis $name: ident) => {
+   |
+
+error: repetition matches empty token tree
+  --> $DIR/remove-repetition-issue-139480.rs:8:20
+   |
+LL |     ($name: ident $($v: vis)?) => {
+   |                    ^^^^^^^^^
+   |
+   = note: a `vis` fragment can already be empty
+help: remove the `$(` and `)?`
+   |
+LL -     ($name: ident $($v: vis)?) => {
+LL +     ($name: ident $v: vis) => {
+   |
+
+error: repetition matches empty token tree
+  --> $DIR/remove-repetition-issue-139480.rs:17:9
+   |
+LL |         ($v: vis)?) => {
+   |         ^^^^^^^^^
+   |
+   = note: a `vis` fragment can already be empty
+help: remove the `$(` and `)?`
+   |
+LL -     ($name: ident $/*
+LL -         this comment gets removed by the suggestion
+LL -         */
+LL -         ($v: vis)?) => {
+LL +     ($name: ident $v: vis) => {
+   |
+
+error: aborting due to 3 previous errors
+