Skip to content

Comments

refactor(formatter): memoize text width#7983

Draft
denbezrukov wants to merge 2 commits intomainfrom
dbezrukov/text-width-mem
Draft

refactor(formatter): memoize text width#7983
denbezrukov wants to merge 2 commits intomainfrom
dbezrukov/text-width-mem

Conversation

@denbezrukov
Copy link
Contributor

Summary

Port of astral-sh/ruff#6552

This change introduces a new TextWidth field on all text FormatElements, which stores either the precomputed width of the text or indicates that it is multiline.

Test Plan

Green CI

Docs

@changeset-bot
Copy link

changeset-bot bot commented Nov 4, 2025

🦋 Changeset detected

Latest commit: f018e50

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@biomejs/biome Patch
@biomejs/cli-win32-x64 Patch
@biomejs/cli-win32-arm64 Patch
@biomejs/cli-darwin-x64 Patch
@biomejs/cli-darwin-arm64 Patch
@biomejs/cli-linux-x64 Patch
@biomejs/cli-linux-arm64 Patch
@biomejs/cli-linux-x64-musl Patch
@biomejs/cli-linux-arm64-musl Patch
@biomejs/wasm-web Patch
@biomejs/wasm-bundler Patch
@biomejs/wasm-nodejs Patch
@biomejs/backend-jsonrpc Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages L-HTML Language: HTML and super languages labels Nov 4, 2025
@denbezrukov denbezrukov marked this pull request as draft November 4, 2025 09:15
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 4, 2025

CodSpeed Performance Report

Merging #7983 will not alter performance

Comparing dbezrukov/text-width-mem (f018e50) with main (0b28f5f)

Summary

✅ 53 untouched
⏩ 85 skipped1

Footnotes

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Walkthrough

This PR introduces width-tracking for text formatting elements. A new TextWidth enum and Width struct are added to store precomputed text widths or mark multiline content. The Text and LocatedTokenText variants of FormatElement gain a text_width field. Format implementations in builders and printer logic are updated to compute and propagate text width information, replacing direct text inspection for multiline detection. Public re-exports are extended to include TextWidth.

Possibly related PRs

  • biomejs/biome#7968: Modifies text-related FormatElement variants and printer handling with updates to FormatElement::Text, LocatedTokenText, and the internal Text enum representation.

Suggested labels

A-Formatter, A-Project, A-Core, L-JavaScript, L-HTML

Suggested reviewers

  • ematipico

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarises the main change: memoising (precomputing and storing) text width information in FormatElements for performance optimisation.
Description check ✅ Passed The description clearly relates to the changeset, explaining the purpose (port from Ruff), the core change (TextWidth field on FormatElements), and test coverage (green CI).
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dbezrukov/text-width-mem

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b28f5f and 7f93f9a.

📒 Files selected for processing (8)
  • .changeset/long-cars-beam.md (1 hunks)
  • crates/biome_formatter/src/builders.rs (5 hunks)
  • crates/biome_formatter/src/format_element.rs (6 hunks)
  • crates/biome_formatter/src/format_element/document.rs (4 hunks)
  • crates/biome_formatter/src/lib.rs (2 hunks)
  • crates/biome_formatter/src/printer/mod.rs (6 hunks)
  • crates/biome_html_formatter/src/utils/children.rs (2 hunks)
  • crates/biome_js_formatter/src/utils/jsx.rs (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
crates/biome_js_formatter/**/*.rs

📄 CodeRabbit inference engine (crates/biome_js_formatter/CONTRIBUTING.md)

crates/biome_js_formatter/**/*.rs: Implement the Format trait for your node type and use JsFormatter with write!/format_args! to define formatting
Import and use the FormatNode trait for AST nodes
When a token is mandatory and present in the AST, use the AST-provided token (e.g., node.l_paren_token().format()) instead of emitting a static token
For non-mandatory tokens, use the provided helper constructors (e.g., token, space_token, dynamic_token)
Do not attempt to fix code; if a mandatory token/node is missing, return None instead
Use dbg_write! to debug and inspect the emitted IR during formatting

Files:

  • crates/biome_js_formatter/src/utils/jsx.rs
crates/biome_formatter/src/lib.rs

📄 CodeRabbit inference engine (crates/biome_formatter/CONTRIBUTING.md)

crates/biome_formatter/src/lib.rs: Add the provided AsFormat, IntoFormat, and iterator plumbing code to lib.rs
Implement FormatLanguage for HtmlFormatLanguage with associated types: SyntaxLanguage=HtmlLanguage, Context=HtmlFormatContext, FormatRule=FormatHtmlSyntaxNode
Define the HtmlFormatter type alias: type HtmlFormatter<'buf> = Formatter<'buf, HtmlFormatContext>;
Expose a documented public function format_node(options: HtmlFormatOptions, root: &HtmlSyntaxNode) -> FormatResult<Formatted<HtmlFormatContext>> delegating to biome_formatter::format_node

Files:

  • crates/biome_formatter/src/lib.rs
crates/biome_formatter/src/**/*.rs

📄 CodeRabbit inference engine (crates/biome_formatter/CONTRIBUTING.md)

After generation, remove usages of format_verbatim_node and implement real formatting with biome_formatter utilities

Files:

  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/printer/mod.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/format_element.rs
  • crates/biome_formatter/src/builders.rs
🧠 Learnings (23)
📚 Learning: 2025-10-15T09:22:46.002Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:46.002Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Import and use the `FormatNode` trait for AST nodes

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/printer/mod.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/format_element.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:46.002Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:46.002Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : When a token is mandatory and present in the AST, use the AST-provided token (e.g., `node.l_paren_token().format()`) instead of emitting a static token

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_formatter/src/format_element.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:46.002Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:46.002Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Implement the `Format` trait for your node type and use `JsFormatter` with `write!`/`format_args!` to define formatting

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:46.002Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:46.002Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : For non-mandatory tokens, use the provided helper constructors (e.g., `token`, `space_token`, `dynamic_token`)

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/format_element.rs
📚 Learning: 2025-10-15T09:22:46.002Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:46.002Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Do not attempt to fix code; if a mandatory token/node is missing, return `None` instead

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_formatter/src/format_element.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/**/*.rs : After generation, remove usages of `format_verbatim_node` and implement real formatting with biome_formatter utilities

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/printer/mod.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/format_element.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/context.rs : Create HtmlFormatContext in context.rs with comments and source_map fields and implement FormatContext and CstFormatContext

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/printer/mod.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/format_element.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:46.002Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:46.002Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Use `dbg_write!` to debug and inspect the emitted IR during formatting

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/format_element.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/cst.rs : Create FormatHtmlSyntaxNode in cst.rs implementing FormatRule<HtmlSyntaxNode> and AsFormat/IntoFormat for HtmlSyntaxNode using the provided plumbing

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/format_element.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:46.002Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:46.002Z
Learning: Applies to crates/biome_js_formatter/**/Cargo.toml : Declare the dependency `biome_js_formatter = { version = "0.0.1", path = "../biome_js_formatter" }` for internal installation

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/comments.rs : Define HtmlCommentStyle implementing CommentStyle in comments.rs

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/format_element.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/comments.rs : Expose a public HtmlComments type alias: `pub type HtmlComments = Comments<HtmlLanguage>;`

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_html_formatter/src/utils/children.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/lib.rs : Expose a documented public function `format_node(options: HtmlFormatOptions, root: &HtmlSyntaxNode) -> FormatResult<Formatted<HtmlFormatContext>>` delegating to `biome_formatter::format_node`

Applied to files:

  • crates/biome_js_formatter/src/utils/jsx.rs
  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/printer/mod.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/lib.rs : Implement FormatLanguage for HtmlFormatLanguage with associated types: SyntaxLanguage=HtmlLanguage, Context=HtmlFormatContext, FormatRule=FormatHtmlSyntaxNode

Applied to files:

  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/printer/mod.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/format_element.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/tests/language.rs : Create tests/language.rs defining `HtmlTestFormatLanguage` and implement the TestFormatLanguage trait

Applied to files:

  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/format_element.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/lib.rs : Define the HtmlFormatter type alias: `type HtmlFormatter<'buf> = Formatter<'buf, HtmlFormatContext>;`

Applied to files:

  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/lib.rs : Add the provided AsFormat, IntoFormat, and iterator plumbing code to lib.rs

Applied to files:

  • crates/biome_html_formatter/src/utils/children.rs
  • crates/biome_formatter/src/lib.rs
  • crates/biome_formatter/src/printer/mod.rs
  • crates/biome_formatter/src/format_element/document.rs
  • crates/biome_formatter/src/format_element.rs
  • crates/biome_formatter/src/builders.rs
📚 Learning: 2025-10-15T09:23:33.055Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:23:33.055Z
Learning: Applies to crates/biome_js_type_info/src/{type_info,local_inference,resolver,flattening}.rs : Avoid recursive type structures and cross-module Arcs; represent links between types using TypeReference and TypeData::Reference.

Applied to files:

  • crates/biome_formatter/src/printer/mod.rs
📚 Learning: 2025-10-15T09:23:33.055Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:23:33.055Z
Learning: Applies to crates/biome_js_type_info/src/type_info.rs : Add new TypeScript type support by extending the TypeData enum rather than introducing parallel structures.

Applied to files:

  • crates/biome_formatter/src/printer/mod.rs
📚 Learning: 2025-09-13T16:16:06.459Z
Learnt from: ematipico
Repo: biomejs/biome PR: 7467
File: crates/biome_service/src/file_handlers/html.rs:456-466
Timestamp: 2025-09-13T16:16:06.459Z
Learning: In biome_formatter, the printer correctly handles consecutive LineMode::Hard elements without creating extra blank lines. Multiple consecutive FormatElement::Line(LineMode::Hard) elements in the formatting code do not result in duplicate blank lines in the output because the printer has logic to track line states and handle indentation properly.

Applied to files:

  • crates/biome_formatter/src/printer/mod.rs
📚 Learning: 2025-09-13T16:16:06.459Z
Learnt from: ematipico
Repo: biomejs/biome PR: 7467
File: crates/biome_service/src/file_handlers/html.rs:456-466
Timestamp: 2025-09-13T16:16:06.459Z
Learning: The biome_formatter printer deduplicates consecutive LineMode::Hard elements by checking if the current line is already empty (line_width > 0) before printing a newline. This prevents multiple consecutive hard line breaks from creating extra blank lines in the output.

Applied to files:

  • crates/biome_formatter/src/printer/mod.rs
📚 Learning: 2025-10-03T12:28:56.788Z
Learnt from: ematipico
Repo: biomejs/biome PR: 7670
File: crates/biome_service/src/file_handlers/html.rs:744-748
Timestamp: 2025-10-03T12:28:56.788Z
Learning: In Biome's codebase, when creating tokens with new text content, use the factory pattern with functions like `ident(text)` from the respective `*_factory` crates (e.g., `biome_html_factory::make::ident`). There is no `.with_text()` method on tokens. The `ident()` function creates a new detached token with the IDENT kind, which is the standard approach for token construction.

Applied to files:

  • crates/biome_formatter/src/printer/mod.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/tests/spec_test.rs : Implement a `run` function in tests/spec_test.rs that wires SpecSnapshot and includes!("language.rs") as shown

Applied to files:

  • crates/biome_formatter/src/format_element/document.rs
🧬 Code graph analysis (6)
crates/biome_js_formatter/src/utils/jsx.rs (3)
crates/biome_formatter/src/lib.rs (2)
  • comments (881-881)
  • write (1428-1435)
crates/biome_formatter/src/formatter.rs (1)
  • comments (250-252)
crates/biome_formatter/src/format_element.rs (1)
  • from_text (410-423)
crates/biome_html_formatter/src/utils/children.rs (2)
crates/biome_formatter/src/lib.rs (1)
  • write (1428-1435)
crates/biome_formatter/src/format_element.rs (1)
  • from_text (410-423)
crates/biome_formatter/src/printer/mod.rs (3)
crates/biome_formatter/src/builders.rs (1)
  • text (282-286)
crates/biome_diagnostics/src/display/frame.rs (2)
  • text_width (341-343)
  • char_width (356-368)
crates/biome_formatter/src/format_element.rs (1)
  • width (425-430)
crates/biome_formatter/src/format_element/document.rs (3)
crates/biome_formatter/src/builders.rs (2)
  • text (282-286)
  • token (254-262)
crates/biome_formatter/src/lib.rs (13)
  • format (1468-1484)
  • write (1428-1435)
  • default (306-308)
  • default (408-410)
  • default (675-677)
  • document (993-995)
  • from (474-476)
  • from (480-482)
  • from (486-488)
  • from (568-570)
  • from (650-655)
  • from (681-683)
  • from (767-769)
crates/biome_formatter/src/format_element.rs (1)
  • from_text (410-423)
crates/biome_formatter/src/format_element.rs (5)
crates/biome_diagnostics/src/display/frame.rs (2)
  • text_width (341-343)
  • char_width (356-368)
crates/biome_formatter/src/format_element/document.rs (1)
  • indent_width (251-253)
crates/biome_formatter/src/lib.rs (6)
  • value (300-302)
  • value (402-404)
  • value (669-671)
  • value (761-763)
  • indent_width (857-857)
  • indent_width (920-922)
crates/biome_html_formatter/src/context.rs (3)
  • value (359-361)
  • indent_width (139-141)
  • indent_width (239-241)
crates/biome_formatter/src/comments/map.rs (1)
  • value (629-631)
crates/biome_formatter/src/builders.rs (1)
crates/biome_formatter/src/format_element.rs (1)
  • from_text (410-423)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: autofix
  • GitHub Check: Documentation
  • GitHub Check: Test (depot-ubuntu-24.04-arm-16)
  • GitHub Check: Test (depot-windows-2022-16)
  • GitHub Check: Lint project (depot-windows-2022)
  • GitHub Check: Check Dependencies
  • GitHub Check: Test Node.js API
  • GitHub Check: Bench (biome_js_parser)
  • GitHub Check: Bench (biome_js_analyze)
  • GitHub Check: Bench (biome_js_formatter)

Comment on lines +413 to +420
for c in text.chars() {
let char_width = match c {
'\t' => indent_width.value() as u32,
'\n' => return TextWidth::Multiline,
c => c.width().unwrap_or(0) as u32,
};
width += char_width;
}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 4, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Prevent overflow while tallying TextWidth.

width is a plain u32. A sufficiently long token (think multi‑gigabyte minified files or wide CJK text) will make width += char_width overflow: debug builds panic, release builds wrap to zero, and we then under-report the width so groups stay flat when they should break. Please make the accumulation saturating.

Apply this diff to stay safe:

-            width += char_width;
+            width = width.saturating_add(char_width);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for c in text.chars() {
let char_width = match c {
'\t' => indent_width.value() as u32,
'\n' => return TextWidth::Multiline,
c => c.width().unwrap_or(0) as u32,
};
width += char_width;
}
for c in text.chars() {
let char_width = match c {
'\t' => indent_width.value() as u32,
'\n' => return TextWidth::Multiline,
c => c.width().unwrap_or(0) as u32,
};
width = width.saturating_add(char_width);
}
🤖 Prompt for AI Agents
In crates/biome_formatter/src/format_element.rs around lines 413 to 420, the
loop adds char widths into a plain u32 `width`, which can overflow for very long
or wide tokens; replace the non-saturating addition with a saturating
accumulation (e.g., use width = width.saturating_add(char_width) or equivalent)
so the total never wraps—keep the rest of the match logic unchanged and ensure
types remain u32.

Copy link
Member

Choose a reason for hiding this comment

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

@coderabbitai Biome trees work on u32 constraint. Do you think it's safe to assume we don't need the saturing_add?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ematipico You're right that the u32 tree constraint provides a practical upper bound—most text width will track close to byte size. The main edge case would be a massive file full of tabs with a large indent_width, but that's admittedly theoretical.

Since Width::new already saturates (NonZeroU32::MIN.saturating_add(width)), adding saturating_add here is pure defense in depth. The performance cost is negligible, but if you're confident the constraint holds in practice, plain += is fine. Your call!


🧠 Learnings used
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/lib.rs : Implement FormatLanguage for HtmlFormatLanguage with associated types: SyntaxLanguage=HtmlLanguage, Context=HtmlFormatContext, FormatRule=FormatHtmlSyntaxNode

Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/cst.rs : Create FormatHtmlSyntaxNode in cst.rs implementing FormatRule<HtmlSyntaxNode> and AsFormat/IntoFormat for HtmlSyntaxNode using the provided plumbing

Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/lib.rs : Define the HtmlFormatter type alias: `type HtmlFormatter<'buf> = Formatter<'buf, HtmlFormatContext>;`

Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/context.rs : Create HtmlFormatContext in context.rs with comments and source_map fields and implement FormatContext and CstFormatContext

Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/comments.rs : Define HtmlCommentStyle implementing CommentStyle in comments.rs

Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:46.002Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Import and use the `FormatNode` trait for AST nodes

Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/tests/language.rs : Create tests/language.rs defining `HtmlTestFormatLanguage` and implement the TestFormatLanguage trait

Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:46.002Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Do not attempt to fix code; if a mandatory token/node is missing, return `None` instead

Copy link
Member

Choose a reason for hiding this comment

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

@denbezrukov can you check this comment?

Comment on lines +1352 to +1360
for c in text_str.chars() {
let char_width = match c {
'\t' => self.options().indent_width.value() as usize,
'\n' => {
return if self.must_be_flat
|| self.state.line_width > self.options().print_width.into()
{
Fits::No
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Mind the missing parentheses.

indent_width is a method on PrinterOptions; dropping the () yields a function item instead of the indent size, so this won’t even compile. Please call the method before reading .value().

-                            '\t' => self.options().indent_width.value() as usize,
+                            '\t' => self.options().indent_width().value() as usize,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for c in text_str.chars() {
let char_width = match c {
'\t' => self.options().indent_width.value() as usize,
'\n' => {
return if self.must_be_flat
|| self.state.line_width > self.options().print_width.into()
{
Fits::No
} else {
for c in text_str.chars() {
let char_width = match c {
'\t' => self.options().indent_width().value() as usize,
'\n' => {
return if self.must_be_flat
|| self.state.line_width > self.options().print_width.into()
{
Fits::No
} else {
🤖 Prompt for AI Agents
In crates/biome_formatter/src/printer/mod.rs around lines 1352 to 1360, the code
references self.options().indent_width.value() but forgot the parentheses on
indent_width, which yields a function item instead of calling the method; change
it to self.options().indent_width().value() (keeping the existing cast to usize)
so the indent width method is invoked before reading .value().

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

It seems we need to update our benchmarks, because we forgot to add the biome_formatter crate. For example

https://github.com/biomejs/biome/blob/main/.github%2Fworkflows%2Fbenchmark_css.yml#L10-L29

/// allowing [`TextWidth`] or [`Option<Width>`] fit in 4 bytes rather than 8.
///
/// This means that 2^32 can not be precisely represented and instead has the same value as 2^32-1.
/// This imprecision shouldn't matter in practice because either text are longer than any configured line width
Copy link
Member

Choose a reason for hiding this comment

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

This is true only for text that is actually formatted by us. This means that this isn't applicable in cases where we format in verbatim mode. Can you update the comment to include this?

Comment on lines +413 to +420
for c in text.chars() {
let char_width = match c {
'\t' => indent_width.value() as u32,
'\n' => return TextWidth::Multiline,
c => c.width().unwrap_or(0) as u32,
};
width += char_width;
}
Copy link
Member

Choose a reason for hiding this comment

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

@denbezrukov can you check this comment?

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

Labels

A-Formatter Area: formatter L-HTML Language: HTML and super languages L-JavaScript Language: JavaScript and super languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants