fix(hir): user class shadowing a global name loses to the intrinsic in new expressions (#6233)#6270
Merged
Merged
Conversation
…n `new` expressions (#6233) A module-scope `class Symbol extends Base {}` legally shadows the global Symbol, but the built-in constructor arms in lower_new fired by NAME alone, so `new Symbol()` bound to the intrinsic and threw "Symbol is not a constructor" at module init (effect's SchemaAST.ts declares AST node classes named Symbol and BigInt — importing the effect barrel crashed at startup under perry.compilePackages). Depending on the name this crashed (Symbol/BigInt/Proxy/WeakRef/FinalizationRegistry/AggregateError) or silently constructed the wrong object (Map/Set/Date/Number/String/ Boolean/Error/TypeError/Uint8Array/Int32Array: native instance, field initializers never ran, instanceof the user class false). 16 of the issue's 22 blast-radius shapes misbehaved on main. Same family as #5912/#5913 (new URL) and #6003 (class Headers): unconditional by-name native routing that ignores lexical shadowing. - lower/expr_new.rs: snapshot `shadowed_by_user_binding` once at the ident-arm top (same scope-stability hazard as callee_local_at_entry: argument lowering inside an arm can disturb the locals stack) and gate every built-in constructor arm on it: Map/Set/Date/RegExp, the Symbol/BigInt/Math/JSON non-constructible rejection (#2883's missing shadow guard), Proxy, Number/String/Boolean boxed primitives, AggregateError, the Error family, WeakRef, FinalizationRegistry, Uint8Array, the other typed arrays, and the bare-global MessageChannel/BroadcastChannel arm. The existing Object/Function/URL/ URLSearchParams/URLPattern/TextEncoder/TextDecoder guards are unified onto the same snapshot (adds the forward-declared sibling-class case they missed). - lower_types.rs: infer_type_from_expr typed `new Map()` as the builtin Generic{base:"Map"} by name, which routes the binding's method calls down the collection intrinsic fast paths — SIGSEGV when the receiver is a user-class instance. A user class in classes_index now wins (also gated url_encoding_constructor_type). Mirrored in destructuring/var_decl/type_infer.rs (user-class arm moved ahead of the builtin-name arms). - lower/lower_expr/arm_bin.rs: the `x instanceof WeakRef | FinalizationRegistry` compile-time fold answered from the weak-locals pre-scan even when the name is shadowed; it now backs off so the generic instanceof path tests the user class id. - lower/pre_scan.rs: the weak-locals pre-scan tagged `const w = new WeakRef(x)` as a native weak instance by constructor name alone, so `w.deref()` dispatched to the intrinsic fast path; collect shadowing declarations (class/fn/var/import — same name-keyed scope-blind granularity as the existing poison set) and skip tracking shadowed names. Regression test test_issue_6233_shadowed_global_class_new.ts covers the issue repro plus the 22-name blast-radius matrix, reserved native method names on shadowed classes (Map.get/set, WeakRef.deref), ctor args through a shadowed RegExp, and instanceof for every case — matches node byte-for-byte. Targeted parity sweep (~160 existing collection/error/ weak/url/typed-array tests) shows no regressions vs main. Fixes #6233.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (5)
📝 WalkthroughWalkthroughUser-defined classes and bindings that shadow built-in constructor names are respected across constructor lowering, type inference, intrinsic pre-scanning, ChangesConstructor shadowing
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
crates/perry-hir/src/lower_types.rs (1)
553-572: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHoist the shadow guard above the generic return
new Map<number>()on a user-definedclass Map<T>still lowers toType::Generic { base: "Map", .. }, so the downstream Map fast paths can misclassify it as the builtin. Move theclasses_indexcheck ahead of thetype_argsearly-return, or handle shadowed classes in that branch.🤖 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 `@crates/perry-hir/src/lower_types.rs` around lines 553 - 572, In the new-expression type lowering logic, user-defined classes must take precedence over generic builtin handling. Update the logic around the type_args early return and the classes_index shadow check so classes_index.contains_key(name.as_str()) returns Type::Named(name) before constructing Type::Generic, including for new Map<number>().
🤖 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/destructuring/var_decl/type_infer.rs`:
- Around line 98-112: Update the constructor handling around the class-name
lookup to apply the same shadows_unqualified_global guard used by the other
constructor paths. Ensure user-defined or imported Map, Set, URLSearchParams,
and typed-array names bypass the builtin inference arms, while unshadowed
globals continue inferring Type::Named or Type::Generic correctly.
---
Outside diff comments:
In `@crates/perry-hir/src/lower_types.rs`:
- Around line 553-572: In the new-expression type lowering logic, user-defined
classes must take precedence over generic builtin handling. Update the logic
around the type_args early return and the classes_index shadow check so
classes_index.contains_key(name.as_str()) returns Type::Named(name) before
constructing Type::Generic, including for new Map<number>().
🪄 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: c3b7d37e-16cf-431b-9681-f34812dbda10
📒 Files selected for processing (6)
crates/perry-hir/src/destructuring/var_decl/type_infer.rscrates/perry-hir/src/lower/expr_new.rscrates/perry-hir/src/lower/lower_expr/arm_bin.rscrates/perry-hir/src/lower/pre_scan.rscrates/perry-hir/src/lower_types.rstest-files/test_issue_6233_shadowed_global_class_new.ts
…typing - lint: lower_types.rs hit 2007 lines (2000-line file cap) after the #6233 guards. Split per the repo recipe: the TS-annotation extraction group (extract_ts_type & friends, decorator lowering) moves to lower_types/extract.rs as a pure re-exported submodule — named re-exports keep every existing call path compiling unchanged. The six `use crate::lower_types::*` globs that became redundant are removed or replaced with named imports (back to the exact pre-change warning count). - CodeRabbit Major: the type-inference shadow guards only honored user CLASSES — a local, `function Map() {}`, or import shadowing a built-in name still got the built-in declared type, routing method calls on the constructed instance down the intrinsic fast paths. Both inference sites (infer_type_from_expr's New arm + url_encoding_constructor_type, and the var-decl chain in destructuring/var_decl/type_infer.rs) now mirror the construction-side predicate (shadows_unqualified_global), scoped to a conservative builtin_constructor_inference_name set so module-export names that legitimately arrive through local bindings (Buffer, the stream and event classes) keep their arms. - CodeRabbit Minor (outside-diff): `new Map<number>()` on a generic user `class Map<T>` took the explicit-type-args early return and still produced `Generic { base: "Map" }` — exactly what the collection fast-path recognizers key on. The user-class check is hoisted above that early return; built-in-colliding user classes stay `Named` even with explicit type args (both inference sites). - New regression test test_issue_6233_shadowed_global_fn_new.ts: function/const constructors named Map / Set / Uint8Array / URLSearchParams whose instances carry intrinsic-colliding members (get/set/has/size/length) — matches node byte-for-byte.
proggeramlug
pushed a commit
that referenced
this pull request
Jul 11, 2026
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
1 participant
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.
Summary
A module-scope user class that legally shadows a global name —
class Symbol extends Base {}…new Symbol()— lost to the intrinsic: the built-in constructor arms inlower_new(perry-hirlower/expr_new.rs) fired by NAME alone. Depending on the name this either crashed at module init (Symbol/BigInt→ "is not a constructor",Proxy/WeakRef/FinalizationRegistry/AggregateError→ the intrinsic's argument validation) or silently constructed the wrong object (Map/Set/Date/Number/String/Boolean/Error/TypeError/Uint8Array/Int32Array→ native instance, so field initializers never ran andinstanceofthe user class was false).Real-world blocker:
effect@4.0.0-beta.97'sSchemaAST.tsdeclares AST node classes namedSymbolandBigIntand constructs them at module init, so importing the effect barrel crashed underperry.compilePackages.Baseline on current
main(22-case blast-radius matrix from the issue): 16 of 22 shapes misbehaved. With this PR all 22 matchnode --experimental-strip-typesbyte-for-byte.Root cause & fix
Same bug family as #5912/#5913 (
new URL()) and #6003 (class Headers): unconditional by-name native routing that ignores lexical shadowing. Four surfaces:perry-hir/src/lower/expr_new.rs— the core fix. Ashadowed_by_user_bindingflag is snapshotted ONCE at the top of the ident arm (next tocallee_local_at_entry, for the same scope-stability reason: argument lowering inside an arm can disturb the locals stack, so fresh lookups later are unreliable). It checkslookup_class/ the local snapshot /lookup_func/lookup_imported_func/forward_class_names(a siblingclass Xdeclared later in the same function body). Every built-in constructor arm now backs off when the name is shadowed, so the construct falls through to the user-class/local-dispatch paths: Map, Set, Date, RegExp, the Symbol/BigInt/Math/JSON non-constructible rejection (runtime: throw TypeError for new Symbol and new BigInt #2883's missing shadow guard), Proxy, Number/String/Boolean boxed primitives, AggregateError, the Error family, WeakRef, FinalizationRegistry, Uint8Array, the other typed arrays, the bare-global MessageChannel/BroadcastChannel arm, and the existing Object/Function/URL/URLSearchParams/URLPattern/TextEncoder/TextDecoder guards are unified onto the same snapshot (they previously missed the forward-declared-class case).perry-hir/src/lower_types.rs—infer_type_from_expr'snew C()arm typednew Map()as the builtinGeneric { base: "Map" }by name, which routes the binding's later method calls (m.get(...)) down the collection intrinsic fast paths instead of the user methods. A user class inclasses_indexnow wins (also gatedurl_encoding_constructor_type). Same guard added to the sibling inference chain indestructuring/var_decl/type_infer.rs(user-class arm moved ahead of the builtin-name arms).perry-hir/src/lower/lower_expr/arm_bin.rs— thex instanceof WeakRef|FinalizationRegistrycompile-time fold answered from the weak-locals pre-scan even when a user class shadows the name; it now backs off (shadows_unqualified_global) so the generic instanceof path tests the user class id.perry-hir/src/lower/pre_scan.rs— the weak-locals pre-scan taggedconst w = new WeakRef(...)as a native WeakRef instance by constructor name alone, sow.deref()dispatched to the intrinsic fast path even whenWeakRefis a user class. The scan now collects shadowing declarations (class/function/var/import, same name-keyed scope-blind granularity as its existing poison mechanism) and skips tracking for shadowed names.Testing
test-files/test_issue_6233_shadowed_global_class_new.ts: the exact issue repro plus the full blast-radius matrix (Symbol, BigInt, Proxy, WeakRef with a reservedderefmethod name, FinalizationRegistry, AggregateError, Map with reservedget/setmethod names, Set, Date, Number, String, Boolean, Error, TypeError, Uint8Array, Int32Array, RegExp with ctor args, and the previously-working Widget/Object/Array/Promise/Function controls). Matches node byte-for-byte.new Symbol()TypeError, iteration and.size/.execfast paths) — byte-identical to node, so the guards don't disturb the normal native routing.cargo test -p perry-hir: 18/19 pass; the 1 failure (logical_property_assignment_short_circuits_the_store_4586) fails identically on pristineorigin/mainin a fresh worktree — pre-existing, unrelated to this change.cargo fmt --all -- --checkclean.Not covered (separate defect noted in the issue):
globalThis.Symbol("x")returning atypeof "object"value — that's the globalThis property read path, notnew-expression lowering.Fixes #6233.
Summary by CodeRabbit
Bug Fixes
newresolution and constructor type inference when user-defined bindings shadow built-in constructors (includingSymbol,BigInt,Proxy,WeakRef,FinalizationRegistry,Map,Set,Date, typed arrays, and errors).instanceoffolding forWeakRef/FinalizationRegistrywhen the names are shadowed, ensuring correct runtime dispatch.Tests
#6233regression tests covering shadowed global classes and shadowed constructor-like functions/values.