Skip to content

Fix: cluster of formatting bugs surfaced by Rails idioms#102

Merged
sorafujitani merged 1 commit intomainfrom
fix/rails-idiom-bugs
Apr 24, 2026
Merged

Fix: cluster of formatting bugs surfaced by Rails idioms#102
sorafujitani merged 1 commit intomainfrom
fix/rails-idiom-bugs

Conversation

@sorafujitani
Copy link
Copy Markdown
Owner

@sorafujitani sorafujitani commented Apr 23, 2026

Summary

A systematic pass across Rails-style Ruby (controllers, models, routes, concerns, services, jobs, migrations, specs, factories, endless defs, heredocs, rescue modifiers, block comments) surfaced several latent formatter bugs. This PR fixes them all in one shot and ships regression coverage.

Bugs fixed

ID Symptom Root cause / fix
A Blank lines between class/module/def members contained trailing spaces (RuboCop Layout/TrailingWhitespace everywhere). hardline always writes "\n" + indent_str; two hardlines in a row leak the indent onto the blank line. Post-process the printer output to strip per-line trailing spaces/tabs.
C Ruby 3 endless defs (def active? = status == "active"), inline methods (def simple; 1; end), and inline exception-hierarchy classes (class Error < StandardError; end) were always expanded into multi-line def … end / class … end. In format_body_end, when start_line == end_line, emit the source slice verbatim instead of forcing the <header><newline><body><newline>end layout.
D Chains ending in `.foo do x
E Spurious blank line before end after a heredoc argument (foo(<<~SQL) … SQL end) and dropped separator line between two heredoc-valued constants. (1) Source-extracting rules (Call / Fallback / VariableWrite) strip at most one trailing \n via a new strip_one_trailing_newline helper — a full trim_end would eat a blank separator that legitimately belongs inside the node's range. (2) The bridge's heredoc closing_loc projection was reporting <terminator_line + 1>:0 as the end, so format_statements' line_diff treated the blank line after the terminator as "adjacent". Snap the reported end_line back to the terminator's own line.
B1 A # note on the rescue comment written just before rescue was absorbed as a leading comment of the first statement inside the rescue body. format_rescue now claims leading comments of the rescue line itself.
B2 A standalone comment sitting between two statements was counted as a blank line; format_comments_before_end's last entry ended with hard_line_after: true and left a blank line before end. (1) format_statements / format_children_with_spacing subtract comment-occupied lines from the blank-line count, and deduct one more if the gap contains a block comment (whose literalline already supplies a break). (2) Drop the trailing hardline on the final comment returned by format_comments_before_end.
B3 =begin … =end comments were attributed to the wrong node (often hoisted out of the enclosing class) and, once attached, printed =begin at the surrounding indent — a syntax error. (1) The bridge clamps an embdoc's end_line from <end>:0 back to the =end line so it no longer overlaps the next statement. (2) Block comments emit through literalline + text + hardline so =begin/=end land at column 0. Strip the trailing newline in the slice and deduct one blank-line hardline from the surrounding gap so the literalline doesn't double up.

Before / after highlights

# Endless def (C)
class Thing
  def active? = status == "active"   # was: expanded to def … end
end

# Inline exception hierarchy (C)
module Payments
  class Error < StandardError; end   # was: expanded to 2 lines, blank between lost
  class DeclinedError < Error; end
end

# Chain + block body (D)
User.active
  .where(role: :admin)
  .each do |u|
    puts u.name                      # was: at outer indent, end floated up
  end

# Heredoc argument (E)
def foo
  User.find_by_sql(<<~SQL)
    SELECT *
  SQL
end                                   # was: blank line inserted before end

# Rescue annotation (B1)
def foo
  work!
  # rescue annotation below          # was: moved inside rescue body
rescue => e
  handle(e)
end

Changes

  • ext/rfmt/src/doc/printer.rs — per-line trailing-whitespace strip (strip_trailing_line_whitespace).
  • ext/rfmt/src/format/rule.rs — push_leading_comment block-comment path; strip_one_trailing_newline helper; comment-aware blank-line accounting in format_statements; last-comment tail suppression in format_comments_before_end; extended CommentRef with is_block.
  • ext/rfmt/src/format/formatter.rs — same comment-aware blank-line accounting for the program-root / statements-root loop.
  • ext/rfmt/src/format/rules/body_end.rs — single-line construct shortcut (C).
  • ext/rfmt/src/format/rules/begin.rs — leading comments on rescue (B1).
  • ext/rfmt/src/format/rules/call.rs — Align(indent_width, …) around chain-with-block; heredoc tail trim (D, E).
  • ext/rfmt/src/format/rules/fallback.rs, …/variable_write.rs — same heredoc tail trim for the other source-extracting paths (E).
  • lib/rfmt/prism_bridge.rb — end_line clamp for heredoc closing_loc and embdoc comments (E, B3).
  • spec/rails_idioms_spec.rb — 12 new end-to-end + idempotence tests, one per category.

Test plan

  • cargo test --manifest-path ext/rfmt/Cargo.toml --lib — 127 passed
  • bundle exec rspec — 119 passed (100 existing + 19 new)
  • cargo clippy -- -D warnings — clean
  • cargo fmt --check — clean
  • bundle exec rubocop — clean
  • All 27 synthetic Rails-idiom fixtures (controllers/models/routes/jobs/concerns/migrations/blocks/heredocs/rescue modifiers/comments/etc.) are idempotent after the first format.

Out of scope

  • The aligned → indented chain reformat introduced in Fix: preserve source indent in method chain reformatting #100 still fires for plain assignments (@x = User.chain.chain). That is existing design and deserves a config switch, which I'd like to split off.
  • When multiple =begin … =end comments appear in the same file with additional blank lines between them, one extra blank line can still slip in between consecutive embdocs. Block comments are uncommon enough that this can be tightened in a follow-up.

@sorafujitani
Fix: cluster of formatting bugs surfaced by Rails idioms
650c823 
A sweep across common Rails-style Ruby code (controllers, models, routes,
concerns, services, jobs, migrations, specs, factories, endless defs,
heredocs, rescue modifiers, block comments) turned up several latent
bugs. This change fixes all of them in one pass and ships regression
coverage.

A. Trailing whitespace on blank lines
   Every class / module / def body turned `""` blank lines into
   `"<indent>"` (two trailing spaces), tripping RuboCop
   Layout/TrailingWhitespace on virtually every formatted file. The
   printer's `hardline` always writes `"\n" + indent_str`, so two
   consecutive hardlines leak the indent onto the line between them.
   Post-process the final buffer to strip per-line trailing spaces and
   tabs.

C. Ruby 3+ endless defs and the inline one-liner idiom
   `def active? = status == "active"`, `def simple; 1; end`, and
   `class Error < StandardError; end` (pervasive in Rails exception
   hierarchies) were all expanded into multi-line `def … end` /
   `class … end` forms. `format_body_end` now emits such single-line
   constructs verbatim when `start_line == end_line`, preserving both
   the endless-def syntax and the compact one-liners.

D. Method-chain + block body indentation
   A chain that ends with `.foo do |x| … end` printed the block body
   one indent level below the `.foo` receiver (bodies aligned with the
   chain start instead of nesting under the chain's last line; `end`
   floated above the opener). When `reformat_chain_lines` re-indents a
   chain, wrap the block doc in `Align(indent_width, …)` so the body
   and matching `end` sit one level under the chain's tail.

E. Spurious blank lines around heredocs
   Two symptoms, one cause:
     - `foo(<<~SQL)\n…\nSQL` as the last statement in a body produced
       a blank line before `end`.
     - `CONST = <<~TEXT\n…\nTEXT\nNEXT = …` swallowed or duplicated
       separators between consecutive heredoc-valued constants.
   Two-part fix:
     - Source-extracting rules (Call, Fallback, VariableWrite) now
       strip at most one trailing `\n` from the extracted slice via
       a shared `strip_one_trailing_newline` helper — the trailing
       newline Prism includes past the terminator was compounding with
       the surrounding hardline. Using the full `trim_end` would
       instead eat a blank separator that legitimately belongs inside
       the node's range.
     - The bridge's heredoc `closing_loc` projection was reporting
       `<terminator_line + 1>:0` as the end, which made
       `format_statements`' `line_diff` check treat the blank line
       after the terminator as "adjacent", collapsing the separator.
       Snap the reported `end_line` back to the terminator's own line.

B1. Comment immediately before `rescue`/`ensure`
   An annotation like `# rescue if auth fails` written on the line
   before `rescue` used to get picked up as a leading comment of the
   first statement *inside* the rescue body, visually moving the
   annotation into the wrong branch. `format_rescue` now consumes
   leading comments attached to the rescue line itself.

B2. Blank-line accounting in statement gaps
   Two issues resolved together:
     - A standalone `# note` comment on the line between two
       statements was counted as a blank line (line_diff > 1),
       producing a spurious blank line between the comment and the
       statement below. `format_statements` and the top-level
       `format_children_with_spacing` now subtract comment-occupied
       lines before deciding whether to emit a blank-line hardline.
     - `format_comments_before_end` used to pass `hard_line_after:
       true` even for the last comment in the run, which combined
       with the caller's own `hardline + "end"` to produce a blank
       line between the comment and `end`. Drop the trailing
       hardline for the final entry.

B3. `=begin … =end` block comments
   Embdoc comments were being mis-attached and mis-indented:
     - The bridge inherited Prism's convention that an embdoc's
       end_line sits at `<line after =end>:0`. That overlapped the
       next statement and caused the comment to re-attach to a
       deeper node, sometimes hoisting a comment that lived *inside*
       a class out to the program body. Clamp the reported end_line
       back to the `=end` line.
     - Once attached correctly, emitting the comment through the
       normal leading-comment path printed `=begin` at the surrounding
       indent, which is a *syntax error* (embdocs must live in
       column 0). Route block comments through `literalline + text +
       hardline` so they break out to column 0. Strip the trailing
       newline inside the comment slice and deduct one blank-line
       hardline from the surrounding gap so the `literalline` doesn't
       double up.

Tests
  - spec/rails_idioms_spec.rb exercises every category above as a
    round-trip + idempotence check: inline def / class / endless def,
    blank-line ws, chain + block body alignment, heredoc in a call
    argument, heredoc between constants, comment-in-gap, trailing
    body comment, and embdoc attachment/position.
  - All 127 cargo unit tests and 119 rspec examples (100 existing +
    19 new) pass. `cargo clippy -D warnings`, `cargo fmt --check`,
    and `rubocop` are clean.

Out of scope
  - The aligned → indented chain reformat introduced in PR #100 still
    fires for plain assignments (`@x = User.chain.chain`). That's
    existing design and should be guarded by a config option in a
    follow-up.
  - A second and third embdoc in the same file can still leave one
    extra blank line between them; uncommon enough to defer.
@sorafujitani sorafujitani mentioned this pull request Apr 24, 2026
Closed
6 tasks
@sorafujitani sorafujitani merged commit 4f5a9cb into main Apr 24, 2026
8 checks passed
@sorafujitani sorafujitani deleted the fix/rails-idiom-bugs branch April 24, 2026 02:06
@sorafujitani sorafujitani mentioned this pull request Apr 24, 2026
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@sorafujitani