Skip to content

Report code coverage correctly for Kotlin methods with default arguments#774

Merged
marchof merged 23 commits intomasterfrom
kotlin_default_arguments
Nov 27, 2018
Merged

Report code coverage correctly for Kotlin methods with default arguments#774
marchof merged 23 commits intomasterfrom
kotlin_default_arguments

Conversation

@Godin
Copy link
Member

@Godin Godin commented Oct 24, 2018 •
edited
Loading

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 :

report

@Godin Godin self-assigned this Oct 24, 2018
Godin
Godin commented Oct 24, 2018
View reviewed changes
@Godin Godin added type: bug 🐛 Something isn't working and removed type: enhancement labels Oct 26, 2018
@Godin
Copy link
Member Author

Godin commented Oct 26, 2018

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

kotlindefaultargumentstarget

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 Godin force-pushed the kotlin_default_arguments branch 4 times, most recently from 8ad85e9 to 12a9757 Compare October 26, 2018 20:08
@Godin Godin force-pushed the kotlin_default_arguments branch from 12a9757 to 8ff17aa Compare October 26, 2018 20:09
@Godin Godin changed the title Do not filter out Kotlin default arguments Report code coverage correctly for Kotlin methods with default arguments Oct 26, 2018
@Godin Godin force-pushed the kotlin_default_arguments branch from bdb5be3 to 45e2dbe Compare October 26, 2018 20:54
@Godin Godin requested a review from marchof November 4, 2018 00:07
@Godin
Copy link
Member Author

Godin commented Nov 4, 2018

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

marchof
marchof requested changes Nov 5, 2018
View reviewed changes
@Godin Godin requested a review from marchof November 19, 2018 20:27
marchof
marchof reviewed Nov 22, 2018
View reviewed changes
Copy link
Member

@marchof marchof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member Author

Godin commented Nov 27, 2018

@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.

@Godin Godin force-pushed the kotlin_default_arguments branch from 64f74c7 to 5c1e744 Compare November 27, 2018 10:25
@Godin Godin force-pushed the kotlin_default_arguments branch from 5c1e744 to 081c49e Compare November 27, 2018 10:28
@marchof
Copy link
Member

marchof commented Nov 27, 2018

@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!

@marchof marchof merged commit 78b28dc into master Nov 27, 2018
@marchof marchof deleted the kotlin_default_arguments branch November 27, 2018 12:16
@Godin Godin added this to the 0.8.3 milestone Nov 27, 2018
@Godin Godin mentioned this pull request Jan 7, 2019
Closed
@jacoco jacoco locked as resolved and limited conversation to collaborators May 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@marchof marchof marchof approved these changes

Assignees

@Godin Godin

Labels

component: core.filters language: Kotlin type: bug 🐛 Something isn't working

Projects

Current work items
Archived in project
Filtering
Status: Done

Milestone

0.8.3

Development

Successfully merging this pull request may close these issues.

Kotlin default arguments should not be filtered out

2 participants

@Godin @marchof