Ibotta / gradle-aspectj-pipeline-plugin

A Gradle plugin for Android projects which performs AspectJ weaving using Android's bytcode manipulation pipeline.
Apache License 2.0
75 stars 15 forks source link

Kotlin 1.5.0 Issues #8

Closed jdvp closed 3 years ago

jdvp commented 3 years ago

More of a heads up, but this plugin (and probably any AspectJ plugin) doesn't seem to play nicely with lambdas in Kotlin 1.5.0 due to changes to Single Abstract Method conversions during compilation.

As a concrete example, in my sample AOP project, if you only change kotlin_version to 1.5.0 instead of 1.4.32, the following pointcut:

@Pointcut("execution(void *.onClick(..))")
fun onButtonClick() {
}

doesn't generate a join point for the following lamba:

button2.setOnClickListener {
    Toast.makeText(this@MainActivity, "Button 2 clicked", Toast.LENGTH_SHORT).show()
}

whereas it does if you just revert the version back to 1.4

This issue is resolved for me by adding the following kotlinOptions block:

kotlinOptions {
    freeCompilerArgs = ["-Xsam-conversions=class"]
}

This option is explained a bit more in the "What's New" article for Kotlin 1.5.0 here. However, I'm wondering if there is a way to fix this without needing the additional compiler arg (since using the new feature would be ideal as it generates less bytecode). The bytecode method signature inside the apk seems to be the same to me with and without the new compiler option, so I'm not totally sure what's going on here.

Attaching generated bytecode for both 1.4 and 1.5 versions of Kotlin in case it's helpful to have as reference 1.4.32.txt 1.5.0.txt

eschlenz commented 3 years ago

This is sounding familiar to me. We ran into something like this, and if I remember correctly, attributed it to the new JVM IR. We had to disable it.

@vdelricco @yoxjames Can you confirm whether what I just said is accurate?

eschlenz commented 3 years ago

@jdvp Thanks for the heads up, and the sample app! We'll take a look a closer look at this for sure.

eschlenz commented 3 years ago

@jdvp I've been playing around with your sample app trying to get a pointcut to match on lambdas. I used javap to analyze the bytecode for MainActivity.class. I can see mention of onCreate$lambda-0 in there, and figured that would be what the pointcut should target. But it's just not working.

After reading the Kotlin documentation on the 1.5.0 changes, invokedynamic is now used for lambdas. And I think the important snippet I came across is this:

Kotlin 1.5.0 is introducing experimental support for compiling plain Kotlin lambdas (which are not 
converted to an instance of a functional interface) into dynamic invocations (invokedynamic ). The 
implementation produces lighter binaries by using LambdaMetafactory.metafactory(), which effectively 
generates the necessary classes at runtime. Currently, it has three limitations compared to
ordinary lambda compilation:

More specifically: "which effectively generates the necessary classes at runtime"

Seeing as AspectJ is performing compile time weaving, the runtime classes generated wouldn't be captured by any AspectJ pointcuts. I'm wondering if it's as simple as that? And I'm thinking there's probably no way around it, other than the compile argument you found to force the old sam conversion mechanism.

Granted, a lot of the stuff I was looking at felt a bit over my head. But if my understanding is correct, then I think we're SOL in supporting the new 1.5.0 invokedynamic feature for lambdas.

jdvp commented 3 years ago

@eschlenz thanks for looking into it. What you're saying about invokedynamic makes sense to me and my impressions were similar about not being really having any workaround for doing both invokedynamic and using aspectj

I also felt similarly though. I don't know if there is anything to do at this point so we can close this issue! I wonder though as Kotlin gets more and more features if we will get more issues like this.

eschlenz commented 3 years ago

@jdvp Yeah, I share those concerns about the future of Kotlin + AspectJ. I'm going to keep this open until I at least document this issue on our readme. Thanks for the report!

eschlenz commented 3 years ago

Updated documentation. Closing this out for now.