fix(macros): detect circular references and report clear error #26486
+181
−16
Conversation
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
Collaborator
Author
Contributor
WalkthroughAdds sentinel-based circular-reference detection during macro coercion for arrays and objects; defers creation of final Expr nodes until after all items/properties are processed; returns a circular-reference error when a cycle is detected. New regression tests verify behavior for direct, indirect, and array cycles and for non-circular nesting. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/regression/issue/22552.test.ts`:
- Around line 32-36: The test captures stdout into the unused variable `stdout`
via Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) — remove
the unused `stdout` element from the array and corresponding destructuring, or
alternatively add an explicit assertion (e.g., expect(stdout).toBe('') or
expect(stdout).toHaveLength(0)) to document that nothing was written to stdout;
update the destructuring that currently references `stdout`, `stderr`, and
`exitCode` so it matches the chosen approach and keep the retrieval of
`proc.stderr.text()` and `proc.exited` unchanged.
- Around line 125-136: The test currently only asserts exitCode after spawning
the build process via Bun.spawn (proc) and awaiting proc.stdout.text(),
proc.stderr.text(), and proc.exited; update the test to assert on stdout and/or
stderr contents before or alongside the exitCode to provide better failure
messages and verify build output: read the captured stdout/stderr strings (from
the variables stdout and stderr) and add expectations that stdout contains the
expected build success markers (or that stderr is empty/does not contain error
text) prior to asserting expect(exitCode).toBe(0), referencing proc, stdout,
stderr, and exitCode to locate the relevant code.
Comment on lines
125
to
137
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "build", "./main.ts", "--outdir", "./out"], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| // Non-circular structures should work fine | ||
| expect(exitCode).toBe(0); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider verifying build output or adding stdout/stderr assertions.
Per coding guidelines, for tests expecting success, checking output before exit code provides more useful error messages on failure. The test could verify the build produced valid output.
♻️ Suggested enhancement for better debugging on failure
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
// Non-circular structures should work fine
+ expect(stderr).toBe("");
expect(exitCode).toBe(0);📝 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
| await using proc = Bun.spawn({ | |
| cmd: [bunExe(), "build", "./main.ts", "--outdir", "./out"], | |
| env: bunEnv, | |
| cwd: String(dir), | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| // Non-circular structures should work fine | |
| expect(exitCode).toBe(0); | |
| await using proc = Bun.spawn({ | |
| cmd: [bunExe(), "build", "./main.ts", "--outdir", "./out"], | |
| env: bunEnv, | |
| cwd: String(dir), | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| // Non-circular structures should work fine | |
| expect(stderr).toBe(""); | |
| expect(exitCode).toBe(0); |
🤖 Prompt for AI Agents
In `@test/regression/issue/22552.test.ts` around lines 125 - 136, The test
currently only asserts exitCode after spawning the build process via Bun.spawn
(proc) and awaiting proc.stdout.text(), proc.stderr.text(), and proc.exited;
update the test to assert on stdout and/or stderr contents before or alongside
the exitCode to provide better failure messages and verify build output: read
the captured stdout/stderr strings (from the variables stdout and stderr) and
add expectations that stdout contains the expected build success markers (or
that stderr is empty/does not contain error text) prior to asserting
expect(exitCode).toBe(0), referencing proc, stdout, stderr, and exitCode to
locate the relevant code.
When a macro returns an object with a circular reference (either direct like `obj.self = obj` or indirect like `a.ref = b; b.ref = a`), Bun would crash with a segmentation fault due to infinite recursion in the AST visitor. This fix adds proper cycle detection during JavaScript-to-AST conversion: - Use a sentinel value (Expr.empty) to mark objects being processed - When encountering a value already being processed, emit a clear error instead of allowing circular AST nodes to be created - Move expression creation to after all children are processed Fixes #22552 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
robobun
force-pushed
the
claude/fix-macro-circular-reference-crash
branch
from
f19e9e5 to
ed440fb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes #22552
Root Cause
When a macro returned an object with a circular reference, Bun would crash with a segmentation fault. This happened because:
Solution
Use a sentinel value (
Expr.emptywithe_missingdata) to mark objects being processed:This prevents circular AST nodes from being created and provides a helpful error message.
Test plan
obj.self = obj)a.ref = b; b.ref = a)🤖 Generated with Claude Code