Skip to content

Fix TLetCorrection corrupting files with interior heredocs - #387

Open
dduugg wants to merge 2 commits into
Shopify:mainfrom
dduugg:fix-t-let-interior-heredoc-corruption
Open

Fix TLetCorrection corrupting files with interior heredocs#387
dduugg wants to merge 2 commits into
Shopify:mainfrom
dduugg:fix-t-let-interior-heredoc-corruption

Conversation

@dduugg

@dduugg dduugg commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Problem

TLetCorrection (shared by Sorbet/RedundantTLet and Sorbet/RedundantTLetForLiteral) corrupts files when the T.let value contains an interior heredoc — one that is followed by more of the value's own source.

The mixin was written for heredocs at the tail of the value, where value_node.source stops at the marker and the body must be reattached. But when a heredoc sits in the interior of a larger value (e.g. a constructor call that continues after the heredoc), value_node.source already contains the heredoc body inline, so reattaching it duplicates the body.

Minimal repro:

PATH = T.let(
  Foo.new(
    a: <<~DIR,
      /usr/local
    DIR
    b: 2,
  ),
  Foo,
)

Autocorrect currently produces a duplicated, syntactically broken body:

PATH = Foo.new(
    a: <<~DIR,
      /usr/local
    DIR
    b: 2,
  )
      /usr/local   # <- duplicated
    DIR            # <- duplicated

Fix

Only reattach heredocs whose body/terminator extend past the value node's own source range (the tail heredocs). Interior heredocs already live inside value_node.source, so the plain corrector.replace(t_let_node, value_node.source) path handles them correctly.

Tests

Added test_offense_on_constant_constructor_with_interior_heredoc covering the case above. Full suite green (bundle exec rake test).

dduugg added 2 commits July 16, 2026 21:09
…ior heredocs

TLetCorrection re-appended every heredoc in the value when unwrapping
T.let. That is only correct for a heredoc at the tail of the value, whose
body sits past the value node's source range. An interior heredoc — one
followed by more of the value, e.g. `Foo.new(a: <<~X, b: 2)` — already
lives inside `value_node.source`, so re-appending duplicated its body and
corrupted the file. Only reattach tail heredocs.
Shorten the tail_heredocs and interior-heredoc test comments to match
the concise, 2-3 line style used by sibling comments elsewhere in this
file and the surrounding test file.
@dduugg
dduugg marked this pull request as ready for review July 20, 2026 23:15
@dduugg
dduugg requested a review from a team as a code owner July 20, 2026 23:15
def replace_t_let(corrector, t_let_node, value_node)
heredocs = value_node.each_node(:any_str).select(&:heredoc?)
heredocs = tail_heredocs(value_node)
return corrector.replace(t_let_node, value_node.source) if heredocs.empty?

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.

Nit: if heredocs.empty? reads funny. Can we either change the variable name (heredocs_to_reattach?) or add a comment to indicate these heredocs require special handling.

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.

2 participants