This repository was archived by the owner on Aug 31, 2023. It is now read-only.
feat(rome_formatter): Source Map iteration over deleted ranges #3219
Conversation
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
This PR adds a `DeletedRanges` iterator to the `TransformedSourceMap` that allows iterating over all deleted ranges including the deleted text. The comments refactoring needs a way to identify if a trivia piece is right before the closing `)` in the original tree because we want to keep trailing comments inside parentheses with the preceding nodes ```javascript !( a // some comment ); b ``` The comment should be a trailing comment of `a` and not become a leading comment of `b` because the transformed tree has the following shape ```javascript !a // Some comment b; ``` Such an iterator requires a fast way to get the text of a deleted range. This isn't possible with `SyntaxNodeText` because resolving the text requires iterating to the token with the given offset which is `O(n)`. That's why this PR changes the `TransfromedSourceMap` to hold a `String` with the original source text to get `O(1)` access to the original text. The downside of doing so is that the source text must be built up insideof the `SyntaxRewriter` and is stored twice in memory. The cost of traversal should offset the cost in the comments builder and the memory allocation is short-lived, not adding a lot of pressure if Rome's running as a server. ## Tests I added a new unit test that shows how the new iterator can be used
✅ Deploy Preview for rometools canceled.
|
MichaReiser
commented
Sep 14, 2022
Comment on lines
-268
to
-272
| fn map_verbatim_ranges(&self, ranges: &mut [TextRange]) { | ||
| for range in ranges { | ||
| *range = self.source_range(*range) | ||
| } | ||
| } |
There was a problem hiding this comment.
mapping the verbatim ranges isn't necessary because these ranges represent ranges in the printed document and not in the source tree.
This has gone unnoticed thus far because no formatter used the verbatim formatting.
MichaReiser
commented
Sep 14, 2022
Comment on lines
+16
to
18
| let mut rewriter = JsFormatSyntaxRewriter::default(); | ||
| let transformed = rewriter.transform(root); | ||
| (transformed, rewriter.finish()) |
There was a problem hiding this comment.
@ematipico this will make you happy... It's no longer passing root twice ;)
|
!bench_formatter |
Formatter Benchmark Results |
ematipico
approved these changes
Sep 14, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR adds a
DeletedRangesiterator to theTransformedSourceMapthat allows iterating over all deleted ranges including the deleted text.The comments refactoring needs a way to identify if a trivia piece is right before the closing
)in the original tree because we want to keep trailing comments inside parentheses with the preceding nodesThe comment should be a trailing comment of
aand not become a leading comment ofbbecause the transformed tree has the following shapeSuch an iterator requires a fast way to get the text of a deleted range. This isn't possible with
SyntaxNodeTextbecause resolving the text requires iterating to the token with the given offset which isO(n). That's why this PR changes theTransfromedSourceMapto hold aStringwith the original source text to getO(1)access to the original text.The downside of doing so is that the source text must be built up insideof the
SyntaxRewriterand is stored twice in memory.The cost of traversal should offset the cost in the comments builder and the memory allocation is short-lived, not adding a lot of pressure if Rome's running as a server.
Tests
I added a new unit test that shows how the new iterator can be used