Skip to content

fix(codegen): preserve renamed namespace constructor args - #9296

Merged
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/9285-namespace-reexport-constructor
Aug 31, 2026
Merged

fix(codegen): preserve renamed namespace constructor args#9296
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/9285-namespace-reexport-constructor

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Restore constructor argument forwarding for classes reached as new ns.VisibleAlias(...) through a renamed re-export. The namespace import now keeps the class's origin metadata under its visible scoped key, and static class resolution consumes that collision-free key instead of falling through to a function wrapper.

Changes

  • Resolve renamed namespace class metadata through the defining export name in both namespace-import paths.
  • Route namespace-qualified new through the scoped (namespace, member) class entry introduced by fix(compile): support full OpenCode source builds #9133.
  • Preserve the existing dynamic-constructor fallback for namespace members that are functions or closure-valued variables rather than classes.

Related issue

Fixes #9285

Test plan

Run on root@perrymaster.skelpo.net with coherent compiler/runtime archives:

  • ./scripts/pre-tag-check.sh --quick
  • cargo build -p perry -p perry-runtime-static -p perry-stdlib-static
  • PERRY_NO_AUTO_OPTIMIZE=1 RUST_TEST_THREADS=1 cargo test -p perry --test functional_batch2_regressions imported_class_namespace_reexport_uses_visible_alias --quiet
  • cargo test -p perry-codegen --lib --quiet (1,378 passed; 1 ignored)
  • PERRY_NO_AUTO_OPTIMIZE=1 RUST_TEST_THREADS=1 cargo test -p perry --test functional_batch2_regressions --quiet (7 passed)
  • PERRY_NO_AUTO_OPTIMIZE=1 RUST_TEST_THREADS=1 cargo test -p perry --test issue_5437_cross_module_member_new_capture --quiet (1 passed)
  • PERRY_NO_AUTO_OPTIMIZE=1 RUST_TEST_THREADS=1 cargo test -p perry --test module_import_forms --quiet (6 passed)

The issue fixture was also compiled with --trace llvm: before the fix it called js_new_function_construct; after the fix it directly calls model_ts__Child_constructor with the user argument.

  • cargo build --release clean
  • cargo test --workspace --exclude perry-ui-ios --exclude perry-ui-tvos --exclude perry-ui-watchos --exclude perry-ui-gtk4 --exclude perry-ui-android --exclude perry-ui-windows passes
  • Existing regression test covers the user-visible failure
  • (if CLI / stdlib / runtime API changed) Updated docs/src/
  • (if touching a platform UI backend) Built -p perry-ui-<backend> locally on that platform

Screenshots / output

Before:

value: undefined
instanceof: true

After:

value: namespace
instanceof: true

Checklist

  • I have NOT bumped the workspace version or edited CLAUDE.md / CHANGELOG.md (maintainer handles these at merge)
  • My commits follow the loose feat: / fix: / docs: / chore: prefix convention used in the log
  • I've read CONTRIBUTING.md and agree to the Code of Conduct

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue where classes accessed through renamed namespace exports could fail to receive constructor arguments correctly.
    • Constructor parameter properties and other initialization now run as expected when creating instances through a renamed namespace alias.
    • Improved class resolution for namespace imports and re-exports while preserving reliable behavior when names overlap.

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review Change Stack

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: b57da9e1-a484-40ee-a75c-22b899b077cb

📥 Commits

Reviewing files that changed from the base of the PR and between 7a6d5a1 and 14773ef.

📒 Files selected for processing (5)
  • changelog.d/9296-renamed-namespace-constructor.md
  • crates/perry-codegen/src/expr/new_dynamic.rs
  • crates/perry-codegen/src/expr/v8_interop.rs
  • crates/perry-codegen/src/type_analysis/predicates.rs
  • crates/perry/src/commands/compile/run_pipeline.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The compiler now preserves origin class metadata and resolves renamed namespace re-exports through collision-free registry keys, allowing constructor arguments and parameter properties to initialize correctly.

Changes

Renamed namespace class resolution

Layer / File(s) Summary
Class metadata registration
crates/perry/src/commands/compile/run_pipeline.rs, changelog.d/9296-renamed-namespace-constructor.md
Namespace-like and namespace re-export imports fall back to origin-name class metadata when the visible export name is not registered directly. The changelog records the constructor fix.
Static class lookup and ownership
crates/perry-codegen/src/expr/v8_interop.rs, crates/perry-codegen/src/expr/new_dynamic.rs, crates/perry-codegen/src/type_analysis/predicates.rs
try_static_class_name returns borrowed or owned names and resolves namespace members through collision-free registry keys. Callers borrow the result where required, and dynamic construction passes the member name by reference.

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

Merge Risk: ⚪ Minimal · up to 14773

Renamed namespace constructors now forward arguments correctly while non-class namespace members retain their existing behavior; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: thehypnoo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary fix: preserving constructor arguments for classes accessed through renamed namespace re-exports.
Description check ✅ Passed The description includes the required Summary, Changes, Related issue, Test plan, Screenshots/output, and Checklist sections. It documents the implementation, targeted verification, regression behavio…
Linked Issues check ✅ Passed The changes satisfy issue #9285. They fix constructor argument forwarding for renamed namespace re-exports, preserve namespace class identity, and retain correct instanceof behavior without modifying …
Out of Scope Changes check ✅ Passed The changes are within scope for issue #9285. The compiler updates, namespace metadata resolution, regression documentation, and targeted tests all support the reported re-export constructor bug. No u…
Full details: Description check

Explanation

The description includes the required Summary, Changes, Related issue, Test plan, Screenshots/output, and Checklist sections. It documents the implementation, targeted verification, regression behavior, and unchecked full-suite commands.

Full details: Linked Issues check

Explanation

The changes satisfy issue #9285. They fix constructor argument forwarding for renamed namespace re-exports, preserve namespace class identity, and retain correct instanceof behavior without modifying the fixture.

Full details: Out of Scope Changes check

Explanation

The changes are within scope for issue #9285. The compiler updates, namespace metadata resolution, regression documentation, and targeted tests all support the reported re-export constructor bug. No unrelated code changes are identified.

Full details: Docstring Coverage

Explanation

Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ 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

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

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Audited the diff — root-cause fix on both sides, and it explains the symptom exactly. I'd merge.

The reported failure was odd in a specific way: value: undefined but instanceof: true. This diff accounts for both halves.

Codegen side. try_static_class_name returned the bare property for namespace member access, relying on ctx.classes holding imported classes under their original name. That holds for export { Child } and breaks for export { Child as PublicChild }: the property is PublicChild while the class is registered as Child. Returning the collision-free (namespace, member) registry key removes the assumption rather than special-casing the rename. Widening the return to Cow<'a, str> is the minimum needed to return a composed key without forcing allocation on the common borrowed paths.

Pipeline side. The registration path had the same origin-vs-alias mismatch, and the comment names the consequence precisely:

new ns.PublicChild(arg) falls through to a function wrapper and never runs the class constructor

That is the missing half of the explanation. A function wrapper still satisfies instanceof through the class registry, which is why identity looked correct while the parameter property was never initialized — the constructor simply never ran. Falling back to the origin key here, matching what the var-classification path above already did, makes the two paths agree.

Fixing the lookup key and the registration is the right scope: fixing only one would leave the other resolving a different entity for the same source expression.

Small note

The two fallback blocks in run_pipeline.rs are near-identical get(&key).or_else(|| origin_key…) chains. If a third site ever needs the same resolution, it is worth naming — resolve_exported_class(&exported_classes, &key, &origin_key_under_origin_name) or similar — so the origin-vs-alias rule lives in one place. Not worth churning this PR for; a release blocker should land.

@proggeramlug
proggeramlug merged commit 986f4e1 into PerryTS:main Aug 31, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

release blocker: parameter property is undefined through a renamed re-export via namespace import (shard 7)

1 participant