-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Fix using transform variable shadowing in for-of loops #61934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… loops Co-authored-by: RyanCavanaugh <[email protected]>
Co-authored-by: RyanCavanaugh <[email protected]>
using
transform throws when a for body binding shadows the for head bindingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run hereby format
to fix your line endings
Add importHelpers: true
to your testcase so the emit is tidier
Co-authored-by: RyanCavanaugh <[email protected]>
Fixed line endings by adding missing newline at end of file. Added Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@copilot I'd prefer a different approach. Instead of renaming anything, inject an extra block scope between the moved and downleveled for (const foo_1 of []) {
try {
const foo = __addDisposableResource(env_1, foo_1, false);
{
const foo = new Foo();
}
}
// ...
} instead. |
…or-of loops Co-authored-by: weswigham <[email protected]>
Implemented the block scoping approach as suggested. Instead of renaming variables, the transformation now wraps the loop body in an additional block scope to handle shadowing naturally. The output now matches your specification exactly. Commit 8f3df02. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@@ -88,6 +88,8 @@ function main() { | |||
const env_1 = { stack: [], error: void 0, hasError: false }; | |||
try { | |||
const d1 = __addDisposableResource(env_1, d1_1, true); | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the existing block is empty, we shouldn't bother adding a new, empty block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 0b3bf33. The implementation now checks if the loop body is empty and skips adding the extra wrapping block in those cases. Empty using for-of loops no longer generate redundant empty blocks.
src/compiler/transformers/esnext.ts
Outdated
return visitEachChild(node, visitor, context); | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted the comment in commit 0b3bf33. The obsolete comment about collecting variable declarations has been removed from esnext.ts.
…ve obsolete comment Co-authored-by: weswigham <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also don't change the line endings that are already in the edited code.
@@ -118,7 +118,9 @@ try { | |||
var env_1 = { stack: [], error: void 0, hasError: false }; | |||
try { | |||
var _e = __addDisposableResource(env_1, _e_1, true); | |||
; | |||
{ | |||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider a block with just an empty statement the same as an empty block and avoid emitting the new block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 957cf79. The implementation now considers blocks containing only empty statements as effectively empty and avoids generating the extra wrapping block in those cases.
Co-authored-by: weswigham <[email protected]>
Co-authored-by: weswigham <[email protected]>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
The
using
declaration transform was throwing "redeclaration of const" errors when the for-of loop body contained variables that shadowed theusing
variable name.Problem:
The transformed output would generate:
Solution:
This PR implements variable renaming for shadowing variables in
using
for-of loops, similar to how ES5 for-of transforms handle variable conflicts. The fix:using
variable namefoo
→foo_2
)After the fix:
Features:
const
,let
,var
)var
vs block-scoped conflicts)using
for-of loops, regular for-of loops unchangedFixes #61714.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.