Repository navigation
Match CRuby’s line coverage and branch locations - #9770
Conversation
headius
left a comment
There was a problem hiding this comment.
Yes, looks good. Glad to see the simplification of predicate handling in the parser. More state is added to AST nodes so still need to revisit the size increases and look at packing state. Only changes needed are moving the coverage tests into RubySpec under spec/ruby (matching style and aiming for small expectations where possible).
An IfNode is always a newline node, so the IR builder emits a line event for its predicate, but the parser only marked the lines of statements. A conditional used as a value (`@a ||=\n if x`, an `unless` array element, a ternary argument) and every elsif therefore counted on a line the results report as having no code: the line read 0 after running once, where MRI reads 1, and an elsif that never ran read nil instead of 0. The parser now marks the predicate's line when it builds the IfNode. So that a node found later not to be a line event can give its line back whichever node marked it last, the marks are now kept per node, with a count per line, instead of remembering only the most recent one.
MRI compiles nothing for a statement whose value goes unused when it can have no effect: a literal, self, a read of a local, instance or class variable, a static array or hash, and a local variable assigned to itself, which its peephole optimizer removes. Such a statement has no instruction to carry a line event, so its line has no count. JRuby marked these lines as it parsed them but never counted them, so each read 0, a line that never ran, though the method around it did. void_stmts, which already visits each statement before the last of a list, now unmarks those statements and everything in them.
MRI folds a conditional on a literal away (`if false`, `unless true`, `x = 1 unless true`): it compiles neither the arm that cannot run nor, when only literals decide it, the predicate, and a statement made of such a conditional starts with the first instruction of the arm that runs. JRuby marked every one of those lines and counted the predicate's, so the folded `if` line read 1 and the dead arm's lines 0 where MRI reports nil, and a void `if false ... end` read as a statement that ran. new_if now records whether the predicate folds and unmarks the dead arm, LineEvents starts a folded conditional at its live arm, and a folded conditional whose live arm compiles to nothing compiles to nothing itself. A constant predicate that still runs code, as in `if x and false`, keeps its line event. The constant predicate is now found once, in new_if, so markBranch no longer needs the predicate.
To report the right line for `foo "a", "x\ny" unless cond` (jruby#9293), fixpos makes the statement of a modifier conditional a newline node. With coverage on, that made it a line event of its own, counted on a line the parser never marked: a multi-line statement before its condition, as in `foo 1,\n 2 if y`, read 0 on its first line where MRI reports nil. A node can now be a newline for backtraces only: the IR builder restores its line without a coverage event, and Coverage.line_stub skips it. In an interpolation, prism unsets the newline flag of a lone statement even when it is a conditional. The conditional's statements remain line events, and a modifier's statement, whose own event the conditional's otherwise hides, then counts: `"#{n} item#{'s' unless n == 1}"` counts its line once more, as in MRI.
A statement starting on the line of the last line event is no line event of its own, and which event came last follows the order the builder emits them in. MRI compiles a case's arms in order and its else after them. JRuby kept a case/when's bodies in a HashMap and built them in whatever order it iterated, and built a case/in's else before its arms. With an arm and the else on one line, as in `case x; in 1 then 2; else 3; end`, the line read 0 after the arm ran and 1 after only the else did, where MRI reads 1 and 0.
MRI reports a branch arm that is a call with a block, as in
`x ? list.map { |v| v } : 0`, through to the end of the block, and a
safe-navigation branch only to the end of the call's arguments: without
a block or block argument (`o&.m(&b)`, `o&.m 1, &b`, `o&.m { }`), and to
the end of the method name when there are no other arguments
(`o&.m()`). JRuby kept a call to the span of the production that made
it, so an arm ended before its block, and a safe navigation included a
block argument.
A call now ends with its block. The grammar records where a safe
navigation ends from the positions of the method name and arguments,
and its branch is reported to there.
Four kinds of branch arm started or ended in the wrong place:
- A heredoc token (`x ? <<~A : 2`) started at the end of its line,
where the lexer moves on to read the body, and so after its own end.
It now spans its `<<~A`.
- A character literal (`?a`) started one column late: newtok moved the
token's start past its `?`.
- A lambda started at its body, the span method coverage reports. As an
arm it now starts at its `->`.
- An `END { }` had no span of its own and was reported at the end of
the previous line. It now spans its keyword through its brace.
When the IR builder emits a line event for a line the parser did not mark, the count went from -1 to 0, and on to n - 1 after n runs: a line that ran read as one that never did. MRI only counts lines that have a line event, so such a line now keeps no count and reads nil, as it would if the event were not there.
7c94bbe to
4a8e60d
Compare
What would you propose as the order of operations? My inclination would be to:
Does that make sense or would you propose something different? |
|
Specs added to spec/ruby in our repo our periodically synced with the master copy at https://github.com/ruby/spec, so just including them in this PR would be fine. I'll double check documentation to make sure that is pointed out more clearly for humans and agents. It has been difficult getting agents to write ruby specs rather than JRuby specific tests. |
The line, branch and method coverage tests in test_coverage.rb mostly describe what any Ruby reports, so they belong in spec/ruby. Each example holds one construct and one small expectation, with the source next to the lines and columns it expects.
|
Ah, I didn’t realize that. How does this look? |
headius
left a comment
There was a problem hiding this comment.
Yep that looks better, and the individual specs are pretty good size.
Since #9682 and #9751 were merged, I tried running the SimpleCov test suite against jruby-head and found a few more discrepancies between CRuby and JRuby’s Coverage implementations:
IfNodeis always a line event, but the parser only marked the lines of statements. A conditional used as a value (@a ||=\n if x, an array element, a call argument) read 0 after running, and anelsifthat never ran read nil instead of 0. The parser's line marks are now kept per node, so a node can give its line back later.self, variable reads, static arrays and hashes, orx = x. JRuby marked those lines and never counted them, so they read 0.if false,unless trueand similar: neither the dead arm nor a literal predicate has a line event, and the statement counts on its live arm.if x and falsekeeps its line, since it still callsx.foo 1,\n 2 if y) a line event. That line read 0 where MRI reads nil. Such a node now restores its line for backtraces without a coverage event, and the backtraces from that issue are unchanged. A conditional alone in an interpolation is no line event either, as in prism.case/whenbuilt its bodies inHashMaporder, and acase/inbuilt itselsefirst. With an arm and theelseon one line, the line read 0 when the arm ran and 1 when only theelseran. MRI reads 1 and 0.&.branch ends before a block argument or block, and at the method name when there are no other arguments (o&.m(),o&.m(&b)).<<~Aand?ain the lexer, start a lambda arm at its->, and giveEND { }its own span.