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

Update filter for suspending lambdas for Kotlin 1.4.20 #1149

Merged
merged 5 commits into from Jan 8, 2021

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 Godin added this to To Do in Filtering via automation Jan 6, 2021
@Godin Godin added this to Implementation in Current work items via automation 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
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 moved this from Implementation to Review in Current work items Jan 8, 2021
@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
Filtering automation moved this from To Do to Done Jan 8, 2021
Current work items automation moved this from Review to Done 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Filtering
  
Done
Development

Successfully merging this pull request may close these issues.

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