Skip to content

fix(runtime): expose module namespace data descriptors - #9899

Closed
proggeramlug wants to merge 1 commit into
PerryTS:mainfrom
proggeramlug:fix/9889-module-namespace-descriptors
Closed

fix(runtime): expose module namespace data descriptors#9899
proggeramlug wants to merge 1 commit into
PerryTS:mainfrom
proggeramlug:fix/9889-module-namespace-descriptors

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Module namespace live exports are stored as accessors so their values track reassignment, which made Object.getOwnPropertyDescriptor(ns, key) expose the internal getter instead of the data descriptor required for namespace exotic objects. Namespace own-property reflection now reads the current export through the rooted live-binding path and returns { value, writable: true, enumerable: true, configurable: false } for both live and snapshot exports.

The dynamic-import alias fixture now checks descriptor shape and value before and after reassignment, including its existing forced moving-GC configuration.

Validation:

  • Exact dynamic-import alias parity fixture: 1/1 pass against Node 26.5.1 on coherent post-rebase compiler/runtime artifacts; 3 copying collections and 18,908 moved objects in the pre-rebase equivalent run
  • cargo test -p perry-runtime --profile perry-dev -- --test-threads=1: 3,237 passed, 4 ignored; doc tests green
  • scripts/run_lint_gates.sh: all 64 gates passed, 2 CI-only checks skipped locally
  • No version bump

Fixes #9889

Summary by CodeRabbit

  • Bug Fixes
    • Dynamic-import namespace exports now report standard data property descriptors.
    • Descriptors correctly expose current live-binding values and writable, enumerable, and non-configurable flags.
    • Accessor details are no longer exposed for these exports.
    • Added coverage for descriptors before and after export value updates.

@proggeramlug
proggeramlug force-pushed the fix/9889-module-namespace-descriptors branch from 22ff043 to 8b004bb Compare September 6, 2026 16:15
@coderabbitai

coderabbitai Bot commented Sep 6, 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: Team

Run ID: 67de5006-f2de-44cf-9565-b2ac9b2ee55f

📥 Commits

Reviewing files that changed from the base of the PR and between 0866940 and 8b004bb.

📒 Files selected for processing (4)
  • changelog.d/9899-module-namespace-descriptors.md
  • crates/perry-runtime/src/object/descriptors.rs
  • crates/perry-runtime/src/object/namespace_create.rs
  • test-files/test_gap_dynamic_import_alias_binding.ts

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


📝 Walkthrough

Walkthrough

The runtime now synthesizes standard data descriptors for module namespace properties while preserving live binding values. The dynamic import alias test validates descriptor flags, accessor absence, and values before and after reassignment.

Changes

Module namespace descriptors

Layer / File(s) Summary
Runtime descriptor synthesis
crates/perry-runtime/src/object/descriptors.rs, crates/perry-runtime/src/object/namespace_create.rs
Module namespace descriptor requests use the current field value and return writable, enumerable, non-configurable data descriptors.
Descriptor behavior validation
test-files/test_gap_dynamic_import_alias_binding.ts, changelog.d/9899-module-namespace-descriptors.md
The dynamic import test checks descriptor shape and live values before and after reassignment. The changelog records the fix.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 8b004

Module namespace exports now report standard data descriptors while retaining current live-binding values. The change is covered for alias reassignment and descriptor shape, with no current merge-blocking risk identified.

Suggested reviewers: thehypnoo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 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 and concisely identifies the primary runtime change: exposing module namespace data descriptors.
Description check ✅ Passed The description explains the bug, implementation, affected test, validation results, linked issue, and no version bump. It omits the template headings and checklist details, but the required informati…
Linked Issues check ✅ Passed The changes satisfy issue [#9889] by preserving live bindings while reporting current values through writable, enumerable, non-configurable data descriptors. Tests cover descriptor shape, reassignment…
Out of Scope Changes check ✅ Passed All changes are related to the linked issue: runtime descriptor handling, changelog documentation, and focused regression coverage. No unrelated code changes are shown.
Full details: Docstring Coverage

Explanation

Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 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

Landed on main via merge train #9903. Validated as a tree: 64/64 lint gates, and perry-runtime/codegen/hir/stdlib all green (5,944 tests, 0 failures). Thanks!

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.

Module namespace exports are accessors, not data descriptors, for live bindings

1 participant