Skip to content

fix: is_top_level incorrectly treats strict-mode scopes as top-level#8878

Merged
hyf0 merged 2 commits intomainfrom
fix/8793-colliding-variable-names
Mar 24, 2026
Merged

fix: is_top_level incorrectly treats strict-mode scopes as top-level#8878
hyf0 merged 2 commits intomainfrom
fix/8793-colliding-variable-names

Conversation

@Dunqing
Copy link
Copy Markdown
Collaborator

@Dunqing Dunqing commented Mar 23, 2026

Closes #8793

Summary

Fix is_top_level() in crates/rolldown_ecmascript_utils/src/scope.rs which incorrectly treated scopes with ScopeFlags::StrictMode as "top-level" (eagerly evaluated during module evaluation).

Root Cause

is_top_level() determines whether the current AST position executes eagerly during module evaluation (as opposed to being deferred inside a function). It checks that all scopes in the stack are "transparent" — i.e., Top, ClassStaticBlock, or empty block scopes.

The bug was that ScopeFlags::StrictMode was included in the transparency check via intersects:

flag.intersects(ScopeFlags::Top | ScopeFlags::StrictMode | ScopeFlags::ClassStaticBlock)

StrictMode is a modifier flag inherited by child scopes, not a scope boundary indicator. An arrow function with "use strict" gets scope flags like Arrow | Function | StrictMode — the intersects(StrictMode) check made this appear "transparent", so code inside the arrow function was incorrectly considered top-level.

How This Causes Colliding Variable Names

Given this input:

// index.js (entry)
import { Pb as He } from "./chunk.js";
console.log(() => { "use strict"; return 0 })
var Ub = ["open", "opening", "toggle"];
console.log(class {
  constructor() { console.log(Ub); }
  static p = He({});
})

The chain of failure:

  1. is_top_level() returns true inside the arrow function — because its StrictMode flag matches the intersects check
  2. Scanner detects return 0 as a top-level return — top-level return is only valid in CommonJS, so the module gets ExportsKind::CommonJs
  3. Module gets CJS-wrapped — its code is placed inside a __commonJSMin(() => { ... }) closure
  4. Deconfliction skips local variables in CJS-wrapped modules — because their code runs in a nested closure, not at chunk root scope. So the local var Ub (array) is NOT registered in the root scope renamer
  5. Cross-chunk import gets name Ub — since the local var Ub wasn't registered, no conflict is detected, and the imported function keeps the name Ub
  6. Collision at runtimestatic p = Ub({}) resolves to the local array ["open", "opening", "toggle"] instead of the imported function, causing TypeError: Ub is not a function

Fix

flag.is_top() || flag.contains(ScopeFlags::ClassStaticBlock) || flag.is_block()

This correctly handles all cases:

  • Top scope + StrictMode (ESM modules): Top after stripping → transparent ✓
  • Block scope + StrictMode: empty after stripping → transparent ✓
  • Arrow/function + StrictMode: Arrow|Function after stripping → NOT transparent ✓

Test plan

  • Added reproduction test case in crates/rolldown/tests/rolldown/issues/8793/
  • All 1645 integration tests pass
  • rolldown_ecmascript_utils unit tests pass

🤖 Generated with Claude Code

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 23, 2026

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit d145781
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/69c1ed75173e4b0007cd8a2e

@Dunqing Dunqing force-pushed the fix/8793-colliding-variable-names branch 2 times, most recently from b6f24e1 to 5e00ad6 Compare March 23, 2026 16:43
@Dunqing Dunqing force-pushed the fix/8793-colliding-variable-names branch from 5e00ad6 to f926213 Compare March 23, 2026 16:51
@Dunqing Dunqing marked this pull request as ready for review March 23, 2026 16:55
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 23, 2026

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing fix/8793-colliding-variable-names (d145781) with main (5f3724a)

Open in CodSpeed

Footnotes

  1. 10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@hyf0 hyf0 enabled auto-merge (squash) March 24, 2026 01:48
@hyf0 hyf0 merged commit 53e0b0a into main Mar 24, 2026
30 checks passed
@hyf0 hyf0 deleted the fix/8793-colliding-variable-names branch March 24, 2026 01:51
This was referenced Mar 25, 2026
shulaoda added a commit that referenced this pull request Mar 25, 2026
## [1.0.0-rc.12] - 2026-03-25

### 🚀 Features

- chunk-optimizer: skip circular dependency check when strict execution order is enabled (#8886) by @hyf0

### 🐛 Bug Fixes

- emit build warnings during watch mode rebuilds (#8897) by @IWANABETHATGUY
- lazy-barrel: load import-then-export specifiers when barrel has local exports (#8895) by @shulaoda
- correct execution order of transferred CJS init calls (#8877) by @IWANABETHATGUY
- mcs: `entriesAware` should calculate sizes without duplication (#8887) by @hyf0
- non-deterministic chunk generation (#8882) by @sapphi-red
- `is_top_level` incorrectly treats strict-mode scopes as top-level (#8878) by @Dunqing

### 🚜 Refactor

- treeshake: migrate SideEffectDetector to Oxc's MayHaveSideEffects trait (#8624) by @Dunqing

### 🧪 Testing

- make dev server tests deterministic by replacing fixed sleeps with event-driven polling (#8561) by @Boshen

### ⚙️ Miscellaneous Tasks

- deps: update dependency vite-plus to v0.1.14 (#8902) by @camc314
- deps: update dependency oxfmt to ^0.42.0 (#8891) by @renovate[bot]
- deps: update rust crate oxc_sourcemap to v6.1.1 (#8890) by @renovate[bot]
- remove Rolldown MF plan (#8883) by @shulaoda
- deps: update rollup submodule for tests to v4.60.0 (#8881) by @sapphi-red
- deps: update test262 submodule for tests (#8880) by @sapphi-red
- deps: upgrade oxc crates to 0.122.0 (#8879) by @shulaoda

Co-authored-by: shulaoda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: colliding variable names

3 participants