fix(cjs-wrap): function-nested class named export lost (ajv 'undefined is not a constructor')#5325
Merged
Merged
Conversation
📝 WalkthroughWalkthroughIn ChangesNested class name shadowing fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
added 2 commits
June 17, 2026 15:22
A function-nested `class X {}` whose name collides with an outer
same-named local (the cjs_wrap `export const X = _cjs.X` re-export
binding) failed to shadow it, so an in-scope `exports.X = X` resolved
`X` through lookup_local to the circular module-scope const → undefined.
A nested `function X(){}` already define_local's its name and shadows
correctly; mirror that for classes by binding the name to a ClassRef.
Fixes ajv `require('./code').Name === undefined` (new code_1.Name throws
'undefined is not a constructor').
End-to-end regression for fix/ajv-not-constructor: a compilePackages package with a function-nested `class Name` (kept in the IIFE by the #5251 guard) exported via `exports.Name = Name` must surface on `require('./code').Name`. Guards both the class case (the bug) and the function case in the identical shape (the asymmetry).
proggeramlug
force-pushed
the
fix/ajv-not-constructor
branch
from
7ae1f8c to
ee20121
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/test_cjs_nested_class_named_export.sh`:
- Around line 100-113: The script may exit prematurely due to `set -e` when
`./test_bin` exits with a non-zero status, preventing the failure diagnostics
from being printed. Modify the RUN_OUTPUT command execution to gracefully handle
non-zero exit codes from `./test_bin` so the script continues to execute and
print the Expected/Got output for debugging purposes. This can be done by either
temporarily disabling the exit-on-error behavior before running the test
command, or by appending `|| true` to the command to suppress the exit code
propagation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ec9a79e-35f5-4d49-9e2b-7b9d77ccfd28
📒 Files selected for processing (2)
crates/perry-hir/src/lower_decl/body_stmt.rstests/test_cjs_nested_class_named_export.sh
Comment on lines
+100
to
+113
| RUN_OUTPUT=$(./test_bin 2>&1) | ||
| # Matches `node main.ts`: the class & function both export as functions, and the | ||
| # cross-module `new code_1.Name(...)` / `new func_1.Make(...)` succeed. | ||
| EXPECTED="Name=function Make=function kind=[object Object] made=vx" | ||
|
|
||
| if [ "$RUN_OUTPUT" = "$EXPECTED" ]; then | ||
| echo "PASS" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "FAIL: kept-in-IIFE class named export not visible on require()" | ||
| echo "Expected: $EXPECTED" | ||
| echo "Got: $RUN_OUTPUT" | ||
| exit 1 |
There was a problem hiding this comment.
Guard runtime execution so failures still print diagnostics.
At Line 100, set -e can terminate the script before the Expected/Got failure output runs. Add explicit handling around ./test_bin so runtime regressions remain debuggable.
Proposed fix
-RUN_OUTPUT=$(./test_bin 2>&1)
+RUN_OUTPUT=$(./test_bin 2>&1) || {
+ echo "FAIL: runtime error"
+ echo "$RUN_OUTPUT" | tail -20
+ exit 1
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RUN_OUTPUT=$(./test_bin 2>&1) | |
| # Matches `node main.ts`: the class & function both export as functions, and the | |
| # cross-module `new code_1.Name(...)` / `new func_1.Make(...)` succeed. | |
| EXPECTED="Name=function Make=function kind=[object Object] made=vx" | |
| if [ "$RUN_OUTPUT" = "$EXPECTED" ]; then | |
| echo "PASS" | |
| exit 0 | |
| fi | |
| echo "FAIL: kept-in-IIFE class named export not visible on require()" | |
| echo "Expected: $EXPECTED" | |
| echo "Got: $RUN_OUTPUT" | |
| exit 1 | |
| RUN_OUTPUT=$(./test_bin 2>&1) || { | |
| echo "FAIL: runtime error" | |
| echo "$RUN_OUTPUT" | tail -20 | |
| exit 1 | |
| } | |
| # Matches `node main.ts`: the class & function both export as functions, and the | |
| # cross-module `new code_1.Name(...)` / `new func_1.Make(...)` succeed. | |
| EXPECTED="Name=function Make=function kind=[object Object] made=vx" | |
| if [ "$RUN_OUTPUT" = "$EXPECTED" ]; then | |
| echo "PASS" | |
| exit 0 | |
| fi | |
| echo "FAIL: kept-in-IIFE class named export not visible on require()" | |
| echo "Expected: $EXPECTED" | |
| echo "Got: $RUN_OUTPUT" | |
| exit 1 |
🤖 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 `@tests/test_cjs_nested_class_named_export.sh` around lines 100 - 113, The
script may exit prematurely due to `set -e` when `./test_bin` exits with a
non-zero status, preventing the failure diagnostics from being printed. Modify
the RUN_OUTPUT command execution to gracefully handle non-zero exit codes from
`./test_bin` so the script continues to execute and print the Expected/Got
output for debugging purposes. This can be done by either temporarily disabling
the exit-on-error behavior before running the test command, or by appending `||
true` to the command to suppress the exit code propagation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root
In a compiled CJS package (`perry.compilePackages`), a function-nested `class X {}` whose name collides with an outer same-named local failed to shadow that local, so an in-scope read of `X` resolved to the wrong binding.
Concretely: the #5251 hoist guard correctly keeps a class inside the wrapping IIFE when its body references the injected `exports`/`module`/`require` (ajv's `codegen/code.js`: `class Name { constructor(s){ if(!exports.IDENTIFIER.test(s)) ... } }`). The cjs_wrap then emits a module-scope re-export binding `export const Name = _cjs.Name;` (= local `Name`). When the IIFE body runs `exports.Name = Name` (recording the module's named export), the identifier `Name` was resolved via `lookup_local` — which walked past the (absent) in-IIFE binding up to that module-scope `const Name = _cjs.Name`. That is a circular self-read → `undefined`. So `require('./code').Name` was `undefined`, and ajv's `scope.js` `new code_1.Name(...)` threw `TypeError: undefined is not a constructor` (at `compile/codegen/scope.js:17`).
Class-vs-function asymmetry: a nested `function X(){}` already `define_local`s its name (`lower_nested_fn_decl`) and shadows the outer const, so the identical shape exported fine. A nested `class X {}` only registered into the global class table and created no scope-local binding — the gap.
Fix
`crates/perry-hir/src/lower_decl/body_stmt.rs` — when lowering a function-nested class whose name has a pre-existing outer local, define a scope-local binding and emit `Stmt::Let { name, init: ClassRef(name) }` (the same shape `var C = class {…}` lowers to, already recorded by codegen in `local_class_aliases`). This shadows the outer re-export const, mirroring the function path. Gated on a pre-existing outer binding so packages without a collision are byte-for-byte unaffected.
Minimal repro (before/after)
CJS package `pk` with `code.js` (`class Name`; `exports.Name = Name`), `scope.js` (`new require('./code').Name(...)`), `index.js` (re-exports), consumed by `import pk from "pk"`:
Swapping `class Name` → `function Name` made perry succeed pre-fix (the asymmetry). A new regression test `tests/test_cjs_nested_class_named_export.sh` exercises both the class case (the bug) and the function case (the guard) end-to-end.
ajv (before/after)
Validation
Note for maintainer
Per contributor convention, this PR does not touch `[workspace.package] version` (Cargo.toml), the `Current Version:` line (CLAUDE.md), `CHANGELOG.md`, or `Cargo.lock` — please fold the version bump + changelog entry at merge.
Summary by CodeRabbit
Bug Fixes
Tests