fix(hir): construct real intrinsic for new globalThis.<Builtin>() (#6726)

[proggeramlug]

Summary

Fixes #6726. new globalThis.Set() compiled but produced an object without any Set methods — new globalThis.Set().has(42) threw TypeError: has is not a function. The plain new Set() and the local-alias const S = globalThis.Set; new S() forms both worked, so the failure was specific to constructing through the inline member-expression callee.

Root cause

The bare-identifier new <Builtin>() arm in lower_new routes every built-in constructor to its real allocator — Set → Expr::SetNew, Map → MapNew, Date → DateNew, the Error family, typed arrays, boxed primitives, Proxy, WeakRef, and so on. A member-expression callee (globalThis.Set) never reaches that arm: it falls through to lower_new_non_ident → Expr::NewDynamic.

For the data-structure builtins — whose construction lives in a dedicated HIR variant rather than in codegen's lower_builtin_new — codegen's try_static_class_name then rerouted the PropertyGet { globalThis, "Set" } callee to lower_new("Set"), which has no Set/Map/Date arm and dead-ended at a class_id=0 empty-object placeholder. (The const S = globalThis.Set; new S() form works because a LocalGet callee instead reaches the runtime js_new_function_construct helper, which does recognise the reified global builtin thunks.)

Fix

In lower_new, detect the new globalThis.<Ident>(...) callee — the real global object (globalThis not lexically rebound) with a recognised built-in constructor name — and re-dispatch through a synthesized bare-identifier new <Ident>(...). A member callee then constructs the exact same intrinsic as the plain form.

• Placed after lower_new_member_native, so the already-working member paths (URL / streams / fetch / worker-messaging) are untouched.
• A shadowing local (const globalThis = { Set: ... }) suppresses the re-dispatch via the ctx.lookup_local("globalThis").is_none() guard, so it keeps using the local object — matching Node.
• Recursion terminates: the synthesized callee is an Ident, not a Member.

Testing

test-files/test_gap_new_globalthis_builtin_6726.ts covers the repro plus Set/Map/Date/WeakMap/WeakSet/TypeError/Array/Promise through globalThis, and the two forms that already worked (bare + local alias) as regression guards. Perry output is byte-for-byte identical to Node.

Verified manually against Node, all matching:

new globalThis.Set().has(42)            → false
new globalThis.Set([1,2,2,3]).size      → 3
new globalThis.Map([["a",1]]).get("a")  → 1
new globalThis.Date(0).getTime()        → 0
new globalThis.TypeError("boom")        → instanceof Error, name "TypeError"
new globalThis.Uint8Array(3)            → real typed array
new globalThis.RegExp("a.c","g")        → real regex
new globalThis.Proxy(...)               → real proxy
const globalThis = {...}; new globalThis.Set()  → uses the local (shadow respected)

cargo test -p perry-hir --lib passes (247/247). No version bump / changelog per the contributor-PR convention.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed new globalThis.<Builtin>(...) construction to match new <Builtin>(...) for supported JavaScript and Web/DOM built-in constructors.
  • Added safeguards so this works correctly despite user shadowing of the bare constructor name and non-intrinsic rebindings of globalThis.

• Tests

  • Added a regression test covering many built-ins (e.g., Set, Map, Date, WeakMap/WeakSet, Array, Promise, and more) and verifying equivalent behavior across the different construction forms.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 355bab53-1f35-46ad-9108-9d09871a3470

📥 Commits

Reviewing files that changed from the base of the PR and between 155b7ee and 26ff6b3.

📒 Files selected for processing (4)

• crates/perry-hir/src/lower/context.rs
• crates/perry-hir/src/lower/expr_new.rs
• crates/perry-hir/src/lower/lowering_context.rs
• test-files/test_gap_new_globalthis_builtin_6726.ts

---

📝 Walkthrough

Walkthrough

The lowering logic recognizes eligible unshadowed globalThis built-in constructors, rewrites them to bare constructor calls, and reuses existing intrinsic handling. A one-shot context flag preserves the re-dispatch semantics, with regression tests covering built-ins and shadowing boundaries.

Changes

GlobalThis built-in constructors

Layer / File(s)	Summary
Constructor recognition crates/perry-hir/src/lower/expr_new_builtins.rs	Adds a whitelist for reified global built-in constructor names.
Lowering and intrinsic state crates/perry-hir/src/lower/expr_new.rs, crates/perry-hir/src/lower/lowering_context.rs, crates/perry-hir/src/lower/context.rs	Rewrites eligible unshadowed new globalThis.<Builtin>(...) calls to bare constructor calls, consumes a one-shot intrinsic flag, and initializes that flag in new lowering contexts.
Regression coverage test-files/test_gap_new_globalthis_builtin_6726.ts	Tests construction across multiple built-ins, direct constructors, local aliases, lexical shadowing, and globalThis rebinding.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: andrewtdiz, machineloop

Sequence Diagram(s)

sequenceDiagram
  participant Source as new globalThis.<Builtin>(...)
  participant Lowering as lower_new
  participant Intrinsic as Builtin intrinsic path
  Source->>Lowering: lower member-expression constructor
  Lowering->>Lowering: verify globalThis and whitelist constructor
  Lowering->>Lowering: re-dispatch as new <Builtin>(...)
  Lowering->>Intrinsic: lower with intrinsic handling
  Intrinsic-->>Source: emit intrinsic allocation

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main change to handle new globalThis.<Builtin>().
Description check	✅ Passed	The description is detailed and covers summary, root cause, fix, and testing, though it doesn't follow the template exactly.
Linked Issues check	✅ Passed	The changes address #6726 by making new globalThis.Set() construct the real intrinsic and adding regression coverage.
Out of Scope Changes check	✅ Passed	The added helper, context flag, and tests all support the same intrinsic-construction fix and appear in scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test-files/test_gap_new_globalthis_builtin_6726.ts (1)

53-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add regression tests for local shadowing.

Please add test cases to ensure that new globalThis.Set() behaves correctly even when both globalThis and the built-in (Set) are locally shadowed. This will safeguard against the bugs in the lowering logic and guarantee that globalThis explicitly constructs the intrinsic.

🧪 Suggested test cases

// 1. globalThis should bypass local shadowing of the built-in
{
  class Set {
    constructor() { (this as any).isLocal = true; }
  }
  const s = new globalThis.Set([1, 2]);
  console.log(s.has(1), (s as any).isLocal); // Expected: true, undefined
}

// 2. A locally shadowed globalThis should not construct the intrinsic
{
  const globalThis = {
    Set: class {
      constructor() { (this as any).isFake = true; }
    }
  };
  const s = new globalThis.Set();
  console.log((s as any).isFake); // Expected: true
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-files/test_gap_new_globalthis_builtin_6726.ts` around lines 53 - 60, Add
regression cases in test_gap_new_globalthis_builtin_6726.ts covering new
globalThis.Set() with a locally shadowed Set, verifying it constructs the
intrinsic and supports Set behavior, and a separately scoped locally shadowed
globalThis, verifying that new globalThis.Set() constructs the shadowed fake
implementation. Follow the suggested scoped setups and assertions while
preserving the existing tests.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/perry-hir/src/lower/expr_new.rs`:
- Around line 145-179: The globalThis rewrite in lower_new has incomplete
globalThis shadowing validation and allows the synthesized built-in callee to be
shadowed lexically. Replace lookup_local("globalThis") with comprehensive
non-class and class binding checks using shadows_unqualified_global and
lookup_class, and route the synthesized constructor through built-in lowering
with force_unshadowed behavior so local bindings such as class Set cannot
override the global intrinsic.

---

Outside diff comments:
In `@test-files/test_gap_new_globalthis_builtin_6726.ts`:
- Around line 53-60: Add regression cases in
test_gap_new_globalthis_builtin_6726.ts covering new globalThis.Set() with a
locally shadowed Set, verifying it constructs the intrinsic and supports Set
behavior, and a separately scoped locally shadowed globalThis, verifying that
new globalThis.Set() constructs the shadowed fake implementation. Follow the
suggested scoped setups and assertions while preserving the existing tests.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4ab5c68d-9e38-462e-ab90-a201051928d5

📥 Commits

Reviewing files that changed from the base of the PR and between f073bbf and 155b7ee.

📒 Files selected for processing (3)

• crates/perry-hir/src/lower/expr_new.rs
• crates/perry-hir/src/lower/expr_new_builtins.rs
• test-files/test_gap_new_globalthis_builtin_6726.ts

[proggeramlug]

Thanks @coderabbitai — both points addressed in 26ff6b3:

1. Incomplete globalThis shadowing check. Swapped lookup_local("globalThis") for shadows_unqualified_global("globalThis"), which already covers locals + functions + imports + classes (context.rs). A function/class/imported globalThis now suppresses the rewrite and the member reads that user binding.

2. Synthetic callee must bypass lexical shadowing of the built-in. globalThis.Set is the intrinsic even when a class Set {} is in scope. Added a one-shot global_intrinsic_new_once flag: the re-dispatch sets it, and lower_new consumes it at the top (so it can't leak into argument or nested-new lowering) and uses it to null out the callee_local_at_entry snapshot and force shadowed_by_user_binding = false for that single callee — so the built-in arm fires and constructs the intrinsic.

Both of your suggested test cases were added to test_gap_new_globalthis_builtin_6726.ts and pass byte-for-byte against Node:

• block-local class Set {} → new globalThis.Set([1,2]) builds the intrinsic (has(1)===true, isLocal===undefined), while a bare new Set() in the same scope still resolves to the local class;
• rebound const globalThis = { Set: class {...} } → new globalThis.Set() builds that object's Set (isFake===true).

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@proggeramlug Thanks for the thorough follow-up. The broadened globalThis shadowing check and the one-shot intrinsic-dispatch guard address both concerns, and the two Node-parity regressions cover the important lexical-shadowing cases well.

🐇 ✅

---

✏️ Learnings added

Learnt from: proggeramlug
URL: https://github.com/PerryTS/perry/pull/6730

Timestamp: 2026-07-20T17:49:28.588Z
Learning: In `crates/perry-hir/src/lower/context.rs`, `shadows_unqualified_global(name)` is the appropriate check for whether an unqualified global is shadowed: it covers locals, functions, imports, and classes. For `new globalThis.<Builtin>()` lowering, a real unshadowed `globalThis` property access denotes the intrinsic constructor even when an identifier with the same built-in name (such as a local `class Set {}`) is lexically shadowed.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!