Skip to content

fix(nitrogen): prevent exponential traversal and support cyclic struct references#1232

Open
puckey wants to merge 9 commits into
mrousavy:mainfrom
puckey:fix/cyclic-struct-references
Open

fix(nitrogen): prevent exponential traversal and support cyclic struct references#1232
puckey wants to merge 9 commits into
mrousavy:mainfrom
puckey:fix/cyclic-struct-references

Conversation

@puckey

@puckey puckey commented Mar 2, 2026

Copy link
Copy Markdown
Contributor

Two related fixes for nitrogen code generation with complex/self-referential struct types:

1. Prevent exponential type re-traversal (fixes #1079)

  • getRequiredImports() and getExtraFiles() in SwiftCxxBridgedType and KotlinCxxBridgedType were visiting the same types exponentially many times for interconnected struct graphs
  • Added per-call-tree visited sets (as default parameters) to skip already-visited types
  • Tested with a 7-struct Gallery graph that previously caused stack overflow

2. Support self-referential/cyclic struct types (fixes #1070)

  • Structs referencing themselves through reference types (arrays, callbacks, promises, records) caused infinite recursion during code generation
  • Break the cycle by tracking structs being initialized and using lazy property resolution in StructType — when a struct is encountered while it's already being processed, the cached instance is returned instead of recursing infinitely
  • Simpler approach to the problem addressed in fix(nitrogen): handle cyclic struct references #1074, per mrousavy's feedback about reducing complexity

Test plan

  • Added a 7-struct Gallery graph that triggers exponential traversal
  • Added a self-referential TreeNode struct (children: TreeNode[], onChange?: (node: TreeNode) => void)
  • bun run specs in packages/react-native-nitro-test/ succeeds without stack overflow
  • Generated C++ uses forward declarations and reference types correctly

@vercel

vercel Bot commented Mar 2, 2026

Copy link
Copy Markdown

@puckey is attempting to deploy a commit to the Margelo Team on Vercel.

A member of the Team first needs to authorize it.

@puckey puckey force-pushed the fix/cyclic-struct-references branch from 738f160 to 75008ff Compare March 2, 2026 11:42
using namespace facebook;

// Forward declarations for cyclic dependencies
${forwardDeclarations}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be move to the right and used with indent(...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — using getForwardDeclaration() now handles the formatting.


// Forward declarations for cyclic function types
const forwardDeclarations = Array.from(cyclicNames)
.map((name) => ` struct J${name};`)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a utility for that - getForwardDeclaration I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — switched to getForwardDeclaration() utility in both KotlinStruct.ts and KotlinFunction.ts.

using namespace facebook;

// Forward declarations for cyclic dependencies
${forwardDeclarations}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also shouldn't be inside the namespace. What about forward declarations from external packages?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — forward declarations are now placed before the namespace block using getForwardDeclaration(), which wraps each declaration in its own namespace. Applied to both KotlinStruct.ts and KotlinFunction.ts.

#include <NitroModules/JPromise.hpp>
#include <NitroModules/JInstant.hpp>
#include <NitroModules/JAnyMap.hpp>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't look correct. We definitely use those types, so we have to either forward-declare or import them. Ideally forward declare here, and remove the whole bodies here, and move it into a .cpp file with an import. But thats for the future, for now import should work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — this was caused by the visited sets being module-level, so types visited while generating one file were skipped for all subsequent files in the same codegen run. Changed to per-call-tree visited sets (passed as default parameters) so each file generation starts fresh.

@mrousavy

mrousavy commented Mar 2, 2026

Copy link
Copy Markdown
Owner

How will a struct that references itself work? This is not possible in C++ and Swift, as it has an infinite size

@puckey puckey changed the title fix(nitrogen): support cyclic/self-referential struct types fix(nitrogen): prevent exponential traversal and support cyclic struct references Mar 2, 2026
@puckey puckey force-pushed the fix/cyclic-struct-references branch from 75008ff to b23fa05 Compare March 2, 2026 13:19
@puckey

puckey commented Mar 2, 2026

Copy link
Copy Markdown
Contributor Author

Self-references are only allowed through reference types (arrays, callbacks, promises, records) — these use heap indirection so the struct size stays finite. Direct self-references like next: TreeNode throw an error at codegen time suggesting to wrap in an array or callback.

@puckey

puckey commented Mar 2, 2026

Copy link
Copy Markdown
Contributor Author

Verified that direct self-references are rejected at codegen time:

❌ Failed to generate spec for TestObjectCpp! Error: Cyclic struct reference detected:
   "TreeNode" references itself directly. Structs cannot have direct cyclic references
   because they are value types with fixed size in C++ and Swift.
   See: https://nitro.margelo.com/docs/types/custom-structs#direct-cyclic-references-are-not-supported

puckey added a commit to radio-garden/react-native-audio-browser that referenced this pull request Mar 4, 2026
…es (PR #1232)

Replace vendored codegen with upstream nitrogen from
mrousavy/nitro#1232, adding support for cyclic/self-referential
struct types and preventing exponential type re-traversal.
@puckey puckey force-pushed the fix/cyclic-struct-references branch from d95a14e to 49faf43 Compare March 4, 2026 14:37
hyochan added a commit to hyochan/react-native-iap that referenced this pull request Mar 9, 2026
Upgrade to Nitro Modules 0.35.0 which fixes a critical memory leak in
Kotlin HybridObjects and removes the deprecated `updateNative` API.

- nitrogen: ^0.31.1 → ^0.35.0
- react-native-nitro-modules: ^0.31.1 → ^0.35.0 (devDeps, peerDeps, example, example-expo)
- Use --stack-size=65536 for nitrogen scripts to work around upstream
  exponential type traversal bug (mrousavy/nitro#1232)

Closes #3159

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hyochan added a commit to hyochan/react-native-iap that referenced this pull request Mar 9, 2026
….0 (#3162)

## Summary

- Upgrade `nitrogen` and `react-native-nitro-modules` from `^0.31.1` to
`^0.35.0`
- Fixes build failure with `react-native-nitro-modules` 0.35.0
(`Unresolved reference 'updateNative'`)
- Adds `--stack-size=65536` workaround for nitrogen codegen exponential
type traversal bug

## Background

Nitro Modules
[v0.35.0](https://github.com/mrousavy/nitro/releases/tag/v0.35.0)
includes a **breaking change** for Kotlin HybridObjects — the
`updateNative()` API was removed to fix a critical memory leak. This
caused the build failure reported in #3159.

### Why `--stack-size=65536`?

Nitrogen 0.35.0 has a known [exponential type traversal
bug](mrousavy/nitro#1232) where
`getReferencedTypes()` revisits the same types repeatedly without a
visited set. For large specs like ours (40+ methods, complex nested
structs), this exceeds the default Node.js stack size. Increasing the
stack size is a safe workaround until the upstream fix
([mrousavy/nitro#1232](mrousavy/nitro#1232)) is
merged — at which point we can revert to the standard `nitrogen` CLI.

## Changes

### Dependencies
- `nitrogen`: `^0.31.1` → `^0.35.0`
- `react-native-nitro-modules`: `^0.31.1` → `^0.35.0` (root devDeps,
peerDeps, example, example-expo)

### Scripts (`package.json`)
- `nitrogen` and `specs` scripts now use `node --stack-size=65536
node_modules/nitrogen/lib/index.js` instead of bare `nitrogen` CLI

### Generated files (`nitrogen/generated/` — gitignored)
- Regenerated with nitrogen 0.35.0
- `HybridRnIapSpec.kt`: `updateNative()` removed (fixes #3159)
- JNI initialization updated internally to `facebook::jni::initialize()`
pattern

## Test plan

- [x] `yarn typecheck` passes
- [x] `yarn lint` passes
- [x] `yarn test` — 251 tests passed
- [x] `yarn specs` — generates 1/1 HybridObject successfully

Closes #3159

🤖 Generated with [Claude Code](https://claude.ai/code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Chores**
* Bumped react-native-nitro-modules and related tooling to ^0.35.0 and
updated CLI execution scripts.

* **Improvements**
* More consistent, null-safe in-app purchase data across iOS and Android
with standardized init/connection flows, improved serialization, and a
minimal promoted-product fallback.
* Public API shapes now consistently use variant-wrapped optional values
for more predictable behavior.

* **Bug Fixes**
  * Safer purchase error handling that preserves original error details.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
@puckey

puckey commented Mar 20, 2026

Copy link
Copy Markdown
Contributor Author

Another library running into this issue is forced to solve it by increasing their node stack size: hyochan/react-native-iap@ed55e2a

Why --stack-size=65536?

Nitrogen 0.35.0 has a known exponential type traversal
bug
where
getReferencedTypes() revisits the same types repeatedly without a
visited set. For large specs like ours (40+ methods, complex nested
structs), this exceeds the default Node.js stack size. Increasing the
stack size is a safe workaround until the upstream fix
(#1232) is
merged — at which point we can revert to the standard nitrogen CLI.

@mrousavy

Copy link
Copy Markdown
Owner

Okay thanks, I will give this another look.

puckey added 9 commits June 12, 2026 11:49
A deep/wide struct graph (7 interconnected types) where multiple
structs reference the same shared types from many paths. Without
a fix, nitrogen code generation stack-overflows because shared types
get re-traversed from every path in the type graph (mrousavy#1079).

Note: `bun run specs` will fail on this commit — the fix follows
in the next commit.
`getRequiredImports()` and `getExtraFiles()` in nitrogen's
`SwiftCxxBridgedType` (the TypeScript class that generates the
Swift <-> C++ bridge code) have no protection against re-traversing
the same type from multiple paths in the type graph, causing
exponential blowup and stack overflow for projects with many
interconnected struct types.

mrousavy#1247 added a visited-set guard to the equivalent
`KotlinCxxBridgedType`; this applies the same per-call-tree
visited-set guard to `SwiftCxxBridgedType`. The visited sets also
act as the cycle guard for cyclic struct references later in this
branch.

Fixes mrousavy#1079
Adds a TreeNode struct that references itself through an array
(children: TreeNode[]) and a callback (onChange?: (node: TreeNode) => void),
plus a bounceTreeNode method on SharedTestObjectProps.

Note: `bun run specs` will stack-overflow on this commit — the fix
follows in the next commit.
Structs that reference themselves through reference types (arrays,
callbacks, promises, records) previously caused a stack overflow
during code generation.

Break the cycle by tracking structs being initialized and using
lazy property resolution in StructType. When a struct is encountered
while it's already being processed, the cached instance is returned
instead of recursing infinitely.

Follows up on PR mrousavy#1074 with a simpler approach per mrousavy's
feedback about reducing complexity.

Fixes mrousavy#1070
The cyclic initialization fallback in StructType.getRequiredImports
used the bare struct name (e.g. "TreeNode") instead of the filename
("TreeNode.hpp"), causing a self-include that slipped past the
CppStruct filter. Harmless due to #pragma once, but unnecessary.
Address review feedback:
- Use the existing `getForwardDeclaration()` utility instead of
  manually constructing forward declaration strings
- Move forward declarations before the namespace block, matching
  the pattern used in CppStruct.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

maximum call stack error after upgrade 0.31.10 Nitrogen crashes with self-referential struct types (Maximum call stack size exceeded)

2 participants