Properly transform enum values, rewrite enum implementation #621
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
Fixes #620 The previous enum implementation was a bit tangled, handling several different cases in one big `while` loop. This PR refactors the code into three distinct cases: string enum members, calculated/constant enum members, and autoincrementing enum members, and the transform details are handled separately for each. Each case still needs to think about the case where we save a variable and the case where we don't, but hopefully the whole thing is more understandable now, and certainly better-documented. This refactor makes it much easier to fix #620, where we weren't actually passing the RHS expressions to the root transformer for their own transforms. In particular, that meant that imported values weren't transformed correctly. To fix, we now apply the enum transform in a purely left-to-right way rather than needing to save and re-emit a region of code. As part of the refactor, I also changed the emitted code so that when autoincrementing a previous value, we always reference it rather than copying the entire expression. This should reduce cases where we need to copy a large range of code, which is generally questionable in Sucrase.
Codecov Report
@@ Coverage Diff @@
## main #621 +/- ##
==========================================
+ Coverage 82.05% 82.16% +0.11%
==========================================
Files 56 56
Lines 5622 5870 +248
Branches 1321 1324 +3
==========================================
+ Hits 4613 4823 +210
- Misses 725 763 +38
Partials 284 284
Continue to review full report at Codecov.
|
Benchmark resultsBefore this PR: 276.8 thousand lines per second Measured change: 0.81% slower (2.51% slower to 0.44% faster) |
Closed
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.
Fixes #620
The previous enum implementation was a bit tangled, handling several different
cases in one big
whileloop. This PR refactors the code into three distinctcases: string enum members, calculated/constant enum members, and
autoincrementing enum members, and the transform details are handled separately
for each. Each case still needs to think about the case where we save a variable
and the case where we don't, but hopefully the whole thing is more
understandable now, and certainly better-documented.
This refactor makes it much easier to fix #620, where we weren't actually
passing the RHS expressions to the root transformer for their own transforms. In
particular, that meant that imported values weren't transformed correctly. To
fix, we now call out to the root transformer and apply the enum transform in a
purely left-to-right way rather than needing to save and re-emit a region of code.
As part of the refactor, I also changed the emitted code so that when
autoincrementing a previous value, we always reference from the enum it rather
than copying the entire expression. This should reduce cases where we need to
copy a large range of code, which is generally questionable in Sucrase.