Skip to content

Preserve trailing line comment on defer statements in baml fmt (B-629) - #3919

Merged
antoniosarosi merged 2 commits into
canaryfrom
antonio/b-629
Jul 6, 2026
Merged

antoniosarosi merged 2 commits into
canaryfrom
antonio/b-629

Conversation

@antoniosarosi

@antoniosarosi antoniosarosi commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor

What

baml fmt silently deleted the trailing line-comment on a defer { … } statement, while an identical trailing comment on a normal statement survived. This is source-data loss, distinct from the known fmt over-expansion cluster (B-575/600/602/606).

Repro (before):

function f() -> string[] {
    let out: string[] = [];
    defer { out.push("x") }   // THIS comment on a defer line
    out.push("body");         // this one on a normal line
    out
}

After baml fmt, the // THIS comment on a defer line was gone; the normal-line comment stayed.

Why

A defer statement (DEFER_STMT) is not modeled by the formatter's strong AST, so it is held as Expression::Unknown and printed verbatim. Unknown reported the whole node span as both its leftmost_token and rightmost_token. But the trivia classifier keys leading/trailing comments to individual token ranges (the comment after defer { … } is anchored to the closing } token). A whole-node span never matches a per-token key, so get_trailing_for_element returned nothing and the comment was dropped.

The whole-node span also begins inside leading trivia the CST folds into the node, which is why a leading comment on the line before a defer was re-printed verbatim at its original (wrong) indent.

How

Expression::Unknown now carries the node's true first and last non-trivia token ranges instead of the whole span:

  • leftmost_token / rightmost_token return those exact token ranges, so leading/trailing comments attach and emit like any normal statement.
  • Printing uses a content_range (first-token start … last-token end), which excludes any leading/trailing trivia the CST folded into the node — so the leading comment is emitted once (via the trivia path) at the correct indent, not duplicated.

This is a general fix for all unmodeled nodes (defer, throw, await, spawn, x.as<T>), and it keeps them printed verbatim, so it does not touch the over-expansion behavior.

Tests

Added defer_comment_tests in baml_fmt: trailing-comment preservation (ticket repro), trailing comment on a last-statement defer, and leading-comment re-indentation — each asserts idempotency. Full baml_tests suite passes with zero formatter-snapshot changes.

Fixes B-629

Summary by CodeRabbit

  • Bug Fixes

    • Improved formatting of comments around defer statements, preserving both leading and trailing // comments and aligning them to canonical indentation/spacing.
    • Refined formatting for unrecognized expressions to better preserve the original source slice, improving line-width decisions and multi-line behavior.
    • Improved wrapping/printing of braceless return bodies in match/catch arms so semicolons and inline comments remain correctly attached.
  • Tests

    • Added regression tests to cover defer comment formatting and idempotency.

@linear

linear Bot commented Jul 6, 2026

Copy link
Copy Markdown

B-629

@vercel

vercel Bot commented Jul 6, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
beps Ready Ready Preview, Comment Jul 6, 2026 5:55pm
promptfiddle Ready Ready Preview, Comment Jul 6, 2026 5:55pm
promptfiddle2 Ready Ready Preview, Comment Jul 6, 2026 5:55pm

Request Review

@vercel
vercel Bot temporarily deployed to Preview – beps July 6, 2026 15:53 Inactive
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

⏭️ Performance benchmarks were skipped

Perf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to canary/main.

To run them on this PR, do any of the following, then push a commit (or re-run CI):

  • Add RUN_CODSPEED=1 to the PR description, or
  • Include run-perf or /perf in the PR title or any commit message.

@coderabbitai

coderabbitai Bot commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e594e996-0ce2-4d52-9636-483151754094

📥 Commits

Reviewing files that changed from the base of the PR and between 028cc9c and 5242ac0.

📒 Files selected for processing (3)
  • baml_language/crates/baml_fmt/src/ast/expressions.rs
  • baml_language/crates/baml_fmt/src/lib.rs
  • baml_language/crates/baml_fmt/src/printer.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • baml_language/crates/baml_fmt/src/lib.rs

📝 Walkthrough

Walkthrough

Refactors return and unknown expression formatting to use token-anchored verbatim spans, centralizes wrapped arm printing, and adds regression tests for comment preservation around defer statements.

Changes

Verbatim expression formatting and arm wrapping

Layer / File(s) Summary
Verbatim span storage and expression construction
baml_language/crates/baml_fmt/src/ast/expressions.rs
Adds VerbatimSpan and updates Expression::from_cst to store it for return and unknown expressions.
Verbain expression printing and wrapped arm flow
baml_language/crates/baml_fmt/src/ast/expressions.rs, baml_language/crates/baml_fmt/src/printer.rs
Updates width, print, and token accessors to use verbatim spans, adds print_standalone_leading_and_body, and routes wrapped match and catch arm bodies through the shared helper.
Defer comment regression tests
baml_language/crates/baml_fmt/src/lib.rs
Adds defer comment formatting regression tests with an idempotent formatting helper.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: hellovai

Poem

A bunny hopped through spans so neat,
With tokens first and last complete.
Braces wrapped and comments stayed,
In tidy lines the formatter played.
🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Clear and specific; it summarizes the main bug fix around preserving trailing comments on defer statements.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch antonio/b-629

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.

@vercel
vercel Bot temporarily deployed to Preview – promptfiddle2 July 6, 2026 15:59 Inactive

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@baml_language/crates/baml_fmt/src/ast/expressions.rs`:
- Around line 387-388: The token range handling for Expression::Return still
relies on elem.text_range(), so trailing comments on a braceless return arm can
be skipped. Update the leftmost_token()/rightmost_token() logic in
expressions.rs to use the underlying token span from the Return variant’s range,
similar to how Unknown uses unknown.last_token, so the anchor is based on the
actual tokens rather than the full text range.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f02a1ef6-a40c-4de3-9cb5-2017c1216bbe

📥 Commits

Reviewing files that changed from the base of the PR and between 8cb5144 and 028cc9c.

📒 Files selected for processing (2)
  • baml_language/crates/baml_fmt/src/ast/expressions.rs
  • baml_language/crates/baml_fmt/src/lib.rs

@github-actions

github-actions Bot commented Jul 6, 2026 •

Copy link
Copy Markdown

Binary size checks passed

✅ 7 passed

Artifact Platform File Gzip Gated on Baseline Delta Status
✅ baml-cli Linux 🔒 21.6 MB 9.2 MB file 21.5 MB +76.9 KB (+0.4%) OK
✅ packed-program Linux 🔒 15.8 MB 6.6 MB file 15.6 MB +133.5 KB (+0.9%) OK
✅ baml-cli macOS 🔒 16.6 MB 8.0 MB file 16.6 MB +66.3 KB (+0.4%) OK
✅ packed-program macOS 🔒 12.2 MB 5.8 MB file 12.1 MB +99.6 KB (+0.8%) OK
✅ baml-cli Windows 🔒 18.2 MB 8.2 MB file 18.1 MB +60.4 KB (+0.3%) OK
✅ packed-program Windows 🔒 13.1 MB 5.9 MB file 13.0 MB +103.8 KB (+0.8%) OK
✅ bridge_wasm WASM 14.5 MB 🔒 4.1 MB gzip 4.1 MB +53.5 KB (+1.3%) OK

🔒 = the size this artifact is GATED on (ceiling + delta). Binaries gate on file size (installed binary); WASM gates on gzip (download size). The other size is shown for information only.


Generated by cargo size-gate · workflow run

@vercel
vercel Bot temporarily deployed to Preview – promptfiddle July 6, 2026 16:12 Inactive
A defer statement is an unmodeled node held as Expression::Unknown and
printed verbatim. It reported the whole node span as both its leftmost and
rightmost token, but the trivia classifier keys leading/trailing comments to
individual token ranges. The whole-node span never matched the closing-brace
token key, so a trailing line comment on a defer statement was silently
dropped, while an identical comment on a normal statement survived.

Carry the node's true first/last non-trivia token ranges on Expression::Unknown
so leading/trailing comments attach and emit correctly. Print only the
content range (first token start to last token end), which also excludes any
leading comment the CST folds into the node span (previously re-printed
verbatim at the wrong indent).

Fixes B-629
…629)

CodeRabbit follow-up: Expression::Return had the same comment-loss bug as
Expression::Unknown — it reported the whole node span as its leftmost/rightmost
token. A braceless return arm with no trailing comma delegates the arm's
rightmost token to the return body, so a trailing comment (anchored to the
return value's last token) never matched and was silently dropped.

- Back both Return and Unknown with a shared VerbatimSpan carrying the node's
  true first/last non-trivia token ranges (renamed from UnknownExpr).
- Factor the three arm block-wrap sites into print_wrapped_arm_body. For a
  braceless return it emits 'return ...;' but leaves the trailing trivia to the
  arm level, so a same-line comment is preserved once (after the wrapped '},'),
  not split from its ';' nor duplicated when the arm has no comma. The existing
  { return ...; } wrapping and the catch_arm_return snapshot are unchanged.
- Add print_standalone_leading_and_body printer helper.
- Drop the rustdoc intra-doc link to the private content_range method, which
  failed 'cargo doc' under -D warnings.

Regression tests: trailing comment on a braceless return arm survives (comma
and no-comma), plus a catch-arm round-trip guard.

Fixes B-629
@vercel
vercel Bot temporarily deployed to Preview – beps July 6, 2026 17:36 Inactive
@vercel
vercel Bot temporarily deployed to Preview – promptfiddle2 July 6, 2026 17:44 Inactive
@vercel
vercel Bot temporarily deployed to Preview – promptfiddle July 6, 2026 17:55 Inactive
@antoniosarosi
antoniosarosi added this pull request to the merge queue Jul 6, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 6, 2026
@antoniosarosi
antoniosarosi added this pull request to the merge queue Jul 6, 2026
Merged via the queue into canary with commit fd32171 Jul 6, 2026
88 of 90 checks passed
@antoniosarosi
antoniosarosi deleted the antonio/b-629 branch July 6, 2026 19:13

This branch was previously deployed

3 inactive deployments
Preview – promptfiddle — 5242ac05 Deployed Jul 6, 2026 by vercel[bot]
Preview – promptfiddle2 — 5242ac05 Deployed Jul 6, 2026 by vercel[bot]
Preview – beps — 5242ac05 Deployed Jul 6, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant