Fix deadcode removal eating a preceding heredoc

[dduugg]

Motivation

When spoom deadcode remove removes a definition, NodeRemover walks upward from it to absorb any attached sigs, comments, and a single preceding blank line. To decide whether a blank line separates the definition from the statement above it, it compared against that statement's location.end_line.

Prism reports a node that ends in a heredoc as ending on the heredoc's opening line, exposing the terminator separately through closing_loc. So a statement like:

def_node_matcher :definition_name, <<~PATTERN
  (send nil? :foo)
PATTERN

sig { void }
def on_send(node)
  ...
end

is reported as ending on its first line. The remover then treated the heredoc body and its PATTERN terminator as blank filler between the two statements and deleted them together with on_send, leaving a dangling, unterminated heredoc and thus invalid Ruby.

Implementation

Add node_end_line, which descends into a node's children and, for any heredoc string, uses its closing_loc start line as the real last line the node occupies. The blank-line accounting in delete_node_and_comments_and_sigs now uses this instead of location.end_line, so it stops at the terminator rather than eating the heredoc.

Tests

Added test_removes_method_defined_after_a_heredoc_argument and test_removes_method_defined_after_an_interpolated_heredoc_argument (covering the InterpolatedStringNode path, which is what real def_node_matcher patterns with #{...} produce). The full remover suite and srb tc pass.

[amomchilov]

Can this ever be false?

[dduugg]

Yes, in two distinct ways — this is a necessary filter, not dead code:

1. opening_loc can be nil entirely, e.g. for implicit string concatenation:

   x = "foo" "bar"

   Prism represents this as an InterpolatedStringNode with opening_loc == nil (and closing_loc == nil), wrapping the individual StringNode parts. Hence the &. on opening.

2. opening_loc can be present but not start with "<<" for any non-heredoc string/xstring, since the case here matches on StringNode/InterpolatedStringNode/XStringNode/InterpolatedXStringNode broadly, not just heredocs:

   x = "foo"     # opening_loc.slice == "\""
   x = ?a        # opening_loc.slice == "?", closing_loc == nil
   x = `foo`     # opening_loc.slice == "`"

   Only actual heredocs have opening text like "<<~HEREDOC" or "<<-EOS".

Since node_end_line walks every descendant node (via compact_child_nodes), heredoc_terminator_line gets called on all string-like nodes it encounters, not just heredocs — so this check is what distinguishes the heredoc case from the rest.

Good catch that neither case was tested — pushed test_removes_method_defined_after_an_adjacent_string_concatenation and test_removes_method_defined_after_a_plain_string_argument to cover them.

[KaanOzkan]

node_end_line(before) can be cached and re-used, currently there are 3 calls.

[dduugg]

Done — cached as before_end_line.

It ends up as two calls rather than one, though: a few lines above, before is swapped for an intervening comment (before = comment if comment), so the value passed to comments_between_lines is computed on a different object than the one the comparison below needs. Cached the post-swap value, which is where the duplicated pair was.

[KaanOzkan]

Should node_end_line be used here too? Then could we remove the case statement right below?

[dduugg]

Yes to both — done. end_line = node_end_line(node), and the case statement (plus its TODO: remove once Prism location are fixed, which I moved onto node_end_line) is gone.

It turned out to be more than a cleanup: the special case only recognized a value that was directly a Prism::StringNode, so it missed two shapes that node_end_line handles:

BAR = build(<<~MSG)   # value is a CallNode
  Some text
MSG

BAR = <<~MSG          # value is an InterpolatedStringNode
  Some #{text}
MSG

Removing BAR in either case used to leave the body and terminator behind as orphaned lines:

class Foo
  def bar; end
    Some text
  MSG

  def baz; end
end

Added test_removes_multiline_const_with_heredoc_passed_to_a_call and test_removes_multiline_const_with_interpolation; both fail without the change.

[KaanOzkan]

Suggested change

	def test_removes_method_defined_after_a_plain_string_argument
	def test_removes_method_defined_after_a_plain_string_assignment

[dduugg]

Applied — thanks, "assignment" is right, there's no argument involved.

[KaanOzkan]

Suggested change

	stack = node.compact_child_nodes
	stack = [node]

We should include node itself in the traversal, since a heredoc can be the node itself.

[dduugg]

Applied — and you're right that it's reachable, not just theoretical. A bare heredoc statement ahead of a dead definition:

class Foo
  <<~MSG
    hello
  MSG

  sig { void }
  def on_send(node)
    something
  end

  def baz; end
end

Removing on_send produced an unterminated heredoc, i.e. invalid Ruby:

class Foo
  <<~MSG

  def baz; end
end

Added test_removes_method_defined_after_a_bare_heredoc, which fails without this change.