Fix class range removal logic after a method overload #593
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
Class field handling generates a `rangesToRemove` array describing the tokens that we should remove as we walk the class, but the logic expected us to always visit exactly the start token of each range. When a class body has method overloads, the `processToken` logic to handle that overload (also removing it) was skipping over one of the ranges to remove, so none of the later ranges to remove were being matched. To fix, we can use a `>=` comparison to make sure we handle ranges to remove if they're not already handled. Possibly a safer approach could be to revisit which ranges to remove we actually emit so that we can be confident that `processToken`isn't copying them unexpectedly, but that would require a bit more care, and hopefully this area of code will go away when class field support is dropped in the future.
Codecov Report
@@ Coverage Diff @@
## main #593 +/- ##
=======================================
Coverage ? 81.87%
=======================================
Files ? 55
Lines ? 5571
Branches ? 1314
=======================================
Hits ? 4561
Misses ? 726
Partials ? 284 Continue to review full report at Codecov.
|
Benchmark resultsBefore this PR: 252.1 thousand lines per second Measured change: 0.67% faster (0.07% faster to 2.28% faster) |
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.
Class field handling generates a
rangesToRemovearray describing the tokensthat we should remove as we walk the class, but the logic expected us to always
visit exactly the start token of each range. When a class body has method
overloads, the
processTokenlogic to handle that overload (also removing it)was skipping over one of the ranges to remove, so none of the later ranges to
remove were being matched. To fix, we can use a
>=comparison to make sure wehandle ranges to remove if they're not already handled.
Possibly a safer approach could be to revisit which ranges to remove we actually
emit so that we can be confident that
processTokenisn't copying themunexpectedly, but that would require a bit more care, and hopefully this area of
code will go away when class field support is dropped in the future.