fix(napi): preserve generator class methods#3231
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughWalkthroughAdds a safe runtime helper to set up Iterator inheritance for exported classes, changes class registration to carry an Changes
Sequence Diagram(s)sequenceDiagram
participant ModuleReg as ModuleRegister
participant Runtime as BindgenRuntime
participant Env as NAPI_Env
participant JS as JS_Global
ModuleReg->>Runtime: for each registered class with implement_iterator
Runtime->>Env: setup_iterator_class(env, class_ctor)
Env->>JS: get global.Iterator
JS-->>Env: Iterator (function) or undefined
alt Iterator is function
Env->>Env: get class.prototype and its parent prototype
Env->>JS: napi_strict_equals(class_proto_parent, Iterator.prototype)
alt already inherits
Env-->>Runtime: no-op
else not inheriting
Env->>JS: Object.setPrototypeOf(class.prototype, Iterator.prototype)
Env-->>Runtime: prototype updated
end
else Iterator missing
Env-->>Runtime: skip setup
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3274123565
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/napi/src/bindgen_runtime/iterator.rs (1)
255-306:⚠️ Potential issue | 🟠 MajorFix prototype chain flattening in Iterator setup.
Calling
Object.setPrototypeOf(class_proto, iterator_proto)replaces the existing[[Prototype]]link ofclass_proto, completely removing any previously inherited base prototype from the chain. If a user subclasses a derived generator class, methods from the base class become inaccessible. Instead, walk the full prototype chain and insertIterator.prototypebetween the highest non-Iterator prototype and its current parent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/napi/src/bindgen_runtime/iterator.rs` around lines 255 - 306, The current code calls Object.setPrototypeOf(class_proto, iterator_proto) which flattens the prototype chain and discards any intermediate base prototypes; instead, walk the prototype chain starting at class_proto using repeated napi_get_prototype and napi_strict_equals (the same checks used for class_proto_parent/already_inherits_iterator) to find the highest prototype whose parent is not Iterator.prototype, then call Object.setPrototypeOf on that prototype to set its parent to iterator_proto (preserving existing links); update the argv/ni_get_named_property/Object.setPrototypeOf usage so the setPrototypeOf call targets the identified prototype rather than class_proto, ensuring generator classes keep their own methods while inheriting Iterator.prototype.
🧹 Nitpick comments (1)
examples/napi/__tests__/generator.spec.ts (1)
104-117: Please add a derived-class regression case here.This proves the single-level
Fib4.prototype -> Iterator.prototypepath, but notclass Child extends Fib4 {}. That is the case most likely to regress if the runtime rewires the wrong link, so it would be worth asserting that bothChild.prototypeandFib4.prototypestay in the chain and thattoJSON()still resolves from the base prototype.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/napi/__tests__/generator.spec.ts` around lines 104 - 117, Add a derived-class regression test by defining class Child extends Fib4 and instantiating it (e.g., const child = new Child(0,1)); assert that Object.getPrototypeOf(child) === Child.prototype, Object.getPrototypeOf(Child.prototype) === Fib4.prototype and Object.getPrototypeOf(Fib4.prototype) === Iterator.prototype (when Iterator exists); also assert typeof child.toJSON === 'function' and child.toJSON() returns the same array (and JSON.stringify(child) matches), and that iterator helpers like child.map are available — this ensures Child.prototype and Fib4.prototype remain in the chain and toJSON still resolves from Fib4.prototype.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/napi/src/bindgen_runtime/iterator.rs`:
- Around line 255-306: The current code calls Object.setPrototypeOf(class_proto,
iterator_proto) which flattens the prototype chain and discards any intermediate
base prototypes; instead, walk the prototype chain starting at class_proto using
repeated napi_get_prototype and napi_strict_equals (the same checks used for
class_proto_parent/already_inherits_iterator) to find the highest prototype
whose parent is not Iterator.prototype, then call Object.setPrototypeOf on that
prototype to set its parent to iterator_proto (preserving existing links);
update the argv/ni_get_named_property/Object.setPrototypeOf usage so the
setPrototypeOf call targets the identified prototype rather than class_proto,
ensuring generator classes keep their own methods while inheriting
Iterator.prototype.
---
Nitpick comments:
In `@examples/napi/__tests__/generator.spec.ts`:
- Around line 104-117: Add a derived-class regression test by defining class
Child extends Fib4 and instantiating it (e.g., const child = new Child(0,1));
assert that Object.getPrototypeOf(child) === Child.prototype,
Object.getPrototypeOf(Child.prototype) === Fib4.prototype and
Object.getPrototypeOf(Fib4.prototype) === Iterator.prototype (when Iterator
exists); also assert typeof child.toJSON === 'function' and child.toJSON()
returns the same array (and JSON.stringify(child) matches), and that iterator
helpers like child.map are available — this ensures Child.prototype and
Fib4.prototype remain in the chain and toJSON still resolves from
Fib4.prototype.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54fed686-e476-4577-95bf-83ae218873fc
⛔ Files ignored due to path filters (1)
examples/napi/__tests__/__snapshots__/values.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
crates/napi/src/bindgen_runtime/iterator.rsexamples/napi/__tests__/__snapshots__/values.spec.ts.mdexamples/napi/__tests__/generator.spec.tsexamples/napi/index.d.ctsexamples/napi/src/generator.rs

Note
Medium Risk
Touches N-API class registration and iterator prototype wiring, which can affect JS-visible behavior for all iterator-enabled classes across runtimes. While scoped behind
implement_iteratorand covered by new tests, regressions could surface as broken prototype chains or missing methods in consumer code.Overview
Ensures
#[napi(iterator)]classes gain JavaScriptIteratorhelper methods without overwriting their own prototype methods by moving prototype setup from per-instance mutation to class-level inheritance.This threads an
implement_iteratorflag through codegen andregister_class, records it in module registration metadata, and during module init callssetup_iterator_classto setClass.prototype’s parent toIterator.prototypewhen available.Updates the
Fib4example to addtoJSON()and expands generator tests to assert correctinstanceof Iteratorbehavior, preserved prototype chains (including subclasses), and JSON serialization support.Reviewed by Cursor Bugbot for commit 01fd42a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Improvements
Tests