Repository navigation
Copy in Aether's parser as the new air_r_parser - #548
Merged
Merged
Conversation
Copying the aether parser in, directly overwriting the air one. This turned out to be the easiest approach for now, and we can split back out later if needed.
Matching our existing tasks
… with slashes Both of which came with aether
Otherwise `<< 1; >> 2` could reformat as `1 2` which is invalid R code
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.
Related preliminary PRs that led up to this one:
#533
#534
#535
#536
#537
#538
#543
#544
#545
#546
#547
These PRs were important because they allowed us to shift the Air syntax tree one step at a time, even though tree-sitter-r was still the backend. Because of that, this PR has 0 changes to formatting snapshots.
Aether was crafted with blood, sweat, and tears here https://github.com/DavisVaughan/aether. This repo will be marked read only, as it has been absorbed into Air now.
This PR switches us to Aether's new Top Down Pratt parser for R, written entirely in Rust with no dependency on tree-sitter-r (or C!!!!) whatsoever. This switch has a number of important benefits:
It is 7x faster at parsing than the tree-sitter-r approach. This is important for Air and gives us 2x faster formatting, but will also be great for Jarl and Ark which also have to parse a lot of R files!
It means C is no longer in the Air dependency chain. So we should be able to compile to WASM wayyyy easier now. Possibly q2 or wasm-r could use Air easily now.
It means we are fully in control of both error recovery and error reporting. We are going to have much improved error reporting and should be able to report both the exact problem and the surrounding code snippet (surfacing this is still a follow up PR). We also are much better at error recovery because we control it precisely and have much more context available to us to recover per-concept (error recovery for calls is different from error recovery for function definitions, for example). I still don't think we are error resilient enough to perform formatting on broken files, but we might be able to do inference on them.
It dropped 1.2 MB from the binary (10 MB -> 8.8 MB)
The parser tests use a new markdown style testing scheme, and brings over EVERY SINGLE TEST from both tree-sitter-r and the previous air parser tests, so I am extremely confident in them.
Additionally, I did an "ecosystem check" against the following repos, and nothing changed! So that's great.
Originally I thought it might be nice if aether lived on its own, and on the air side we just depended on it. And that still might be nice in the future! But for now it doesn't give us many practical benefits besides being a project that stands alone, and it came with a number of headaches:
xtasks/were coupled because biome's tooling expects the parser and formatter to live together in the same repo. We will want to update the xtask code from upstream biome in the future too, so rewriting them too much would be painfulWe can tell people they can use
air_r_parseras Jarl already does without really needing aether on its own yet. And now that it's in air, it's much easier to split back out into a standalone aether if we want to, rather than doing the split + rewrite all at once.