fix(compile): keep CJS classes reading exports inside the IIFE (#5251)

[proggeramlug]

Summary

Fixes #5251. Inside a class method/constructor of a natively-compiled CJS package (compilePackages), reading a module-level exports.X yielded perry's numeric 0 sentinel instead of the value — the dominant root of the native-compile "sentinel-0" class (e.g. ajv's exports.IDENTIFIER.test(s) inside a class constructor → (number).test is not a function).

Root cause

The cjs_wrap preamble injects var exports, var module, and a require function as IIFE-local bindings. extract_top_level_class_decls (the #652 class-hoist pass) hoists top-level classes out of the IIFE so consumers can import { X } and resolve to the real class. The #2310 guard already refuses to hoist a class whose body references a top-level let/const/var — but its textual scan only sees declarations in the original source, and exports/module/require are never declared there (they come from the preamble). So a class reading exports.X got hoisted, severing its closure over the injected var exports; exports then resolved as an unknown global (warning: "unknown identifier 'exports' … bare reads lower to 0") and exports.X lowered to 0. Free functions and module-level code stay inside the IIFE, which is why they worked.

Fix

Always add exports, module, require to the IIFE-local set that gates hoisting (extract_top_level_class_decls). A class reading any of them now stays inside the IIFE and closes over the injected bindings — same resolution path as free functions / module-level code. Classes that don't touch those bindings still hoist normally.

One file of logic (hoist_classes.rs, +16 lines) plus two unit tests (mod.rs).

Test evidence

Minimal re repro (class method reads exports.TAG):

• Before: 0!
• After: hi! (matches Node)

Extended fixture, all matching Node byte-for-byte:

• constructor this.v = exports.TAG → hi
• exports.RE = /x/i; … m(){ return exports.RE.test("X") } → true
• free-function exports.TAG read → hi? (regression: still works)
• module-level const TOP = 42 read from a class method → 42 (that class still hoists; regression intact)

Added unit tests: issue_5251_class_reading_exports_stays_in_iife, issue_5251_class_without_exports_still_hoists. cargo test --release -p perry -p perry-hir green (one HTTP integration test flaked under full parallel load — ext-http link race — and passes when run isolated; unrelated to this change).

ajv before/after (payoff check)

import Ajv from "ajv"; const a = new Ajv(); const v = a.compile({…}); console.log(v({n:1}), v({})) with compilePackages:["*"]:

• Before: compiled with unknown identifier 'exports' warnings; the (number).test is not a function sentinel-0 error.
• After: clean compile + link, no exports warnings, the (number).test error is gone. It now hits a different, later wall: TypeError: undefined is not a constructor at new Ajv() — the Ajv default-export/constructor binding, a separate issue not addressed here.

Risks

Low. The change only widens the set of bindings that keep a class inside the IIFE (more conservative hoisting). Word-boundary matching (class_body_references_any) already excludes .-prefixed property accesses, so this.exports etc. won't trip it. Worst case: a class that merely mentions exports/module/require as a bare identifier stays un-hoisted and is reached via _cjs.X — which still works.

Maintainer note

No version / CHANGELOG.md / Cargo.lock edits — left for fold-in at merge per CLAUDE.md.

Summary by CodeRabbit

• Bug Fixes

  • Fixed incorrect hoisting of top-level classes in CommonJS output when they reference injected bindings (exports, module, require), preventing potential runtime errors.

• Tests

  • Added regression tests for CommonJS wrapper class hoisting behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

extract_top_level_class_decls is updated to append the CJS-injected identifiers (exports, module, require) to iife_locals before the hoisting gate check, preventing classes that reference those bindings from being moved outside the IIFE. Two regression tests for issue #5251 are added to verify both the stay-in and hoist-out paths.

Changes

CJS Class Hoisting Fix

Layer / File(s)	Summary
Hoisting fix and regression tests crates/perry/src/commands/compile/cjs_wrap/hoist_classes.rs, crates/perry/src/commands/compile/cjs_wrap/mod.rs	iife_locals is changed to mutable and extended with exports, module, and require before the hoisting gate check in extract_top_level_class_decls. Two tests assert that a class referencing exports stays inside the _cjs IIFE and that a class without such references still hoists above it.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related issues

• compilePackages: exports.X reads as the 0-sentinel inside a class method/constructor (CJS exports not captured in class scope) #5251: The fix directly addresses this issue by adding the CJS-injected bindings to iife_locals, preventing classes that close over exports, module, or require from being incorrectly hoisted out of the IIFE.

Poem

🐇 A class clung to exports with care,
But the hoister would lift it to air!
Now iife_locals knows
Where each injection flows,
And classes stay put — fair and square! 🎉

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main fix: preventing CJS classes that read exports from being hoisted outside the IIFE, directly addressing issue #5251.
Description check	✅ Passed	The description comprehensively covers the summary, root cause, fix, test evidence, and risks. It includes the related issue (#5251), test plan with cargo commands, and follows the template structure with all critical sections completed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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/cjs-exports-in-class-scope

---

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

[coderabbitai]

🧹 Nitpick comments (1)

crates/perry/src/commands/compile/cjs_wrap/mod.rs (1)

651-698: ⚡ Quick win

Add direct regression cases for module and require binding reads.

Nice coverage for exports; consider adding one class case each that references module.exports and require(...) so all three injected bindings guarded by the fix are explicitly pinned by tests.

Proposed test additions

+    #[test]
+    fn issue_5251_class_reading_module_stays_in_iife() {
+        let src = "\"use strict\";\nclass M { get() { return module.exports; } }\nexports.m = M;\n";
+        let wrapped = wrap_commonjs(src, &PathBuf::from("/tmp/app/node_modules/re/index.js"));
+        let iife_start = wrapped.find("const _cjs = (function() {").expect("expected IIFE");
+        let class_pos = wrapped.find("class M ").expect("class M missing");
+        assert!(class_pos > iife_start, "class reading module must stay inside IIFE");
+    }
+
+    #[test]
+    fn issue_5251_class_reading_require_stays_in_iife() {
+        let src = "\"use strict\";\nclass R { load() { return require('./x'); } }\nexports.r = R;\n";
+        let wrapped = wrap_commonjs(src, &PathBuf::from("/tmp/app/node_modules/re/index.js"));
+        let iife_start = wrapped.find("const _cjs = (function() {").expect("expected IIFE");
+        let class_pos = wrapped.find("class R ").expect("class R missing");
+        assert!(class_pos > iife_start, "class reading require must stay inside IIFE");
+    }

🤖 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/src/commands/compile/cjs_wrap/mod.rs` around lines 651 - 698,
The current regression tests only cover the case where a class reads the
injected `exports` binding, but the fix also guards against classes reading
`module` and `require` bindings. Add two new test functions following the same
pattern as `issue_5251_class_reading_exports_stays_in_iife()`: one named
`issue_5251_class_reading_module_stays_in_iife()` that tests a class with
`module.exports` access inside a method, and one named
`issue_5251_class_reading_require_stays_in_iife()` that tests a class with a
`require(...)` call inside a method. Both should verify that the class stays
inside the IIFE and the output parses correctly, mirroring the structure and
assertions of the existing exports test.

🤖 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/src/commands/compile/cjs_wrap/mod.rs`:
- Around line 651-698: The current regression tests only cover the case where a
class reads the injected `exports` binding, but the fix also guards against
classes reading `module` and `require` bindings. Add two new test functions
following the same pattern as
`issue_5251_class_reading_exports_stays_in_iife()`: one named
`issue_5251_class_reading_module_stays_in_iife()` that tests a class with
`module.exports` access inside a method, and one named
`issue_5251_class_reading_require_stays_in_iife()` that tests a class with a
`require(...)` call inside a method. Both should verify that the class stays
inside the IIFE and the output parses correctly, mirroring the structure and
assertions of the existing exports test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 06ebf3ce-5e88-4442-a1e7-e70d25ded676

📥 Commits

Reviewing files that changed from the base of the PR and between 690e4e5 and e2e038d.

📒 Files selected for processing (2)

• crates/perry/src/commands/compile/cjs_wrap/hoist_classes.rs
• crates/perry/src/commands/compile/cjs_wrap/mod.rs