Improve Zod compatibility support#5874
Open
TheHypnoo wants to merge 130 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds namespace and constructor metadata plumbing through codegen and the compile pipeline, updates HIR lowering for re-exports and static initialization, revises runtime object-model and reflection behavior, and adds a broad regression suite plus a zod3-basic release fixture. ChangesCodegen and HIR plumbing
Constructor and class lowering
Runtime object-model, reflection, and value fixes
Integration tests and fixtures
Estimated code review effort: 5 (Critical) | ~150 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
# Conflicts: # crates/perry-codegen/src/codegen/method.rs # crates/perry-hir/src/lower_decl/body_stmt.rs # crates/perry-runtime/src/object/class_registry/construct.rs # crates/perry-runtime/src/object/object_ops/define_properties.rs # crates/perry-runtime/src/value/dynamic_arith.rs
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
crates/perry-codegen/tests/native_proof_buffer_views.rs (1)
10-64: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
namespace_import_prefixestoempty_opts()
CompileOptionsnow requiresnamespace_import_prefixes, but this test helper still omits it. That makescrates/perry-codegen/tests/native_proof_buffer_views.rsfail to compile with E0063.Fix
namespace_member_namespace_prefixes: std::collections::HashMap::new(), + namespace_import_prefixes: std::collections::HashMap::new(), emit_ir_only: 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 `@crates/perry-codegen/tests/native_proof_buffer_views.rs` around lines 10 - 64, The empty_opts() test helper is missing the new CompileOptions field namespace_import_prefixes, which causes the struct literal to be incomplete. Update empty_opts() to initialize namespace_import_prefixes alongside the other namespace-related collections in CompileOptions so native_proof_buffer_views.rs continues to compile.crates/perry-codegen/tests/native_proof_regressions.rs (1)
11-65: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
namespace_import_prefixestoempty_opts()
CompileOptionsnow requiresnamespace_import_prefixes, so this struct literal will not compile until that field is initialized (or the literal uses..Default::default()).🐛 Fix
namespace_member_namespace_prefixes: std::collections::HashMap::new(), + namespace_import_prefixes: std::collections::HashMap::new(), emit_ir_only: 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 `@crates/perry-codegen/tests/native_proof_regressions.rs` around lines 11 - 65, The CompileOptions literal in empty_opts() is missing the new namespace_import_prefixes field, so update this helper to initialize that field consistently with the other namespace/import maps (or switch the struct literal to use default initialization where appropriate). Keep the fix local to empty_opts() and ensure the CompileOptions construction still covers all required fields referenced elsewhere in native_proof_regressions.rs.crates/perry-codegen/tests/shadow_slot_hygiene.rs (1)
5-59: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
namespace_import_prefixestoempty_opts()CompileOptionsnow requires this field, so the fixture won't compile until it’s included.🐛 Fix
namespace_member_namespace_prefixes: std::collections::HashMap::new(), + namespace_import_prefixes: std::collections::HashMap::new(), emit_ir_only: 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 `@crates/perry-codegen/tests/shadow_slot_hygiene.rs` around lines 5 - 59, The empty_opts() test fixture is missing the new CompileOptions field namespace_import_prefixes, so it no longer matches the struct definition. Update empty_opts() in shadow_slot_hygiene.rs to initialize namespace_import_prefixes alongside the other namespace_* collections, using the appropriate empty collection type consistent with the CompileOptions fields.crates/perry-codegen/tests/static_symbol_hygiene.rs (1)
5-59: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
namespace_import_prefixestoempty_opts()—CompileOptionsrequiresnamespace_import_prefixes: std::collections::HashMap::new(), so this test fixture will not compile without it.🤖 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-codegen/tests/static_symbol_hygiene.rs` around lines 5 - 59, The test fixture in empty_opts() is missing the CompileOptions field namespace_import_prefixes, so the struct initialization is incomplete and won’t compile. Update empty_opts() to include namespace_import_prefixes with an empty std::collections::HashMap, keeping it consistent with the other namespace_* option fields in the CompileOptions setup.crates/perry-codegen/tests/typed_feedback.rs (1)
36-90: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
namespace_import_prefixestoempty_opts()
This struct literal is missing the requirednamespace_import_prefixesfield, socrates/perry-codegen/tests/typed_feedback.rswill not compile until it’s initialized here.🤖 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-codegen/tests/typed_feedback.rs` around lines 36 - 90, The CompileOptions literal in empty_opts is missing the required namespace_import_prefixes field, so update that constructor to initialize it alongside the other namespace_* maps and sets. Use the empty_opts helper in typed_feedback.rs as the fix location and ensure the new field is set consistently with the rest of the default CompileOptions values so the test file compiles.crates/perry-codegen/tests/argless_builtin_extra_args.rs (1)
10-64: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
namespace_import_prefixestoempty_opts
CompileOptionsnow includesnamespace_import_prefixes, but this struct literal still omits it and has no..Default::default(), so the test file won't compile.Suggested fix
namespace_member_origin_names: std::collections::HashMap::new(), namespace_member_vars: std::collections::HashSet::new(), namespace_member_namespace_prefixes: std::collections::HashMap::new(), + namespace_import_prefixes: std::collections::HashMap::new(), emit_ir_only: 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 `@crates/perry-codegen/tests/argless_builtin_extra_args.rs` around lines 10 - 64, The empty_opts helper is missing the new CompileOptions field namespace_import_prefixes, so the struct literal no longer matches the type. Update empty_opts to initialize namespace_import_prefixes alongside the other namespace_* maps, using the same pattern as the nearby CompileOptions fields, so the test helper stays in sync with CompileOptions.crates/perry-codegen/tests/destructure_call_location.rs (1)
21-75: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
namespace_import_prefixesto thisCompileOptionsliteral. This field is required byCompileOptions, so this test file will fail to compile until it’s added here too.🤖 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-codegen/tests/destructure_call_location.rs` around lines 21 - 75, Add the missing namespace_import_prefixes field to the CompileOptions literal in base_opts so the test stays in sync with the CompileOptions struct; use the same pattern as the other HashMap/HashSet initializers in this helper and update any related constructor-style test setup in destructure_call_location.rs if needed.crates/perry-codegen/tests/class_keys_gc_root.rs (1)
29-83: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
namespace_import_prefixestoentry_opts()
ThisCompileOptionsliteral is missing the newnamespace_import_prefixesfield, so the test crate no longer matchesCompileOptions. Add the same initializer used in the other helpers.🐛 Suggested fix
namespace_member_origin_names: std::collections::HashMap::new(), namespace_member_vars: std::collections::HashSet::new(), namespace_member_namespace_prefixes: std::collections::HashMap::new(), + namespace_import_prefixes: std::collections::HashMap::new(), emit_ir_only: 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 `@crates/perry-codegen/tests/class_keys_gc_root.rs` around lines 29 - 83, The CompileOptions initializer in entry_opts() is missing the new namespace_import_prefixes field, so the test helper no longer matches the struct definition. Update the entry_opts() literal to include namespace_import_prefixes with the same initialization pattern used by the other CompileOptions helper constructors in this test suite. Keep the field ordering consistent with the surrounding CompileOptions setup so the test compiles cleanly.crates/perry-codegen/tests/large_object_barriers.rs (1)
5-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
namespace_import_prefixestoempty_opts()incrates/perry-codegen/tests/large_object_barriers.rs
CompileOptionsincludes this field, so the helper is incomplete untilnamespace_import_prefixes: std::collections::HashMap::new(),is added.🤖 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-codegen/tests/large_object_barriers.rs` around lines 5 - 59, The CompileOptions builder in empty_opts() is missing the namespace_import_prefixes field, so update that helper to initialize it alongside the other namespace_* maps. Add namespace_import_prefixes with an empty HashMap in empty_opts() so the test setup matches the current CompileOptions definition.crates/perry-codegen/tests/macos_bundle_chdir_gate.rs (1)
11-65: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
namespace_import_prefixesto thisCompileOptionsliteral. This field is part ofCompileOptions, so the helper is incomplete and won’t compile as written.🤖 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-codegen/tests/macos_bundle_chdir_gate.rs` around lines 11 - 65, The `entry_opts` helper builds a `CompileOptions` literal but is missing the `namespace_import_prefixes` field, so update that constructor to include it alongside the other namespace-related maps/sets. Use the `CompileOptions` struct in `entry_opts` as the anchor and add the missing field with the appropriate empty collection value so the test compiles again.crates/perry-codegen/tests/constructor_recursion.rs (1)
5-59: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
namespace_import_prefixesto thisCompileOptionsliteral. This initializer is missing a required field and breaks compilation.🤖 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-codegen/tests/constructor_recursion.rs` around lines 5 - 59, The empty_opts helper’s CompileOptions initializer is missing the namespace_import_prefixes field, so update this CompileOptions literal in empty_opts to include that required member alongside the other namespace-related maps/sets. Use the existing CompileOptions struct definition and the surrounding fields in empty_opts to add namespace_import_prefixes with the appropriate empty default so the test compiles again.
🧹 Nitpick comments (7)
crates/perry-runtime/src/object/class_registry/state.rs (1)
510-551: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated parent-prototype resolution.
The
parent_proto_bitscomputation inclass_decl_prototype_value(Lines 510-522) andrefresh_class_decl_prototype_parent(Lines 535-547) is identical. Extracting a shared helper keeps the two paths from drifting.♻️ Proposed helper
fn resolve_class_parent_prototype_bits(class_id: u32) -> Option<u64> { dynamic_parent_prototype_bits(class_id).or_else(|| { get_parent_class_id(class_id) .filter(|parent_id| *parent_id != 0 && *parent_id != class_id) .and_then(|parent_id| { builtin_prototype_value_for_class_id(parent_id) .or_else(|| { let parent_proto = class_decl_prototype_value(parent_id); ((parent_proto.to_bits() >> 48) == 0x7FFD).then_some(parent_proto) }) .map(f64::to_bits) }) .or_else(global_object_prototype_bits) }) }Then both sites become
let parent_proto_bits = resolve_class_parent_prototype_bits(class_id);.🤖 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-runtime/src/object/class_registry/state.rs` around lines 510 - 551, The parent-prototype resolution logic is duplicated in class_decl_prototype_value and refresh_class_decl_prototype_parent, so extract it into a shared helper such as resolve_class_parent_prototype_bits(class_id: u32) -> Option<u64>. Move the existing dynamic_parent_prototype_bits/get_parent_class_id/builtin_prototype_value_for_class_id/class_decl_prototype_value/global_object_prototype_bits chain into that helper, then have both call sites use it to compute parent_proto_bits before object_set_static_prototype.crates/perry-runtime/src/object/native_call_method.rs (1)
1268-1289: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid per-call
StringHeaderallocation for the default toString/valueOf fallback.This fallback resolves the default
Object.prototype.toString/valueOffor objects with no own/vtable/prototype-sidetable method — likely the common case for plain objects/class instances. Each hit allocates a freshStringHeaderviajs_string_from_bytes. Consider interning the three method-nameStringHeaders once (e.g. thread-local orOnceLock) to avoid the allocation on this now-common path.♻️ Suggested direction
- if matches!(method_name, "toString" | "toLocaleString" | "valueOf") { - let method_key = - crate::string::js_string_from_bytes(method_name.as_ptr(), method_name.len() as u32); + if matches!(method_name, "toString" | "toLocaleString" | "valueOf") { + let method_key = interned_method_key(method_name); // cached StringHeader per name if !method_key.is_null() {🤖 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-runtime/src/object/native_call_method.rs` around lines 1268 - 1289, The default fallback in native_call_method for toString/toLocaleString/valueOf is allocating a fresh StringHeader on every call via js_string_from_bytes. Cache or intern these three method-name keys once (for example with thread-local storage or OnceLock) and reuse them in the method_name match path before calling js_object_get_field_by_name, so the common Object.prototype fallback avoids per-call allocation.crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs (1)
30-57: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsolidate string
lengthhandling
The new receiver fast path already covers stringlength/canonical-index reads, so the later SSO and heap-stringlengthbranches are duplicated for string receivers.constructorstill needs the existing fallback.🤖 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-runtime/src/object/field_get_set/get_field_by_name.rs` around lines 30 - 57, The get_field_by_name fast path already handles string receivers for canonical index and length lookups, so remove the duplicated SSO and heap-string length branches later in the same lookup path. Keep the existing fallback for constructor in place, and ensure the string-receiver handling remains centralized in get_field_by_name using the existing string-pointer logic and JSValue/string index helpers.crates/perry-hir/src/lower_patterns.rs (1)
162-165: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMinor: no ASCII fast-path for regex pattern repair.
Unlike
normalize_swc_string_literal(Line 174), which short-circuits onraw.is_ascii() || decoded.is_ascii()before callingrepair_latin1_decoded_utf8, the regex path always runs the byte-reinterpretation loop even for pure-ASCII patterns (the overwhelming majority). Functionally harmless (round-trips to the same string) but adds needless work per regex literal.🤖 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_patterns.rs` around lines 162 - 165, The regex literal lowering in ast::Lit::Regex currently always calls repair_latin1_decoded_utf8 on re.exp.to_string(), unlike normalize_swc_string_literal which skips the repair work for ASCII input. Update the Expr::RegExp path to add the same ASCII fast-path so pure-ASCII regex patterns return the original string without entering the byte-reinterpretation loop, while still preserving the existing repair behavior for non-ASCII patterns.tests/test_reflect_set_frozen_array.sh (1)
6-7: 📐 Maintainability & Code Quality | 🔵 TrivialBinary preference order flipped vs. sibling tests.
Other test scripts default to
target/release/perrythen fall back todebug; this one defaults todebugthen falls back torelease, and usesperry runinstead of the compile+execute pattern used elsewhere (so it skips--no-auto-optimize/optimized-path coverage). Functionally harmless, just inconsistent with the rest of the suite.🤖 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 `@tests/test_reflect_set_frozen_array.sh` around lines 6 - 7, The test script’s binary selection and execution flow are inconsistent with the sibling tests: it prefers the debug build first and uses a direct run path instead of the usual compile-then-execute pattern. Update the setup in test_reflect_set_frozen_array.sh to match the other test scripts by preferring target/release/perry first and falling back to debug, and switch the invocation to the same compile+execute style used elsewhere so it exercises the optimized path and --no-auto-optimize coverage consistently.crates/perry-codegen/src/codegen/opts.rs (1)
178-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTerse docs vs. surrounding convention.
Every neighboring field in this file (e.g.
namespace_member_prefixesat Line 166-177,import_function_origin_namesat Line 104-116) carries an "Issue#NNN" rationale explaining the specific bug it fixes and why the field is needed. The four new fields here have only a one-line description of what they store, not why (e.g. what breaks withoutnamespace_member_origin_namesvs. relying onnamespace_member_prefixesalone). Given how easy it is to conflate these similarly-shaped maps, a short rationale would help future maintainers.Also applies to: 583-590
🤖 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-codegen/src/codegen/opts.rs` around lines 178 - 190, The new namespace-related option fields in opts.rs are missing the same “Issue `#NNN`” rationale used by neighboring entries, so add concise docs explaining what breaks without each of namespace_member_origin_names, namespace_member_vars, namespace_member_namespace_prefixes, and namespace_import_prefixes. Update the field comments near the Options struct and the related doc block around namespace_member_prefixes to clarify why these separate maps/sets are needed instead of relying on the existing namespace_member_prefixes behavior alone.tests/test_dynamic_import_package_registry.sh (1)
20-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
compilePackagesconfig.
compilePackagesis declared both underperry.allow.compilePackagesand directly underperry.compilePackageswith identical values. If only one is the real schema key, consider dropping the other to avoid confusion for future readers of this fixture.🤖 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 `@tests/test_dynamic_import_package_registry.sh` around lines 20 - 33, The package fixture in the dynamic import registry test has duplicate compilePackages settings with the same value, which is confusing. Update the package.json fixture used in this test to keep only the correct schema location for compilePackages and remove the redundant one, using the perry and compilePackages keys in the test fixture as the reference points.
🤖 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-codegen/src/codegen/artifacts.rs`:
- Around line 1351-1371: The extern emission in artifacts::cross_module
namespace handling can declare the same `@__perry_ns_`<prefix> more than once if a
prefix is collected here and also later via dynamic_import_path_to_prefix. Add a
shared deduplication check before calling LlModule::add_external_global so both
loops reuse the same seen set or helper, and ensure the
namespace_extern_prefixes emission and the later dynamic import emission both
skip already-emitted prefixes.
In `@crates/perry-codegen/src/codegen/method.rs`:
- Around line 238-255: Add a traversal guard to map_set_default_super_kind to
match node_stream_parent_kind: the extends_name walk currently has no
cycle/depth limit and can loop forever on malformed or cyclic class hierarchies.
Update the loop to track depth (or another visited/limit check) and stop after
the same kind of bounded traversal used in node_stream_parent_kind, returning
None when the chain is too deep or unsafe to follow further.
In `@crates/perry-hir/src/dynamic_import.rs`:
- Around line 159-178: The Export::ReExport resolution in flatten_exports is
starting a fresh DFS each time, which loses the current visited state and can
recurse indefinitely on circular re-exports. Update the ReExport branch in
dynamic_import.rs to reuse the existing DFS state by calling flatten_into with
the current visited set instead of invoking flatten_exports, keeping the cycle
guard intact.
In `@crates/perry-hir/src/lower_decl/body_stmt.rs`:
- Around line 314-352: The fresh-class lowering in body_stmt.rs evaluates the
class extends expression twice: once through RegisterClassParentDynamic and
again when building ClassExprFresh via the "__perry_parent_value" initializer.
Update the lowering logic around bind_fresh_class_value and the
class.extends_expr handling so the extends expression is lowered once into a
temporary and that same Expr is reused for both the parent registration and the
fresh class initializer, preserving single evaluation and side-effect behavior.
- Around line 362-382: The `bind_fresh_class_value` branch in
`build_interleaved_static_init_stmts` is replaying `__perry_static_init_*`
blocks after all static fields, which breaks source-order behavior for `static
{}` blocks. Update the fresh-class path to use the same interleaved replay logic
as the non-fresh path by preserving the original ordering from
`class_decl.class.body`, while still using the fresh snapshot data for static
fields and methods. Refer to `build_interleaved_static_init_stmts`,
`bind_fresh_class_value`, and the `ClassExprFresh` handling to keep the fix
localized.
In `@crates/perry/src/commands/compile/object_cache.rs`:
- Around line 540-571: The object cache key computed in
compute_object_cache_key_with_env is missing the new namespace_import_prefixes
field, which can cause stale object reuse when only that map changes. Update the
hash serialization alongside the existing namespace_member_origin_names,
namespace_member_vars, and namespace_member_namespace_prefixes handling to
include opts.namespace_import_prefixes in a stable, deterministic order, using
the same pattern as the other namespace-related maps. Make sure the field is
also reflected consistently with empty_opts() and CompileOptions so the cache
key fully captures all namespace resolution inputs.
In `@crates/perry/src/commands/compile/run_pipeline.rs`:
- Around line 1760-1771: `normalize_namespace_path` is doing repeated uncached
`std::fs::canonicalize` calls in the hot per-module compile path, which is
causing redundant filesystem work. Update the `run_pipeline` setup so this
closure uses a shared cache for normalized paths, reusing previously computed
canonicalized results instead of re-hitting the filesystem on every invocation.
You can mirror the existing upfront native-module caching already done with
`path_to_module_name` and `module_name_to_path`, and wire the cache into all
call sites that use `normalize_namespace_path`, including the per-module loop
and `register_namespace_export_surface!`.
- Around line 104-154: The parent-class resolution in
class_chain_uses_new_target can pick the wrong class when multiple exports share
the same name. Update exported_class_by_name to accept and use the
class_canonical_path (instead of falling back to the first same-named export) so
it prefers the canonical module path consistently with the transitive closure
lookup, and then thread that value through class_chain_uses_new_target.
- Around line 2732-2890: Namespace re-export aliases are missing class/enum
dispatch metadata because the direct_namespace_source,
transitive_namespace_source, and Named re-export paths return immediately after
register_namespace_export_surface! and never populate imported_classes or
imported_enums. Update run_pipeline.rs by centralizing the namespace re-export
handling in the same helper/macro used for namespace aliases, and ensure all
re-export branches that call register_namespace_export_surface! also add the
class/enum metadata before continuing, matching the NamespaceReExport path’s
behavior.
In `@tests/test_builtin_prototype_and_fetch_reflection.sh`:
- Line 61: The oracle in tests/test_builtin_prototype_and_fetch_reflection.sh
currently uses node "$TMPDIR/main.ts", which depends on host Node supporting
direct TypeScript execution. Update the test to avoid this runtime dependency by
hardcoding the expected log like the sibling tests, or by using the same pinned
Node version/flags as the rest of the suite; keep the change focused in the test
script around the main.ts oracle setup.
In `@tests/test_unicode_regex_literal.sh`:
- Line 35: The oracle generation step in the test script unconditionally runs
node, so add a node availability guard before the expected output is produced.
Mirror the existing perry check pattern by checking for node, emitting SKIP, and
exiting 0 when it is missing; otherwise continue to run node "$TMPDIR/main.ts"
in the same test flow.
In `@tests/test_url_href_normalization.sh`:
- Line 31: The test script calls node in the expected-output setup without
checking availability, so it should be guarded like
test_unicode_regex_literal.sh. Update the test_url_href_normalization.sh flow
around the node invocation that builds expected.log to first verify node exists,
and if not, skip the test instead of failing. Use the existing shell test setup
and matching guard pattern from the other test to keep behavior consistent.
---
Outside diff comments:
In `@crates/perry-codegen/tests/argless_builtin_extra_args.rs`:
- Around line 10-64: The empty_opts helper is missing the new CompileOptions
field namespace_import_prefixes, so the struct literal no longer matches the
type. Update empty_opts to initialize namespace_import_prefixes alongside the
other namespace_* maps, using the same pattern as the nearby CompileOptions
fields, so the test helper stays in sync with CompileOptions.
In `@crates/perry-codegen/tests/class_keys_gc_root.rs`:
- Around line 29-83: The CompileOptions initializer in entry_opts() is missing
the new namespace_import_prefixes field, so the test helper no longer matches
the struct definition. Update the entry_opts() literal to include
namespace_import_prefixes with the same initialization pattern used by the other
CompileOptions helper constructors in this test suite. Keep the field ordering
consistent with the surrounding CompileOptions setup so the test compiles
cleanly.
In `@crates/perry-codegen/tests/constructor_recursion.rs`:
- Around line 5-59: The empty_opts helper’s CompileOptions initializer is
missing the namespace_import_prefixes field, so update this CompileOptions
literal in empty_opts to include that required member alongside the other
namespace-related maps/sets. Use the existing CompileOptions struct definition
and the surrounding fields in empty_opts to add namespace_import_prefixes with
the appropriate empty default so the test compiles again.
In `@crates/perry-codegen/tests/destructure_call_location.rs`:
- Around line 21-75: Add the missing namespace_import_prefixes field to the
CompileOptions literal in base_opts so the test stays in sync with the
CompileOptions struct; use the same pattern as the other HashMap/HashSet
initializers in this helper and update any related constructor-style test setup
in destructure_call_location.rs if needed.
In `@crates/perry-codegen/tests/large_object_barriers.rs`:
- Around line 5-59: The CompileOptions builder in empty_opts() is missing the
namespace_import_prefixes field, so update that helper to initialize it
alongside the other namespace_* maps. Add namespace_import_prefixes with an
empty HashMap in empty_opts() so the test setup matches the current
CompileOptions definition.
In `@crates/perry-codegen/tests/macos_bundle_chdir_gate.rs`:
- Around line 11-65: The `entry_opts` helper builds a `CompileOptions` literal
but is missing the `namespace_import_prefixes` field, so update that constructor
to include it alongside the other namespace-related maps/sets. Use the
`CompileOptions` struct in `entry_opts` as the anchor and add the missing field
with the appropriate empty collection value so the test compiles again.
In `@crates/perry-codegen/tests/native_proof_buffer_views.rs`:
- Around line 10-64: The empty_opts() test helper is missing the new
CompileOptions field namespace_import_prefixes, which causes the struct literal
to be incomplete. Update empty_opts() to initialize namespace_import_prefixes
alongside the other namespace-related collections in CompileOptions so
native_proof_buffer_views.rs continues to compile.
In `@crates/perry-codegen/tests/native_proof_regressions.rs`:
- Around line 11-65: The CompileOptions literal in empty_opts() is missing the
new namespace_import_prefixes field, so update this helper to initialize that
field consistently with the other namespace/import maps (or switch the struct
literal to use default initialization where appropriate). Keep the fix local to
empty_opts() and ensure the CompileOptions construction still covers all
required fields referenced elsewhere in native_proof_regressions.rs.
In `@crates/perry-codegen/tests/shadow_slot_hygiene.rs`:
- Around line 5-59: The empty_opts() test fixture is missing the new
CompileOptions field namespace_import_prefixes, so it no longer matches the
struct definition. Update empty_opts() in shadow_slot_hygiene.rs to initialize
namespace_import_prefixes alongside the other namespace_* collections, using the
appropriate empty collection type consistent with the CompileOptions fields.
In `@crates/perry-codegen/tests/static_symbol_hygiene.rs`:
- Around line 5-59: The test fixture in empty_opts() is missing the
CompileOptions field namespace_import_prefixes, so the struct initialization is
incomplete and won’t compile. Update empty_opts() to include
namespace_import_prefixes with an empty std::collections::HashMap, keeping it
consistent with the other namespace_* option fields in the CompileOptions setup.
In `@crates/perry-codegen/tests/typed_feedback.rs`:
- Around line 36-90: The CompileOptions literal in empty_opts is missing the
required namespace_import_prefixes field, so update that constructor to
initialize it alongside the other namespace_* maps and sets. Use the empty_opts
helper in typed_feedback.rs as the fix location and ensure the new field is set
consistently with the rest of the default CompileOptions values so the test file
compiles.
---
Nitpick comments:
In `@crates/perry-codegen/src/codegen/opts.rs`:
- Around line 178-190: The new namespace-related option fields in opts.rs are
missing the same “Issue `#NNN`” rationale used by neighboring entries, so add
concise docs explaining what breaks without each of
namespace_member_origin_names, namespace_member_vars,
namespace_member_namespace_prefixes, and namespace_import_prefixes. Update the
field comments near the Options struct and the related doc block around
namespace_member_prefixes to clarify why these separate maps/sets are needed
instead of relying on the existing namespace_member_prefixes behavior alone.
In `@crates/perry-hir/src/lower_patterns.rs`:
- Around line 162-165: The regex literal lowering in ast::Lit::Regex currently
always calls repair_latin1_decoded_utf8 on re.exp.to_string(), unlike
normalize_swc_string_literal which skips the repair work for ASCII input. Update
the Expr::RegExp path to add the same ASCII fast-path so pure-ASCII regex
patterns return the original string without entering the byte-reinterpretation
loop, while still preserving the existing repair behavior for non-ASCII
patterns.
In `@crates/perry-runtime/src/object/class_registry/state.rs`:
- Around line 510-551: The parent-prototype resolution logic is duplicated in
class_decl_prototype_value and refresh_class_decl_prototype_parent, so extract
it into a shared helper such as resolve_class_parent_prototype_bits(class_id:
u32) -> Option<u64>. Move the existing
dynamic_parent_prototype_bits/get_parent_class_id/builtin_prototype_value_for_class_id/class_decl_prototype_value/global_object_prototype_bits
chain into that helper, then have both call sites use it to compute
parent_proto_bits before object_set_static_prototype.
In `@crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs`:
- Around line 30-57: The get_field_by_name fast path already handles string
receivers for canonical index and length lookups, so remove the duplicated SSO
and heap-string length branches later in the same lookup path. Keep the existing
fallback for constructor in place, and ensure the string-receiver handling
remains centralized in get_field_by_name using the existing string-pointer logic
and JSValue/string index helpers.
In `@crates/perry-runtime/src/object/native_call_method.rs`:
- Around line 1268-1289: The default fallback in native_call_method for
toString/toLocaleString/valueOf is allocating a fresh StringHeader on every call
via js_string_from_bytes. Cache or intern these three method-name keys once (for
example with thread-local storage or OnceLock) and reuse them in the method_name
match path before calling js_object_get_field_by_name, so the common
Object.prototype fallback avoids per-call allocation.
In `@tests/test_dynamic_import_package_registry.sh`:
- Around line 20-33: The package fixture in the dynamic import registry test has
duplicate compilePackages settings with the same value, which is confusing.
Update the package.json fixture used in this test to keep only the correct
schema location for compilePackages and remove the redundant one, using the
perry and compilePackages keys in the test fixture as the reference points.
In `@tests/test_reflect_set_frozen_array.sh`:
- Around line 6-7: The test script’s binary selection and execution flow are
inconsistent with the sibling tests: it prefers the debug build first and uses a
direct run path instead of the usual compile-then-execute pattern. Update the
setup in test_reflect_set_frozen_array.sh to match the other test scripts by
preferring target/release/perry first and falling back to debug, and switch the
invocation to the same compile+execute style used elsewhere so it exercises the
optimized path and --no-auto-optimize coverage consistently.
🪄 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: 36f07724-70bf-4513-96ef-de3e079d22fb
⛔ Files ignored due to path filters (1)
tests/release/packages/zod3-basic/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (133)
crates/perry-codegen/src/codegen/artifacts.rscrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/helpers.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/codegen/module_globals_emit.rscrates/perry-codegen/src/codegen/opts.rscrates/perry-codegen/src/expr/binary.rscrates/perry-codegen/src/expr/dyn_extern_i18n.rscrates/perry-codegen/src/expr/index_get.rscrates/perry-codegen/src/expr/instance_misc1.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/property_get.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/console_promise.rscrates/perry-codegen/src/lower_call/namespace_call.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/lower_call/new_helpers.rscrates/perry-codegen/src/lower_call/property_get.rscrates/perry-codegen/src/lower_call/property_get/number_string.rscrates/perry-codegen/tests/argless_builtin_extra_args.rscrates/perry-codegen/tests/class_keys_gc_root.rscrates/perry-codegen/tests/constructor_recursion.rscrates/perry-codegen/tests/destructure_call_location.rscrates/perry-codegen/tests/large_object_barriers.rscrates/perry-codegen/tests/macos_bundle_chdir_gate.rscrates/perry-codegen/tests/native_proof_buffer_views.rscrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-codegen/tests/shadow_slot_hygiene.rscrates/perry-codegen/tests/static_symbol_hygiene.rscrates/perry-codegen/tests/typed_feedback.rscrates/perry-codegen/tests/typed_shape_descriptor.rscrates/perry-codegen/tests/typed_shape_descriptors.rscrates/perry-hir/src/dynamic_import.rscrates/perry-hir/src/dynamic_import/visitors.rscrates/perry-hir/src/eval_classifier.rscrates/perry-hir/src/lower/closure_analysis.rscrates/perry-hir/src/lower/const_fold_fn.rscrates/perry-hir/src/lower/expr_call/array_only_methods.rscrates/perry-hir/src/lower/expr_call/imported_array_methods.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower/expr_new.rscrates/perry-hir/src/lower/lower_module_fn.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-hir/src/lower/module_decl/reexports.rscrates/perry-hir/src/lower/stmt.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/enum_decl.rscrates/perry-hir/src/lower_decl/mod.rscrates/perry-hir/src/lower_patterns.rscrates/perry-runtime/src/collection_iter.rscrates/perry-runtime/src/json/replacer.rscrates/perry-runtime/src/object/array_object_ops.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/class_registry/parent_static.rscrates/perry-runtime/src/object/class_registry/state.rscrates/perry-runtime/src/object/descriptors.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rscrates/perry-runtime/src/object/field_get_set/has_property.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/builtin_thunks.rscrates/perry-runtime/src/object/global_this/proto_methods.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/object/native_call_method/collection_methods.rscrates/perry-runtime/src/object/native_call_method/common_methods.rscrates/perry-runtime/src/object/native_call_method/primitive_methods.rscrates/perry-runtime/src/object/object_ops/define_properties.rscrates/perry-runtime/src/object/object_ops/define_property.rscrates/perry-runtime/src/object/object_ops/has_own.rscrates/perry-runtime/src/object/object_ops/prototype.rscrates/perry-runtime/src/object/to_string_tag.rscrates/perry-runtime/src/proxy.rscrates/perry-runtime/src/set.rscrates/perry-runtime/src/string/char_ops.rscrates/perry-runtime/src/string/format.rscrates/perry-runtime/src/symbol/get.rscrates/perry-runtime/src/url/mod.rscrates/perry-runtime/src/url/parse.rscrates/perry-runtime/src/value/dyn_index.rscrates/perry-runtime/src/value/dynamic_arith.rscrates/perry-stdlib/src/fetch/dispatch.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/object_cache.rscrates/perry/src/commands/compile/run_pipeline.rstests/release/packages/zod3-basic/entry.tstests/release/packages/zod3-basic/expected.txttests/release/packages/zod3-basic/fixture.shtests/release/packages/zod3-basic/package.jsontests/test_array_not_instanceof_error_subclass.shtests/test_bigint_samevalue_set.shtests/test_builtin_prototype_and_fetch_reflection.shtests/test_class_define_property_name.shtests/test_dynamic_arithmetic_bigint_any.shtests/test_dynamic_extends_class_binding.shtests/test_dynamic_function_probe_fallback_stderr.shtests/test_dynamic_import_package_registry.shtests/test_dynamic_parent_builtin_no_downgrade.shtests/test_error_subclass_arrow_field_init.shtests/test_export_enum_runtime_object.shtests/test_file_instanceof.shtests/test_forward_captured_let_box.shtests/test_function_prototype_instanceof.shtests/test_imported_default_named_reexport.shtests/test_imported_error_new_target.shtests/test_imported_static_field_alias_method.shtests/test_json_stringify_replacer_key_order.shtests/test_map_set_subclass_constructor_iterable.shtests/test_missing_error_property_undefined.shtests/test_namespace_default_alias_call.shtests/test_namespace_homonymous_class_export.shtests/test_namespace_homonymous_var_function_exports.shtests/test_namespace_reexport_binding.shtests/test_namespace_reexport_optional_arg.shtests/test_namespace_subobject_method_call.shtests/test_new_parameter_shadows_class.shtests/test_number_to_string_large_exponent.shtests/test_object_create_for_in_prototype.shtests/test_object_create_in_operator_prototype.shtests/test_object_create_inherited_constructor.shtests/test_object_prototype_method_call_fallback.shtests/test_own_to_string_method.shtests/test_reflect_set_frozen_array.shtests/test_string_computed_length.shtests/test_unicode_regex_literal.shtests/test_url_href_normalization.shtests/test_url_instanceof_reflection.sh
| })); | ||
| TS | ||
|
|
||
| node "$TMPDIR/main.ts" > "$TMPDIR/expected.log" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What is the minimum Node.js version required to run a .ts file directly without any CLI flags?
💡 Result:
The minimum Node.js version required to run a.ts file directly without any CLI flags is Node.js 23.6.0 [1][2]. Starting with version 23.6.0, Node.js includes stable, built-in support for "type stripping," which allows the Node.js runtime to execute TypeScript files directly by removing type annotations and executing the resulting JavaScript [3][2]. This feature eliminates the need for external tools like ts-node or tsx for basic execution [4][5]. Key details regarding this feature: - It performs type stripping only; no type checking is performed [6][7]. - It does not support features that require code generation (such as enums or namespaces) unless the --experimental-transform-types flag is used [1]. - It is intended to be a lightweight way to execute TypeScript files and does not replace full-featured compilers for complex projects requiring tsconfig.json configurations [6][1].
Citations:
- 1: https://nodejs.org/download/release/v23.6.0/docs/api/typescript.html
- 2: https://www.infoq.com/news/2025/03/node-23-runs-typescript-natively/
- 3: https://nodejs.org/api/typescript.html
- 4: https://ishu.dev/post/nodejs-24-native-typescript-2026-04-26
- 5: https://techinsights.manisuec.com/nodejs/nodejs-typescript/
- 6: https://nodejs.org/docs/latest/api/typescript.html
- 7: module: add --experimental-strip-types nodejs/node#53725
🏁 Script executed:
git ls-files | rg '(^|/)(test_builtin_prototype_and_fetch_reflection\.sh|test_array_not_instanceof_error_subclass\.sh|test_number_to_string_large_exponent\.sh|\.github/workflows/|package\.json|README|package-lock\.json|pnpm-lock\.yaml|yarn\.lock)$'Repository: PerryTS/perry
Length of output: 3245
🏁 Script executed:
sed -n '1,220p' tests/test_builtin_prototype_and_fetch_reflection.shRepository: PerryTS/perry
Length of output: 2923
🏁 Script executed:
sed -n '1,220p' tests/test_array_not_instanceof_error_subclass.sh
printf '\n---\n'
sed -n '1,220p' tests/test_number_to_string_large_exponent.shRepository: PerryTS/perry
Length of output: 5588
🏁 Script executed:
git ls-files .github/workflows package.json README* | xargs -r -I{} sh -c 'echo "--- {}"; sed -n "1,220p" "{}"'Repository: PerryTS/perry
Length of output: 50371
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
for p in [
Path("tests/test_builtin_prototype_and_fetch_reflection.sh"),
Path("tests/test_array_not_instanceof_error_subclass.sh"),
Path("tests/test_number_to_string_large_exponent.sh"),
]:
print(f"--- {p} ---")
if p.exists():
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if "node \"$TMPDIR/main.ts\"" in line or "expected.log" in line or "TMPDIR/main.ts" in line:
print(f"{i}: {line}")
else:
print("missing")
PYRepository: PerryTS/perry
Length of output: 1091
🏁 Script executed:
rg -n "setup-node|node-version|tests/test_builtin_prototype_and_fetch_reflection.sh|test_array_not_instanceof_error_subclass.sh|test_number_to_string_large_exponent.sh" .github tests -g '*.yml' -g '*.yaml' -g '*.sh'Repository: PerryTS/perry
Length of output: 1803
🏁 Script executed:
rg -n "tests/test_.*\.sh|bash tests/|sh tests/|./tests/" .github -g '*.yml' -g '*.yaml'Repository: PerryTS/perry
Length of output: 648
🏁 Script executed:
git ls-files .github/workflows/test.yml .github/workflows/simctl-tests.yml .github/workflows/*.yml | sed -n '1,120p'Repository: PerryTS/perry
Length of output: 647
🏁 Script executed:
rg -n "test_builtin_prototype_and_fetch_reflection\.sh|test_array_not_instanceof_error_subclass\.sh|test_number_to_string_large_exponent\.sh" .Repository: PerryTS/perry
Length of output: 151
🏁 Script executed:
ast-grep outline .github/workflows/test.yml --view expandedRepository: PerryTS/perry
Length of output: 192
🏁 Script executed:
sed -n '620,760p' .github/workflows/test.ymlRepository: PerryTS/perry
Length of output: 6195
🏁 Script executed:
rg -n "bash tests/|tests/test_.*\.sh|shell tests|node-version|setup-node" .github/workflows/test.yml .github/workflows/simctl-tests.ymlRepository: PerryTS/perry
Length of output: 876
🏁 Script executed:
sed -n '640,740p' .github/workflows/test.ymlRepository: PerryTS/perry
Length of output: 4253
🏁 Script executed:
rg -n "tests/.*\.sh|bash .*tests|sh .*tests|\.sh" scripts .github/workflows tests -g '*.py' -g '*.sh' -g '*.yml' -g '*.yaml'Repository: PerryTS/perry
Length of output: 28464
🏁 Script executed:
rg -n "bash tests/|tests/test_.*\\.sh|shell tests|node-version|setup-node" .github/workflows/test.yml .github/workflows/simctl-tests.ymlRepository: PerryTS/perry
Length of output: 876
🏁 Script executed:
rg -n "tests/.*\\.sh|bash .*tests|sh .*tests|\\.sh" scripts .github/workflows tests -g '*.py' -g '*.sh' -g '*.yml' -g '*.yaml'Repository: PerryTS/perry
Length of output: 28464
Avoid using node "$TMPDIR/main.ts" for the oracle in tests/test_builtin_prototype_and_fetch_reflection.sh:61. It depends on the host Node having direct .ts support; on older releases this fails before the diff. Hardcode the expected log like the sibling tests, or pin the Node version/flags used here.
🤖 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 `@tests/test_builtin_prototype_and_fetch_reflection.sh` at line 61, The oracle
in tests/test_builtin_prototype_and_fetch_reflection.sh currently uses node
"$TMPDIR/main.ts", which depends on host Node supporting direct TypeScript
execution. Update the test to avoid this runtime dependency by hardcoding the
expected log like the sibling tests, or by using the same pinned Node
version/flags as the rest of the suite; keep the change focused in the test
script around the main.ts oracle setup.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-hir/src/lower_decl/static_init.rs (1)
94-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the shared class-body iteration skeleton.
build_interleaved_static_init_exprsduplicates the member-walk (ClassProp/PrivateProp/StaticBlock with parallelfield_idx/block_idxcounters) already present inbuild_interleaved_static_init_stmts(Lines 31-92) and, partially,build_fresh_class_static_init(Lines 156-216). Three copies of this walk can silently diverge. A single generic driver taking per-field / per-block callbacks would collapse them. Note the emit differences are intentional and must be preserved: the expr variant usesRegisterClassStaticSymbol(sequenced-in-front, per#894) vs the stmt variant'sClassStaticSymbolSet.🤖 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_decl/static_init.rs` around lines 94 - 154, Refactor the duplicated class-body walk in build_interleaved_static_init_exprs by reusing a shared iteration driver with callbacks for field and static-block handling, so it stays aligned with build_interleaved_static_init_stmts and build_fresh_class_static_init. Keep the current sequencing and symbol behavior intact: the expr path must still emit RegisterClassStaticSymbol for keyed fields and StaticFieldSet for plain fields, while static blocks should still map through the per-block __perry_static_init_* naming and only emit when the matching method exists.
🤖 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.
Nitpick comments:
In `@crates/perry-hir/src/lower_decl/static_init.rs`:
- Around line 94-154: Refactor the duplicated class-body walk in
build_interleaved_static_init_exprs by reusing a shared iteration driver with
callbacks for field and static-block handling, so it stays aligned with
build_interleaved_static_init_stmts and build_fresh_class_static_init. Keep the
current sequencing and symbol behavior intact: the expr path must still emit
RegisterClassStaticSymbol for keyed fields and StaticFieldSet for plain fields,
while static blocks should still map through the per-block __perry_static_init_*
naming and only emit when the matching method exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f5b9206-cca9-4a27-b6ec-3f429db539e8
📒 Files selected for processing (5)
crates/perry-hir/src/lower/lower_expr/arm_class.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/mod.rscrates/perry-hir/src/lower_decl/static_init.rstests/test_dynamic_class_extends_once.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/perry-hir/src/lower_decl/mod.rs
- crates/perry-hir/src/lower_decl/body_stmt.rs
- tests/test_dynamic_class_extends_once.sh
…t key equality to Map - js_object_has_property: the prototype-chain tail now recurses through the entry point; bound total depth to 1024 (the same limit the manual walk uses) so a pathologically deep chain, or a cycle slipping past setPrototypeOf's cycle check, cannot overflow the stack. - map.rs jsvalue_eq: compare BigInt keys by value (js_bigint_eq), mirroring the Set path, so new Map([[1n,'a']]).get(1n) resolves across allocations. - Restore the #3986 synthetic-prototype rationale comment at both moved sites.
… in closure-capture analysis - map_set_default_super_kind: bound the extends-chain walk at 64 (matching its twin in method.rs and ctor_chain_uses_new_target) so a cyclic constructorless extends graph cannot loop forever. - collect_closure_captures_expr: descend into ClassExprFresh (statics + captured args), InstanceOf, In, and tagged-template exprs. ClassExprFresh is reachable via the class-extends binding path, so a closure capturing a forward-declared let there must be seen or its box is not preallocated.
- property_get: reword the trim/case-method dispatch comment to be library-agnostic (no Zod name); logic unchanged. - namespace_call: document the arity-1 padding fallback for renamed re-exports. - dynamic_arith: note that '+' intentionally skips ToNumber coercion. - builtin_thunks: drop the unused _body computation on the dynamic-Function path. - tests: SKIP (not FAIL) when node is absent in six node-oracle tests; make the non-canonical index assertion in test_string_computed_length explicit.
…ssion tests The tests/test_*.sh regression suite copy-pastes ~40 lines of perry-binary and runtime-lib detection, temp-dir, compile/run error handling, and diff logic into every file. Add a sourced _perry_test_lib.sh (leading underscore keeps it out of run_tests.sh's glob, matching the fixture harness convention) exposing perry_run / perry_write / perry_expect / perry_expect_node / perry_pass. Convert test_object_create_for_in_prototype (literal oracle, 72->26 lines) and test_url_instanceof_reflection (node oracle, 69->21 lines) as the adoption pattern; remaining tests can migrate incrementally.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/_perry_test_lib.sh`:
- Around line 29-30: Make the binary and runtime-lib selection in the test
harness use the same resolved build profile, because PERRY currently prefers
release while perry_run resolves runtime archives in the opposite order. Update
the shared selection logic in tests/_perry_test_lib.sh around PERRY and
perry_run so both choose either debug or release consistently from one profile
decision, and then use that same profile for the binary and the runtime/stdlib
archives.
🪄 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: 29972d64-ceb1-4b59-8b0a-aed56226a4d7
📒 Files selected for processing (3)
tests/_perry_test_lib.shtests/test_object_create_for_in_prototype.shtests/test_url_instanceof_reflection.sh
js_switch_strict_equals special-cased strings and numbers, then fell through to
bit-identity — so `switch (1n + 0n) { case 1n: }` never matched (two distinct
allocations). Add a BigInt value-compare arm (js_bigint_eq), mirroring the
Set/Map key and general === paths; a BigInt is never === a non-BigInt.
…subclasses
A class extending a builtin (Error/Map/Set/Array) with no explicit constructor
dropped its instance field initializers — `class E extends Error { tag = "e" }`
gave new E().tag === 0 instead of "e". The post-super field-init fallback used
AfterRoot (chain[1..]), but with a builtin parent the leaf is the sole user-class
chain entry, so AfterRoot applied nothing. Use SelfOnly when the parent is not a
user class, matching the explicit-super() arm in this_super_call.rs. The explicit
-constructor path is unaffected.
instanceof.rs hardcoded the Blob/File/URL class-ids as inline hex in the name->id match and re-declared them as function-local consts, while the error ids in the same match already reference crate::error::CLASS_ID_*. Give Blob/File a canonical home in node_submodules::blob and URL one in the url module (matching the per-module CLASS_ID_* convention), and reference them everywhere so the three ids have a single source of truth. Values unchanged; compiler-verified identical. (Left untouched: the local CLASS_ID_RESPONSE/REQUEST/HEADERS block, whose values coincide numerically with weakref ids — likely distinct handle-band vs ObjectHeader namespaces, worth a separate look.)
…_test_lib.sh CodeRabbit: binary selection preferred release-then-debug while the runtime-lib selection checked debug-then-release, so a release binary could pair with debug static libs. Derive PERRY_RUNTIME_DIR from the chosen binary's own target dir (matching the pattern already used in test_builtin_prototype_and_fetch_reflection.sh) so the two never mismatch; fall back to auto-optimize when that dir has no libs.
…h copies The two copies of map_set_default_super_kind (codegen/method.rs and lower_call/new_helpers.rs) capped the extends-chain walk at 32 and 64 respectively. Unify both at 64 (matching the neighboring ctor_chain_uses_new_target) and add a cross-reference note so the twins stay in sync. Guard-only; the bound is unreachable for real class hierarchies.
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
2 participants
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
Behavior change
new Function("<string>")now throws by default in every build: an ahead-of-time compiled binary cannot compile a code string built from runtime data. Feature-detecting libraries fall back cleanly to their non-Functionpath. Opt out withPERRY_EVAL_CSP=0.Validation
Summary by CodeRabbit
new.targetbehavior for imported constructors and improved Map/Set subclass initialization.Object.prototypemethod fallback behavior across built-in types.instanceof,Object.create,Object.defineProperty,Error.errors,Symbol.toStringTag,URL/File/Blob, string indexing, numeric dispatch, and BigInt equality inMap/Set.JSON.stringifyreplacer iteration/key ordering and prototype-walk safety.