Fix syntax fixup producing invalid punctuation

Fixes #19206.
Fixes #18244.

5 files changed, 105 insertions, 88 deletions

diff --git a/src/tools/rust-analyzer/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs b/src/tools/rust-analyzer/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs
index c365a603d2a..138db1b498b 100644
--- a/src/tools/rust-analyzer/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs
+++ b/src/tools/rust-analyzer/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs
@@ -93,15 +93,15 @@ fn broken_parenthesis_sequence() {
 macro_rules! m1 { ($x:ident) => { ($x } }
 macro_rules! m2 { ($x:ident) => {} }
 
-m1!();
-m2!(x
+fn f1() { m1!(x); }
+fn f2() { m2!(x }
 "#,
         expect![[r#"
 macro_rules! m1 { ($x:ident) => { ($x } }
 macro_rules! m2 { ($x:ident) => {} }
 
-/* error: macro definition has parse errors */
-/* error: expected ident */
+fn f1() { (x); }
+fn f2() { /* error: expected ident */ }
 "#]],
     )
 }
diff --git a/src/tools/rust-analyzer/crates/hir-expand/src/fixup.rs b/src/tools/rust-analyzer/crates/hir-expand/src/fixup.rs
index eb430177390..28894537d48 100644
--- a/src/tools/rust-analyzer/crates/hir-expand/src/fixup.rs
+++ b/src/tools/rust-analyzer/crates/hir-expand/src/fixup.rs
@@ -148,7 +148,6 @@ pub(crate) fn fixup_syntax(
                     }
                     if it.then_branch().is_none() {
                         append.insert(node.clone().into(), vec![
-                            // FIXME: THis should be a subtree no?
                             Leaf::Punct(Punct {
                                 char: '{',
                                 spacing: Spacing::Alone,
@@ -179,7 +178,6 @@ pub(crate) fn fixup_syntax(
                     }
                     if it.loop_body().is_none() {
                         append.insert(node.clone().into(), vec![
-                            // FIXME: THis should be a subtree no?
                             Leaf::Punct(Punct {
                                 char: '{',
                                 spacing: Spacing::Alone,
@@ -196,7 +194,6 @@ pub(crate) fn fixup_syntax(
                 ast::LoopExpr(it) => {
                     if it.loop_body().is_none() {
                         append.insert(node.clone().into(), vec![
-                            // FIXME: THis should be a subtree no?
                             Leaf::Punct(Punct {
                                 char: '{',
                                 spacing: Spacing::Alone,
@@ -228,7 +225,6 @@ pub(crate) fn fixup_syntax(
                     if it.match_arm_list().is_none() {
                         // No match arms
                         append.insert(node.clone().into(), vec![
-                            // FIXME: THis should be a subtree no?
                             Leaf::Punct(Punct {
                                 char: '{',
                                 spacing: Spacing::Alone,
@@ -269,7 +265,6 @@ pub(crate) fn fixup_syntax(
 
                     if it.loop_body().is_none() {
                         append.insert(node.clone().into(), vec![
-                            // FIXME: THis should be a subtree no?
                             Leaf::Punct(Punct {
                                 char: '{',
                                 spacing: Spacing::Alone,
@@ -309,28 +304,6 @@ pub(crate) fn fixup_syntax(
                         }
                     }
                 },
-                ast::ArgList(it) => {
-                    if it.r_paren_token().is_none() {
-                        append.insert(node.into(), vec![
-                            Leaf::Punct(Punct {
-                                span: fake_span(node_range),
-                                char: ')',
-                                spacing: Spacing::Alone
-                            })
-                        ]);
-                    }
-                },
-                ast::ArgList(it) => {
-                    if it.r_paren_token().is_none() {
-                        append.insert(node.into(), vec![
-                            Leaf::Punct(Punct {
-                                span: fake_span(node_range),
-                                char: ')',
-                                spacing: Spacing::Alone
-                            })
-                        ]);
-                    }
-                },
                 ast::ClosureExpr(it) => {
                     if it.body().is_none() {
                         append.insert(node.into(), vec![
@@ -476,12 +449,12 @@ fn reverse_fixups_(tt: &mut TopSubtree, undo_info: &[TopSubtree]) {
             }
         }
         tt::TokenTree::Subtree(tt) => {
+            // fixup should only create matching delimiters, but proc macros
+            // could just copy the span to one of the delimiters. We don't want
+            // to leak the dummy ID, so we remove both.
             if tt.delimiter.close.anchor.ast_id == FIXUP_DUMMY_AST_ID
                 || tt.delimiter.open.anchor.ast_id == FIXUP_DUMMY_AST_ID
             {
-                // Even though fixup never creates subtrees with fixup spans, the old proc-macro server
-                // might copy them if the proc-macro asks for it, so we need to filter those out
-                // here as well.
                 return TransformTtAction::remove();
             }
             TransformTtAction::Keep
@@ -571,6 +544,17 @@ mod tests {
             parse.syntax_node()
         );
 
+        // the fixed-up tree should not contain braces as punct
+        // FIXME: should probably instead check that it's a valid punctuation character
+        for x in tt.token_trees().flat_tokens() {
+            match x {
+                ::tt::TokenTree::Leaf(::tt::Leaf::Punct(punct)) => {
+                    assert!(!matches!(punct.char, '{' | '}' | '(' | ')' | '[' | ']'))
+                }
+                _ => (),
+            }
+        }
+
         reverse_fixups(&mut tt, &fixups.undo_info);
 
         // the fixed-up + reversed version should be equivalent to the original input
@@ -596,7 +580,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () {for _ in __ra_fixup { }}
+fn foo () {for _ in __ra_fixup {}}
 "#]],
         )
     }
@@ -624,7 +608,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () {for bar in qux { }}
+fn foo () {for bar in qux {}}
 "#]],
         )
     }
@@ -655,7 +639,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () {match __ra_fixup { }}
+fn foo () {match __ra_fixup {}}
 "#]],
         )
     }
@@ -687,7 +671,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () {match __ra_fixup { }}
+fn foo () {match __ra_fixup {}}
 "#]],
         )
     }
@@ -802,7 +786,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () {if a { }}
+fn foo () {if a {}}
 "#]],
         )
     }
@@ -816,7 +800,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () {if __ra_fixup { }}
+fn foo () {if __ra_fixup {}}
 "#]],
         )
     }
@@ -830,7 +814,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () {if __ra_fixup {} { }}
+fn foo () {if __ra_fixup {} {}}
 "#]],
         )
     }
@@ -844,7 +828,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () {while __ra_fixup { }}
+fn foo () {while __ra_fixup {}}
 "#]],
         )
     }
@@ -858,7 +842,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () {while foo { }}
+fn foo () {while foo {}}
 "#]],
         )
     }
@@ -885,7 +869,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () {loop { }}
+fn foo () {loop {}}
 "#]],
         )
     }
@@ -941,7 +925,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () { foo ( a ) }
+fn foo () {foo (a)}
 "#]],
         );
         check(
@@ -951,7 +935,7 @@ fn foo() {
 }
 "#,
             expect![[r#"
-fn foo () { bar . foo ( a ) }
+fn foo () {bar . foo (a)}
 "#]],
         );
     }
diff --git a/src/tools/rust-analyzer/crates/mbe/src/tests.rs b/src/tools/rust-analyzer/crates/mbe/src/tests.rs
index fb68d35a4c8..4a73b6fa05a 100644
--- a/src/tools/rust-analyzer/crates/mbe/src/tests.rs
+++ b/src/tools/rust-analyzer/crates/mbe/src/tests.rs
@@ -100,6 +100,23 @@ fn check(
 }
 
 #[test]
+fn unbalanced_brace() {
+    check(
+        Edition::CURRENT,
+        Edition::CURRENT,
+        r#"
+() => { { }
+"#,
+        r#""#,
+        expect![[r#"
+            SUBTREE $$ 1:0@0..0#2 1:0@0..0#2
+              SUBTREE {} 0:0@9..10#2 0:0@11..12#2
+
+            {}"#]],
+    );
+}
+
+#[test]
 fn token_mapping_smoke_test() {
     check(
         Edition::CURRENT,
diff --git a/src/tools/rust-analyzer/crates/syntax-bridge/src/lib.rs b/src/tools/rust-analyzer/crates/syntax-bridge/src/lib.rs
index 19801c49e43..a59a3270c9d 100644
--- a/src/tools/rust-analyzer/crates/syntax-bridge/src/lib.rs
+++ b/src/tools/rust-analyzer/crates/syntax-bridge/src/lib.rs
@@ -12,7 +12,7 @@ use syntax::{
     SyntaxKind::{self, *},
     SyntaxNode, SyntaxToken, SyntaxTreeBuilder, TextRange, TextSize, WalkEvent, T,
 };
-use tt::{buffer::Cursor, token_to_literal};
+use tt::{buffer::Cursor, token_to_literal, Punct};
 
 pub mod prettify_macro_expansion;
 mod to_parser_input;
@@ -217,8 +217,39 @@ where
         tt::TopSubtreeBuilder::new(tt::Delimiter::invisible_spanned(conv.call_site()));
 
     while let Some((token, abs_range)) = conv.bump() {
-        let delimiter = builder.expected_delimiter().map(|it| it.kind);
         let tt = match token.as_leaf() {
+            // These delimiters are not actually valid punctuation, but we produce them in syntax fixup.
+            // So we need to handle them specially here.
+            Some(&tt::Leaf::Punct(Punct {
+                char: char @ ('(' | ')' | '{' | '}' | '[' | ']'),
+                span,
+                spacing: _,
+            })) => {
+                let found_expected_delimiter =
+                    builder.expected_delimiters().enumerate().find(|(_, delim)| match delim.kind {
+                        tt::DelimiterKind::Parenthesis => char == ')',
+                        tt::DelimiterKind::Brace => char == '}',
+                        tt::DelimiterKind::Bracket => char == ']',
+                        tt::DelimiterKind::Invisible => false,
+                    });
+                if let Some((idx, _)) = found_expected_delimiter {
+                    for _ in 0..=idx {
+                        builder.close(span);
+                    }
+                    continue;
+                }
+
+                let delim = match char {
+                    '(' => tt::DelimiterKind::Parenthesis,
+                    '{' => tt::DelimiterKind::Brace,
+                    '[' => tt::DelimiterKind::Bracket,
+                    _ => panic!("unmatched closing delimiter from syntax fixup"),
+                };
+
+                // Start a new subtree
+                builder.open(delim, span);
+                continue;
+            }
             Some(leaf) => leaf.clone(),
             None => match token.kind(conv) {
                 // Desugar doc comments into doc attributes
@@ -228,17 +259,24 @@ where
                     continue;
                 }
                 kind if kind.is_punct() && kind != UNDERSCORE => {
-                    let expected = match delimiter {
-                        Some(tt::DelimiterKind::Parenthesis) => Some(T![')']),
-                        Some(tt::DelimiterKind::Brace) => Some(T!['}']),
-                        Some(tt::DelimiterKind::Bracket) => Some(T![']']),
-                        Some(tt::DelimiterKind::Invisible) | None => None,
-                    };
+                    let found_expected_delimiter =
+                        builder.expected_delimiters().enumerate().find(|(_, delim)| {
+                            match delim.kind {
+                                tt::DelimiterKind::Parenthesis => kind == T![')'],
+                                tt::DelimiterKind::Brace => kind == T!['}'],
+                                tt::DelimiterKind::Bracket => kind == T![']'],
+                                tt::DelimiterKind::Invisible => false,
+                            }
+                        });
 
                     // Current token is a closing delimiter that we expect, fix up the closing span
-                    // and end the subtree here
-                    if matches!(expected, Some(expected) if expected == kind) {
-                        builder.close(conv.span_for(abs_range));
+                    // and end the subtree here.
+                    // We also close any open inner subtrees that might be missing their delimiter.
+                    if let Some((idx, _)) = found_expected_delimiter {
+                        for _ in 0..=idx {
+                            // FIXME: record an error somewhere if we're closing more than one tree here?
+                            builder.close(conv.span_for(abs_range));
+                        }
                         continue;
                     }
 
@@ -262,6 +300,7 @@ where
                     let Some(char) = token.to_char(conv) else {
                         panic!("Token from lexer must be single char: token = {token:#?}")
                     };
+                    // FIXME: this might still be an unmatched closing delimiter? Maybe we should assert here
                     tt::Leaf::from(tt::Punct { char, spacing, span: conv.span_for(abs_range) })
                 }
                 kind => {
@@ -317,11 +356,10 @@ where
         builder.push(tt);
     }
 
-    // If we get here, we've consumed all input tokens.
-    // We might have more than one subtree in the stack, if the delimiters are improperly balanced.
-    // Merge them so we're left with one.
-    builder.flatten_unclosed_subtrees();
-
+    while builder.expected_delimiters().next().is_some() {
+        // FIXME: record an error somewhere?
+        builder.close(conv.call_site());
+    }
     builder.build_skip_top_subtree()
 }
 
diff --git a/src/tools/rust-analyzer/crates/tt/src/lib.rs b/src/tools/rust-analyzer/crates/tt/src/lib.rs
index 7705ba876e1..1cfead54f19 100644
--- a/src/tools/rust-analyzer/crates/tt/src/lib.rs
+++ b/src/tools/rust-analyzer/crates/tt/src/lib.rs
@@ -243,8 +243,8 @@ impl<S: Copy> TopSubtreeBuilder<S> {
         self.token_trees.extend(tt.0.iter().cloned());
     }
 
-    pub fn expected_delimiter(&self) -> Option<&Delimiter<S>> {
-        self.unclosed_subtree_indices.last().map(|&subtree_idx| {
+    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 {
                 unreachable!("unclosed token tree is always a subtree")
             };
@@ -252,28 +252,6 @@ impl<S: Copy> TopSubtreeBuilder<S> {
         })
     }
 
-    /// Converts unclosed subtree to a punct of their open delimiter.
-    // FIXME: This is incorrect to do, delimiters can never be puncts. See #18244.
-    pub fn flatten_unclosed_subtrees(&mut self) {
-        for &subtree_idx in &self.unclosed_subtree_indices {
-            let TokenTree::Subtree(subtree) = &self.token_trees[subtree_idx] else {
-                unreachable!("unclosed token tree is always a subtree")
-            };
-            let char = match subtree.delimiter.kind {
-                DelimiterKind::Parenthesis => '(',
-                DelimiterKind::Brace => '{',
-                DelimiterKind::Bracket => '[',
-                DelimiterKind::Invisible => '$',
-            };
-            self.token_trees[subtree_idx] = TokenTree::Leaf(Leaf::Punct(Punct {
-                char,
-                spacing: Spacing::Alone,
-                span: subtree.delimiter.open,
-            }));
-        }
-        self.unclosed_subtree_indices.clear();
-    }
-
     /// Builds, and remove the top subtree if it has only one subtree child.
     pub fn build_skip_top_subtree(mut self) -> TopSubtree<S> {
         let top_tts = TokenTreesView::new(&self.token_trees[1..]);
@@ -731,9 +709,9 @@ fn print_debug_subtree<S: fmt::Debug>(
     };
 
     write!(f, "{align}SUBTREE {delim} ",)?;
-    fmt::Debug::fmt(&open, f)?;
+    write!(f, "{:#?}", open)?;
     write!(f, " ")?;
-    fmt::Debug::fmt(&close, f)?;
+    write!(f, "{:#?}", close)?;
     for child in iter {
         writeln!(f)?;
         print_debug_token(f, level + 1, child)?;