refactor(formatter): make `Format::fmt` non-failable by Dunqing · Pull Request #16036 · oxc-project/oxc

Dunqing · 2025-11-24T08:36:18Z

This PR is a follow-up to astral-sh/ruff#6613.

Why can we do it

oxc/crates/oxc_formatter/src/formatter/diagnostics.rs

Lines 11 to 29 in 622e000

    
           pub enum FormatError { 
        
               /// In case a node can't be formatted because it either misses a require child element or 
        
               /// a child is present that should not (e.g. a trailing comma after a rest element). 
        
               SyntaxError, 
        
               /// In case range formatting failed because the provided range was larger 
        
               /// than the formatted syntax tree 
        
               RangeError { input: TextRange, tree: TextRange }, 
        
               /// In case printing the document failed because it has an invalid structure. 
        
               InvalidDocument(InvalidDocumentError), 
        
               /// Formatting failed because some content encountered a situation where a layout 
        
               /// choice by an enclosing [crate::Format] resulted in a poor layout for a child [crate::Format]. 
        
               /// 
        
               /// It's up to an enclosing [crate::Format] to handle the error and pick another layout. 
        
               /// This error should not be raised if there's no outer [crate::Format] handling the poor layout error, 
        
               /// avoiding that formatting of the whole document fails. 
        
               PoorLayout, 
        
           }

We can also make it non-failable since the entire program only has two points that can trigger an error, neither of which needs to interrupt the program immediately, and both can be improved to avoid making FormatResult everywhere

FormatError::PoorLayout

This error may occur only when handling call arguments, and only the first or the last argument is a Function or ArrowFunctionExpression. The error may be thrown during the formatting of them. I commented out the related logic of this in this PR and fixed it in #16093 to make it easier to review

FormatError::InvalidDocument

This is a printer error, but it may also occur only when we format TaggedTemplateExpression. There is a formatting logic that will format the template expression and print the IR immediately. If the IR we generated is incorrect, then the error will occur, but this is quite rare! So it's not worth using FormatResult everywhere to handle it. Currently, I just unwrap it. If we really need to handle it, we can store the error somewhere and return it after formatting in the future.

oxc/crates/oxc_formatter/src/write/template.rs

Lines 602 to 603 in 78cd591

    
           // TODO: if `unwrap()` panics here, it's a internal error 
        
           let printed = Printer::new(print_options).print(&root).unwrap();

Key changes

The most significant change is:

pub trait Format<'ast, T = ()> {
    - fn fmt(&self, f: &mut Formatter<'_, 'ast>) -> FormatResult<()>;
    + fn fmt(&self, f: &mut Formatter<'_, 'ast>);
}

All remaining changes to the order stem from the above change:

Remove all FormatResult usages
Remove all ? operators that pair with the format! macro
Commented out PoorLayout related logic, which is going to be fixed in the fix(formatter): handle poor layout for grouped call arguments #16093

Benefits

We don't need to deal with Option, and removing all Option handling makes the code cleaner and more readable. Of course, the performance improved a little bit

Dunqing · 2025-11-24T08:36:35Z

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 · 2025-11-24T08:42:20Z

CodSpeed Performance Report

Merging #16036 will improve performances by 5.36%

_{Comparing 11-24-refactor_formatter_make_format_trait_non-failable (cb8c1fd) with 11-24-refactor_formatter_adjust_implementations_of_function_and_arrowfunctionexpression (e1f3d4d)}

Summary

⚡ 9 improvements
✅ 29 untouched
⏩ 7 skipped¹

Benchmarks breakdown

	Mode	Benchmark	`BASE`	`HEAD`	Change
⚡	Simulation	`formatter[App.tsx]`	51.9 ms	50.2 ms	+3.52%
⚡	Simulation	`formatter[RadixUIAdoptionSection.jsx]`	483.7 µs	459.8 µs	+5.19%
⚡	Simulation	`formatter[Search.tsx]`	2 ms	1.9 ms	+4.31%
⚡	Simulation	`formatter[core.js]`	2.3 ms	2.2 ms	+3.75%
⚡	Simulation	`formatter[errors.ts]`	808.7 µs	767.5 µs	+5.36%
⚡	Simulation	`formatter[handle-comments.js]`	3.7 ms	3.6 ms	+3.41%
⚡	Simulation	`formatter[index.tsx]`	4.8 ms	4.6 ms	+3.94%
⚡	Simulation	`formatter[next.ts]`	2.9 ms	2.8 ms	+4.35%
⚡	Simulation	`formatter[types.ts]`	15.8 ms	15.3 ms	+3.72%

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. ↩

graphite-app · 2025-11-25T09:50:33Z

Merge activity

Nov 25, 9:50 AM UTC: leaysgur added this pull request to the Graphite merge queue.
Nov 25, 9:54 AM UTC: The Graphite merge queue couldn't merge this PR because it had merge conflicts.
Nov 25, 11:12 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Nov 25, 11:12 AM UTC: This pull request can not be added to the Graphite merge queue. Please try rebasing and resubmitting to merge when ready.
Nov 25, 11:12 AM UTC: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
Nov 25, 11:12 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Nov 25, 3:11 PM UTC: This pull request can not be added to the Graphite merge queue. Please try rebasing and resubmitting to merge when ready.
Nov 25, 3:12 PM UTC: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
Nov 25, 3:12 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Nov 25, 3:12 PM UTC: This pull request can not be added to the Graphite merge queue. Please try rebasing and resubmitting to merge when ready.
Nov 25, 3:12 PM UTC: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
Nov 25, 3:12 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Nov 25, 3:35 PM UTC: Dunqing added this pull request to the Graphite merge queue.
Nov 25, 3:40 PM UTC: Merged by the Graphite merge queue.

Copilot

Pull request overview

This PR refactors the oxc formatter to make Format::fmt non-failable by removing the FormatResult<()> return type and changing it to return (). The change is based on the observation that only two error cases existed in the codebase (PoorLayout and InvalidDocument), neither of which truly required failing immediately.

Key Changes:

Removed FormatResult return types from all Format::fmt and FormatWrite::write implementations
Removed ? operators from all write!, .fmt(), and formatting calls
Commented out PoorLayout error handling logic (to be fixed in #16093)
Changed InvalidDocument handling to use unwrap() in template formatting

Trade-offs:
The PR notes a reduction in prettier compatibility scores (TypeScript: 94.85% → 94.68%, JavaScript: 96.05% → 95.26%) due to temporarily commenting out the PoorLayout logic. This will be addressed in a follow-up PR (#16093).

Reviewed changes

Copilot reviewed 71 out of 73 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tasks/prettier_conformance/snapshots/*.snap.md	Compatibility score reductions due to commented-out PoorLayout logic
tasks/ast_tools/src/generators/formatter/*.rs	Updated code generators to produce non-failable formatting code
crates/oxc_formatter/src/write/*.rs	Removed FormatResult returns and ? operators from all write implementations
crates/oxc_formatter/src/utils/*.rs	Removed FormatResult returns from utility formatting functions
crates/oxc_formatter/src/lib.rs	Removed unwrap() after format call (now non-failable)

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

This PR is a follow-up to astral-sh/ruff#6613. ### Why can we do it https://github.com/oxc-project/oxc/blob/622e000934d97ade5588f1e8cc3badde96e7328e/crates/oxc_formatter/src/formatter/diagnostics.rs#L11-L29 We can also make it non-failable since the entire program only has two points that can trigger an error, neither of which needs to interrupt the program immediately, and both can be improved to avoid making FormatResult everywhere 1. `FormatError::PoorLayout` This error may occur only when handling call arguments, and only the first or the last argument is a `Function` or `ArrowFunctionExpression`. The error may be thrown during the formatting of them. I commented out the related logic of this in this PR and fixed it in #16093 to make it easier to review 2. `FormatError::InvalidDocument` This is a printer error, but it may also occur only when we format `TaggedTemplateExpression`. There is a formatting logic that will format the template expression and print the IR immediately. If the IR we generated is incorrect, then the error will occur, but this is quite rare! So it's not worth using `FormatResult` everywhere to handle it. Currently, I just `unwrap` it. If we really need to handle it, we can store the error somewhere and return it after formatting in the future. https://github.com/oxc-project/oxc/blob/78cd5910985cece594e74884717f77ad75347b8a/crates/oxc_formatter/src/write/template.rs#L602-L603 ### Key changes The most significant change is: ```diff pub trait Format<'ast, T = ()> { - fn fmt(&self, f: &mut Formatter<'_, 'ast>) -> FormatResult<()>; + fn fmt(&self, f: &mut Formatter<'_, 'ast>); } ``` All remaining changes to the order stem from the above change: 1. Remove all `FormatResult` usages 2. Remove all `?` operators that pair with the `format!` macro 4. Commented out `PoorLayout` related logic, which is going to be fixed in the #16093 ### Benefits We don't need to deal with `Option`, and removing all `Option` handling makes the code cleaner and more readable. Of course, the performance improved a little bit

This PR is a follow-up on #16036 Move `PoorLayout` handling from `Function` and `ArrowFunctionExpression` to call arguments

This PR is a follow-up to astral-sh/ruff#6613. ### Why can we do it https://github.com/oxc-project/oxc/blob/622e000934d97ade5588f1e8cc3badde96e7328e/crates/oxc_formatter/src/formatter/diagnostics.rs#L11-L29 We can also make it non-failable since the entire program only has two points that can trigger an error, neither of which needs to interrupt the program immediately, and both can be improved to avoid making FormatResult everywhere 1. `FormatError::PoorLayout` This error may occur only when handling call arguments, and only the first or the last argument is a `Function` or `ArrowFunctionExpression`. The error may be thrown during the formatting of them. I commented out the related logic of this in this PR and fixed it in #16093 to make it easier to review 2. `FormatError::InvalidDocument` This is a printer error, but it may also occur only when we format `TaggedTemplateExpression`. There is a formatting logic that will format the template expression and print the IR immediately. If the IR we generated is incorrect, then the error will occur, but this is quite rare! So it's not worth using `FormatResult` everywhere to handle it. Currently, I just `unwrap` it. If we really need to handle it, we can store the error somewhere and return it after formatting in the future. https://github.com/oxc-project/oxc/blob/78cd5910985cece594e74884717f77ad75347b8a/crates/oxc_formatter/src/write/template.rs#L602-L603 ### Key changes The most significant change is: ```diff pub trait Format<'ast, T = ()> { - fn fmt(&self, f: &mut Formatter<'_, 'ast>) -> FormatResult<()>; + fn fmt(&self, f: &mut Formatter<'_, 'ast>); } ``` All remaining changes to the order stem from the above change: 1. Remove all `FormatResult` usages 2. Remove all `?` operators that pair with the `format!` macro 4. Commented out `PoorLayout` related logic, which is going to be fixed in the #16093 ### Benefits We don't need to deal with `Option`, and removing all `Option` handling makes the code cleaner and more readable. Of course, the performance improved a little bit

This PR is a follow-up on #16036 Move `PoorLayout` handling from `Function` and `ArrowFunctionExpression` to call arguments

This PR is a follow-up to astral-sh/ruff#6613. ### Why can we do it https://github.com/oxc-project/oxc/blob/622e000934d97ade5588f1e8cc3badde96e7328e/crates/oxc_formatter/src/formatter/diagnostics.rs#L11-L29 We can also make it non-failable since the entire program only has two points that can trigger an error, neither of which needs to interrupt the program immediately, and both can be improved to avoid making FormatResult everywhere 1. `FormatError::PoorLayout` This error may occur only when handling call arguments, and only the first or the last argument is a `Function` or `ArrowFunctionExpression`. The error may be thrown during the formatting of them. I commented out the related logic of this in this PR and fixed it in oxc-project#16093 to make it easier to review 2. `FormatError::InvalidDocument` This is a printer error, but it may also occur only when we format `TaggedTemplateExpression`. There is a formatting logic that will format the template expression and print the IR immediately. If the IR we generated is incorrect, then the error will occur, but this is quite rare! So it's not worth using `FormatResult` everywhere to handle it. Currently, I just `unwrap` it. If we really need to handle it, we can store the error somewhere and return it after formatting in the future. https://github.com/oxc-project/oxc/blob/78cd5910985cece594e74884717f77ad75347b8a/crates/oxc_formatter/src/write/template.rs#L602-L603 ### Key changes The most significant change is: ```diff pub trait Format<'ast, T = ()> { - fn fmt(&self, f: &mut Formatter<'_, 'ast>) -> FormatResult<()>; + fn fmt(&self, f: &mut Formatter<'_, 'ast>); } ``` All remaining changes to the order stem from the above change: 1. Remove all `FormatResult` usages 2. Remove all `?` operators that pair with the `format!` macro 4. Commented out `PoorLayout` related logic, which is going to be fixed in the oxc-project#16093 ### Benefits We don't need to deal with `Option`, and removing all `Option` handling makes the code cleaner and more readable. Of course, the performance improved a little bit

…oject#16093) This PR is a follow-up on oxc-project#16036 Move `PoorLayout` handling from `Function` and `ArrowFunctionExpression` to call arguments

github-actions bot added A-ast-tools Area - AST tools A-formatter Area - Formatter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Nov 24, 2025

This was referenced Nov 24, 2025

fix(formatter): incorrect formatting method chain with trailing comments #16027

Merged

fix(formatter): revert FormatElement::BestFitting printing logic #16028

Merged

This was referenced Nov 24, 2025

refactor(formatter): replace format_once with format_with where possible #16031

Merged

refactor(formatter): adjust implementations of Function and ArrowFunctionExpression #16035

Merged

graphite-app bot force-pushed the 11-24-refactor_formatter_adjust_implementations_of_function_and_arrowfunctionexpression branch 2 times, most recently from 76fddc3 to b478d7f Compare November 24, 2025 09:09

graphite-app bot force-pushed the 11-24-refactor_formatter_make_format_trait_non-failable branch from adf4922 to d4d6801 Compare November 24, 2025 09:09

Dunqing force-pushed the 11-24-refactor_formatter_adjust_implementations_of_function_and_arrowfunctionexpression branch from b478d7f to e1f3d4d Compare November 25, 2025 06:21

Dunqing force-pushed the 11-24-refactor_formatter_make_format_trait_non-failable branch from d4d6801 to b821907 Compare November 25, 2025 06:21

This was referenced Nov 25, 2025

fix(formatter): handle poor layout for grouped call arguments #16093

Merged

formatter: backport FormatElement performance improvements from Ruff #16022

Closed

Dunqing force-pushed the 11-24-refactor_formatter_make_format_trait_non-failable branch from b821907 to 622e000 Compare November 25, 2025 08:04

Dunqing changed the title ~~refactor(formatter): make Format trait non-failable~~ refactor(formatter): make Format::fmt non-failable Nov 25, 2025

Dunqing force-pushed the 11-24-refactor_formatter_make_format_trait_non-failable branch 2 times, most recently from 78cd591 to cb8c1fd Compare November 25, 2025 09:10

leaysgur marked this pull request as ready for review November 25, 2025 09:50

leaysgur requested a review from overlookmotel as a code owner November 25, 2025 09:50

graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Nov 25, 2025

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Nov 25, 2025

graphite-app bot changed the base branch from 11-24-refactor_formatter_adjust_implementations_of_function_and_arrowfunctionexpression to graphite-base/16036 November 25, 2025 09:55

Boshen approved these changes Nov 25, 2025

View reviewed changes

Boshen added the 0-merge Merge with Graphite Merge Queue label Nov 25, 2025

Dunqing force-pushed the graphite-base/16036 branch from e1f3d4d to 2da2fc1 Compare November 25, 2025 11:58

Dunqing force-pushed the 11-24-refactor_formatter_make_format_trait_non-failable branch from cb8c1fd to 531dce4 Compare November 25, 2025 11:58

Dunqing changed the base branch from graphite-base/16036 to main November 25, 2025 11:58

Copilot AI review requested due to automatic review settings November 25, 2025 15:10

Dunqing force-pushed the 11-24-refactor_formatter_make_format_trait_non-failable branch from 531dce4 to 9c2ec78 Compare November 25, 2025 15:10

Copilot started reviewing on behalf of Dunqing November 25, 2025 15:10 View session

Copilot finished reviewing on behalf of Dunqing November 25, 2025 15:11

Copilot AI reviewed Nov 25, 2025

View reviewed changes

Dunqing force-pushed the 11-24-refactor_formatter_make_format_trait_non-failable branch from 9c2ec78 to a73b116 Compare November 25, 2025 15:29

graphite-app bot force-pushed the 11-24-refactor_formatter_make_format_trait_non-failable branch from a73b116 to 39fd864 Compare November 25, 2025 15:35

graphite-app bot merged commit 39fd864 into main Nov 25, 2025
20 checks passed

graphite-app bot deleted the 11-24-refactor_formatter_make_format_trait_non-failable branch November 25, 2025 15:40

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Nov 25, 2025

graphite-app bot pushed a commit that referenced this pull request Nov 26, 2025

fix(formatter): handle poor layout for grouped call arguments (#16093)

889d2e7

This PR is a follow-up on #16036 Move `PoorLayout` handling from `Function` and `ArrowFunctionExpression` to call arguments

leaysgur pushed a commit that referenced this pull request Nov 26, 2025

fix(formatter): handle poor layout for grouped call arguments (#16093)

de35d91

This PR is a follow-up on #16036 Move `PoorLayout` handling from `Function` and `ArrowFunctionExpression` to call arguments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

refactor(formatter): make `Format::fmt` non-failable#16036

refactor(formatter): make `Format::fmt` non-failable#16036
graphite-app[bot] merged 1 commit intomainfrom
11-24-refactor_formatter_make_format_trait_non-failable

Dunqing commented Nov 24, 2025 •

edited

Loading

Uh oh!

Dunqing commented Nov 24, 2025 •

edited

Loading

Uh oh!

codspeed-hq bot commented Nov 24, 2025 •

edited

Loading

Uh oh!

graphite-app bot commented Nov 25, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	pub enum FormatError {
	/// In case a node can't be formatted because it either misses a require child element or
	/// a child is present that should not (e.g. a trailing comma after a rest element).
	SyntaxError,
	/// In case range formatting failed because the provided range was larger
	/// than the formatted syntax tree
	RangeError { input: TextRange, tree: TextRange },

	/// In case printing the document failed because it has an invalid structure.
	InvalidDocument(InvalidDocumentError),

	/// Formatting failed because some content encountered a situation where a layout
	/// choice by an enclosing [crate::Format] resulted in a poor layout for a child [crate::Format].
	///
	/// It's up to an enclosing [crate::Format] to handle the error and pick another layout.
	/// This error should not be raised if there's no outer [crate::Format] handling the poor layout error,
	/// avoiding that formatting of the whole document fails.
	PoorLayout,
	}

	// TODO: if `unwrap()` panics here, it's a internal error
	let printed = Printer::new(print_options).print(&root).unwrap();

Uh oh!

Comments

Conversation

Dunqing commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why can we do it

Key changes

Benefits

Uh oh!

Dunqing commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

codspeed-hq bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #16036 will improve performances by 5.36%

Summary

Benchmarks breakdown

Footnotes

Uh oh!

graphite-app bot commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Dunqing commented Nov 24, 2025 •

edited

Loading

Dunqing commented Nov 24, 2025 •

edited

Loading

codspeed-hq bot commented Nov 24, 2025 •

edited

Loading

graphite-app bot commented Nov 25, 2025 •

edited

Loading