Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter unreachable branches in Kotlin open functions with default arguments #887

Merged
merged 6 commits into from Jun 3, 2019

Conversation

Godin
Copy link
Member

@Godin Godin commented May 31, 2019

For open functions with default arguments

open class Open {
    open fun f(a: String = "a") {
    }
}

Kotlin compiler generates check that default arguments are not used for super calls - https://github.com/JetBrains/kotlin/blob/v1.3.31/compiler/backend/src/org/jetbrains/kotlin/codegen/FunctionCodegen.java#L1341-L1359 , even if currently such calls not allowed - compiler rejects following

class Derived : Open() {
    override fun f(a: String) {
        super.f() // Error: Super-calls with default arguments are not allowed. Please specify all arguments of 'super.f' explicitly
    }
}

I suppose that check is introduced for future versions of language, where such calls might be allowed.

This PR adds handling of open functions to KotlinDefaultArgumentsFilter.

Fixes #882

@Godin Godin added this to the 0.8.5 milestone May 31, 2019
@Godin Godin self-assigned this May 31, 2019
@Godin Godin added this to To Do in Filtering via automation May 31, 2019
@Godin Godin added this to Implementation in Current work items via automation May 31, 2019
@Godin Godin requested a review from marchof May 31, 2019 12:12
@Godin Godin moved this from Implementation to Review in Current work items May 31, 2019
@@ -123,6 +123,17 @@ public void last_line_in_coverage_data_should_be_less_or_equal_to_number_of_line
source.getCoverage().getLastLine() <= source.getLines().size());
}

@Test
public void all_branches_should_have_line_number() {
Copy link
Member Author

@Godin Godin May 31, 2019

Choose a reason for hiding this comment

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

@marchof here is some notes about this new test:

Unfortunately in our validation tests all assertions are about code that has line numbers, so validation test was green without this - see first commit.

Similar generic assertion for instructions counter is not possible, because it does not hold for KotlinInlineTarget.

Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like this!

I actually tried with instructions and it fails with most Kotlin tests.

I wonder whether we should add this test to the base class and explicitly overwrite it for those tests where it does not hold true (with comments why this is).

Copy link
Member Author

Choose a reason for hiding this comment

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

@marchof

I actually tried with instructions and it fails with most Kotlin tests.

You're right - instruction counters won't be equal for most Kotlin targets, because first entry in LineNumberTable is past non-null checks of parameters:

  public static final void main(java.lang.String[]);
    descriptor: ([Ljava/lang/String;)V
    flags: (0x0019) ACC_PUBLIC, ACC_STATIC, ACC_FINAL
    Code:
      stack=8, locals=2, 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: new           #18                 // class org/jacoco/core/test/validation/kotlin/targets/KotlinDataClassTarget$DataClass
         ...
      LineNumberTable:
        line 40: 6

And in fact I wasn't precise enough about description of "similar generic assertion": what holds for every target except KotlinInlineTarget - is equality of number of missed instructions with line number and without, i.e. that all missed instructions are visible in source, while covered might be invisible, which is ok since they are covered 😉

explicitly overwrite it for those tests where it does not hold true (with comments why this is)

Haven't thought about override. Nice idea! 👍 Added it.

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.

Looks good to me!

@Godin Godin merged commit b7efb62 into master Jun 3, 2019
Filtering automation moved this from To Do to Done Jun 3, 2019
Current work items automation moved this from Review to Done Jun 3, 2019
@Godin Godin deleted the kotlin_default_arguments branch June 3, 2019 11:11
@jacoco jacoco locked as resolved and limited conversation to collaborators Oct 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Filtering
  
Done
Development

Successfully merging this pull request may close these issues.

Default arguments in open Kotlin function cause untestable branches
2 participants