fix: handle for-of/for-in loop declarations in regex scanner#522
fix: handle for-of/for-in loop declarations in regex scanner#522
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #522 +/- ##
==========================================
- Coverage 98.73% 95.08% -3.66%
==========================================
Files 14 13 -1
Lines 1817 976 -841
Branches 374 322 -52
==========================================
- Hits 1794 928 -866
- Misses 23 45 +22
- Partials 0 3 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/existing-scanning.test.ts (1)
12-18: Consider addingvarloop cases to match the regex contract.Nice expansion of coverage. Since Line 13 in
src/regexp.tsexplicitly supportsvar, addingfor (var key in obj) {}andfor (var [a, b] of items) {}would close the matrix and guard future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/existing-scanning.test.ts` around lines 12 - 18, Add missing `var` loop test cases to mirror the existing `let`/`const` coverage: update the test data in existing-scanning.test.ts by adding entries for "for (var [a, b] of items) {}" expecting ['a','b'] and "for (var key in obj) {}" expecting ['key']; this aligns the tests with the regex behavior implemented in src/regexp.ts (the support for `var` at the pattern near Line 13) and prevents regressions for `var`-declared loop bindings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/existing-scanning.test.ts`:
- Around line 12-18: Add missing `var` loop test cases to mirror the existing
`let`/`const` coverage: update the test data in existing-scanning.test.ts by
adding entries for "for (var [a, b] of items) {}" expecting ['a','b'] and "for
(var key in obj) {}" expecting ['key']; this aligns the tests with the regex
behavior implemented in src/regexp.ts (the support for `var` at the pattern near
Line 13) and prevents regressions for `var`-declared loop bindings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5686255c-77e0-4537-8482-24e0851a14e8
📒 Files selected for processing (3)
src/regexp.tstest/cases/negative/for-loop-destructuring.jstest/existing-scanning.test.ts
The `RE_EXCLUDE[3]` regex for detecting local variable declarations only accepted `=`, `;`, or `\n` as terminators after the variable pattern. This meant `for (const [a, b] of ...)` destructuring was not recognized as a local declaration, because `of` did not satisfy the terminator. With the `s` (dotall) flag, the `\[.*?\]` branch could then extend across newlines and swallow subsequent `var`/`let`/ `const` declarations, causing unimport to inject duplicate imports. Add `\bof\b` and `\bin\b` as valid terminators so for-of and for-in loop variables are correctly excluded from auto-import candidates. Closes unjs#521
harlan-zw
force-pushed
the
fix/for-loop-destructuring-regex
branch
from
e1f926e to
1cb41b3
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a regex-based local-variable exclusion edge case so for (const/let/var … of …) and for (… in …) loop declarations are properly treated as local declarations during regex scanning, preventing unintended “swallowing” and subsequent incorrect auto-import injection.
Changes:
- Extend
RE_EXCLUDE[3]to acceptof/inas valid terminators for loop declarations. - Add targeted unit tests for
for-of/for-indeclarations and a regression case ensuring subsequent declarations aren’t swallowed. - Add a negative fixture ensuring
refisn’t auto-imported when declared after afor-ofdestructuring loop.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🔗 Linked issue
Resolves #521
Resolves #487
Supersedes #488
❓ Type of change
📚 Description
RE_EXCLUDE[3]only accepted=,;, or\nas terminators after variable patterns. Forfor (const [a, b] of ...), the\[.*?\]branch matched[a, b]but then couldn't terminate onof. With thes(dotall) flag,.*?extended across newlines searching for another]followed by a valid terminator, swallowing any variable declarations in between and causing duplicate import injection.Added
\bof\band\bin\bas valid terminators so for-of and for-in loop variables are correctly excluded from auto-import candidates. Word boundaries prevent false matches inside identifiers (e.g.const offset = 5still works correctly), which is an improvement over #488's approach that used bareofwithout boundaries.All 197 tests pass (including 7 new ones), no snapshot changes.
Summary by CodeRabbit
Bug Fixes
for...ofandfor...inloops, including destructuring patterns.Tests