refactor(formatter): `ArrowChain` avoid double-references by overlookmotel · Pull Request #18300 · oxc-project/oxc

overlookmotel · 2026-01-20T13:58:17Z

Refactor.

ArrowChain::arrows was returning an iterator of &&AstNode<ArrowFunctionExpression>>s (double references). Instead make it return an iterator of &AstNode<ArrowFunctionExpression>>s.

I came across this while replacing usage of std::ptr::eq in linter (#18249), and initially thought that the ptr::eq call here was buggy, because it was comparing double-references (&&AstNode not &AstNode):

oxc/crates/oxc_formatter/src/print/arrow_function_expression.rs

Lines 545 to 549 in f79e770

    
           // The arrow of the tail is formatted outside of the group to ensure it never 
        
           // breaks from the body 
        
           if !std::ptr::eq(arrow, tail) { 
        
               write!(f, [space(), "=>"]); 
        
           }

I was wrong about that - it's fine to compare &&AstNodes in this case because they're all stable references. But still IMO it's preferable not to be using double-references where it's avoidable. It's (to me, at least) confusing, and can be less performant due to an extra pointer-fetch on each access, if compiler doesn't manage to optimize that out.

This PR does subtly change behavior of the ptr::eq check. It will now pass if an earlier AstNode and the last AstNode are the same node (e.g. head == tail). But I assume that's impossible here.

overlookmotel · 2026-01-20T13:58:33Z

refactor(formatter): ArrowChain avoid double-references #18300 👈 (View in Graphite)
main

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

0-merge - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

codspeed-hq · 2026-01-20T14:02:31Z

CodSpeed Performance Report

Merging this PR will not alter performance

_{Comparing om/01-20-refactor_formatter_arrowchain_avoid_double-references (f79e770) with main (6803adf)}

Summary

✅ 38 untouched benchmarks
⏩ 7 skipped benchmarks¹

7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

overlookmotel · 2026-01-20T14:09:44Z

@Dunqing Please see my last comment in the PR description

This PR does subtly change behavior of the ptr::eq check. It will now pass if an earlier AstNode and the last AstNode are the same node (e.g. head == tail). But I assume that's impossible here.

If my assumption is wrong, then this PR is wrong, and we should close it.

Copilot

Pull request overview

This PR refactors the ArrowChain struct's arrows() method to return single references (&AstNode) instead of double references (&&AstNode), improving code clarity and potentially performance.

Changes:

Modified arrows() method to return &'b AstNode instead of &&'b AstNode
Updated method implementation to use .copied() on the middle iterator and removed unnecessary reference operators
Adjusted the fmt() method to destructure with *self and removed a dereference operator

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Boshen · 2026-01-20T14:17:49Z

Merge activity

Jan 20, 2:17 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Jan 20, 2:17 PM UTC: Boshen added this pull request to the Graphite merge queue.
Jan 20, 2:25 PM UTC: The Graphite merge queue couldn't merge this PR because it was in draft mode.
Jan 21, 1:15 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Jan 21, 1:15 AM UTC: Dunqing added this pull request to the Graphite merge queue.
Jan 21, 1:24 AM UTC: Merged by the Graphite merge queue.

Refactor. `ArrowChain::arrows` was returning an iterator of `&&AstNode<ArrowFunctionExpression>>`s (double references). Instead make it return an iterator of `&AstNode<ArrowFunctionExpression>>`s. I came across this while replacing usage of `std::ptr::eq` in linter (#18249), and initially thought that the `ptr::eq` call here was buggy, because it was comparing double-references (`&&AstNode` not `&AstNode`): https://github.com/oxc-project/oxc/blob/f79e770a82aee2dc3e64f7935fd7169bc1991ff9/crates/oxc_formatter/src/print/arrow_function_expression.rs#L545-L549 I was wrong about that - it's fine to compare `&&AstNode`s in this case because they're all stable references. But still IMO it's preferable not to be using double-references where it's avoidable. It's (to me, at least) confusing, and can be less performant due to an extra pointer-fetch on each access, if compiler doesn't manage to optimize that out. This PR does subtly change behavior of the `ptr::eq` check. It will now pass if an earlier `AstNode` and the last `AstNode` are the same node (e.g. `head == tail`). But I assume that's impossible here.

overlookmotel · 2026-01-20T14:26:02Z

I stopped Graphite from merging. I think this one needs @Dunqing's input before merging (#18300 (comment)). I don't know the formatter at all, so I don't want to screw it up!

Dunqing

As you said, it is impossible, as ArrowChain must have two arrow functions, so head and tail are different.

Refactor. `ArrowChain::arrows` was returning an iterator of `&&AstNode<ArrowFunctionExpression>>`s (double references). Instead make it return an iterator of `&AstNode<ArrowFunctionExpression>>`s. I came across this while replacing usage of `std::ptr::eq` in linter (#18249), and initially thought that the `ptr::eq` call here was buggy, because it was comparing double-references (`&&AstNode` not `&AstNode`): https://github.com/oxc-project/oxc/blob/f79e770a82aee2dc3e64f7935fd7169bc1991ff9/crates/oxc_formatter/src/print/arrow_function_expression.rs#L545-L549 I was wrong about that - it's fine to compare `&&AstNode`s in this case because they're all stable references. But still IMO it's preferable not to be using double-references where it's avoidable. It's (to me, at least) confusing, and can be less performant due to an extra pointer-fetch on each access, if compiler doesn't manage to optimize that out. This PR does subtly change behavior of the `ptr::eq` check. It will now pass if an earlier `AstNode` and the last `AstNode` are the same node (e.g. `head == tail`). But I assume that's impossible here.

github-actions bot added the A-formatter Area - Formatter label Jan 20, 2026

github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Jan 20, 2026

overlookmotel marked this pull request as ready for review January 20, 2026 14:07

overlookmotel requested a review from Dunqing as a code owner January 20, 2026 14:07

Copilot AI review requested due to automatic review settings January 20, 2026 14:07

overlookmotel assigned Dunqing Jan 20, 2026

Copilot started reviewing on behalf of overlookmotel January 20, 2026 14:08 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

Boshen added the 0-merge Merge with Graphite Merge Queue label Jan 20, 2026

graphite-app bot force-pushed the om/01-20-refactor_formatter_arrowchain_avoid_double-references branch from f79e770 to cac2909 Compare January 20, 2026 14:23

overlookmotel marked this pull request as draft January 20, 2026 14:24

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Jan 20, 2026

overlookmotel marked this pull request as ready for review January 20, 2026 14:26

Dunqing approved these changes Jan 21, 2026

View reviewed changes

Dunqing added the 0-merge Merge with Graphite Merge Queue label Jan 21, 2026

graphite-app bot force-pushed the om/01-20-refactor_formatter_arrowchain_avoid_double-references branch from cac2909 to 07bdea7 Compare January 21, 2026 01:18

graphite-app bot merged commit 07bdea7 into main Jan 21, 2026
21 checks passed

graphite-app bot deleted the om/01-20-refactor_formatter_arrowchain_avoid_double-references branch January 21, 2026 01:24

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Jan 21, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(formatter): `ArrowChain` avoid double-references#18300

refactor(formatter): `ArrowChain` avoid double-references#18300
graphite-app[bot] merged 1 commit intomainfrom
om/01-20-refactor_formatter_arrowchain_avoid_double-references

overlookmotel commented Jan 20, 2026 •

edited

Loading

Uh oh!

overlookmotel commented Jan 20, 2026

Uh oh!

codspeed-hq bot commented Jan 20, 2026

Uh oh!

overlookmotel commented Jan 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Boshen commented Jan 20, 2026 •

edited by graphite-app bot

Loading

Uh oh!

overlookmotel commented Jan 20, 2026 •

edited

Loading

Uh oh!

Dunqing left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	// The arrow of the tail is formatted outside of the group to ensure it never
	// breaks from the body
	if !std::ptr::eq(arrow, tail) {
	write!(f, [space(), "=>"]);
	}

Uh oh!

Conversation

overlookmotel commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

overlookmotel commented Jan 20, 2026

How to use the Graphite Merge Queue

Uh oh!

codspeed-hq bot commented Jan 20, 2026

CodSpeed Performance Report

Merging this PR will not alter performance

Summary

Footnotes

Uh oh!

overlookmotel commented Jan 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Boshen commented Jan 20, 2026 • edited by graphite-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

overlookmotel commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dunqing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

overlookmotel commented Jan 20, 2026 •

edited

Loading

Boshen commented Jan 20, 2026 •

edited by graphite-app bot

Loading

overlookmotel commented Jan 20, 2026 •

edited

Loading