diff --git a/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs b/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs index c365a603d2..138db1b498 100644 --- a/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs +++ b/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/crates/hir-expand/src/fixup.rs b/crates/hir-expand/src/fixup.rs index eb43017739..28894537d4 100644 --- a/crates/hir-expand/src/fixup.rs +++ b/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/crates/mbe/src/tests.rs b/crates/mbe/src/tests.rs index fb68d35a4c..4a73b6fa05 100644 --- a/crates/mbe/src/tests.rs +++ b/crates/mbe/src/tests.rs @@ -99,6 +99,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( diff --git a/crates/syntax-bridge/src/lib.rs b/crates/syntax-bridge/src/lib.rs index 19801c49e4..a59a3270c9 100644 --- a/crates/syntax-bridge/src/lib.rs +++ b/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/crates/tt/src/lib.rs b/crates/tt/src/lib.rs index 7705ba876e..1cfead54f1 100644 --- a/crates/tt/src/lib.rs +++ b/crates/tt/src/lib.rs @@ -243,8 +243,8 @@ impl TopSubtreeBuilder { self.token_trees.extend(tt.0.iter().cloned()); } - pub fn expected_delimiter(&self) -> Option<&Delimiter> { - self.unclosed_subtree_indices.last().map(|&subtree_idx| { + pub fn expected_delimiters(&self) -> impl Iterator> { + 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 TopSubtreeBuilder { }) } - /// 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 { let top_tts = TokenTreesView::new(&self.token_trees[1..]); @@ -731,9 +709,9 @@ fn print_debug_subtree( }; 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)?;