bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Fix syntax fixup producing invalid punctuation

Browse source 
Fixes #19206.
Fixes #18244.
This commit is contained in:
¨Florian 2025-02-28 17:32:21 +01:00
parent d11c5b8d75
commit e88f892eca
5 changed files with 105 additions and 88 deletions
Download patch file Download diff file Expand all files Collapse all files

8
src/tools/rust-analyzer/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs
View file

@ -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 */ }
"#]],
)
}

68
src/tools/rust-analyzer/crates/hir-expand/src/fixup.rs
View file

@ -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)}
"#]],
);
}

17
src/tools/rust-analyzer/crates/mbe/src/tests.rs
View file

@ -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(

70
src/tools/rust-analyzer/crates/syntax-bridge/src/lib.rs
View file

@ -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()
}

30
src/tools/rust-analyzer/crates/tt/src/lib.rs
View file

@ -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)?;