Skip to content

Conversation

@cknitt
Copy link
Member

@cknitt cknitt commented Dec 20, 2025

Fix @val shadowing (fixes #8093)

Summary

When a @val external is assigned to a local let with the same name (example: "process"), the JS output becomes let process = process; and crashes at runtime. This PR rewrites the initializer to globalThis.process only when a local lexical binding would shadow that global, while leaving all other uses unchanged.

Example:

module X = {
  @val external process: unknown = "process"
}

let process = X.process
let proc = X.process

Output after this change:

let process = globalThis.process;
let proc = process;

Why this approach

  • The failure is a JS runtime error caused by shadowing a global with a local let.
  • We only need to fix the conflicting initializer. Emitting globalThis everywhere would be noisier and change more output than necessary.
  • The rewrite is scoped and conservative: it only applies when a local lexical binding would otherwise read itself.

Why existing name mangling cannot solve it

ReScript’s $1/$2 name mangling only applies to compiler-owned identifiers (Ident.t) that go through Ext_pp_scope. @val externals are emitted as raw JS globals (E.js_global / Ext_ident.create_js) and do not participate in that scope renaming system. That means the compiler cannot “rename” process to process$1 in this case, because the global read is not an Ident.t that the mangler controls. We must explicitly disambiguate the global read instead.

Implementation notes

  • Introduce a small JS pass that detects lexical bindings which shadow JS globals and rewrites only those reads to globalThis.<name>.
  • Skip JS keywords and JS globals to avoid generating invalid JS (delete) or unnecessary rewrites.

Tests

  • Source-level test in tests/tests/src/ExternalShadow.res covers the shadowing case and a non-shadowed alias.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 20, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@8098

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@8098

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@8098

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@8098

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@8098

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@8098

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@8098

commit: 7f90da1

@cknitt cknitt changed the title Fix shadowing @val bindings (rewrite using globalThis) Fix @val shadowing (rewrite using globalThis) Dec 21, 2025
@cknitt cknitt changed the title Fix @val shadowing (rewrite using globalThis) Fix @val shadowing (rewrite using globalThis) Dec 21, 2025
@cknitt cknitt requested review from cristianoc and zth December 21, 2025 08:30
@cknitt
Copy link
Member Author

cknitt commented Dec 21, 2025

Note: a similar issue was reported in #7478. We can later try extending this approach to solve #7478, too.

@cknitt cknitt merged commit 6fd5e2a into rescript-lang:master Dec 21, 2025
25 checks passed
@cknitt cknitt deleted the fix-shadowing-val branch December 21, 2025 11:00
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.

Access before initialization error from @val/@new

2 participants