Fix a bug in MBE expansion that arose from incorrect fixing of an older bug in MBE

Specifically, #18744 was the PR that was supposed to fix the old bug, but it fixed it incorrectly (and didn't add a test!) The underlying reason was that we marked metavariables in expansions as joint if they were joint in the macro call, which is incorrect.

This wrong fix causes other bug, #19497, which this PR fixes by removing the old (incorrect) fix.

Author: ChayimFriedman2

crates/hir-def/src/macro_expansion_tests/mbe.rs

@@ -1979,3 +1979,51 @@ fn f() {
 "#]],
     );
 }
+
+#[test]
+fn semicolon_does_not_glue() {
+    check(
+        r#"
+macro_rules! bug {
+    ($id: expr) => {
+        true
+    };
+    ($id: expr; $($attr: ident),*) => {
+        true
+    };
+    ($id: expr; $($attr: ident),*; $norm: expr) => {
+        true
+    };
+    ($id: expr; $($attr: ident),*;; $print: expr) => {
+        true
+    };
+    ($id: expr; $($attr: ident),*; $norm: expr; $print: expr) => {
+        true
+    };
+}
+
+let _ = bug!(a;;;test);
+    "#,
+        expect![[r#"
+macro_rules! bug {
+    ($id: expr) => {
+        true
+    };
+    ($id: expr; $($attr: ident),*) => {
+        true
+    };
+    ($id: expr; $($attr: ident),*; $norm: expr) => {
+        true
+    };
+    ($id: expr; $($attr: ident),*;; $print: expr) => {
+        true
+    };
+    ($id: expr; $($attr: ident),*; $norm: expr; $print: expr) => {
+        true
+    };
+}
+
+let _ = true;
+    "#]],
+    );
+}

crates/hir-def/src/macro_expansion_tests/mbe/regression.rs

@@ -582,8 +582,8 @@ macro_rules! arbitrary {
 }
 
 impl <A: Arbitrary> $crate::arbitrary::Arbitrary for Vec<A> {
-    type Parameters = RangedParams1<A::Parameters>;
-    type Strategy = VecStrategy<A::Strategy>;
+    type Parameters = RangedParams1<A::Parameters> ;
+    type Strategy = VecStrategy<A::Strategy> ;
     fn arbitrary_with(args: Self::Parameters) -> Self::Strategy { {
             let product_unpack![range, a] = args;
             vec(any_with::<A>(a), range)

crates/ide/src/expand_macro.rs

@@ -677,4 +677,26 @@ crate::Foo;
 crate::Foo;"#]],
         );
     }
+
+    #[test]
+    fn semi_glueing() {
+        check(
+            r#"
+macro_rules! __log_value {
+    ($key:ident :$capture:tt =) => {};
+}
+
+macro_rules! __log {
+    ($key:tt $(:$capture:tt)? $(= $value:expr)?; $($arg:tt)+) => {
+        __log_value!($key $(:$capture)* = $($value)*);
+    };
+}
+
+__log!(written:%; "Test"$0);
+    "#,
+            expect![[r#"
+                __log!
+            "#]],
+        );
+    }
 }

crates/mbe/src/expander/transcriber.rs

@@ -389,8 +389,13 @@ fn expand_var(
     match ctx.bindings.get_fragment(v, id, &mut ctx.nesting, marker) {
         Ok(fragment) => {
             match fragment {
-                Fragment::Tokens(tt) => builder.extend_with_tt(tt.strip_invisible()),
-                Fragment::TokensOwned(tt) => builder.extend_with_tt(tt.view().strip_invisible()),
+                // rustc spacing is not like ours. Ours is like proc macros', it dictates how puncts will actually be joined.
+                // rustc uses them mostly for pretty printing. So we have to deviate a bit from what rustc does here.
+                // Basically, a metavariable can never be joined with whatever after it.
+                Fragment::Tokens(tt) => builder.extend_with_tt_alone(tt.strip_invisible()),
+                Fragment::TokensOwned(tt) => {
+                    builder.extend_with_tt_alone(tt.view().strip_invisible())
+                }
                 Fragment::Expr(sub) => {
                     let sub = sub.strip_invisible();
                     let mut span = id;
@@ -402,7 +407,7 @@ fn expand_var(
                     if wrap_in_parens {
                         builder.open(tt::DelimiterKind::Parenthesis, span);
                     }
-                    builder.extend_with_tt(sub);
+                    builder.extend_with_tt_alone(sub);
                     if wrap_in_parens {
                         builder.close(span);
                     }

crates/mbe/src/parser.rs

@@ -6,7 +6,10 @@ use std::sync::Arc;
 use arrayvec::ArrayVec;
 use intern::{Symbol, sym};
 use span::{Edition, Span, SyntaxContext};
-use tt::iter::{TtElement, TtIter};
+use tt::{
+    MAX_GLUED_PUNCT_LEN,
+    iter::{TtElement, TtIter},
+};
 
 use crate::ParseError;
 
@@ -96,7 +99,7 @@ pub(crate) enum Op {
         delimiter: tt::Delimiter<Span>,
     },
     Literal(tt::Literal<Span>),
-    Punct(Box<ArrayVec<tt::Punct<Span>, 3>>),
+    Punct(Box<ArrayVec<tt::Punct<Span>, MAX_GLUED_PUNCT_LEN>>),
     Ident(tt::Ident<Span>),
 }
 
@@ -151,7 +154,7 @@ pub(crate) enum MetaVarKind {
 pub(crate) enum Separator {
     Literal(tt::Literal<Span>),
     Ident(tt::Ident<Span>),
-    Puncts(ArrayVec<tt::Punct<Span>, 3>),
+    Puncts(ArrayVec<tt::Punct<Span>, MAX_GLUED_PUNCT_LEN>),
 }
 
 // Note that when we compare a Separator, we just care about its textual value.

crates/tt/src/iter.rs

@@ -6,7 +6,7 @@ use std::fmt;
 use arrayvec::ArrayVec;
 use intern::sym;
 
-use crate::{Ident, Leaf, Punct, Spacing, Subtree, TokenTree, TokenTreesView};
+use crate::{Ident, Leaf, MAX_GLUED_PUNCT_LEN, Punct, Spacing, Subtree, TokenTree, TokenTreesView};
 
 #[derive(Clone)]
 pub struct TtIter<'a, S> {
@@ -111,7 +111,7 @@ impl<'a, S: Copy> TtIter<'a, S> {
     ///
     /// This method currently may return a single quotation, which is part of lifetime ident and
     /// conceptually not a punct in the context of mbe. Callers should handle this.
-    pub fn expect_glued_punct(&mut self) -> Result<ArrayVec<Punct<S>, 3>, ()> {
+    pub fn expect_glued_punct(&mut self) -> Result<ArrayVec<Punct<S>, MAX_GLUED_PUNCT_LEN>, ()> {
         let TtElement::Leaf(&Leaf::Punct(first)) = self.next().ok_or(())? else {
             return Err(());
         };
@@ -145,7 +145,6 @@ impl<'a, S: Copy> TtIter<'a, S> {
             }
             ('-' | '!' | '*' | '/' | '&' | '%' | '^' | '+' | '<' | '=' | '>' | '|', '=', _)
             | ('-' | '=' | '>', '>', _)
-            | (_, _, Some(';'))
             | ('<', '-', _)
             | (':', ':', _)
             | ('.', '.', _)

crates/tt/src/lib.rs

@@ -22,6 +22,8 @@ use stdx::{impl_from, itertools::Itertools as _};
 
 pub use text_size::{TextRange, TextSize};
 
+pub const MAX_GLUED_PUNCT_LEN: usize = 3;
+
 #[derive(Clone, PartialEq, Debug)]
 pub struct Lit {
     pub kind: LitKind,
@@ -243,6 +245,23 @@ impl<S: Copy> TopSubtreeBuilder<S> {
         self.token_trees.extend(tt.0.iter().cloned());
     }
 
+    /// Like [`Self::extend_with_tt()`], but makes sure the new tokens will never be
+    /// joint with whatever comes after them.
+    pub fn extend_with_tt_alone(&mut self, tt: TokenTreesView<'_, S>) {
+        if let Some((last, before_last)) = tt.0.split_last() {
+            self.token_trees.reserve(tt.0.len());
+            self.token_trees.extend(before_last.iter().cloned());
+            let last = if let TokenTree::Leaf(Leaf::Punct(last)) = last {
+                let mut last = *last;
+                last.spacing = Spacing::Alone;
+                TokenTree::Leaf(Leaf::Punct(last))
+            } else {
+                last.clone()
+            };
+            self.token_trees.push(last);
+        }
+    }
+
     pub fn expected_delimiters(&self) -> impl Iterator<Item = &Delimiter<S>> {
         self.unclosed_subtree_indices.iter().rev().map(|&subtree_idx| {
             let TokenTree::Subtree(subtree) = &self.token_trees[subtree_idx] else {