Fix _arrayWithHoles bypassing custom Symbol.iterator on array-backed Proxies - #18186
Fix _arrayWithHoles bypassing custom Symbol.iterator on array-backed Proxies#18186webdevelopersrinu wants to merge 6 commits into
_arrayWithHoles bypassing custom Symbol.iterator on array-backed Proxies#18186Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/62121 |
|
commit: |
|
What was the human involvement in this PR? The description really reads like it's AI-generated, and I haven't even checked the code. |
|
I used AI a lot in this PR — it helped me find this issue, analyze the root cause, write the fix and the tests, and yes, the description also. That's why it reads like that. My involvement: I chose to work on this issue, everything ran on my local machine and my account, I reviewed the steps, the full test suites were run locally before pushing (including checking that the new exec test fails without the helper change), and I fixed the T7199 fixture when CI failed. I'm a real developer, not a bot account, and I'm here to answer questions and make any changes you ask. I understand the fix: Array.isArray returns true for a Proxy around an array, so the helper skipped the iterator protocol and read indexes directly. The guard makes the fast path apply only when the value iterates with the default array iterator. If this level of AI involvement is not acceptable for contributions here, I completely understand if you close this and sorry for the noise in that case. If it's OK, I'm happy to rewrite the description in my own words and address any review feedback. |
|
In the future please write at least the PR description yourself. |
| // Only take the fast path for arrays that iterate with the default array | ||
| // iterator: Array.isArray pierces Proxies, so an array-backed Proxy with a | ||
| // custom Symbol.iterator must fall through to the iterator-based helpers | ||
| // to match native destructuring semantics. (#18181) | ||
| if ( |
There was a problem hiding this comment.
Here it's enough to say "protect against arrays (or proxies) with a custom iterator"
There was a problem hiding this comment.
arrayWithHoles is only used by slicedToArray:
babel/packages/babel-helpers/src/helpers/slicedToArray.ts
Lines 9 to 16 in 0530a49
As we can see, the arrayWithHoles helper is followed by the _iterableToArrayLimit helper, which also accesses the iterator property. Is it possible that we somehow merge arrayWithHoles into _iterableToArrayLimit such that 1) the iterator is only accessed once and 2) the arrayWithHoles helper is no longer required?
|
Yes, possible. |
Yes you are right. Their structures are similar, sure you can try to do both in this PR. It's not a very big change. And we should probably add a new test to ensure that the iterator is only accessed once in |
|
Done, pushed. I merged it into both iterableToArrayLimit and iterableToArray. Also added the test that counts how many times the iterator is read, for both slicedToArray and toArray. One note in iterableToArray I used a loop instead of Array.from. Array.from reads the iterator property a second time inside, so the test fails with it. The Node 14.2 CI failure looks unrelated to this change. That test only uses inheritsLoose and objectWithoutProperties, and the same snapshot passes on 13.x and 16.x. |
…WithHoles Array.isArray pierces Proxies, so the _arrayWithHoles fast path returned array-backed Proxies directly and destructuring read them positionally, diverging from native semantics (which always use the iterator protocol). Guard the fast path so it only applies to values that iterate with the default array iterator, falling through to the iterator-based helpers otherwise. Fixes babel#18181
a60dffc to
f337c2a
Compare
|
Some other helpers also use |
Co-authored-by: Nicolò Ribaudo <hello@nicr.dev>
|
I checked this. The two merged helpers are already safe. They compare the method we already fetched, and that fetch also covers But arrayWithHoles had a problem in old environments with no Symbol. I added the |
Fixes #18181
Array destructuring on a Proxy wrapping an array was reading indexes
directly, but it should use the iterator. Added the check @nicolo-ribaudo
suggested in the issue, so the fast path only applies to normal arrays.
Added an exec test and updated the T7199 fixture. Tests pass locally.