This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
refactor(rome_formatter): Pre-compute suppressions #2902
Merged
Conversation
This file contains 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
|
!bench_formatter |
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
Deploying with
|
| Latest commit: |
e65085a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://aa671e40.tools-8rn.pages.dev |
| Branch Preview URL: | https://refactor-formatter-suppressi.tools-8rn.pages.dev |
Formatter Benchmark Results |
MichaReiser
force-pushed
the
refactor/formatter-suppressions
branch
from
739f903 to
8d7816b
Compare
|
!bench_formatter |
MichaReiser
force-pushed
the
refactor/formatter-suppressions
branch
from
8d7816b to
90e6178
Compare
Formatter Benchmark Results |
MichaReiser
force-pushed
the
refactor/formatter-suppressions
branch
from
90e6178 to
0ccd92f
Compare
MichaReiser
force-pushed
the
refactor/formatter-suppressions
branch
from
0ccd92f to
bf630fc
Compare
MichaReiser
force-pushed
the
refactor/formatter-suppressions
branch
from
bf630fc to
f65c7a3
Compare
This PR changes how the formatter extracts formatter suppression comments. The existing implementation used to traverse to a node's first token and then checking if it has any suppression comment when `context.is_suppressed()` was called. This PR iterates over the CST before starting the actual formatting to extract all suppressed nodes and stores them in an index that can be queried during formatting. The benefit of pre-computing the suppressions is that the logic can then be combined with the logic that extracts a nodes leading/trailing comments because that also requires iterating over all trivia pieces in the tree. Adding this traversal on its own would regress performance by about 10% which we can offset if combining it with the suppression handling because the use of `first_token` of the existing implementation is expensive. This change has the added benefit that suppressions are now a core formatter concept. ### Other changes This PR further fixes a few places where the formatter didn't correctly check if a node has any suppression comment and if so, prevents it from changing the formatting. ### Why add suppressions to the `FormatContext` I decided to introduce a new `FormatNodeContext` that replaces the existing `CommentsContext` and `CommentStyle` and now also stores the `suppressions`. The main reason for storing the suppressions on the context rather than on the `state` is that the context, unlike the `FormatState`, is language specific, and thus, it's possible to have a `Language` type parameter on the `Suppressions` type. The one thing I dislike about adding it to the context is that the context is changed AFTER its creation. ### Performance The performance of the two approaches are roughly the same. ### Future improvement It would be possible to improve Rome's overall performance if the workspace provides a way to combine the visitors run for the linter and the visitor for computing the suppression comments.
MichaReiser
force-pushed
the
refactor/formatter-suppressions
branch
from
f65c7a3 to
90e72f7
Compare
Can someone read somewhere what you plan to do? If this is a refactor per preparing something else, I think the reviewers should know what's coming next, so they have more context and everything is out in the open. We can add some comment in the original issue to explain what you have in mind. |
The goal is to address #2768 by distinguishing between leading/node comments and dangling comments as described in this notion document. |
ematipico
reviewed
Jul 20, 2022
There was a problem hiding this comment.
I have mostly nitpicks
This PR further fixes a few places where the formatter didn't correctly check if a node has any suppression comment and if so, prevents it from changing the formatting.
Would it be possible to add new tests to make sure we demonstrate the bugs you found?
ematipico
approved these changes
Jul 20, 2022
Co-authored-by: Emanuele Stoppa <[email protected]>
Co-authored-by: Emanuele Stoppa <[email protected]>
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.
Summary
This PR changes how the formatter extracts formatter suppression comments. The existing implementation lazily traversed to a node's first token and it checks if the token has any leading suppression comments inside of
context.is_suppressed().This PR iterates over the CST before starting the actual formatting to extract all suppressed nodes and it stores them in an index that can be queried during formatting.
This is mainly preparing for the migration to extract leading and trailing node comments with the re-work of the comments handling. Determining if a node is suppressed then becomes as simple as iterating over the leading comments of a node and determining if it has any suppression comment.
This change also shows that the current implementation that calls
node.first_tokenis roughly as expensive as iterating the whole tree ahead of time and computing the (suppression) comments.This change has the added benefit that suppressions are now a core formatter concept.
Part of #2768
Other changes
This PR further fixes a few places where the formatter didn't correctly check if a node has any suppression comment and if so, prevents it from changing the formatting.
Why add suppressions to the
FormatContextThe main reason for storing the suppressions on the context rather than on the
stateis that the context, unlike theFormatState, is language specific, and thus, it's possible to have aLanguagetype parameter on theSuppressionstype.The one thing I dislike about adding it to the context is that the context is changed AFTER its creation.
Performance
The performance of the two approaches is roughly the same.
Future improvement
It would be possible to improve Rome's overall performance if the workspace provides a way to combine the visitors run for the linter and the visitor for computing the suppression comments.
Test Plan
cargo test