fix(formatter): resolve pending space in fits measurer before expanded-mode early exit#20954
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
The fix itself LGTM, thanks!
BTW, May I ask:
- Why do we need
pending_spaceat all? - Is
HardSpacestill needed?
Asked Claude and got answer like:
- 1: Ruff resolves
Spaceimmediately viafits_text(Text::Token(" ")) / print_text(Text::Token(" ")), which eliminates this entire class of bugs (missed resolution on early-return paths).- I looked into whether there's a technical reason oxc can't do the same, but couldn't find one —
trim_trailing_ascii_whitespace()is already called at line breaks regardless, so the deferred approach doesn't save anything
- I looked into whether there's a technical reason oxc can't do the same, but couldn't find one —
- 2: It doesn't exist in Ruff too and its current implementation seems inconsistent
- In the Printer, it's handled identically to
Space(pending_space = true), but inFitsMeasurerit does an immediateline_width += 1without resolvingpending_indent - If we switched to ruff's immediate-resolution approach, both
SpaceandHardSpacecould just callfits_text(" ") / print_text(" "), removing the inconsistency and the need for a separate element
- In the Printer, it's handled identically to
Some related issues/PRs in Prettier require a pending space to solve.
In the early days, I found a few bugs in Prettier, but that doesn't happen in Biome. I think Biome has noticed these bugs and taken a different approach. I am not sure why Ruff doesn't need It's fine to follow Ruff if no tests fail, and removing |
Merge activity
|
…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)
a6aa1d4 to
d10df39
Compare
Yes, that's exactly the point. I want to avoid having to decide "which one to use?" in the future. I'll look into deleting it. 💪🏻 |
Refs: #20954 (comment) `HardSpace` was only used in one place in `oxc_formatter`: `TSTypeOperator` formatting (`print/mod.rs`). Replacing it with `Space` caused no issues. Biome also uses `hard_space` (via `soft_line_indent_or_hard_space`) in only one place in JS formatting. For arrow functions with conditional expression bodies. > https://github.com/biomejs/biome/blob/main/crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs#L156 However, `oxc_formatter` already uses `soft_line_indent_or_space` (not `hard_space`) for that same case, with no issues. --- I also attempted to align the `Space` handling with Ruff's approach, resolving `Space` immediately via `print_text(" ")` / `fits_text(" ")` instead of deferring it through `pending_space`. But this caused significant conformance regressions. 😓 The root cause appears to be that `oxc_formatter`'s IR generation is less strict about where `Space` elements appear (e.g., trailing spaces in `line_suffix`), relying on the Printer's deferred `pending_space` mechanism to silently discard unresolved spaces. So, leave it for now.
# Oxlint ### 💥 BREAKING CHANGES - 22ce6af oxlint/lsp: [**BREAKING**] Show/fix safe suggestions by default (#19816) (Sysix) ### 🚀 Features - 7a7b7b8 oxlint/lsp: Add source.fixAllDangerous.oxc code action kind (#20526) (bab) - 9cfe57e linter/unicorn: Implement prefer-import-meta-properties rule (#20662) (Irfan - ئىرفان) - 1edb391 linter/eslint: Implement `no-restricted-exports` rule (#20592) (Nicolas Le Cam) - 0f12bcd linter/react: Implement `hook-use-state` rule (#20986) (Khaled Labeb) - 1513a9f oxlint/lsp: Show note field for lsp diagnostic (#20983) (Sysix) - 7fdf722 linter/unicorn: Implement `no-useless-iterator-to-array` rule (#20945) (Mikhail Baev) - 39c8f2c linter/jest: Implement padding-around-after-all-blocks (#21034) (Sapphire) - ac39e51 linter/eslint-vitest-plugin: Prefer importing vitest globals (#20960) (Said Atrahouch) - 0b84de1 oxlint: Support allow option for prefer-promise-reject-errors (#20934) (camc314) - 23db851 linter/consistent-return: Move rule from nursery to suspicious (#20920) (camc314) - 9a27e32 linter/no-unnecessary-type-conversion: Move rule from nursery to suspicious (#20919) (camc314) - 1ca7b58 linter/dot-notation: Move rule from nursery to style (#20918) (camc314) - 73ba81a linter/consistent-type-exports: Move rule from nursery to style (#20917) (camc314) - b9199b1 linter/unicorn: Implement switch-case-break-position (#20872) (Mikhail Baev) - 3435ff8 linter: Implements `prefer-snapshot-hint` rule in Jest and Vitest (#20870) (Said Atrahouch) - 98510d2 linter: Implement react/prefer-function-component (#19652) (Connor Shea) - 871f9d9 linter: Implement no-useless-assignment (#15466) (Zhaoting Zhou) - 0f01fbd linter: Implement eslint/object-shorthand (#17688) (yue) ### 🐛 Bug Fixes - dd2df87 npm: Export package.json for oxlint and oxfmt (#20784) (kazuya kawaguchi) - 9bc77dd linter/no-unused-private-class-members: False positive with await expr (#21067) (camc314) - 60a57cd linter/const-comparisons: Detect equality contradictions (#21065) (camc314) - 2bb2be2 linter/no-array-index-key: False positive when index is passed as function argument (#21012) (bab) - 6492953 linter/no-this-in-sfc: Only flag `this` used as member expression object (#20961) (bab) - 9446dcc oxlint/lsp: Skip `node_modules` in oxlint config walker (#21004) (copilot-swe-agent) - af89923 linter/no-namespace: Support glob pattern matching against basename (#21031) (bab) - 64a1a7e oxlint: Don't search for nested config outside base config (#21051) (Sysix) - 3b953bc linter/button-has-type: Ignore `document.createElement` calls (#21008) (Said Atrahouch) - 8c36070 linter/unicorn: Add support for `Array.from()` for `prefer-set-size` rule (#21016) (Mikhail Baev) - c1a48f0 linter: Detect vitest import from vite-plus/test (#20976) (Said Atrahouch) - 5c32fd1 lsp: Prevent corrupted autofix output from overlapping text edits (#19793) (Peter Wagenet) - ca79960 linter/no-array-index-key: Move span to `key` property (#20947) (camc314) - 2098274 linter: Add suggestion for `jest/prefer-equality-matcher` (#20925) (eryue0220) - 6eb77ec linter: Allow default-import barrels in import/named (#20757) (Bazyli Brzóska) - 9c218ef linter/eslint-vitest-plugin: Remove pending fix status for require-local-test-context-for-concurrent-snapshot (#20890) (Said Atrahouch) ### ⚡ Performance - fb52383 napi/parser, linter/plugins: Clear buffers and source texts earlier (#21025) (overlookmotel) - 3b7dec4 napi/parser, linter/plugins: Use `utf8Slice` for decoding UTF-8 strings (#21022) (overlookmotel) - 012c924 napi/parser, linter/plugins: Speed up decoding strings in raw transfer (#21021) (overlookmotel) - 55e1e9b napi/parser, linter/plugins: Initialize vars as 0 (#21020) (overlookmotel) - c25ef02 napi/parser, linter/plugins: Simplify branch condition in `deserializeStr` (#21019) (overlookmotel) - 9f494c3 napi/parser, linter/plugins: Raw transfer use `String.fromCharCode` in string decoding (#21018) (overlookmotel) - 0503a78 napi/parser, linter/plugins: Faster deserialization of `raw` fields (#20923) (overlookmotel) - a24f75e napi/parser: Optimize string deserialization for non-ASCII sources (#20834) (Joshua Tuddenham) ### 📚 Documentation - af72b80 oxlint: Fix typo for --tsconfig (#20889) (leaysgur) - 70c53b1 linter: Highlight that tsconfig is not respected in type aware linting (#20884) (camc314) # Oxfmt ### 🚀 Features - 35cf6e8 oxfmt: Add node version hint for ts config import failures (#21046) (camc314) ### 🐛 Bug Fixes - dd2df87 npm: Export package.json for oxlint and oxfmt (#20784) (kazuya kawaguchi) - 9d45511 oxfmt: Propagate file write errors instead of panicking (#20997) (leaysgur) - 139ddd9 formatter: Handle leading comment after array elision (#20987) (leaysgur) - 4216380 oxfmt: Support `.editorconfig` `tab_width` fallback (#20988) (leaysgur) - d10df39 formatter: Resolve pending space in fits measurer before expanded-mode early exit (#20954) (Dunqing) - f9ef1bd formatter: Avoid breaking after `=>` when arrow body has JSDoc type cast (#20857) (bab) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Closes #20519
Alternative to #20858
Summary
The
fits()measurer in the printer defersSpaceelements via apending_spaceboolean flag. When the measurer exits early through an expanded-mode line break (Fits::Yes), any unresolvedpending_spaceis silently lost — causing the measured width to be off by one.This caused the decorator in
@property(...) prop: Typeto incorrectly stay on the same line because the space after:was not counted in the width measurement.Root cause
In
FitsMeasurer::fits_element,Spacesetspending_space = true(deferred), whileHardSpaceimmediately incrementsline_width. Whenfits()returnsFits::Yesvia the expanded-mode line break path (line 1040), the pending space is never resolved.For the decorator case:
@property(...)+ space +tooltipPlacement+:→line_width = 80Spaceafter:→pending_space = true(line_width stays 80)if_group_breaks(separator)includedSoftOrSpacein Expanded mode →return Fits::Yeswithout resolving pending_spaceFix
Resolve
pending_spacebefore returningFits::Yesfrom the expanded-mode early exit path. This matches Ruff's approach whereSpaceis counted eagerly viafits_text(Text::Token(" ")).Conformance
18148.tsfixed,comment.tsregressed)The
comment.tsregression (typescript/union/consistent-with-flow/comment.ts) is a pre-existing Prettier bug — atprintWidth: 81, Prettier itself produces the same double-indented output with a spurious blank line: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
cargo test -p oxc_formatter— 262 passedcargo test -p oxc_formatter -- fixtures— 213 passed (including newissue-20519.ts)cargo run -p oxc_prettier_conformance— JS 746/753, TS 591/601cargo clippy -p oxc_formatter— clean🤖 Generated with Claude Code