Implement if statement autobracing#340
Merged
Merged
Conversation
…fs in `consequence` They should get to compute their own value, because the user could have validly written a one liner if statement inside a braced if to begin with, and we want to be consistent with that.
This causes too many issues with idempotence and verbatim handling, even though it would be quite nice
It seems like it is actually hard to get this right, as there are many other places where one liners feel "pretty good" and expanding them out causes more churn than it is worth.
And use in if statement handling as a big improvement on context awareness!
DavisVaughan
commented
May 16, 2025
Comment on lines
+136
to
+139
| # If statements and comments | ||
|
|
||
| # fmt: skip | ||
| if (TRUE) 1 # hi |
Collaborator
Author
There was a problem hiding this comment.
Very important test related to comment placement and verbatim formatting
The # hi comment must be trailing on the whole if statement, rather than trailing on the 1 node, for this to work correctly. Otherwise verbatim printing will drop the # hi comment entirely, which would be bad.
When you remove the # fmt: skip, you get this
if (TRUE) {
1
} # hiwhich I can live with for this rare-ish scenario if the alternative is that a comment would be dropped.
lionel-
approved these changes
May 29, 2025
lionel-
left a comment
Collaborator
There was a problem hiding this comment.
Looks great!
I'm very happy with the final set of rules we came up with.
…ons (#344) * Add autobracing for `for_statement` * Add autobracing for `repeat_statement` * Add autobracing for `while_statement` * Add autobracing for `function_definition` * In `for_statement`, push onto the `body` again * In `while_statement`, push onto the `body` again * In `repeat_statement`, push onto the `body` again * Add documentation on autobracing * CHANGELOG bullet * Mention synergy with persistent line breaks * Declare that any `RFunctionDefinition` can benefit from argument grouping Allowing you to put a line break here `map(xs, function(x)<here>x + 1)`, which then causes autobracing to kick in, and now gives you the "middle variant" that you probably want of ``` map(xs, function(x) { x + 1 }) ``` * Rework if statement section with value vs effect position * Tweak CHANGELOG * Portability is for multiline if statements * Consistency with if statements * Make effect/value comparison clearer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #334
Closes #225
Closes #348
Closes #321
Closes #22
Closes #9
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
{ }This ensures that the if statement is portable. The above original if statement actually won't even parse at top level due to the
elseon 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:We still allow short single line if statements when the if statement is in a value position (as opposed to an effect position). The if statement must also meet some other criteria (no existing newlines, no existing braces, etc) to be considered a single line candidate.
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 if/else 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.
Documentation for if/else autobracing ended up in the new Autobracing section of this PR
#344