"editor.formatOnType": true (vscode) behaves weirdly with calls (and attributes on calls)

[WaffleLapkin]

rust-analyzer version: 0.4.1881-standalone (14558af 2024-03-14)

relevant settings: "editor.formatOnType": true

code snippet to reproduce:

fn main() {
    f();

    #[allow(unreachable_code)]
    g();
}

If I type { before f(); nothing happens (well, except the { that was typed, obv). If I type { before g(); a matching } is added but not where I'd expect:

fn main() {
    {f();

    #[allow(unreachable_code)]
    {g}();
}

This is what I would expect to see instead:

fn main() {
    { f() };

    #[allow(unreachable_code)]
    { g() };
}

[Veykril]

Relevant code is here

rust-analyzer/crates/ide/src/typing.rs

Lines 105 to 189 in 0dd89d7

	fn on_opening_bracket_typed(
	file: &Parse<SourceFile>,
	offset: TextSize,
	opening_bracket: char,
	) -> Option<TextEdit> {
	let (closing_bracket, expected_ast_bracket) = match opening_bracket {
	'{' => ('}', SyntaxKind::L_CURLY),
	'(' => (')', SyntaxKind::L_PAREN),
	_ => return None,
	};
	if !stdx::always!(file.tree().syntax().text().char_at(offset) == Some(opening_bracket)) {
	return None;
	}
	let brace_token = file.tree().syntax().token_at_offset(offset).right_biased()?;
	if brace_token.kind() != expected_ast_bracket {
	return None;
	}
	// Remove the opening bracket to get a better parse tree, and reparse.
	let range = brace_token.text_range();
	if !stdx::always!(range.len() == TextSize::of(opening_bracket)) {
	return None;
	}
	let file = file.reparse(&Indel::delete(range));
	if let Some(edit) = bracket_expr(&file.tree(), offset, opening_bracket, closing_bracket) {
	return Some(edit);
	}
	if closing_bracket == '}' {
	if let Some(edit) = brace_use_path(&file.tree(), offset) {
	return Some(edit);
	}
	}
	return None;
	fn brace_use_path(file: &SourceFile, offset: TextSize) -> Option<TextEdit> {
	let segment: ast::PathSegment = find_node_at_offset(file.syntax(), offset)?;
	if segment.syntax().text_range().start() != offset {
	return None;
	}
	let tree: ast::UseTree = find_node_at_offset(file.syntax(), offset)?;
	Some(TextEdit::insert(tree.syntax().text_range().end() + TextSize::of("{"), "}".to_owned()))
	}
	fn bracket_expr(
	file: &SourceFile,
	offset: TextSize,
	opening_bracket: char,
	closing_bracket: char,
	) -> Option<TextEdit> {
	let mut expr: ast::Expr = find_node_at_offset(file.syntax(), offset)?;
	if expr.syntax().text_range().start() != offset {
	return None;
	}
	// Enclose the outermost expression starting at `offset`
	while let Some(parent) = expr.syntax().parent() {
	if parent.text_range().start() != expr.syntax().text_range().start() {
	break;
	}
	match ast::Expr::cast(parent) {
	Some(parent) => expr = parent,
	None => break,
	}
	}
	// If it's a statement in a block, we don't know how many statements should be included
	if ast::ExprStmt::can_cast(expr.syntax().parent()?.kind()) {
	return None;
	}
	// Insert the closing bracket right after the expression.
	Some(TextEdit::insert(
	expr.syntax().text_range().end() + TextSize::of(opening_bracket),
	closing_bracket.to_string(),
	))
	}
	}

[roife]

@rustbot claim

[roife]

For the first issue (f()), I found that it is intentional not to insert brackets automatically when the expression is used as a statement.

rust-analyzer/crates/ide/src/typing.rs

Lines 178 to 181 in 0dd89d7

	// If it's a statement in a block, we don't know how many statements should be included
	if ast::ExprStmt::can_cast(expr.syntax().parent()?.kind()) {
	return None;
	}

For the second point (g()), the attributes are parsed as part of the call-expr, resulting in strange behavior.

            EXPR_STMT@28..63
              CALL_EXPR@28..62
                ATTR@28..54
                  POUND@28..29 "#"
                  L_BRACK@29..30 "["
                  META@30..53
                    PATH@30..35
                      PATH_SEGMENT@30..35
                        NAME_REF@30..35
                          IDENT@30..35 "allow"
                    TOKEN_TREE@35..53
                      L_PAREN@35..36 "("
                      IDENT@36..52 "unreachable_code"
                      R_PAREN@52..53 ")"
                  R_BRACK@53..54 "]"
                WHITESPACE@54..59 "\n    "
                PATH_EXPR@59..60
                  PATH@59..60
                    PATH_SEGMENT@59..60
                      NAME_REF@59..60
                        IDENT@59..60 "g"
                ARG_LIST@60..62
                  L_PAREN@60..61 "("
                  R_PAREN@61..62 ")"

Maybe we should identify attributes before inserting.

[WaffleLapkin]

If it's a statement in a block, we don't know how many statements should be included

I'm not sure I understand the reasoning here. Why don't we wrap just the whole expression in a statement? If the user wants more, they can move things inside the braces.

For the second point (g()), the attributes are parsed as part of the call-expr, resulting in strange behavior.

Uh, that's fun. I guess a parsing which would make more sense is to have "attribute expr" which would wrap the call expr. But ig that would make things hard elsewhere, because to check attributes you'd have to go outwards...

[roife]

If the user wants more, they can move things inside the braces.

That makes sense to me.

I guess a parsing which would make more sense is to have "attribute expr" which would wrap the call expr.

I'm not sure. Perhaps we can also directly hack here: if it is identified that the front part of expr is attr, we consider it as inserting {} around the entire expr (excluding the attr part).