fix(web_generator): resolve infinite recursion for self-referential class types - #525
fix(web_generator): resolve infinite recursion for self-referential class types#525wisdomaj wants to merge 2 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces a pre-registration step for declarations in the Transformer to prevent infinite recursion when members reference their parent type. While this addresses the immediate issue, feedback highlights that the implementation is incomplete for nested declarations and causes global symbol pollution, which is evident in the updated test expectations where types are resolved to underlying class names instead of intended typedefs. A more robust solution involving proper scope registration is recommended.
…lass types Problem: - Classes with members whose return types reference the class itself (e.g. `static open(): Promise<Database>` inside `Database`) cause infinite recursion and a stack overflow - `_searchForDeclRecursive` fails to find the class in `nodeMap` because it hasn't been registered yet, then re-enters `transformAndReturn` for the same class, looping forever - This blocks usage of `web_generator` with most real-world npm packages, where `Promise<Self>` factory methods are standard Solution: - Add a `_pendingTypes` map to track declarations mid-transformation - In `_transformClassOrInterface`, register the declaration as pending before processing members and remove it after via try/finally - In `_searchForDeclRecursive`, check `_pendingTypes` before calling `transformAndReturn` -- if found, use the in-progress instance to break the cycle - Guard the post-loop re-lookup so it doesn't overwrite a pending match This approach avoids scope registration duplication and does not pollute the global `nodeMap` with nested declarations. It works identically for top-level and namespace-nested classes.
8782045 to
dc2f526
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a cycle-detection mechanism in the Transformer class to prevent infinite recursion when a class member's type references the class itself. It introduces a _pendingTypes map to track active transformations and updates the declaration lookup logic to utilize these in-progress instances. I have no feedback to provide.
|
@wisdomaj – tests would be nice 😄 |
Adds test cases for classes whose members reference the class itself, covering patterns that previously caused infinite recursion: - Static factory returning Promise<Self> - Instance method returning Promise<Self> - Mutual references between classes via Promise - Namespace-nested class with self-referential Promise - Generic class with self-referential type argument - Property typed as the class itself - Constructor parameter typed as the class - Callback parameter containing the class type
|
@kevmoo Just added all of the test cases I could think of at the moment 😄 |
|
This is interesting... something I was currently dealing with concerning infinite recursion is dealing with multiple of the same/similar nodes defined in different files. Sometimes these nodes may be overloaded and the node properties may need to be referenced from another file. For reference, this issue comes when parsing types from the ES Spec implementations in TS declarations by the TypeScript team. While I understand the scope of this PR may not be to cover that as well, I wanted to make this comment as a reference so that if that isn't covered here, I'd continue from this to find a longer-term solution. |
|
FYI: I'm vibe hacking on bootstrapping the generator on itself and hitting the same issue! I have a fix locally (stacked with a BUNCH of other stuff) that took inspiration from this fix! |
|
Looks like we need to rebase on HEAD and fix tests, please! |
|
Apologies for the late response to this PR. I've been crazy busy the last few weeks and haven't had any time to dedicate to this. I did just submit github issue #550 that better explains my motivation for this PR and other work I planned to contribute. This PR is pretty far behind main now and it looks like from PR #548 that the issues I attempted to address in this PR may have been taken care of already. I can close this PR if you'd like. |
|
#549 is the latest that @nikeokoronkwo is going to look at. If you want to help, I'd love eyes on that HUGE PR. It has many individual commits that should be easier to look through. Are the tests robust? Are there obvious issues with the implementation? That would be a HUGE help! 🙏 |
|
Awesome! I'll try and take a look later today if time allows. |
| } | ||
| } | ||
| } finally { | ||
| _pendingTypes.remove(typeDecl); |
There was a problem hiding this comment.
Do you expect the system to keep moving after an exception here?
There was a problem hiding this comment.
I was trying to retain the same behavior with how exceptions were handled. I think we would want to fail fast and loud. If we kept moving after an exception I think we would generate bindings that are not complete, compile fine, but cause runtime errors.
Using try/finally are probably not necessary right now since an exception should stop transformation anyways. Using try/finally is more so defensive programming in case exception handling is changed in the future. If someone later adds a higher level catch (e.g. to print a different error message), removing finally would leave the half built type stuck in _pendingTypes, and later types that reference it would pick up that broken version instead of a fully transformed one.
Summary
Fixes a stack overflow in
web_generator's interop_gen transformer when a class member's return type references the class being transformed (e.g.static open(): Promise<Database>insideDatabase).Problem
When transforming a class,
_transformClassOrInterfacebuilds the declaration object and then iterates over members. When it encountersPromise<Database>, the type resolver calls_searchForDeclRecursive("Database"), which looks innodeMap. The class hasn't been registered yet (that happens aftertransformAndReturncompletes), so the lookup fails and triggers a newtransformAndReturn(Database)call, causing infinite recursion.Reproducing the issue
input.d.ts:Removing the
Promise<Database>return type (or replacing it withPromise<void>) makes the crash go away, confirming the self-referential type is the trigger.Solution
Uses an in-progress guard map (
_pendingTypes)._transformClassOrInterfaceregisters the declaration as pending before processing members and removes it after viatry/finally_searchForDeclRecursivechecks_pendingTypesbefore callingtransformAndReturn. If found, it uses the in-progress instance to break the cycleThis approach:
_transformNamespaceor_searchForDeclRecursive's existing registration paths)nodeMapwith nested declarationsTest plan