Skip to content

fix(formatter): Use hard_space between : and TSTypeAnnotation#20858

Closed
leaysgur wants to merge 1 commit intomainfrom
03-30-fix_formatter_place_decorator_on_own_line_with_long_property
Closed

fix(formatter): Use hard_space between : and TSTypeAnnotation#20858
leaysgur wants to merge 1 commit intomainfrom
03-30-fix_formatter_place_decorator_on_own_line_with_long_property

Conversation

@leaysgur
Copy link
Copy Markdown
Member

@leaysgur leaysgur commented Mar 30, 2026

Fixes #20519

This ensures that the print_width is calculated correctly in fits().

Copy link
Copy Markdown
Member Author

leaysgur commented Mar 30, 2026


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 changes, fast-track this PR to the front of the merge queue

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.

@github-actions github-actions bot added A-formatter Area - Formatter C-bug Category - Bug labels Mar 30, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 30, 2026

Merging this PR will not alter performance

✅ 44 untouched benchmarks
⏩ 12 skipped benchmarks1


Comparing 03-30-fix_formatter_place_decorator_on_own_line_with_long_property (02ca7b3) with main (c9cd809)

Open in CodSpeed

Footnotes

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

@leaysgur leaysgur force-pushed the 03-30-fix_formatter_place_decorator_on_own_line_with_long_property branch from 8c3a7c0 to 9d7b6e3 Compare March 30, 2026 06:49
@github-actions github-actions bot added the A-cli Area - CLI label Mar 30, 2026
@leaysgur leaysgur changed the title fix(formatter): Place decorator on own line with long property fix(formatter): Check pending space width in fits measurement Mar 30, 2026
@leaysgur leaysgur force-pushed the 03-30-fix_formatter_place_decorator_on_own_line_with_long_property branch from 9d7b6e3 to 02ca7b3 Compare March 30, 2026 07:04
@leaysgur leaysgur changed the title fix(formatter): Check pending space width in fits measurement fix(formatter): Use hard_space between : and TSTypeAnnotation Mar 30, 2026
@leaysgur leaysgur marked this pull request as ready for review March 30, 2026 07:21
@leaysgur leaysgur requested a review from Dunqing as a code owner March 30, 2026 07:21
@leaysgur
Copy link
Copy Markdown
Member Author

leaysgur commented Mar 31, 2026

I noticed something while researching hard_space().

  • It is almost never actually used
  • The only difference between Space and HardSpace in fits_element() is whether or not the width is calculated
    • Both are discarded when a line break occurs
    • I don't know why Space doesn't calculate width in the first place
  • The issue could also be resolved by changing Space to calculate width just like HardSpace
  • Incidentally, HardSpace does not exist in the Ruff implementation

Honestly, I can't think of any reason to put trailing spaces at the end of a line...🤔

@Dunqing
Copy link
Copy Markdown
Member

Dunqing commented Mar 31, 2026

I will take a look soon! For HardSpace, you can take a look at #16708 (comment)

Copy link
Copy Markdown
Member

@Dunqing Dunqing left a comment

Choose a reason for hiding this comment

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

LGTM

@leaysgur
Copy link
Copy Markdown
Member Author

leaysgur commented Apr 1, 2026

@Dunqing How do you think about:

  • Just remove HardSpace
  • Calculate fits width for Space

?

@Dunqing
Copy link
Copy Markdown
Member

Dunqing commented Apr 2, 2026

@Dunqing How do you think about:

  • Just remove HardSpace
  • Calculate fits width for Space

?

It is indeed a bug; I sent a PR to fix it instead of working around it. #20858

@leaysgur
Copy link
Copy Markdown
Member Author

leaysgur commented Apr 2, 2026

Closing, at the very least, this PR is useless now.

@leaysgur leaysgur closed this Apr 2, 2026
@leaysgur leaysgur deleted the 03-30-fix_formatter_place_decorator_on_own_line_with_long_property branch April 2, 2026 08:05
graphite-app bot pushed a commit that referenced this pull request Apr 2, 2026
…d-mode early exit (#20954)

Closes #20519
Alternative to #20858

## Summary

The `fits()` measurer in the printer defers `Space` elements via a `pending_space` boolean flag. When the measurer exits early through an expanded-mode line break (`Fits::Yes`), any unresolved `pending_space` is silently lost — causing the measured width to be off by one.

This caused the decorator in `@property(...) prop: Type` to incorrectly stay on the same line because the space after `:` was not counted in the width measurement.

### Root cause

In `FitsMeasurer::fits_element`, `Space` sets `pending_space = true` (deferred), while `HardSpace` immediately increments `line_width`. When `fits()` returns `Fits::Yes` via the expanded-mode line break path (line 1040), the pending space is never resolved.

For the decorator case:
1. After `@property(...)` + space + `tooltipPlacement` + `:` → `line_width = 80`
2. `Space` after `:` → `pending_space = true` (line_width stays 80)
3. Union type group inherits Expanded mode → `if_group_breaks(separator)` included
4. Separator's `SoftOrSpace` in Expanded mode → `return Fits::Yes` **without resolving pending_space**
5. The +1 char from the space is lost, making the decorator group incorrectly "fit"

### Fix

Resolve `pending_space` before returning `Fits::Yes` from the expanded-mode early exit path. This matches Ruff's approach where `Space` is counted eagerly via `fits_text(Text::Token(" "))`.

### Conformance

- JS: 746/753 (unchanged)
- TS: 591/601 (unchanged — `18148.ts` fixed, `comment.ts` regressed)

The `comment.ts` regression (`typescript/union/consistent-with-flow/comment.ts`) is a **pre-existing Prettier bug** — at `printWidth: 81`, Prettier itself produces the same double-indented output with a spurious blank line:

```ts
// printWidth: 81 → Prettier also double-indents:
type SuperLong...Blaa =
                         ← spurious blank line
    | Fooo1000           ← 4 spaces (double indent)
```

At exactly 80 chars, Prettier avoids this only because its own fits check has the same off-by-one (the space is not counted), causing the Fluid layout's inner group to accidentally "fit".

## Test plan

- [x] `cargo test -p oxc_formatter` — 262 passed
- [x] `cargo test -p oxc_formatter -- fixtures` — 213 passed (including new `issue-20519.ts`)
- [x] `cargo run -p oxc_prettier_conformance` — JS 746/753, TS 591/601
- [x] `cargo clippy -p oxc_formatter` — clean

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

formatter: Diff with Prettier on decorator with union type

2 participants