Skip to content

Conversation

Godin
Copy link
Member

@Godin Godin commented Jan 6, 2021

Fixes #1126

@Godin Godin added this to the 0.8.7 milestone Jan 6, 2021
@Godin Godin self-assigned this Jan 6, 2021
@Godin
Copy link
Member Author

Godin commented Jan 6, 2021

While kotlin-maven-plugin version 1.4.10 works fine on JDK 16 and 17 EA, versions 1.4.20 and latest as of today 1.4.21 fail with

java.lang.reflect.InaccessibleObjectException: Unable to make protected void java.util.ResourceBundle.setParent(java.util.ResourceBundle) accessible: module java.base does not "opens java.util" to unnamed module @2543651b

due to integration of https://openjdk.java.net/jeps/396

There is already ticket https://youtrack.jetbrains.com/issue/KT-43704

For us I can imagine following options:

  • stay on 1.4.10
  • use MAVEN_OPTS=--illegal-access=warn
  • exclude org.jacoco.core.test.validation.kotlin from builds with JDK 16 and 17

I think the last option is the best. @marchof WDYT?

@marchof
Copy link
Member

marchof commented Jan 6, 2021

@Godin As option 2 is a global setting for the build I also prefer option 3. As soon as there is a Kotlin compiler that can cope with Java 16 we can re-enable Java 16 and 17.

@Godin
Copy link
Member Author

Godin commented Jan 6, 2021

Imagined one more option:
move kotlin.version from org.jacoco.core.test.validation.kotlin to one of its parent modules
and set it to 1.4.10 in profiles for JDK 16 and 17 EA.

However AFAIK Kotlin doesn't generate different bytecode when targeting Java 16 and 17, so still option 3 to exclude it seems the best and easiest.

@marchof
Copy link
Member

marchof commented Jan 6, 2021

I wouldn't spend effort here. We just remove it from the profiles and make a comment about KT-43704

@Godin Godin changed the title Update filter for coroutines for Kotlin 1.4.20 Update filter for suspending lambdas for Kotlin 1.4.20 Jan 6, 2021
@Godin Godin force-pushed the kotlin_1_4_20_coroutine branch from 7ba6806 to 2e0c592 Compare January 6, 2021 16:21
@Godin
Copy link
Member Author

Godin commented Jan 8, 2021

nextIsInvoke can't be used to match first instruction (as like any of our nextIs* matching methods)
it worked before, because prior to Kotlin compiler version 1.4.20 in case of suspending lambdas there was a label at the beginning of method corresponding to entry of LocalVariableTable

import kotlinx.coroutines.runBlocking

fun example() {
  runBlocking {
  }
}
kotlin-compiler-1.4.10/bin/kotlinc -d 1.4.10 -cp kotlin-compiler-1.4.10/lib/kotlinx-coroutines-core.jar Example.kt
kotlin-compiler-1.4.20/bin/kotlinc -d 1.4.20 -cp kotlin-compiler-1.4.20/lib/kotlinx-coroutines-core.jar Example.kt
javap -v -p 1.4.10/ExampleKt\$example\$1.class > 1.4.10.txt
javap -v -p 1.4.20/ExampleKt\$example\$1.class > 1.4.20.txt
diff --text 1.4.10.txt 1.4.20.txt
   public final java.lang.Object invokeSuspend(java.lang.Object);
     descriptor: (Ljava/lang/Object;)Ljava/lang/Object;
     flags: ACC_PUBLIC, ACC_FINAL
     Code:
-      stack=3, locals=4, args_size=2
-         0: invokestatic  #28                 // Method kotlin/coroutines/intrinsics/IntrinsicsKt.getCOROUTINE_SUSPENDED:()Ljava/lang/Object;
-         3: astore_3
+      stack=3, locals=3, args_size=2
+         0: invokestatic  #26                 // Method kotlin/coroutines/intrinsics/IntrinsicsKt.getCOROUTINE_SUSPENDED:()Ljava/lang/Object;
+         3: astore_2
  ...
       LineNumberTable:
         line 4: 3
-        line 5: 37
+        line 5: 32
       LocalVariableTable:
         Start  Length  Slot  Name   Signature
-           37       4     2 $this$runBlocking   Lkotlinx/coroutines/CoroutineScope;
-            0      51     0  this   LExampleKt$example$1;
-            0      51     1 $result   Ljava/lang/Object;
+           32       4     0  this   LExampleKt$example$1;
+           32       4     1 $result   Ljava/lang/Object;

So there seems to be a room for future improvement:

  • Maybe prior to execution of filters we can add artificial label at the beginning of methods. This will allow to use nextIs* methods even for the first instruction. However maybe this is rather workaround than a clean solution.
  • Or maybe we can change nextIs* on currentIs*. However this is a big refactoring that will change code of all filters, and first should anyway be evaluated by an attempt.

@marchof WDYT? Or maybe you have some other ideas?

In any case I think that we should proceed here and merge as is, dealing with these potential improvements separately, so could you please review?

@Godin Godin requested a review from marchof January 8, 2021 11:53
@Godin Godin marked this pull request as ready for review January 8, 2021 11:57
@marchof marchof merged commit a2c723c into master Jan 8, 2021
@marchof
Copy link
Member

marchof commented Jan 8, 2021

Hi @Godin 👋!

Thanks for the fix. I would prefer your proposal to make nextIs* behave identical also for the first node.

Some more aspects that could be cleaned-up:

  • remove firstIs* method
  • make class Matcher final (no subclassing)
  • encapsulate internal state (especially cursormember)

@Godin Godin deleted the kotlin_1_4_20_coroutine branch January 8, 2021 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

Android "viewModelScope.launch" block partially covered for Kotlin 1.4.20
2 participants