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.

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

Conversation

DavisVaughan
Copy link
Collaborator

@DavisVaughan DavisVaughan commented May 9, 2025

  • Wait for an additional PR adding autobracing in for/while/repeat loops and functions before merging this, it should all land at once
  • Add documentation about the general Autobracing feature (highlighting if statements, loops, and functions separately)

Closes #334
Closes #225

This PR also has the side effect of fixing all idempotence issues on both data.table and R core. Those projects probably won't ever use Air, but they are a very good test case for "are we stable between runs?". We should probably set up some CI for ourselves that runs some kind of idempotence check on a set of large community packages (dplyr, data.table, r-svn) to ensure we don't slip here.


This PR introduces the idea of autobracing. This is the idea of wrapping an if statement body in { }

# Before
if (condition)
  this
else
  that

# After
if (condition) {
  this
} else {
  that
}

This ensures that the if statement is portable. The above original if statement actually won't even parse at top level due to the else on its own line. If you wrap the whole thing in { } (like would be the case if you found it inside a function body) then it parses fine. Portability ensures:

  • That you can copy and paste that if statement out to top level and run it without parse issues
  • That when you are debugging, you can highlight and run that if statement and send it to the console without any parse issues. This is otherwise very confusing!

We still allow one liner if statements in a few specific contexts:

  • The RHS of left assignment of any kind (<-, =, <<-)
  • An argument slot of a function call
  • A parameter slot of a function signature

In addition to being in that context, the one liner also has to be "simple enough" for some definition of "simple" which is encoded in compute_braced_expressions() in the PR.

# Not allowed at top level, will be autobraced
if (a) 1
if (a) 1 else 2

# Fine
x <- if (a) 1
x <- if (a) 1 else 2

# Fine
fn(if(a) 1, arg = if(a) 1 else 2)

# Fine
fn <- function(x, ..., arg = if (is.null(x)) 1 else 2) {}

# Examples of "not simple enough" that would be autobraced
x <- if (a)
  1

x <- if (a) 1 else if (b) 2 else 3

x <- if (a) { 1 } else 2
x <- if (a) 1 else { 2 }

I think the formatter code for this feature is rather elegant and easy enough to follow. By far the most complex part of this PR is comment handling. I've added a huge slew of comment related tests to ensure we are doing it "well enough". Movement of comments with something like this will always be a "best effort" kind of transformation, but I think it is worth a little ambiguity. The main promise we make is that we won't ever drop your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

if/else formatting can lead to unexpected breaks Idempotence issue - if/else with comment
1 participant