Repository navigation
Read JS globals through globalThis wherever a same-named binding is in scope - #8785
Merged
Merged
Conversation
…n scope A reference to a @Val global compiled to its bare name, which reads a binding with that name if one is in scope. Previously this was only handled inside the binding's own initializer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Christoph Knittel <ck@cca.io>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Christoph Knittel <ck@cca.io>
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8785 |
cknitt
marked this pull request as ready for review
October 10, 2026 18:27
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8785 +/- ##
==========================================
+ Coverage 80.57% 80.59% +0.02%
==========================================
Files 452 453 +1
Lines 61966 62010 +44
==========================================
+ Hits 49928 49976 +48
+ Misses 12038 12034 -4
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5970d66f2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The discriminant is evaluated before the scope shared by the cases is entered, so case bindings cannot shadow a global read there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Christoph Knittel <ck@cca.io>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
cristianoc
approved these changes
Oct 11, 2026
cknitt
enabled auto-merge (squash)
October 11, 2026 07:22
This was referenced Oct 19, 2026
3 tasks done
mununki
added a commit
that referenced
this pull request
Oct 28, 2026
Why:
A binding printed as `Temporal` hides the global `Temporal` in its
scope. The bindings of the next commit are externals with dotted names
like "Temporal.PlainDate.from", which are printed as written: unlike
`@scope` externals (#8785), they are not read through `globalThis`
when a binding of the same name is in scope. A module named `Temporal`
next to a call of the bindings would then be called instead.
JS modified (module named Temporal):
Example:
type instant
@Val external now: unit => instant = "Temporal.Now.instant"
module Temporal = {
let zone = "UTC"
}
let stamp = () => now()
Before (js):
let Temporal = {
zone: "UTC"
};
function stamp() {
return Temporal.Now.instant(); // the module: a TypeError
}
export {
Temporal,
stamp,
}
After (js):
let $$Temporal = {
zone: "UTC"
};
function stamp() {
return Temporal.Now.instant(); // the global
}
export {
$$Temporal,
stamp,
}
Review: Why not read dotted externals through `globalThis` instead? That
changes every dotted external, not only Temporal's. A reserved name is
how `Intl` and the other built-ins are handled, and it also keeps raw
JavaScript next to the module reading the global. The next commit
tests it in `Stdlib_TemporalTests`.
Breaking: A nested module named `Temporal` is exported to JS as
`$$Temporal`, like one named `Intl`. JavaScript that imports it by name
imports `$$Temporal`.
Signed-off-by: mununki <woonki.moon@gmail.com>
mununki
added a commit
that referenced
this pull request
Oct 28, 2026
* Treat Temporal as a JS global
Why:
A binding printed as `Temporal` hides the global `Temporal` in its
scope. The bindings of the next commit are externals with dotted names
like "Temporal.PlainDate.from", which are printed as written: unlike
`@scope` externals (#8785), they are not read through `globalThis`
when a binding of the same name is in scope. A module named `Temporal`
next to a call of the bindings would then be called instead.
JS modified (module named Temporal):
Example:
type instant
@Val external now: unit => instant = "Temporal.Now.instant"
module Temporal = {
let zone = "UTC"
}
let stamp = () => now()
Before (js):
let Temporal = {
zone: "UTC"
};
function stamp() {
return Temporal.Now.instant(); // the module: a TypeError
}
export {
Temporal,
stamp,
}
After (js):
let $$Temporal = {
zone: "UTC"
};
function stamp() {
return Temporal.Now.instant(); // the global
}
export {
$$Temporal,
stamp,
}
Review: Why not read dotted externals through `globalThis` instead? That
changes every dotted external, not only Temporal's. A reserved name is
how `Intl` and the other built-ins are handled, and it also keeps raw
JavaScript next to the module reading the global. The next commit
tests it in `Stdlib_TemporalTests`.
Breaking: A nested module named `Temporal` is exported to JS as
`$$Temporal`, like one named `Intl`. JavaScript that imports it by name
imports `$$Temporal`.
Signed-off-by: mununki <woonki.moon@gmail.com>
* Add Temporal bindings to the standard library
Why:
JavaScript's Temporal API covers dates, times, time zones and
durations, which `Date` handles poorly, and Node.js 26 ships it. #9044
proposed stdlib bindings for it.
JS added (Temporal bindings):
Example:
let meeting = Temporal.ZonedDateTime.fromStringOrThrow(
"2024-03-09T09:30:00-05:00[America/New_York]",
)
let nextMeeting = meeting->Temporal.ZonedDateTime.addFields({days: 1.})
let length = meeting->Temporal.ZonedDateTime.until(nextMeeting)
let birthday = Temporal.PlainMonthDay.fromString("02-29")
let started = Date.make()->Date.toTemporalInstant
After (js):
import * as Stdlib_Temporal_PlainMonthDay from "@rescript/runtime/lib/es6/Stdlib_Temporal_PlainMonthDay.mjs";
let meeting = Temporal.ZonedDateTime.from("2024-03-09T09:30:00-05:00[America/New_York]");
let nextMeeting = meeting.add({
days: 1
});
let length = meeting.until(nextMeeting);
let birthday = Stdlib_Temporal_PlainMonthDay.fromString("02-29");
let started = new Date().toTemporalInstant();
Review: Why do `==` and `compare` treat any two Temporal values as equal?
Temporal objects keep their state in internal slots and have no
enumerable properties to compare. Each module's docs point to its
`equals` and `compare`; #9044 keeps `==` as it is.
Review: Why does `fromString` rethrow errors other than `RangeError`? Only
a `RangeError` means the string is invalid. Without the `Temporal`
global the call throws a `ReferenceError`, which `None` would hide.
Review: Why are the tests not in `Stdlib_TestSuite`? The installation
tests run that suite on Node.js 22, which lacks Temporal.
`Stdlib_TemporalTests` is its own entry point, which `scripts/test.js`
runs when `globalThis.Temporal` exists. On older Node.js versions,
`DocTest.res` now emits the doc examples it skips as `test.skip`, so
they are still type checked.
Breaking: The stdlib `Temporal` module shadows a project's own top-level
`Temporal` module, such as bindings to the Temporal.io SDK. Rename such
a module.
Signed-off-by: mununki <woonki.moon@gmail.com>
* Map Temporal types to TypeScript in GenType
Why:
GenType emitted the Temporal types as types imported from `Stdlib.gen`
and `Stdlib_Temporal_Common.gen`, files that don't exist, so the
generated TypeScript didn't compile. TypeScript 6.0 declares the same
types in its `esnext.temporal` lib, which `ESNext` includes.
Representation modified (genType's TypeScript type of Temporal values):
Example:
@Gentype
let temporalDateOfDateTime = (x: Temporal.PlainDateTime.t) => x->Temporal.PlainDateTime.toPlainDate
@Gentype
let temporalNextDay = (x: Temporal.PlainDate.t) => x->Temporal.PlainDate.addFields({days: 1.})
Before (ts): tests/gentype_tests/stdlib-no-shims/src/StdlibNoShims.gen.tsx
import type {Temporal_PlainDateTime_t as Stdlib_Temporal_PlainDateTime_t} from './Stdlib.gen';
import type {Temporal_PlainDate_t as Stdlib_Temporal_PlainDate_t} from './Stdlib.gen';
import type {plainDate as Stdlib_Temporal_Common_plainDate} from './Stdlib_Temporal_Common.gen';
export const temporalDateOfDateTime: (x:Stdlib_Temporal_PlainDateTime_t) => Stdlib_Temporal_Common_plainDate = StdlibNoShimsJS.temporalDateOfDateTime as any;
export const temporalNextDay: (x:Stdlib_Temporal_PlainDate_t) => Stdlib_Temporal_PlainDate_t = StdlibNoShimsJS.temporalNextDay as any;
After (ts): tests/gentype_tests/stdlib-no-shims/src/StdlibNoShims.gen.tsx
export const temporalDateOfDateTime: (x:Temporal.PlainDateTime) => Temporal.PlainDate = StdlibNoShimsJS.temporalDateOfDateTime as any;
export const temporalNextDay: (x:Temporal.PlainDate) => Temporal.PlainDate = StdlibNoShimsJS.temporalNextDay as any;
Review: Why match the types of `Temporal.Common` too? Conversions like
`PlainDateTime.toPlainDate` return the shared types of
`Stdlib_Temporal_Common`, which the module types are defined as.
Signed-off-by: mununki <woonki.moon@gmail.com>
---------
Signed-off-by: mununki <woonki.moon@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A reference to a
@valglobal compiles to the global's bare name. If a binding with that name is in scope there, the reference reads the binding instead:Js_pass_external_shadowonly rewrote references inside the binding's own initializer (andx.fieldon toplevel names). The printer avoids some other cases by accident: when it prints a function, it adds the function's free variables, including JS globals, to its scope, so a binding printed after such a function gets a$1suffix. A toplevel binding printed before any function that mentions the global keeps the bare name, and every later reference reads it, as above. This also breaks plain toplevel values (let v = Y.myGlobalA).The pass now tracks the JS scopes: a reference to a JS global is read through
globalThiswherever a binding printed with the same name is in scope:let/function scope covers the whole block;import * as M/let M = require(...)) at toplevel;Bindings are compared by their printed name (
Ext_ident.convert), so bindings named like keywords or reserved globals, which are printed with$$, don't count.globalThisitself is reserved, so a user binding can't capture it.Output changes
Of the 642 benchmark and test files, two outputs change, both cases where master's output was only correct by accident or not at all:
ExternalShadow.mjs:let proc = process→let proc = globalThis.process.processthere is the module's own binding, which only happened to hold the same value; the fixture's comment said "other uses of the external stay as plainprocess", which described the bug.ppx_apply_test.mjs: master renamed the toplevelxxtoxx$1(exported asxx$1 as xx) to keep the externalxxusable; now the binding keeps its name and the external is read asglobalThis.xx.Where master's printer already renamed bindings, the output stays correct but may look different in the same way.
Tests
New
tests/tests/src/external_shadow_test.resdefines real globals with%%rawand checks their values at runtime: a toplevel binding before and after reads (including a plain toplevel value), a read before a toplevel binding, a parameter, a localletafter a read in the same block, a binding in one case of aswitchread in other cases, a loop variable, and a global with the same name as an imported module. It fails on master (the read after the toplevel binding gets the function) and passes with this change.Cost: +0.3% allocated words on the benchmark corpus (the pass now also collects the imported modules and scans each block's declarations), CPU within noise.
🤖 Generated with Claude Code