Skip to content

fix(fs): preserve promises namespace identity - #9887

Closed
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/9202-fs-promises-identity
Closed

fix(fs): preserve promises namespace identity#9887
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/9202-fs-promises-identity

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

The node-suite/fs/imports/namespace-exports row in #9202 fails two identity checks because import { promises } from "node:fs" becomes a generic fs/promises native-module object, while fs.promises correctly returns the canonical fs_promises submodule singleton.

Use the node-submodule namespace whenever an fs/promises or stream/promises native-module reference becomes a runtime value. The value and call paths now share one spelling map and install the matching submodule before reading its singleton.

Validation:

  • node-suite/fs/imports/namespace-exports: 1/1 parity pass against Node 26.5.1
  • three node-suite/fs-promises/imports fixtures: exact Node output
  • two node-suite/stream/imports/promises-* fixtures: exact Node output
  • cargo test -p perry-codegen: 1,431 unit tests plus integration and doc tests pass
  • scripts/run_lint_gates.sh: all 64 gates passed; 2 CI-expression gates skipped locally

No version bump.

Addresses node-suite/fs/imports/namespace-exports in #9202.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed fs/promises and stream/promises namespace handling when native-module references are converted to values.
    • Ensured these namespaces consistently reuse their canonical singleton objects, improving identity consistency across imports and runtime access paths.
  • Tests

    • Added coverage verifying correct initialization and reuse of the fs/promises namespace singleton.

@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: 1e292b00-e360-4a3a-920d-b73ac3b332f1

📥 Commits

Reviewing files that changed from the base of the PR and between 890514a and b7f0952.

📒 Files selected for processing (5)
  • changelog.d/9887-fs-promises-namespace-identity.md
  • crates/perry-codegen/src/expr/property_get/tests.rs
  • crates/perry-codegen/src/expr/static_field_meta.rs
  • crates/perry-codegen/src/lower_call/native/mod.rs
  • crates/perry-codegen/src/nm_install.rs

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


📝 Walkthrough

Walkthrough

The codegen now maps promise native-module names through a shared submodule-key helper. Native module values install the matching node submodule and resolve its canonical namespace singleton. Tests verify call ordering, and the changelog records the fix.

Changes

Promise namespace identity

Layer / File(s) Summary
Submodule key mapping and dispatch
crates/perry-codegen/src/nm_install.rs, crates/perry-codegen/src/lower_call/native/mod.rs
A shared helper maps fs/promises and stream/promises, with optional node: prefixes, to their submodule registry keys. Native submodule dispatch uses the helper.
Native module value lowering and validation
crates/perry-codegen/src/expr/static_field_meta.rs, crates/perry-codegen/src/expr/property_get/tests.rs, changelog.d/9887-fs-promises-namespace-identity.md
Expr::NativeModuleRef installs the matching submodule and calls js_node_submodule_namespace. The test verifies installation occurs first. The changelog documents namespace reuse.

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

Merge Risk: ⚪ Minimal · up to b7f09

Promise namespace values for fs and stream now reuse their canonical submodule namespace singletons, preserving identity with the corresponding promises properties. No concrete current-head merge risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant NativeModuleRef
  participant native_namespace_submodule_key
  participant fs_promises_submodule
  participant js_node_submodule_namespace
  NativeModuleRef->>native_namespace_submodule_key: resolve fs/promises
  native_namespace_submodule_key-->>NativeModuleRef: return fs_promises
  NativeModuleRef->>fs_promises_submodule: install fs/promises submodule
  NativeModuleRef->>js_node_submodule_namespace: resolve canonical namespace
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 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 is concise, specific, and accurately describes the main change: preserving fs and stream promises namespace identity.
Description check ✅ Passed The description explains the bug, implementation, related issue, validation results, and version-bump status. It does not reproduce the template headings or checklist, but it contains the required sub…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

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

Landed on main via merge train #9898. Validated as a tree: 64/64 lint gates, and perry-runtime/codegen/hir/stdlib all green (5,939 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.

1 participant