Fix NPE

[poseidon-mysugr]

firstIsALoad0 produces an NPE if the method instructions contain only Non-Opcode instructions.

[marchof]

@poseidon-mysugr I don't think this would be a valid class file. ASM as well as JaCoCo assumes that the input are valid classfiles and has no safeguards against invalid class files.

[poseidon-mysugr]

I've just encountered this in production code, let me see if I can come up with a simple example.

[poseidon-mysugr]

Pretty much any Kotlin interface will trigger this:

interface Foo {
    fun bar()
}

will result in

java.io.IOException: Error while analyzing Foo.class.
        at org.jacoco.core.analysis.Analyzer.analyzerError(Analyzer.java:163)
        at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:134)
        at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:157)
        at org.jacoco.core.analysis.Analyzer.analyzeAll(Analyzer.java:193)
        at org.jacoco.ant.ReportTask.createBundle(ReportTask.java:573)
        at org.jacoco.ant.ReportTask.createReport(ReportTask.java:545)
        at org.jacoco.ant.ReportTask.execute(ReportTask.java:496)
        ... 117 more
Caused by: java.lang.NullPointerException
        at org.jacoco.core.internal.analysis.filter.AbstractMatcher.firstIsALoad0(AbstractMatcher.java:39)
        at org.jacoco.core.internal.analysis.filter.KotlinDefaultMethodsFilter$Matcher.match(KotlinDefaultMethodsFilter.java:36)
        at org.jacoco.core.internal.analysis.filter.KotlinDefaultMethodsFilter$Matcher.access$100(KotlinDefaultMethodsFilter.java:33)
        at org.jacoco.core.internal.analysis.filter.KotlinDefaultMethodsFilter.filter(KotlinDefaultMethodsFilter.java:30)
        at org.jacoco.core.internal.analysis.filter.Filters.filter(Filters.java:59)
        at org.jacoco.core.internal.analysis.ClassAnalyzer.addMethodCoverage(ClassAnalyzer.java:119)
        at org.jacoco.core.internal.analysis.ClassAnalyzer.access$100(ClassAnalyzer.java:33)
        at org.jacoco.core.internal.analysis.ClassAnalyzer$1.accept(ClassAnalyzer.java:108)
        at org.jacoco.core.internal.flow.ClassProbesAdapter$2.visitEnd(ClassProbesAdapter.java:91)
        at org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1288)
        at org.objectweb.asm.ClassReader.accept(ClassReader.java:688)
        at org.objectweb.asm.ClassReader.accept(ClassReader.java:400)
        at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:116)
        at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:132)
        ... 122 more

[marchof]

@Godin Probably we should apply filters to abstract methods?

[Godin]

@poseidon-mysugr Nice catch! 👍 Thank you for reporting this! ❤️

This is regression introduced in unreleased version by #1012 :
your stacktrace contains added by it KotlinDefaultMethodsFilter
and indeed NPE can be reproduced on Foo.kt

interface Foo {
    fun bar()
}

using latest as of now 0.8.6-SNAPSHOT but not 0.8.5

$ kotlinc -version Foo.kt
info: kotlinc-jvm 1.3.61 (JRE 1.8.0_212-b04)

$ java -jar jacoco-0.8.5/lib/jacococli.jar classinfo Foo.class
  INST   BRAN   LINE   METH   CXTY   ELEMENT
     0      0      0      0      0   class 0xa70d8bcb87d3a1dd Foo

$ java -jar jacoco-0.8.6-20200211.104032-32/lib/jacococli.jar classinfo Foo.class
  INST   BRAN   LINE   METH   CXTY   ELEMENT
Exception in thread "main" java.io.IOException: Error while analyzing Foo.class.

Regarding

firstIsALoad0 produces an NPE if the method instructions contain only Non-Opcode instructions

in this case there is simply no instructions at all - see javap -v -p Foo.class 😉

{
  public abstract void bar();
    descriptor: ()V
    flags: ACC_PUBLIC, ACC_ABSTRACT
}

Pretty much any Kotlin interface will trigger this

Unfortunately this was not caught by KotlinDefaultMethodsTest even though KotlinDefaultMethodsTarget.kt produces KotlinDefaultMethodsTarget$I.class

{
  public abstract void m1();
    descriptor: ()V
    flags: ACC_PUBLIC, ACC_ABSTRACT

  public abstract void m2();
    descriptor: ()V
    flags: ACC_PUBLIC, ACC_ABSTRACT

  public abstract void m3();
    descriptor: ()V
    flags: ACC_PUBLIC, ACC_ABSTRACT
}

that is loaded by InstrumentingLoader - only classes for which execution data exists are analyzed in our validation tests 😞 Would be nice to improve this, but this is another story.

Also unfortunately we don't have examples in validation tests for Kotlin that produce execution data and contain abstract methods, such as for example

abstract class B {
    abstract fun f()
}

class A : B() {
  override fun f() {}
}

fun main() {
  A().f()
}

Regarding validity of class files and absence of instructions see https://docs.oracle.com/javase/specs/jvms/se13/html/jvms-4.html#jvms-4.1 :

If neither of the ACC_NATIVE and ACC_ABSTRACT flags are set in the access_flags item of a method_info structure, the Java Virtual Machine instructions implementing the method are also supplied.

java.lang.ClassFormatError: Absent Code attribute in method that is not native or abstract in class file Example
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)

AFAICS all invocations of firstIsALoad0 in JaCoCo 0.8.5 (EnumEmptyConstructorFilter and PrivateEmptyNoArgConstructorFilter) guarded by

"<init>".equals(methodNode.name) && "()V".equals(methodNode.desc)

and such methods can not be ACC_NATIVE or ACC_ABSTRACT - https://docs.oracle.com/javase/specs/jvms/se13/html/jvms-4.html#jvms-4.6 :

An instance initialization method (§2.9.1) may have at most one of its ACC_PUBLIC, ACC_PRIVATE, and ACC_PROTECTED flags set, and may also have its ACC_VARARGS, ACC_STRICT, and ACC_SYNTHETIC flags set, but must not have any of the other flags

java.lang.ClassFormatError: Method <init> in class Example has illegal modifiers: 0x100
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)
java.lang.ClassFormatError: Method <init> in class Example has illegal modifiers: 0x400
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)

In 0.8.6-SNAPSHOT in addition to the above unchanged filters and broken KotlinDefaultMethodsFilter there is also invocation in RecordsFilter, according to https://openjdk.java.net/jeps/359

Records are implicitly final, and cannot be abstract.

---

@marchof I guess that instead

Probably we should apply filters to abstract methods?

you wanted to say "should not apply"? And wondering whether by "abstract methods" you mean

(methodNode.access & Opcodes.ACC_ABSTRACT) == 0 || (methodNode.access & Opcodes.ACC_NATIVE) == 0

or

methodNode.instructions.getFirst() == null

? 😉 On the one hand, we indeed can prevent similar mistakes in the future by not executing filters in ClassAnalyzer for such methods.

On the other hand, I have a feeling that such an increase in coupling of filters with the rest is not good.

And not counting missing unit test, I like proposed change also because AFAICS firstIsALoad0 is the only method in AbstractMatcher that can't cope with null cursor.

[marchof]

@Godin Ok, I agree to fix this with the filter. But what is missing here is a test that our filters can deal with empty methods. I was thinking of adding such a generic test to FilterTestBase.java but most filters have pre-requisites to actually start matching.

[Godin]

@poseidon-mysugr Thanks a lot for this bug report and fix! 👍 ❤️