Bug #16967: Branch coverage duplicates branches inside ensure

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 2.8.0dev (2020-06-05T21:26:28Z master ca15b7b8ee) [x86_64-openbsd6.7]
• Backport: 2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN

[jeremyevans0 (Jeremy Evans)]

The following file, which should have perfect coverage, is reported as only have 50% of the branches covered:

def a
  yield
ensure
  p 1
  p 2 if $!
end
a{}
a{raise} rescue nil

The branches reported by branch coverage are:

{[:if, 0, 5, 2, 5, 11]=>
  {[:then, 1, 5, 2, 5, 5]=>1, [:else, 2, 5, 2, 5, 11]=>0},
 [:if, 3, 5, 2, 5, 11]=>
  {[:then, 4, 5, 2, 5, 5]=>0, [:else, 5, 5, 2, 5, 11]=>1}}

Instead of reporting 2 branches for the if, 4 branches are reported:

1. exception raised, if condition true
2. exception raised, if condition false
3. exception not raised, if condition true
4. exception not raised, if condition false

In this example, it is impossible to cover branches 2 and 3, because the if condition is only true if an exception is raised.

Note that ensure blocks by themselves are not considered branches. This code results in no branches reported by branch coverage:

def a
  yield
ensure
  p $!
end
a{}
a{raise} rescue nil

Nested ensure usage duplicates all branches. This code with 3 nested ensures generates 16 branches:

def a
  yield
ensure
  begin
  ensure
    begin
    ensure
      p 1
      p 2 if $!
    end
  end
end
a{}
a{raise} rescue nil

I think this is a bug in the coverage library, and that it should not duplicate branches inside ensure. This issue is not theoretical, it affects branch coverage testing in my libraries.

[mame (Yusuke Endoh)]

This occurs because one "ensure" clause is duplicated in bytecode: one is for normal exit, the other is for exceptional exit. Consider the following code:

begin
  maincode
ensure
  hellohello
end

It is compiled as follows:

$ ruby --dump=i t.rb
== disasm: #<ISeq:<main>@t.rb:1 (1,0)-(5,3)> (catch: TRUE)
== catch table
| catch type: ensure st: 0000 ed: 0003 sp: 0001 cont: 0007
| == disasm: #<ISeq:ensure in <main>@t.rb:4 (4,2)-(4,12)> (catch: TRUE)
| local table (size: 1, argc: 0 [opts: 0, rest: -1, post: 0, block: -1, kw: -1@-1, kwrest: -1])
| [ 1] $!@0
| 0000 putself                                                          (   4)[Li]
| 0001 opt_send_without_block                 <calldata!mid:hellohello, argc:0, FCALL|VCALL|ARGS_SIMPLE>
| 0003 pop
| 0004 getlocal_WC_0                          $!@0
| 0006 throw                                  0
|------------------------------------------------------------------------
0000 putself                                                          (   2)[Li]
0001 opt_send_without_block                 <calldata!mid:maincode, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0003 putself                                                          (   4)[Li]
0004 opt_send_without_block                 <calldata!mid:hellohello, argc:0, FCALL|VCALL|ARGS_SIMPLE>
0006 pop
0007 leave

You can see two calls to hellohello. The call in "catch table" is for exception handling case, and the other after the call to "maincode" is for normal exit case.

We need to keep track of the copies, and merge the coverage result. @ko1 (Koichi Sasada) what do you think?

[jeremyevans0 (Jeremy Evans)]

Here is a lightly tested workaround that merges the coverage:

        Coverage.singleton_class.prepend(Module.new do
          def result
            res = super
            check_branch = true
            skip_2nd = lambda do |ary|
              ary = ary.dup
              ary.slice!(1)
              ary
            end
            res.values.each do |hash|
              if check_branch
                unless hash.is_a?(Hash) && hash[:branches]
                  return res
                end
                check_branch = false
              end
              unique_branches = {}
              branch_counters = {}
              new_branches = {}
              branches = hash[:branches]
              branches.each do |k, v|
                new_k = skip_2nd[k]
                if branch_values = unique_branches[new_k]
                  v.each do |k1, v1|
                    branch_counters[skip_2nd[k1]] += v1
                  end
                  branch_values.keys.each do |k1|
                    branch_values[k1] = branch_counters[skip_2nd[k1]]
                  end
                else
                  unique_branches[new_k] = new_branches[k] = v
                  v.each do |k1, v1|
                    branch_counters[skip_2nd[k1]] = v1
                  end
                end
              end
              hash[:branches] = new_branches
            end
            res
          end
        end)
      end

We'd probably want a more efficient version in core. Also, I'm assuming the approach is sound, but I'm not sure about that. It ignores the 2nd array element when merging, which I think is the unique id, relying solely on the line/column information to determine which branches are equivalent.

[mame (Yusuke Endoh)]

https://github.com/ruby/ruby/pull/3240

[mame (Yusuke Endoh)]

• Status changed from Open to Closed

Applied in changeset git|50efa18c6cf102e070ba0f95edc01c72516495a3.

---

compile.c: Improve branch coverage instrumentation [Bug #16967]

Formerly, branch coverage measurement counters are generated for each
compilation traverse of the AST. However, ensure clause node is
traversed twice; one is for normal-exit case (the resulted bytecode is
embedded in its outer scope), and the other is for exceptional case (the
resulted bytecode is used in catch table). Two branch coverage counters
are generated for the two cases, but it is not desired.

This changeset revamps the internal representation of branch coverage
measurement. Branch coverage counters are generated only at the first
visit of a branch node. Visiting the same node reuses the
already-generated counter, so double counting is avoided.