Skip to content

Implement if statement autobracing #340

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Implement if statement autobracing #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f80f39c
First draft of if statement autobracing
DavisVaughan May 1, 2025
5382eea
Correctly recurse into `consequence` when handling non-enclosed comments
DavisVaughan May 1, 2025
c73c951
Handle nested if statements in `consequence`
DavisVaughan May 1, 2025
7b7e458
Add failing test
DavisVaughan May 2, 2025
5f750da
Don't pass pre-computed `braced_expression` value through to nested i…
DavisVaughan May 2, 2025
f440204
Add one more test seen in data.table
DavisVaughan May 2, 2025
5cdef64
More test cases
DavisVaughan May 2, 2025
a7a7a62
Another test case
DavisVaughan May 2, 2025
4e48549
Another test
DavisVaughan May 2, 2025
a855327
Remove handling of comments not enclosed by the if statement
DavisVaughan May 2, 2025
27f782c
Prefer generating leading comments
DavisVaughan May 2, 2025
daeb004
Fix a typo buglet
DavisVaughan May 5, 2025
c178cec
Only allow one liner if statements in specific contexts
DavisVaughan May 8, 2025
dc989dc
Add symmetrical support for function parameters, to mirror call argum…
DavisVaughan May 8, 2025
eb94ecd
Refine end of line comments before `else`
DavisVaughan May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
300 changes: 259 additions & 41 deletions crates/air_r_formatter/src/comments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::prelude::*;
use air_r_syntax::AnyRExpression;
use air_r_syntax::RArgument;
use air_r_syntax::RArgumentNameClause;
use air_r_syntax::RElseClause;
use air_r_syntax::RIfStatement;
use air_r_syntax::RLanguage;
use air_r_syntax::RParenthesizedExpression;
Expand Down Expand Up @@ -59,13 +60,13 @@ impl CommentStyle for RCommentStyle {
&self,
comment: DecoratedComment<Self::Language>,
) -> CommentPlacement<Self::Language> {
// TODO: Implement more rule based comment placement, see `biome_js_formatter`
match comment.text_position() {
CommentTextPosition::EndOfLine => handle_for_comment(comment)
.or_else(handle_function_comment)
.or_else(handle_while_comment)
.or_else(handle_repeat_comment)
.or_else(handle_if_statement_comment)
.or_else(handle_else_clause_comment)
.or_else(handle_parenthesized_expression_comment)
.or_else(handle_argument_name_clause_comment)
.or_else(handle_argument_comment)
Expand All @@ -75,6 +76,7 @@ impl CommentStyle for RCommentStyle {
.or_else(handle_while_comment)
.or_else(handle_repeat_comment)
.or_else(handle_if_statement_comment)
.or_else(handle_else_clause_comment)
.or_else(handle_parenthesized_expression_comment)
.or_else(handle_argument_name_clause_comment)
.or_else(handle_argument_comment)
Expand Down Expand Up @@ -192,55 +194,271 @@ fn handle_function_comment(comment: DecoratedComment<RLanguage>) -> CommentPlace
fn handle_if_statement_comment(
comment: DecoratedComment<RLanguage>,
) -> CommentPlacement<RLanguage> {
match (comment.enclosing_node().kind(), comment.following_node()) {
(RSyntaxKind::R_IF_STATEMENT, Some(following)) => {
let if_statement = RIfStatement::unwrap_cast(comment.enclosing_node().clone());

if let Some(preceding) = comment.preceding_node() {
// Make comments directly before the condition `)` trailing
// comments of the condition itself
//
// ```r
// if (
// cond
// # comment
// ) {
// }
// ```
if comment
.following_token()
.is_some_and(|token| token.kind() == RSyntaxKind::R_PAREN)
{
return CommentPlacement::trailing(preceding.clone(), comment);
}
}
let Some(if_statement) = RIfStatement::cast_ref(comment.enclosing_node()) else {
return CommentPlacement::Default(comment);
};

let Ok(condition) = if_statement.condition() else {
// Bail with default placement if we don't have a `condition`, should
// be unexpected
return CommentPlacement::Default(comment);
};

// Figure out if this is a comment that comes directly before the
// `consequence` and after the `)`, in which case we move it onto
// the `consequence`
//
// ```r
// if (cond) # comment
// TRUE
// ```
if let Ok(consequence) = if_statement.consequence() {
if consequence.syntax() == following {
return place_leading_or_dangling_body_comment(consequence, comment);
}
let Ok(consequence) = if_statement.consequence() else {
// Bail with default placement if we don't have a `consequence`, should
// be unexpected
return CommentPlacement::Default(comment);
};

// Make comments directly after the `condition` trailing
// comments of the `condition` itself
//
// ```r
// if (
// condition
// # comment
// ) {
// }
// ```
//
// But don't steal comments that belong on the `consequence`, like
// below. Here the preceding node is the `condition`. To do this right,
// we also force the following token to be `)`.
//
// ```r
// if (condition) # comment
// consequence
// ```
if let Some(preceding) = comment.preceding_node() {
if let Some(following_token) = comment.following_token() {
if condition.syntax() == preceding
&& matches!(following_token.kind(), RSyntaxKind::R_PAREN)
{
return CommentPlacement::trailing(preceding.clone(), comment);
}
}
(RSyntaxKind::R_ELSE_CLAUSE, _) => {
// TODO: Handle else clause comments in some way? See JS for an example.
// fall through
}

// Figure out if this is a comment that comes directly before the
// `consequence` and after the `)`, in which case we move it onto
// the `consequence` to prepare for autobracing
//
// ```r
// if (condition) # comment
// consequence
// ```
//
// This aligns with behavior when braces already exist. Here, the comment's
// default placement makes it leading on `consequence` (the enclosing node is
// the braced expression, and there are no preceding nodes, so it attaches as leading
// on the following node).
//
// ```r
// if (condition) { # comment
// consequence
// }
// ```
//
// It is important that we first check for comments after the `condition`
// but before the `)`, which we do above, otherwise this will steal this
// comment and place it on `consequence` when it really belongs to `condition`
//
// ```r
// if (
// condition
// # comment
// ) {
// }
// ```
if let Some(following) = comment.following_node() {
if consequence.syntax() == following {
return place_leading_or_dangling_body_comment(consequence, comment);
}
_ => {
// fall through
}

// Move comments directly before an `else_clause` to the correct location
//
// Most `else` related handling is done by `handle_else_clause_comment()`,
// but when the comment is right before the `else` token, it is "enclosed"
// by the if statement node instead. We handle all of those cases here.
//
// We try extremely hard to force `} else` onto the same line with nothing
// between them, for maximum portability of the if/else statement. This
// requires moving the comment onto `consequence` or `alternative`.
//
// We greatly prefer creating leading comments over dangling comments when we move
// them, as they play much nicer with idempotence.
//
// ```r
// {
// if (cond) this # becomes leading on `this`
// else that
// }
// ```
//
// ```r
// {
// if (cond) this
// # becomes leading on `that`
// else that
// }
// ```
//
// ```r
// {
// if (cond) {
// this
// } # becomes leading on `that`
// else {
// that
// }
// }
// ```
//
// ```r
// {
// if (cond) {
//
// } # becomes leading on `that`
// else {
// that
// }
// }
// ```
//
// ```r
// {
// if (cond) {
// this
// }
// # becomes leading on `that`
// else {
// that
// }
// }
// ```
//
// ```r
// {
// if (cond) {
// this
// }
// # becomes dangling on `{}`
// else {
// }
// }
// ```
//
// ```r
// {
// if (cond) {
// this
// }
// # becomes leading on `that`
// else if (cond) {
// that
// }
// }
// ```
if let Some(else_clause) = if_statement.else_clause() {
let Ok(alternative) = else_clause.alternative() else {
// Bail with default placement if we don't have a `alternative`, should
// be unexpected
return CommentPlacement::Default(comment);
};

if let Some(following) = comment.following_node() {
if else_clause.syntax() == following {
return match comment.text_position() {
// End of line comments lead the `consequence` body
CommentTextPosition::EndOfLine => {
place_leading_or_dangling_body_comment(consequence, comment)
}
// Own line comments lead the `alternative` body
CommentTextPosition::OwnLine => {
place_leading_or_dangling_alternative_comment(alternative, comment)
}
CommentTextPosition::SameLine => {
unreachable!("Inline comments aren't possible in R")
}
};
}
}
}

CommentPlacement::Default(comment)
}

fn handle_else_clause_comment(comment: DecoratedComment<RLanguage>) -> CommentPlacement<RLanguage> {
let Some(else_clause) = RElseClause::cast_ref(comment.enclosing_node()) else {
return CommentPlacement::Default(comment);
};

let Ok(alternative) = else_clause.alternative() else {
// Bail with default placement if we don't have a `alternative`, should
// be unexpected
return CommentPlacement::Default(comment);
};

// Comments following the `else` token but before the `alternative` are enclosed by
// the `else_clause`. We make these comments leading on the `alternative`.
//
// ```r
// {
// if (condition) a
// else # becomes leading on `b`
// {
// b
// }
// }
// ```
//
// ```r
// {
// if (condition) a
// else
// # becomes leading on `b`
// {
// b
// }
// }
// ```
if let Some(following) = comment.following_node() {
if alternative.syntax() == following {
return place_leading_or_dangling_alternative_comment(alternative, comment);
}
}

CommentPlacement::Default(comment)
}

/// Basically [place_leading_or_dangling_body_comment()], but moves comments on an if
/// statement `alternative` onto the body of that if statement to handle if chaining a bit
/// nicer
///
/// ```r
/// {
/// if (condition) {
/// a
/// } else # becomes leading on `b` rather than leading on if statement `alternative`
/// if (condition) {
/// b
/// }
/// }
/// ```
fn place_leading_or_dangling_alternative_comment(
alternative: AnyRExpression,
comment: DecoratedComment<RLanguage>,
) -> CommentPlacement<RLanguage> {
match alternative {
AnyRExpression::RIfStatement(node) => match node.consequence() {
Ok(consequence) => place_leading_or_dangling_body_comment(consequence, comment),
Err(_) => CommentPlacement::Default(comment),
},
node => place_leading_or_dangling_body_comment(node, comment),
}
}

fn handle_parenthesized_expression_comment(
comment: DecoratedComment<RLanguage>,
) -> CommentPlacement<RLanguage> {
Expand Down Expand Up @@ -577,7 +795,7 @@ fn handle_hole_argument_comment(
/// ```
///
/// ```r
/// if (cond) # becomes leading on `{}`
/// if (cond) # becomes dangling on `{}`
/// {
/// }
/// ```
Expand Down
Loading
Loading