Apply Jacoco's instruction filtering to Java branch coverage

Jacoco applies a set of filters during its coverage analysis to remove
various compiler constructs that could lead to surprising outputs in
coverage results, leaving behind coverage data that closer fits the
code given to the compiler.

A simple java example is the function:

```
public int foo(String input) {
  switch (input) {
    case "A":
      return 1;
    case "B":
      return 2;
    case "C":
      return 3;
    default:
      return -1
  }
}
```

This would generate at least double the number of expected branches as
the compiler generates a second set of branches to operate on hashCode.
Jacoco would normally ignore the first set of branches, reducing the
number to the expected 4.

Because Bazel applies its own branch analysis step, Bazel's branch
coverage output does not benefit from this filtering.

This change adapts the custom analyzer to record the necessary
information during the ASM Tree walk in the same manner as Jacoco's
regular analyzer in order to apply the filters.

The filters have three output operations:

 * ignore - ignore a range of instructions
 * merge - merges the coverage information of two instructions
 * replaceBranches - retarget the output branches of an instruction

The first of these, ignore, is by far the simplest to implement and
covers the vast majority of use cases in Java.

By mapping the AbstractInsnNode objects recorded during the ASM Tree
walk to the Instruction objects used during branch analysis, we can
simply skip over Instruction objects that have had their associated
AbstractInsnNode object ignored by a filter.

The remaining operations, merge and replaceBranches, are left
unimplemeted for now; these require more care to handle, are harder
to test, and affect only a minority of cases, specifically:

 * merge is only used to handle duplication of finally blocks
 * replaceBranches is used by Kotlin and Eclipse compiler filters

Part of #12696

Closes #13896.

PiperOrigin-RevId: 395235432

Author: c-mita

src/java_tools/junitrunner/java/com/google/testing/coverage/BranchDetailAnalyzer.java

@@ -114,7 +114,7 @@ private IOException analyzerError(final String location, final Exception cause)
 
   // Generate the line to probeExp map so that we can evaluate the coverage.
   private Map<Integer, BranchExp> mapProbes(final ClassReader reader) {
-    final ClassProbesMapper mapper = new ClassProbesMapper();
+    final ClassProbesMapper mapper = new ClassProbesMapper(reader.getClassName());
     final ClassProbesAdapter adapter = new ClassProbesAdapter(mapper, false);
     reader.accept(adapter, 0);
 

src/java_tools/junitrunner/java/com/google/testing/coverage/ClassProbesMapper.java

@@ -14,30 +14,81 @@
 
 package com.google.testing.coverage;
 
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 import java.util.TreeMap;
+import org.jacoco.core.internal.analysis.StringPool;
+import org.jacoco.core.internal.analysis.filter.Filters;
+import org.jacoco.core.internal.analysis.filter.IFilter;
+import org.jacoco.core.internal.analysis.filter.IFilterContext;
 import org.jacoco.core.internal.flow.ClassProbesVisitor;
 import org.jacoco.core.internal.flow.MethodProbesVisitor;
+import org.objectweb.asm.AnnotationVisitor;
+import org.objectweb.asm.Attribute;
 import org.objectweb.asm.FieldVisitor;
 
 /** A visitor that maps each source code line to the probes corresponding to the lines. */
-public class ClassProbesMapper extends ClassProbesVisitor {
+public class ClassProbesMapper extends ClassProbesVisitor implements IFilterContext {
   private Map<Integer, BranchExp> classLineToBranchExp;
 
+  private IFilter allFilters = Filters.all();
+
+  private StringPool stringPool;
+
+  // IFilterContext state updating during visitations
+  private String className;
+  private String superClassName;
+  private Set<String> classAnnotations = new HashSet<>();
+  private Set<String> classAttributes = new HashSet<>();
+  private String sourceFileName;
+  private String sourceDebugExtension;
+
   public Map<Integer, BranchExp> result() {
     return classLineToBranchExp;
   }
 
   /** Create a new probe mapper object. */
-  public ClassProbesMapper() {
+  public ClassProbesMapper(String className) {
     classLineToBranchExp = new TreeMap<Integer, BranchExp>();
+    stringPool = new StringPool();
+    className = stringPool.get(className);
+  }
+
+  @Override
+  public AnnotationVisitor visitAnnotation(final String desc, final boolean visible) {
+    classAnnotations.add(desc);
+    return super.visitAnnotation(desc, visible);
+  }
+
+  @Override
+  public void visitAttribute(final Attribute attribute) {
+    classAttributes.add(attribute.type);
+  }
+
+  @Override
+  public void visitSource(final String source, final String debug) {
+    sourceFileName = stringPool.get(source);
+    sourceDebugExtension = debug;
+  }
+
+  @Override
+  public void visit(
+      int version,
+      int access,
+      String name,
+      String signature,
+      String superName,
+      String[] interfaces) {
+    superClassName = stringPool.get(name);
   }
 
   /** Returns a visitor for mapping method code. */
   @Override
   public MethodProbesVisitor visitMethod(
       int access, String name, String desc, String signature, String[] exceptions) {
-    return new MethodProbesMapper() {
+    return new MethodProbesMapper(this, allFilters) {
+
       @Override
       public void visitEnd() {
         super.visitEnd();
@@ -56,4 +107,34 @@ public FieldVisitor visitField(
   public void visitTotalProbeCount(int count) {
     // Nothing to do. Maybe perform some checks here.
   }
+
+  @Override
+  public String getClassName() {
+    return className;
+  }
+
+  @Override
+  public String getSuperClassName() {
+    return superClassName;
+  }
+
+  @Override
+  public String getSourceDebugExtension() {
+    return sourceDebugExtension;
+  }
+
+  @Override
+  public String getSourceFileName() {
+    return sourceFileName;
+  }
+
+  @Override
+  public Set<String> getClassAnnotations() {
+    return classAnnotations;
+  }
+
+  @Override
+  public Set<String> getClassAttributes() {
+    return classAttributes;
+  }
 }

src/java_tools/junitrunner/java/com/google/testing/coverage/MethodProbesMapper.java

@@ -16,22 +16,30 @@
 
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.TreeMap;
 import org.jacoco.core.internal.analysis.Instruction;
+import org.jacoco.core.internal.analysis.filter.IFilter;
+import org.jacoco.core.internal.analysis.filter.IFilterContext;
+import org.jacoco.core.internal.analysis.filter.IFilterOutput;
 import org.jacoco.core.internal.flow.IFrame;
 import org.jacoco.core.internal.flow.LabelInfo;
 import org.jacoco.core.internal.flow.MethodProbesVisitor;
 import org.objectweb.asm.Handle;
 import org.objectweb.asm.Label;
+import org.objectweb.asm.MethodVisitor;
+import org.objectweb.asm.tree.AbstractInsnNode;
+import org.objectweb.asm.tree.MethodNode;
 
 /**
  * The mapper is a probes visitor that will cache control flow information as well as keeping track
  * of the probes as the main driver generates the probe ids. Upon finishing the method it uses the
  * information collected to generate the mapping information between probes and the instructions.
  */
-public class MethodProbesMapper extends MethodProbesVisitor {
+public class MethodProbesMapper extends MethodProbesVisitor implements IFilterOutput {
   /*
    * The implementation roughly follows the same pattern of the Analyzer class of Jacoco.
    *
@@ -57,6 +65,13 @@ public class MethodProbesMapper extends MethodProbesVisitor {
   private Instruction lastInstruction = null;
   private int currentLine = -1;
   private List<Label> currentLabels = new ArrayList<>();
+  private AbstractInsnNode currentInstructionNode = null;
+  private Map<AbstractInsnNode, Instruction> instructionMap = new HashMap<>();
+
+  // Filtering
+  private IFilter filter;
+  private IFilterContext filterContext;
+  private HashSet<AbstractInsnNode> ignored = new HashSet<>();
 
   // Result
   private Map<Integer, BranchExp> lineToBranchExp = new TreeMap();
@@ -87,6 +102,22 @@ public Map<Integer, BranchExp> result() {
   private Map<Instruction, Instruction> predecessors = new HashMap<>();
   private Map<Label, Instruction> labelToInsn = new HashMap<>();
 
+  public MethodProbesMapper(IFilterContext filterContext, IFilter filter) {
+    this.filterContext = filterContext;
+    this.filter = filter;
+  }
+
+  @Override
+  public void accept(MethodNode methodNode, MethodVisitor methodVisitor) {
+    methodVisitor.visitCode();
+    for (AbstractInsnNode i : methodNode.instructions) {
+      currentInstructionNode = i;
+      i.accept(methodVisitor);
+    }
+    filter.filter(methodNode, filterContext, this);
+    methodVisitor.visitEnd();
+  }
+
   /** Visitor method to append a new Instruction */
   private void visitInsn() {
     Instruction instruction = new Instruction(currentLine);
@@ -101,6 +132,7 @@ private void visitInsn() {
     }
     currentLabels.clear(); // Update states
     lastInstruction = instruction;
+    instructionMap.put(currentInstructionNode, instruction);
   }
 
   // Plain visitors: called from adapter when no probe is needed
@@ -317,6 +349,12 @@ private boolean updateBranchPredecessor(Instruction predecessor, Instruction ins
   /** Finishing the method */
   @Override
   public void visitEnd() {
+    HashSet<Instruction> ignoredInstructions = new HashSet<>();
+    for (Map.Entry<AbstractInsnNode, Instruction> entry : instructionMap.entrySet()) {
+      if (ignored.contains(entry.getKey())) {
+        ignoredInstructions.add(entry.getValue());
+      }
+    }
 
     for (Jump jump : jumps) {
       Instruction insn = labelToInsn.get(jump.target);
@@ -328,6 +366,9 @@ public void visitEnd() {
     for (Map.Entry<Integer, Instruction> entry : probeToInsn.entrySet()) {
       int probeId = entry.getKey();
       Instruction ins = entry.getValue();
+      if (ignoredInstructions.contains(ins)) {
+        continue;
+      }
 
       Instruction insn = ins;
       CovExp exp = new ProbeExp(probeId);
@@ -372,6 +413,9 @@ public void visitEnd() {
 
     // Merge branches in the instructions on the same line
     for (Instruction insn : instructions) {
+      if (ignoredInstructions.contains(insn)) {
+        continue;
+      }
       if (insn.getBranchCounter().getTotalCount() > 1) {
         CovExp insnExp = insnToCovExp.get(insn);
         if (insnExp != null && (insnExp instanceof BranchExp)) {
@@ -391,6 +435,26 @@ public void visitEnd() {
     }
   }
 
+  /** IFilterOutput */
+  // Handle only ignore for now; most filters only use this.
+  @Override
+  public void ignore(AbstractInsnNode fromInclusive, AbstractInsnNode toInclusive) {
+    for (AbstractInsnNode n = fromInclusive; n != toInclusive; n = n.getNext()) {
+      ignored.add(n);
+    }
+    ignored.add(toInclusive);
+  }
+
+  @Override
+  public void merge(AbstractInsnNode i1, AbstractInsnNode i2) {
+    // TODO(cmita): Implement as part of https://github.com/bazelbuild/bazel/issues/12696
+  }
+
+  @Override
+  public void replaceBranches(AbstractInsnNode source, Set<AbstractInsnNode> newTargets) {
+    // TODO(cmita): Implement as part of https://github.com/bazelbuild/bazel/issues/12696
+  }
+
   /** Jumps between instructions and labels */
   class Jump {
     public final Instruction source;

src/test/shell/bazel/BUILD

@@ -479,6 +479,7 @@ sh_test(
         "//src/test/shell/bazel/testdata:jdk_http_archives_filegroup",
     ],
     tags = [
+        "manual",
         "no_windows",
     ],
 )
@@ -502,6 +503,7 @@ sh_test(
             "//src/test/shell/bazel/testdata:jdk_http_archives_filegroup",
         ],
         tags = [
+            "manual",
             "no_windows",
         ],
     )

src/test/shell/bazel/bazel_coverage_java_test.sh

@@ -693,4 +693,95 @@ end_of_record"
   assert_coverage_result "$expected_result_random" ${coverage_file_path}
 }
 
+function test_java_string_switch_coverage() {
+  # Verify that Jacoco's filtering is being applied.
+  # Switches on strings generate over double the number of expected branches
+  # (because a switch on String::hashCode is made first) - these branches should
+  # be filtered.
+  cat <<EOF > BUILD
+java_test(
+    name = "test",
+    srcs = glob(["src/test/**/*.java"]),
+    test_class = "com.example.TestSwitch",
+    deps = [":switch-lib"],
+)
+
+java_library(
+    name = "switch-lib",
+    srcs = glob(["src/main/**/*.java"]),
+)
+EOF
+
+  mkdir -p src/main/com/example
+  cat <<EOF > src/main/com/example/Switch.java
+package com.example;
+
+public class Switch {
+
+  public static int switchString(String input) {
+    switch (input) {
+      case "AA":
+        return 0;
+      case "BB":
+        return 1;
+      case "CC":
+        return 2;
+      default:
+        return -1;
+    }
+  }
+}
+EOF
+
+  mkdir -p src/test/com/example
+  cat <<EOF > src/test/com/example/TestSwitch.java
+package com.example;
+
+import static org.junit.Assert.assertEquals;
+import org.junit.Test;
+
+public class TestSwitch {
+
+  @Test
+  public void testValues() {
+    assertEquals(Switch.switchString("CC"), 2);
+    // "Aa" has a hash collision with "BB"
+    assertEquals(Switch.switchString("Aa"), -1);
+    assertEquals(Switch.switchString("DD"), -1);
+  }
+
+}
+EOF
+
+  bazel coverage --test_output=all //:test --coverage_report_generator=@bazel_tools//tools/test:coverage_report_generator --combined_report=lcov &>$TEST_log \
+   || echo "Coverage for //:test failed"
+
+  local coverage_file_path="$( get_coverage_file_path_from_test_log )"
+
+  local expected_result="SF:src/main/com/example/Switch.java
+FN:3,com/example/Switch::<init> ()V
+FN:6,com/example/Switch::switchString (Ljava/lang/String;)I
+FNDA:0,com/example/Switch::<init> ()V
+FNDA:1,com/example/Switch::switchString (Ljava/lang/String;)I
+FNF:2
+FNH:1
+BRDA:6,0,0,0
+BRDA:6,0,1,0
+BRDA:6,0,2,1
+BRDA:6,0,3,1
+BRF:4
+BRH:2
+DA:3,0
+DA:6,1
+DA:8,0
+DA:10,0
+DA:12,1
+DA:14,1
+LH:3
+LF:6
+end_of_record"
+
+  assert_coverage_result "$expected_result" "$coverage_file_path"
+}
+
 run_suite "test tests"