fix: `for of` with `iterableIsArray` and shadowing variable #16011

liuxingbaoyu · 2023-09-29T02:32:39Z

Q	A
Fixed Issues?	Fixes #16009
Patch: Bug Fix?	√
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	√
Documentation PR Link
Any Dependency Changes?
License	MIT

liuxingbaoyu · 2023-09-29T02:33:23Z

packages/babel-plugin-transform-for-of/src/index.ts

-      bodyPath.scope.hasOwnBinding(id),
-    )
+    Object.keys(path.getBindingIdentifiers())
+      .concat(Object.keys(path.get("right").getBindingIdentifiers()))


Should we fix this in getBindingIdentifiers?

I'm not 100% sure about what's happening here. right does not introduce new bindings, because it cannot contain variable declarations.

I think the fix should be to force memoization in

babel/packages/babel-plugin-transform-for-of/src/index.ts

Line 93 in a345a10

let array: t.Identifier | t.ThisExpression =

if t.isIdentifier(right) and body.scope.hasOwnBinding(right.name).

babel-bot · 2023-09-29T02:34:43Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/55956/

packages/babel-plugin-transform-for-of/src/index.ts

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

JLHwung

We probably need skipTransparentExprWrapperNodes for the right-is-identifier check. Otherwise it looks good to me.

An alternative solution is to wrap the original for-of body within a new lexical scope whenever it might shadow the right variable, so we can insert any new declarations directly.

JLHwung · 2023-12-03T02:36:14Z

...in-transform-for-of/test/fixtures/assumption-iterableIsArray/for-of-modify-iterable/input.js

@@ -0,0 +1,3 @@
+for (let o of arr) {
+  arr = null;
+}


Is this test already passing in the main branch? I will be surprised if the arr binding is constant given then reassignment.

Yes, this is passed in the main branch, I made some bad attempts so adding this test to avoid regressions.

liuxingbaoyu · 2023-12-03T02:55:27Z

An alternative solution is to wrap the original for-of body within a new lexical scope whenever it might shadow the right variable, so we can insert any new declarations directly.

I initially tried doing this, but I found that this duplicated what maybeGenerateMemoised did, and the behavior now is that the output is a bit smaller when targets==node 4. I will add skipTransparentExprWrapperNodes.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@babel/traverse](https://babel.dev/docs/en/next/babel-traverse) ([source](https://github.com/babel/babel)) | resolutions | patch | [`7.23.5` -> `7.23.7`](https://renovatebot.com/diffs/npm/@babel%2ftraverse/7.23.5/7.23.7) | --- ### Release Notes <details> <summary>babel/babel (@babel/traverse)</summary> ### [`v7.23.7`](https://github.com/babel/babel/blob/HEAD/CHANGELOG.md#v7237-2023-12-29) [Compare Source](babel/babel@v7.23.6...v7.23.7) ##### 🐛 Bug Fix - `babel-traverse` - [#16191](babel/babel#16191) fix: Crash when removing without `Program` ([@liuxingbaoyu](https://github.com/liuxingbaoyu)) - `babel-helpers`, `babel-plugin-proposal-decorators` - [#16180](babel/babel#16180) fix: Class decorator `ctx.kind` is wrong ([@liuxingbaoyu](https://github.com/liuxingbaoyu)) - `babel-plugin-proposal-decorators` - [#16170](babel/babel#16170) Fix decorator initProto usage in derived classes ([@JLHwung](https://github.com/JLHwung)) - `babel-core` - [#16167](babel/babel#16167) Avoid unpreventable `unhandledRejection` events ([@nicolo-ribaudo](https://github.com/nicolo-ribaudo)) ##### 🏠 Internal - `babel-helper-create-class-features-plugin` - [#16186](babel/babel#16186) chore: Update deps ([@liuxingbaoyu](https://github.com/liuxingbaoyu)) - `babel-helper-create-class-features-plugin`, `babel-plugin-proposal-decorators` - [#16177](babel/babel#16177) Merge decorators into class features ([@JLHwung](https://github.com/JLHwung)) ### [`v7.23.6`](https://github.com/babel/babel/blob/HEAD/CHANGELOG.md#v7236-2023-12-11) [Compare Source](babel/babel@v7.23.5...v7.23.6) ##### 👓 Spec Compliance - `babel-generator`, `babel-parser`, `babel-types` - [#16154](babel/babel#16154) Remove `TSPropertySignature.initializer` ([@fisker](https://github.com/fisker)) - `babel-helpers`, `babel-plugin-proposal-decorators`, `babel-plugin-transform-class-properties`, `babel-plugin-transform-class-static-block`, `babel-plugin-transform-runtime`, `babel-preset-env`, `babel-runtime-corejs2`, `babel-runtime-corejs3`, `babel-runtime`, `babel-types` - [#16139](babel/babel#16139) Apply `toPropertyKey` on decorator context name ([@JLHwung](https://github.com/JLHwung)) ##### 🐛 Bug Fix - `babel-generator` - [#16166](babel/babel#16166) fix: Correctly indenting when `retainLines` is enabled ([@liuxingbaoyu](https://github.com/liuxingbaoyu)) - `babel-helpers`, `babel-plugin-proposal-explicit-resource-management` - [#16150](babel/babel#16150) `using`: Allow looking up `Symbol.dispose` on a function ([@odinho](https://github.com/odinho)) - `babel-plugin-proposal-decorators`, `babel-plugin-transform-class-properties` - [#16161](babel/babel#16161) Ensure the `[[@@toPrimitive]]` call of a decorated class member key is invoked once ([@JLHwung](https://github.com/JLHwung)) - [#16148](babel/babel#16148) Support named evaluation for decorated anonymous class exp ([@JLHwung](https://github.com/JLHwung)) - `babel-plugin-transform-for-of`, `babel-preset-env` - [#16011](babel/babel#16011) fix: `for of` with `iterableIsArray` and shadowing variable ([@liuxingbaoyu](https://github.com/liuxingbaoyu)) - `babel-helpers`, `babel-plugin-proposal-decorators`, `babel-runtime-corejs2`, `babel-runtime-corejs3`, `babel-runtime` - [#16144](babel/babel#16144) Set function name for decorated private non-field elements ([@JLHwung](https://github.com/JLHwung)) - `babel-plugin-transform-typescript` - [#16137](babel/babel#16137) Fix references to enum values with merging ([@nicolo-ribaudo](https://github.com/nicolo-ribaudo)) ##### 🔬 Output optimization - `babel-helper-create-class-features-plugin`, `babel-plugin-transform-class-properties` - [#16159](babel/babel#16159) Reuse computed key memoiser ([@JLHwung](https://github.com/JLHwung)) - `babel-helpers`, `babel-plugin-proposal-decorators` - [#16160](babel/babel#16160) Optimize decorator helper size ([@liuxingbaoyu](https://github.com/liuxingbaoyu)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://gitea.vylpes.xyz/RabbitLabs/random-bunny/pulls/129 Co-authored-by: Renovate Bot <renovate@vylpes.com> Co-committed-by: Renovate Bot <renovate@vylpes.com>

fix

bad6d40

liuxingbaoyu added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Sep 29, 2023

liuxingbaoyu commented Sep 29, 2023

View reviewed changes

liuxingbaoyu added 3 commits September 29, 2023 12:06

update tests

6ab918a

review

046b18a

fix

7c673d2

liuxingbaoyu force-pushed the fix-for-of-array-scoping branch from fa4918a to 7c673d2 Compare October 24, 2023 07:16

nicolo-ribaudo reviewed Oct 24, 2023

View reviewed changes

packages/babel-plugin-transform-for-of/src/index.ts Outdated Show resolved Hide resolved

liuxingbaoyu and others added 2 commits October 25, 2023 11:05

Update packages/babel-plugin-transform-for-of/src/index.ts

308ea19

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

review

c122eee

nicolo-ribaudo approved these changes Oct 25, 2023

View reviewed changes

liuxingbaoyu requested a review from JLHwung November 5, 2023 01:51

Merge branch 'main' into fix-for-of-array-scoping

8a2da65

JLHwung approved these changes Dec 3, 2023

View reviewed changes

JLHwung reviewed Dec 3, 2023

View reviewed changes

liuxingbaoyu force-pushed the fix-for-of-array-scoping branch 2 times, most recently from 44c0302 to d333b33 Compare December 4, 2023 04:41

review

c7804e8

liuxingbaoyu force-pushed the fix-for-of-array-scoping branch from d333b33 to c7804e8 Compare December 5, 2023 01:59

JLHwung merged commit 380d186 into babel:main Dec 5, 2023
48 checks passed

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 5, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `for of` with `iterableIsArray` and shadowing variable #16011

fix: `for of` with `iterableIsArray` and shadowing variable #16011

liuxingbaoyu commented Sep 29, 2023

liuxingbaoyu Sep 29, 2023

nicolo-ribaudo Oct 13, 2023

nicolo-ribaudo Oct 13, 2023

babel-bot commented Sep 29, 2023 •

edited

JLHwung left a comment

JLHwung Dec 3, 2023

liuxingbaoyu Dec 3, 2023 •

edited

liuxingbaoyu commented Dec 3, 2023

fix: for of with iterableIsArray and shadowing variable #16011

fix: for of with iterableIsArray and shadowing variable #16011

Conversation

liuxingbaoyu commented Sep 29, 2023

liuxingbaoyu Sep 29, 2023

Choose a reason for hiding this comment

nicolo-ribaudo Oct 13, 2023

Choose a reason for hiding this comment

nicolo-ribaudo Oct 13, 2023

Choose a reason for hiding this comment

babel-bot commented Sep 29, 2023 • edited

JLHwung left a comment

Choose a reason for hiding this comment

JLHwung Dec 3, 2023

Choose a reason for hiding this comment

liuxingbaoyu Dec 3, 2023 • edited

Choose a reason for hiding this comment

liuxingbaoyu commented Dec 3, 2023

fix: `for of` with `iterableIsArray` and shadowing variable #16011

fix: `for of` with `iterableIsArray` and shadowing variable #16011

babel-bot commented Sep 29, 2023 •

edited

liuxingbaoyu Dec 3, 2023 •

edited