Fix name generation scoping #58418
Merged
Fix name generation scoping #58418
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
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot perf test this faster |
Comment on lines
+5367
to
+5369
| if (set === stack[i]) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This is partially off-topic, but is there any reason the current scope can't be stored separate of the stack itself? Or if it is possible, when is it the case that the current is not the same as the last in the stack?
There was a problem hiding this comment.
The current scope is stored separate from the stack. The way push/pop works is that we don't reset the current scope until we actually record something so that we defer the cost of reassignment until the moment we actually need to record a name. This check here is an example of that. If the set for the current scope is also in the stack, then we can skip it as nothing else was actually recorded in that scope.
There was a problem hiding this comment.
In other words, the algorithm favors code that requires no generated names so that we can avoid paying the cost for more recent --targets.
|
@DanielRosenwasser Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
andrewbranch
approved these changes
May 6, 2024
|
@rbuckton @andrewbranch this seems to be a regression in 5.4.x, and I've unfortunately hit this after upgrading a large codebase to 5.4.x. Is there any chance we can backport this fix, so that I don't have to revert the upgrade? I see the fix is on 5.5.x but that's not released yet. |
|
TS 5.5 is coming out this week, so I wouldn't think we'd be patching 5.4 anymore. |
|
We have 5.5+ million lines of typescript code so it will probably take us a couple of weeks to get to 5.5 - if there is any chance this can be backported it would be a huge help. No worries if not though! |
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.
There are a number of small oddities in the emitter regarding name generation for temp variables and private names. Despite using
reservedNames/reservedPrivateNamesto reserve names permanently within a scope, we weren't checking all reservations in the respective stacks. We also weren't consistent about name generation in a few places, so I've combined the private and temp variable name scopes so that we enter and exit both consistently.I also removed a place where we exited and then reentered a private name generation scope when visiting a computed property name since both temp variables and private names should be generated immediately as we enter a declaration, not on-demand as we descend into and out of computed property names.
Fixes #57897