Report code coverage correctly for Kotlin methods with default arguments

[Godin]

As far as I understood

• Kotlin generates synthetic method with suffix $default that executes code to create default arguments and pass them to non-synthetic method that contains code of a method
• penultimate argument of synthetic method - is an int that specifies which default arguments are used (bit mask), therefore introduces two branches per default argument
• line numbers in this methods - are line numbers of default arguments
• first line number associated with instruction straight after first bit mask check
• synthetic method is not used for invocations that do not use default arguments

See https://github.com/JetBrains/kotlin/blob/v1.2.71/compiler/ir/backend.common/src/org/jetbrains/kotlin/backend/common/lower/DefaultArgumentStubGenerator.kt

Since

• default arguments are human-written expressions
• and IMO makes sense to test both cases - usage of default and custom argument, i.e. generated branches

I propose to simply stop filtering out such methods. And later we might think about first two branches that are not associated with any line number.

Fixes #773

After this change for the same example as in #773 :

[Godin]

Actually there is one branch that is impossible to cover, because synthetic method is not used for invocation without default arguments, i.e. when bit mask is zero:

object KotlinDefaultArgumentsTarget {

    @JvmStatic
    fun main(args: Array<String>) {
        one()
        /* next invocation doesn't use synthetic method: */
        one("a")
    }

    private fun one(a: String = "a") {
    }

}

  public static final void main(java.lang.String[]);
    descriptor: ([Ljava/lang/String;)V
    flags: ACC_PUBLIC, ACC_STATIC, ACC_FINAL
    Code:
      stack=4, locals=1, args_size=1
         0: aload_0
         1: ldc           #10                 // String args
         3: invokestatic  #16                 // Method kotlin/jvm/internal/Intrinsics.checkParameterIsNotNull:(Ljava/lang/Object;Ljava/lang/String;)V
         6: getstatic     #20                 // Field INSTANCE:LKotlinDefaultArgumentsTarget;
         9: aconst_null
        10: iconst_1
        11: aconst_null
        12: invokestatic  #24                 // Method one$default:(LKotlinDefaultArgumentsTarget;Ljava/lang/String;ILjava/lang/Object;)V
        15: getstatic     #20                 // Field INSTANCE:LKotlinDefaultArgumentsTarget;
        18: ldc           #26                 // String a
        20: invokespecial #30                 // Method one:(Ljava/lang/String;)V
        23: return
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0      24     0  args   [Ljava/lang/String;
      LineNumberTable:
        line 5: 6
        line 7: 15
        line 8: 23

  private final void one(java.lang.String);
    descriptor: (Ljava/lang/String;)V
    flags: ACC_PRIVATE, ACC_FINAL
    Code:
      stack=0, locals=2, args_size=2
         0: return
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       1     0  this   LKotlinDefaultArgumentsTarget;
            0       1     1     a   Ljava/lang/String;
      LineNumberTable:
        line 11: 0

  static void one$default(KotlinDefaultArgumentsTarget, java.lang.String, int, java.lang.Object);
    descriptor: (LKotlinDefaultArgumentsTarget;Ljava/lang/String;ILjava/lang/Object;)V
    flags: ACC_STATIC, ACC_BRIDGE, ACC_SYNTHETIC
    Code:
      stack=2, locals=4, args_size=4
         0: iload_2
         1: iconst_1
         2: iand
         3: ifeq          9
         6: ldc           #26                 // String a
         8: astore_1
         9: aload_0
        10: aload_1
        11: invokespecial #30                 // Method one:(Ljava/lang/String;)V
        14: return
      LineNumberTable:
        line 10: 6

The only idea that comes to my mind - is to exclude all branches. In this case instruction coverage still can be used to determine if all cases were covered or not.

[Godin]

@marchof could you please review?
Don't forget to squash and clean description if you decide to merge 😉

[marchof]

@Godin I wonder whether KotlinGeneratedFilter.isKotlinClass() should also be used in the existing Kotlin filters as a shortcut before iterating through all instructions of a method. WDYT?

[Godin]

@marchof

As this method is used by other filters too, I wonder whether the GeneratedFilter is the right place. Maybe a separate KotlinFilterUtils class would be a better place?

If we want to use it in all Kotlin filters (your other question), then maybe better to introduce KotlinFilter that will have this method and call other filters?

I wonder whether KotlinGeneratedFilter.isKotlinClass() should also be used in the existing Kotlin filters as a shortcut before iterating through all instructions of a method. WDYT?

This also raises question - shouldn't we exclude Kotlin classes from non-Kotlin filters such as one for try-with-resources ? In any case IMO this is unrelated to this PR.

And hence I would prefer to not do any changes regarding these two remaining points in this PR.

[marchof]

@Godin Sure, usage and re-location of isKotlinClass() should be handeled in a separate PR.

After the builds are green I'll merge this. Thanks for addressing my remarks!