Skip to content

Fix line coverage of statements inside string interpolation and heredocs - #9682

Merged
headius merged 13 commits into
jruby:masterfrom
sferik:eval-coverage
Oct 2, 2026
Merged

headius merged 13 commits into
jruby:masterfrom
sferik:eval-coverage

Conversation

@sferik

@sferik sferik commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Coverage counted a statement holding a heredoc twice per execution when the heredoc's interpolation sat on a later line, and marked the interpolation's line as code although nothing ever counted there. A statement's coverage event is now emitted once, when the statement starts. The call site only refreshes the line for backtraces (determineIfWeNeedLineNumberForCall). Also, a lone statement inside #{} is not a line event. The string it is part of is, so the parser no longer leaves that line marked as code (uncoverLine). Several statements in one interpolation each remain a line event.

@headius headius left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Calling all @enebo's for a secondary review. This might be fine but it diverges from parse.y and I want to understand why.

Comment thread core/src/main/java/org/jruby/parser/RubyParser.y Outdated
@headius headius added this to the JRuby 10.1.3.0 milestone Sep 16, 2026
@headius

headius commented Sep 16, 2026

Copy link
Copy Markdown
Member

Let's get this in with the other coverage work in 10.1.3.0.

@sferik

sferik commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Let's get this in with the other coverage work in 10.1.3.0.

Sounds good! I also have a branch for branch-coverage that is based on the method-coverage branch, as well as some fixes to oneshot and multiline line coverage. Once all these are merged, we should be able to completely delete test/mri/excludes/TestCoverage.rb.

If we can ship all of that in 10.1.3.0, I think it will be the biggest change to JRuby’s Coverage support since it was introduced!

@sferik

sferik commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

I pushed two more commits:

  • 741d5e4: an undef statement now takes the line of its undef keyword, not the line of the first name it undefines. Before, undef\n a was counted on the second line. In x = 1; undef\n a it was also counted as a separate line event, where CRuby merges it with the other statement on the first line.
  • 6f7ba4d: each arm of a ternary is now a line event, as in CRuby. In x = y.size ?\n 1 : 2, the second line used to read nil.

@headius

headius commented Sep 30, 2026

Copy link
Copy Markdown
Member

There are conflicts possibly because of the merge of #9676. @sferik could you resolve those quick and make sure things still look good?

@headius

headius commented Sep 30, 2026

Copy link
Copy Markdown
Member

At a minimum the parser should be regenerated so it matches, and double-check the excludes and test changes.

@sferik
sferik force-pushed the eval-coverage branch 2 times, most recently from d174e99 to 9ca4372 Compare September 30, 2026 15:20
@sferik

sferik commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

@headius Done.

I’ve also implemented branch coverage in #9751. Can we get that into 10.1.3.0 as well?

@headius headius left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. The IR and AST changes look fine to me, other than adding a bunch of state to IRBuilder that could be moved into a state object with its own management functions.

I added a few other small suggestions for improvement.

The parser changes are hard to grok without comparing to CRuby's parser. In general we try to keep our "legacy" parser grammar as similar to CRuby's as possible, to make future updates easier.

Comment thread core/src/main/java/org/jruby/ir/builder/IRBuilder.java
Comment thread core/src/main/java/org/jruby/parser/RubyParser.java
Comment thread core/src/main/java/org/jruby/parser/RubyParser.y Outdated
Comment thread test/jruby/test_coverage.rb
sferik added a commit to simplecov-ruby/simplecov that referenced this pull request Oct 2, 2026
SimpleCov warned on every JRuby run without --debug that coverage may be
inaccurate, citing jruby/jruby#1196 from the
JRuby 1.7 days. JRuby's coverage no longer depends on full-trace mode.
The whole suite on JRuby 10.0.7, run with and without --debug, reports
every line of SimpleCov's own code identically: the same lines relevant,
the same lines hit. With the branch and line coverage work in
jruby/jruby#9751 and
jruby/jruby#9682 the same holds for every line,
branch, and method.

--debug also slows JRuby down, so the warning only cost the people who
followed it. Drop the warning, the advice in the troubleshooting guide,
and JRUBY_OPTS=--debug from CI.
@sferik

sferik commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I rebased onto master and pushed two commits for it:

  • f1f877a moves the line coverage state out of IRBuilder into a LineNumberInfo that only exists while coverage is on.
  • 3532e92 makes the grammar read like CRuby's. The rules this PR touched now call nd_set_first_loc, make_list, nd_set_loc and nd_unset_fl_newline like CRuby's do, with the JRuby-specific logic in those helpers.

@headius

headius commented Oct 2, 2026

Copy link
Copy Markdown
Member

It's good! The branch coverage PR is merged so just need to rebase and deal with the conflicts.

@sferik
sferik force-pushed the eval-coverage branch 2 times, most recently from 821e571 to dad8b71 Compare October 2, 2026 14:16
@sferik

sferik commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

@headius Rebased!

sferik added 9 commits October 2, 2026 07:52
Coverage counted the statement holding a heredoc twice per execution when
the heredoc's interpolation sat on a later line, and marked that
interpolation's line as code although nothing ever counted there. MRI
reports the statement once and leaves the line nil. Two fixes:

- A statement that is a call re-emitted its coverage line event when it
  built the call itself, if building its receiver or arguments had moved
  the current line elsewhere (an interpolation on another line, statements
  inside parentheses, ...). A statement's coverage event is now emitted
  once, when the statement starts; the call site only refreshes the line
  for backtraces.

- A lone statement inside #{} is not a line event of its own (the string it
  is part of is), so the parser no longer leaves its line marked as code.
  Several statements in one interpolation each remain a line event, as in
  MRI.

Un-excludes test_eval from the MRI coverage tests.
Setting the interpolation's line back to nil also wiped out a mark left by
another statement on that line, such as a statement in a block earlier on
the same line or the first of several statements in one interpolation.
Undo only the mark the lone statement's newline_node added, and only when
no earlier statement had already marked the line.
A statement's line event waits for the statement's first instruction. When
building its value reached a statement of its own first, such as one of
several statements in an interpolation, that statement's event replaced
the outer one, so coverage never counted the outer statement's line:
x = "#{\n  a = 1\n  a\n}" read [0, 1, 1, nil] where MRI reads [1, 1, 1, nil].
When coverage is on, emit the outer statement's pending event before the
inner one replaces it.

A begin statement now asks for its body's line, the line the parser marks
for it, so emitting its event does not count the begin keyword's line.
A statement's line event waits for its first instruction and then took the
line of whatever call had been built since, so o.b =\n  [].size counted on
the [].size line, a line the parser never marked: the results read
[1, 0, 0] instead of counting the statement. Emit the event on the line it
was requested for, then restore the call's line for backtraces, so
backtraces do not change.

As in MRI, a statement is a line event unless the last line event was on
the same line; calls built in between no longer hide one (the 1 in
if\n  [].size then 1 end was not counted). A pending coverage event is no
longer replaced by a call's backtrace line either.
An array literal took the line of its first element, a hash literal the
line of its first key, a string the line its contents end on (or, for
"" "..." concatenated, the line of the second string), a regexp without
interpolation the line its contents end on, and a dynamic symbol, regexp,
or backtick string the start of its contents, which for an empty
production is not a real position (nor is it for the elements of %W, %I
and %i lists, or a "string": label). MRI reports each on the line of its
opening token; use that line for all of them.
MRI counts a statement on the line of its first instruction, which is a
later line whenever evaluating the statement starts with a part on one:
x =\n  [].size counts on line 2, and so does x = <<~EOS whose body starts
with an interpolation (x = <<~EOS\n  #{1.to_s}\nEOS reads [nil, 1, nil]).
JRuby marked and counted the line the statement starts on.

LineEvents finds the node whose instructions come first, following
assignment values, call receivers, conditions, the first element of an
array or key of a hash that MRI does not build as one literal, the first
interpolation of a string that does not start with a literal part, and
so on. The parser marks its line, Coverage.line_stub reports it, and the
IR builder counts the statement on it. As in MRI, a statement inside
another that starts with the same instruction shares its line event,
and a statement is no line event when the last one started on its line.

Along the way:

- The newline flags the lexer puts on heredoc lines for dedenting are
  cleared after dedenting, so the IR builder no longer takes those lines
  for statements.
- An ensure body is built before the body it protects; its line events
  no longer count as the last ones when building the protected body.
- A lone conditional in an interpolation stays a line event, as MRI
  counts its branches.

Against MRI 3.4, Coverage.line_stub now matches for 749 of the 840
files in lib/ruby/stdlib (510 before), with no line that matched before
differing now.

Un-excludes test_line_coverage_for_multiple_lines from the MRI coverage
tests.
After reading a heredoc the lexer goes back to the line the heredoc
started on, and only skips past the heredoc when it reads the next line.
When the heredoc ends the file there is no next line, so the coverage
array stopped short: x = <<~EOS\n  #{1.to_s}\nEOS read [nil, 1] instead of
[nil, 1, nil]. Size it by the last line read instead.
An undef took the line of the first name it undefines, so with that name
on the next line, line coverage counted undef\n  a on the second line
instead of the first, where MRI counts it.
MRI parses the arms of a ternary as statements, so each is a line event
of its own: in x = y.size ?\n  1 : 2, the arm on the second line is counted
by line coverage and reported to line tracing. JRuby parsed them as plain
expressions, so that line read nil.
sferik added 4 commits October 2, 2026 07:52
A oneshot_lines probe now stays armed until its line is actually
counted, so a line first executed while coverage is suspended is
recorded on its first execution after resuming, as in CRuby. That fix
landed on master with method coverage; this removes the exclude that
still kept test_oneshot_line_coverage from running.
The loop that padded the stub with nils compared against lines.size(),
which grows as the loop appends, so it stopped about halfway to the end:
a five-line method whose last statement is on line 2 got a four-entry
stub. A last line with no newline was not counted at all. Pad until the
stub has one entry per line, counting lines as File.foreach does, which
is what CRuby's line_stub sizes its array by.

SimpleCov builds the line coverage of files that were never loaded from
line_stub, so those files were reported with lines missing.

An empty file could still get a stub of [0]. Every parse shares the
NilImplicitNode.NIL singleton, and an if with an empty else as the last
statement of a block marked it as a newline through fixpos, so from then
on the implicit nil at the root of an empty file counted as a line. Make
NIL ignore setNewline and setLine so no parse can change it for later
ones.
The depth of the build, the pending line event's depth and line, and the
last line event were five fields of IRBuilder that only coverage used.
They are now a LineNumberInfo, which the builder creates only when
Coverage measures the scope.
The grammar set where undef statements, list and hash literals and
strings start, and unmarked the lone statement of an interpolation,
inline. Those rules now read as CRuby's do, calling nd_set_first_loc,
make_list, nd_set_loc and nd_unset_fl_newline, which RubyParserBase
implements for our nodes. nd_set_first_loc was an empty stub until now,
so the find and hash patterns that call it (Const(...) and Const[...])
now start at their constant, as in CRuby.

Both parsers were regenerated with jay.
@headius

headius commented Oct 2, 2026

Copy link
Copy Markdown
Member

Broken again by the branch coverage merge!

@headius

headius commented Oct 2, 2026

Copy link
Copy Markdown
Member

Oops, misfire... it's not broken. I'll do final review and merge.

@headius
headius merged commit b4d5c57 into jruby:master Oct 2, 2026
135 checks passed
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