fix(compile): adopt require() of named-only package as namespace, not default (#5257)

[proggeramlug]

Bug (#5257)

When perry natively compiles a package that does const x = require('pkg') (or const { a } = require('pkg')), the CJS→ESM wrap (#665/#1721) rewrites it to a default import import _req_N from 'pkg'. If pkg exposes no default (a CJS module, or an __esModule-marked module with named-only exports), the package-default-export gate bails the entire build:

Error: The requested package 'isexe' does not provide an export named 'default' (imported as '_req_0' in .../which/lib/index.js).

Per CJS semantics require('pkg') returns the exports object (its namespace), and isexe.sync(...) / const { sync } = require('isexe') work. Blocks cross-spawn, execa, which (via isexe), joi (via @hapi/hoek).

Root cause

enforce_package_default_exports (bootstrap.rs) enforces Node's static-ESM "no default export" rule for any bare-package default import. But an import synthesized by the CJS wrap from a require() is not a static-ESM default import — require() hands back the exports object. Codegen already routes a no-default default import through the namespace machinery (#4872, compile.rs); the gate was simply too eager and threw before that path could run.

Fix

• Add Import::is_adopted_require (perry-hir). Set it on every import of a CJS-wrapped module in collect_modules (where the wrap origin is known — every import there came from a require()).
• Exempt is_adopted_require imports from the default-export gate in bootstrap.rs. They fall through to the namespace machinery: member reads / destructuring resolve per-export; a whole-value read materializes the exports object.
• Genuine user import X from 'pkg' (flag false) keeps Node's error.

Threaded the new field through the stable-hash and the other Import {...} literals.

Test evidence

New crates/perry/tests/issue_5257_require_adopt_no_default_namespace.rs (2 tests, both green):

• adopted_require_of_named_only_package_binds_namespace_and_runs — mirrors the exact isexe shape: a "type":"module" package whose exports.require resolves to a bundled __esModule CJS with named-only exports (no default), consumed via require("foo").bar() and destructuring const { bar, baz } = require("foo"). Asserts the gate error is gone and the binary prints 91.
• module_exports_fn_default_require_still_works — regression guard: module.exports = fn still adopts as a callable default (prints 42).

cargo test -p perry-hir and cargo test -p perry --lib green. Related suites green: issue_4872_barrel_default_reexports, create_require_package, module_import_forms, issue_4841_namespace_cjs_reexport.

Real corpus (before → after)

driver	before	after
cross_spawn_.ts	does not provide an export named 'default'	compiles, links, runs (cross-spawn object)
which_.ts	same error	compiles, links, runs (which object)
execa_.ts	same error	gate passes; hits unrelated later error (missing closure symbol in npm-run-path)
joi_.ts	same error	gate passes; hits unrelated later codegen error (String.trim takes no args, got 2 in joi/lib/validator.js)

The next errors for execa/joi are out of scope for #5257 and not fixed here.

Risk

Low. The change only removes a build-time bail for require-originated imports; the namespace fallback they route into already existed (#4872). Default-shaped requires (module.exports = fn) are unaffected (regression-tested). Real user ESM import X from 'pkg' still errors like Node.

Maintainer note

No version / CHANGELOG.md / Current Version / Cargo.lock edits per the external-contributor flow — please fold the version bump + changelog entry in at merge.

Summary by CodeRabbit

• Bug Fixes
  • Fixed handling of CommonJS require() “adopted require” imports from dependencies that expose only named exports (no default), ensuring they fall back to namespace-style bindings instead of triggering default export resolution errors.
  • Updated validation so these adopted-require imports no longer fail the “package provides default” preflight check.
• Tests
  • Added regression coverage for issue #5257, including namespace/destructuring require cases and a callable default-shaped module.exports scenario.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c5fdf6ca-985a-4f01-8991-65a5bb6beb1d

📥 Commits

Reviewing files that changed from the base of the PR and between 388d964 and 9f3c758.

📒 Files selected for processing (1)

• crates/perry/src/commands/compile/collect_modules.rs

---

📝 Walkthrough

Walkthrough

Adds an is_adopted_require: bool field to the HIR Import struct to track CJS require('S') calls synthesized as default-import-shaped bindings during CJS→ESM wrapping. The flag is initialized to false at all existing import construction sites and set to true in collect_modules for the CJS-wrapped path, then used in bootstrap to bypass the Node static-ESM default-export preflight check. Two regression tests cover the named-only and callable-default cases from issue #5257.

Changes

CJS→ESM adopted-require bypass (issue #5257)

Layer / File(s)	Summary
HIR Import field and stable hash crates/perry-hir/src/ir/decl.rs, crates/perry-hir/src/stable_hash/module.rs	Declares is_adopted_require: bool on the Import struct with documentation describing CJS→ESM synthesis behavior, then destructures and hashes the field in the SH for Import implementation so the stable cache is sensitive to the new flag.
Default-false initialization across import construction sites crates/perry-hir/src/lower/module_decl.rs, crates/perry-transform/src/inline/mod.rs, crates/perry/src/commands/compile/collect_modules.rs, crates/perry-hir/src/stable_hash/tests.rs	Sets is_adopted_require: false in every Import struct literal: AST lowering for static imports, inline-function cross-module synthesis, dynamic-import edge creation, and the stable-hash test fixture.
Mark true in CJS-wrapped require() path crates/perry/src/commands/compile/collect_modules.rs	In the was_cjs_wrapped branch, eligible imports derived from require('S') call sites are assigned is_adopted_require = true with clarifying comments, distinguishing them from normal static import default-gating.
Skip default-export preflight for adopted-require imports crates/perry/src/commands/compile/bootstrap.rs	enforce_package_default_exports adds a guard that skips the Node static-ESM default-export validation for imports where is_adopted_require is true; the unit-test Import literal is updated with the new field.
Regression tests for issue #5257 crates/perry/tests/issue_5257_require_adopt_no_default_namespace.rs	Two end-to-end tests verify: (1) a named-only CJS package required via require() compiles without a "missing default export" error and produces correct runtime output (91); (2) a module.exports-as-function shape still resolves as a callable default in the adopted-require path (42).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

• compilePackages: require() of a CJS/__esModule package with named-only exports (no default) fails — should bind the exports namespace (isexe → cross-spawn/which/execa/joi) #5257: This PR directly implements the fix for this issue by introducing is_adopted_require to distinguish CJS require('S') adopted imports from static ESM default imports and routing them past the default-export validation gate.

Poem

🐇 A little flag, just one small bool,
To tell the bundler: "Play it cool!
This require isn't seeking default—
It's namespace-bound, a CJS export!
So skip the gate, let namespaces reign,
And stdout prints 91 again! 🎉

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: adopting require() of named-only packages as namespace instead of default, directly addressing issue #5257.
Description check	✅ Passed	The description comprehensively covers the bug, root cause, fix strategy, test evidence, and risk assessment, fully matching the template's required sections with detailed technical context.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/require-adopt-no-default-namespace

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

crates/perry-hir/src/stable_hash/tests.rs (1)

231-257: ⚡ Quick win

Add a direct hash-sensitivity assertion for is_adopted_require.

module_metadata_affects_hash now includes the new field in the fixture, but it still doesn’t directly assert that flipping only is_adopted_require changes the hash. Adding that targeted check would lock the new stable-hash contract down explicitly.

🤖 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/stable_hash/tests.rs` around lines 231 - 257, The test
`module_metadata_affects_hash` now includes the `is_adopted_require` field in
the test fixture, but lacks a direct assertion verifying that changing only this
field affects the hash. Add a new test case following the same pattern as the
existing ones (like the name check and import check): create a modified module
with `is_adopted_require` toggled to a different value than the base, then
assert that its hash differs from the base hash. This will explicitly lock in
the stable-hash contract for the `is_adopted_require` field.

🤖 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/stable_hash/tests.rs`:
- Around line 231-257: The test `module_metadata_affects_hash` now includes the
`is_adopted_require` field in the test fixture, but lacks a direct assertion
verifying that changing only this field affects the hash. Add a new test case
following the same pattern as the existing ones (like the name check and import
check): create a modified module with `is_adopted_require` toggled to a
different value than the base, then assert that its hash differs from the base
hash. This will explicitly lock in the stable-hash contract for the
`is_adopted_require` field.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 18a4b485-ec00-4245-b76e-4dffa0341b5e

📥 Commits

Reviewing files that changed from the base of the PR and between 7f0c2fe and 388d964.

📒 Files selected for processing (8)

• crates/perry-hir/src/ir/decl.rs
• crates/perry-hir/src/lower/module_decl.rs
• crates/perry-hir/src/stable_hash/module.rs
• crates/perry-hir/src/stable_hash/tests.rs
• crates/perry-transform/src/inline/mod.rs
• crates/perry/src/commands/compile/bootstrap.rs
• crates/perry/src/commands/compile/collect_modules.rs
• crates/perry/tests/issue_5257_require_adopt_no_default_namespace.rs