Skip to content

Comments

fix(formatter): normalize ChainExpression with TSNonNullExpression to match Prettier#18061

Merged
graphite-app[bot] merged 1 commit intomainfrom
fix/formatter-chain-expression-non-null
Jan 16, 2026
Merged

fix(formatter): normalize ChainExpression with TSNonNullExpression to match Prettier#18061
graphite-app[bot] merged 1 commit intomainfrom
fix/formatter-chain-expression-non-null

Conversation

@Boshen
Copy link
Member

@Boshen Boshen commented Jan 16, 2026

Summary

Fixes the formatting of optional chain expressions combined with non-null assertions to match Prettier's output:

  • (a?.b!).c now prints as (a?.b)!.c (moves ! outside parentheses)
  • (a?.b)!.c correctly preserves parentheses
  • (a?.b)![c?.d!] correctly handles computed member access

Implementation

The fix introduces a shared chain_expression_needs_parens function that handles two AST patterns:

  1. ChainExpression > TSNonNullExpression - handled in write() by printing inner content with parens, then !
  2. TSNonNullExpression > ChainExpression - handled in needs_parentheses() by recursively checking grandparent

This matches Prettier's behavior documented in their clean.js.

Test plan

  • All 18 test cases in typescript/non-null/optional-chain.ts now pass
  • Additional chain-expression tests now pass
  • TS Prettier conformance improved from 95.39% to 96.05%
  • All 144 formatter unit tests pass

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings January 16, 2026 07:08
@Boshen Boshen requested a review from Dunqing as a code owner January 16, 2026 07:08
@github-actions github-actions bot added A-formatter Area - Formatter C-bug Category - Bug labels Jan 16, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the formatting of optional chain expressions combined with non-null assertions to match Prettier's output. The fix handles two AST patterns: ChainExpression > TSNonNullExpression and TSNonNullExpression > ChainExpression.

Changes:

  • Extracted chain_expression_needs_parens as a shared function to determine parenthesization needs
  • Updated ChainExpression::write() to manually handle parentheses when the child is a TSNonNullExpression, printing (a?.b)! instead of (a?.b!)
  • Added recursive case in chain_expression_needs_parens to handle TSNonNullExpression parents by checking the grandparent context

Reviewed changes

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

File Description
tasks/prettier_conformance/snapshots/prettier.ts.snap.md Updates conformance metrics showing improvement from 95.39% to 96.05%, with 4 previously failing test files now passing
crates/oxc_formatter/src/print/mod.rs Adds special handling in ChainExpression::write() to reorder non-null assertions outside parentheses
crates/oxc_formatter/src/parentheses/mod.rs Exports the new chain_expression_needs_parens function
crates/oxc_formatter/src/parentheses/expression.rs Extracts parenthesization logic into shared function and adds recursive handling for TSNonNullExpression parents

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

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 16, 2026

Merging this PR will not alter performance

✅ 38 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing fix/formatter-chain-expression-non-null (f1e1b25) with main (9b902b6)

Open in CodSpeed

Footnotes

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

@Boshen Boshen force-pushed the fix/formatter-chain-expression-non-null branch from 1e33052 to f1e1b25 Compare January 16, 2026 10:46
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Jan 16, 2026
Copy link
Member Author

Boshen commented Jan 16, 2026

Merge activity

…` to match Prettier (#18061)

## Summary

Fixes the formatting of optional chain expressions combined with non-null assertions to match Prettier's output:

- `(a?.b!).c` now prints as `(a?.b)!.c` (moves `!` outside parentheses)
- `(a?.b)!.c` correctly preserves parentheses
- `(a?.b)![c?.d!]` correctly handles computed member access

## Implementation

The fix introduces a shared `chain_expression_needs_parens` function that handles two AST patterns:

1. **`ChainExpression > TSNonNullExpression`** - handled in `write()` by printing inner content with parens, then `!`
2. **`TSNonNullExpression > ChainExpression`** - handled in `needs_parentheses()` by recursively checking grandparent

This matches Prettier's behavior documented in their [clean.js](https://github.com/prettier/prettier/blob/main/src/language-js/clean.js#L180-L188).

## Test plan

- [x] All 18 test cases in `typescript/non-null/optional-chain.ts` now pass
- [x] Additional chain-expression tests now pass
- [x] TS Prettier conformance improved from **95.39%** to **96.05%**
- [x] All 144 formatter unit tests pass

🤖 Generated with [Claude Code](https://claude.ai/code)
@graphite-app graphite-app bot force-pushed the fix/formatter-chain-expression-non-null branch from f1e1b25 to 3e141f0 Compare January 16, 2026 11:20
@graphite-app graphite-app bot merged commit 3e141f0 into main Jan 16, 2026
21 checks passed
@graphite-app graphite-app bot deleted the fix/formatter-chain-expression-non-null branch January 16, 2026 11:26
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Jan 16, 2026
@fisker
Copy link

fisker commented Jan 16, 2026

FYI: I plan to change this to keep AST unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-formatter Area - Formatter C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants