fix(compile): compile commander's real source (#5137)#5212
Merged
Conversation
📝 WalkthroughWalkthroughAdds a ChangesEventEmitter subclass initialization and compilePackages guard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 `@crates/perry-codegen/src/expr/this_super_call.rs`:
- Around line 458-479: The EventEmitter initialization in the super-call handler
is only triggered when the direct parent_class is "EventEmitter" (i.e.,
parent_class == None), which misses inheritance chains like B extends A extends
EventEmitter where A has no own constructor. Modify the condition around the
parent_name == "EventEmitter" check in the this_super_call.rs file to walk up
the class hierarchy and detect if EventEmitter is an ancestor anywhere in the
chain, not just as the direct parent. Then apply the same ancestor-chain
detection logic to the no-own-ctor fallback path in
crates/perry-codegen/src/lower_call/new.rs to ensure EventEmitter initialization
runs consistently across both code paths when EventEmitter appears anywhere in
the inheritance hierarchy.
🪄 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: ad3d8f8f-563b-444c-b841-f2c21efe179c
📒 Files selected for processing (11)
crates/perry-api-manifest/src/entries.rscrates/perry-codegen/src/expr/this_super_call.rscrates/perry-codegen/src/lower_call/new.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi.rscrates/perry-hir/src/ir/constants.rscrates/perry-hir/src/ir/mod.rscrates/perry-hir/src/lower/context.rscrates/perry-runtime/src/node_stream_constructors.rscrates/perry-runtime/src/node_stream_readwrite.rsdocs/api/perry.d.tsdocs/src/api/reference.md
Comment on lines
+458
to
+479
| if parent_name.as_str() == "EventEmitter" { | ||
| for a in super_args { | ||
| let _ = lower_expr(ctx, a)?; | ||
| } | ||
| let this_box = match ctx.this_stack.last().cloned() { | ||
| Some(slot) => ctx.block().load(DOUBLE, &slot), | ||
| None => double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)), | ||
| }; | ||
| ctx.block().call( | ||
| DOUBLE, | ||
| "js_event_emitter_subclass_init", | ||
| &[(DOUBLE, &this_box)], | ||
| ); | ||
| let current_class_name = | ||
| ctx.class_stack.last().cloned().unwrap_or_default(); | ||
| crate::lower_call::apply_field_initializers_recursive( | ||
| ctx, | ||
| ¤t_class_name, | ||
| crate::lower_call::FieldInitMode::SelfOnly, | ||
| )?; | ||
| return Ok(double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))); | ||
| } |
There was a problem hiding this comment.
Handle EventEmitter init for inherited chains, not only direct parents.
At Line 458, the init is gated to parent_name == "EventEmitter" only in the parent_class == None path. This misses cases like class B extends A { constructor(){ super(); } } where A extends EventEmitter and A has no own constructor: parent_class is present (A), so no EventEmitter init runs and .on/.emit stay absent on this.
Please add an ancestor-chain termination check for EventEmitter in this super-call flow, and mirror the same logic in crates/perry-codegen/src/lower_call/new.rs (the no-own-ctor fallback path).
🤖 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-codegen/src/expr/this_super_call.rs` around lines 458 - 479, The
EventEmitter initialization in the super-call handler is only triggered when the
direct parent_class is "EventEmitter" (i.e., parent_class == None), which misses
inheritance chains like B extends A extends EventEmitter where A has no own
constructor. Modify the condition around the parent_name == "EventEmitter" check
in the this_super_call.rs file to walk up the class hierarchy and detect if
EventEmitter is an ancestor anywhere in the chain, not just as the direct
parent. Then apply the same ancestor-chain detection logic to the no-own-ctor
fallback path in crates/perry-codegen/src/lower_call/new.rs to ensure
EventEmitter initialization runs consistently across both code paths when
EventEmitter appears anywhere in the inheritance hierarchy.
… init + compile-package native-instance back-off (#5137) Force-compiling commander's npm source (`perry.compilePackages: ["commander"]` + `allow.compilePackages: ["*"]`) crashed: first an unresolved `js_commander_*` link error, then `TypeError: undefined is not a constructor` / `value is not a function`. The shim path (#5183) already works; this fixes the source-compile path that the "compile the whole dependency tree" goal needs. Three root causes: 1. Native-instance registration ignored compilePackages. `new Command()` was registered as a native `commander` instance off the hardcoded class-name fallback even when commander's real source is being compiled, re-routing its fluent methods to the `js_commander_*` shim (kept off the import path by #5183). Fixed at the chokepoint: `register_native_instance` backs off when the module's package is in the compile-packages override — mirroring the import-side back-off `is_native_module` already does (#665). 2. `class X extends EventEmitter` (commander's `Command`) had no super-init. The stream/Error/Event/Request parents all install their surface onto `this` at `super()`; EventEmitter had no arm, so `.on`/`.emit`/… were undefined. Added `js_event_emitter_subclass_init` (reuses the generic `ns_*` emitter closures the stream subclasses already use, via a new `emitter_methods()` table) and wired it into both super-call paths: the explicit-`super()` arm (this_super_call.rs) and the no-own-constructor implicit path (new.rs). The implicit path is gated on `!has_imported_ctor` so an imported class whose real ctor lives in another module (commander's `Command`) still runs its real constructor instead of being pre-empted. 3. Pre-existing manifest drift from #5183: `commander::args` was a dispatch row with `has_receiver: true` but a manifest `property(...)`, which failed the `every_dispatch_entry_has_manifest_counterpart` guard and emitted a spurious `export const args`. Modeled as a receiver method; docs regenerated. Source-compile and shim paths now both print `in.txt {"verbose":true}`, matching Node, including subcommands/actions/version.
The EventEmitter-subclass-init additions pushed new.rs (1988→2015) and stdlib_ffi.rs (1999→2003) over the file-size gate. Extract the emitter-init call into a shared lower_event_emitter_subclass_init helper in write_barrier.rs (used by both the explicit-super and no-own-ctor paths), carrying the doc comment once, and trim the FFI-decl comment. Both files back to 2000.
proggeramlug
force-pushed
the
worktree-fix-5137-commander
branch
3 times, most recently
from
fe33a8b to
a050be3
Compare
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.
Fixes #5137 (reopened) — force-compiling commander's real npm source still crashed even though the native shim path (#5183) works. This matters for the "natively compile the whole dependency tree" goal where the shim isn't the intended end state.
Repro
With
perry.compilePackages: ["commander"]+allow.compilePackages: ["*"]this went: unresolvedjs_commander_*link error →TypeError: undefined is not a constructor→value is not a function. Now printsin.txt {"verbose":true}, matching Node — same as the shim path.Root causes & fixes
1. Native-instance registration ignored
compilePackages.new Command()was registered as a nativecommanderinstance via the hardcoded class-name fallback even when commander's real source is compiled, re-routing its fluent methods to thejs_commander_*shim (which #5183 deliberately keeps off the import path). Fixed at the chokepoint —register_native_instancebacks off when the module's package is in the compile-packages override, mirroring the import-side back-offis_native_modulealready does (#665).2.
class X extends EventEmitterhad no super-init.Stream/Error/Event/Request parents install their surface onto
thisatsuper(); EventEmitter had no arm, so a source-compiledCommand extends EventEmitterhad undefined.on/.emit/…. Addedjs_event_emitter_subclass_init(reuses the genericns_*emitter closures the stream subclasses already use, via a newemitter_methods()table) and wired it into both super paths: the explicit-super()arm and the no-own-constructor implicit path. The implicit path is gated on!has_imported_ctorso an imported class whose real ctor lives in another module (commander'sCommand) still runs its real constructor instead of being pre-empted.3. Pre-existing manifest drift from #5183.
commander::argswas a dispatch row withhas_receiver: truebut a manifestproperty(...), failing theevery_dispatch_entry_has_manifest_counterpartguard and emitting a spuriousexport const args. Modeled as a receiver method;docs/regenerated.Verification
in.txt {"verbose":true}(matches Node), including subcommands/actions/versionclass X extends EventEmitterwith explicit ctor, implicit ctor, and instance methods callingthis.emitall work;on/once/emit/removeAllListeners/listenerCountverifiedcargo test -p perry-codegen -p perry-hir -p perry-runtimegreen;manifest_consistency5/5;cargo fmt --checkclean; API docs regenerated (api-docs-drift clean)No version bump / changelog (per maintainer-folds-metadata-at-merge convention).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes