Fix line coverage of statements inside string interpolation and heredocs - #9682
Conversation
|
Let's get this in with the other coverage work in 10.1.3.0. |
Sounds good! I also have a branch for 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! |
|
I pushed two more commits:
|
6f7ba4d to
7cf341b
Compare
|
At a minimum the parser should be regenerated so it matches, and double-check the excludes and test changes. |
d174e99 to
9ca4372
Compare
headius
left a comment
There was a problem hiding this comment.
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.
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.
|
Thanks for the review! I rebased onto master and pushed two commits for it:
|
|
It's good! The branch coverage PR is merged so just need to rebase and deal with the conflicts. |
821e571 to
dad8b71
Compare
|
@headius Rebased! |
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.
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.
|
Broken again by the branch coverage merge! |
|
Oops, misfire... it's not broken. I'll do final review and merge. |
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.