google / dagger

A fast dependency injector for Android and Java.
https://dagger.dev
Apache License 2.0
17.41k stars 2.01k forks source link

[Hilt] Incremental kapt does not always regenerate references to other modules #2082

Closed damianw closed 3 years ago

damianw commented 4 years ago

This may be a duplicate of #1684 but I wanted to provide a reproducible example just in case, since that issue seems to have gone stale (and was not reproducible at the time). The sample can be found here: https://github.com/damianw/HiltIncrementality

The sample consists of two Gradle modules:

Issue

The issue has something to do with failing to regenerate code after making changes in library. To start, build the app from clean:

./gradlew :app:assembleDebug

This should build successfully. Then, uncomment the val water: Water lines from Coffee.kt, and build :app:assembleDebug again. The expected behavior is that this builds successfully. Instead, the build will fail with:

/Users/damian/Development/HiltIncrementality/app/build/generated/source/kapt/debug/com/example/hiltincrementality/DaggerApp_HiltComponents_SingletonC.java:61: error: constructor DripCoffeeMaker in class DripCoffeeMaker cannot be applied to given types;
          local = new DripCoffeeMaker();
                  ^
  required: Water
  found: no arguments
  reason: actual and formal argument lists differ in length
/Users/damian/Development/HiltIncrementality/app/build/generated/source/kapt/debug/com/example/hiltincrementality/DaggerApp_HiltComponents_SingletonC.java:75: error: constructor PourOverCoffeeMaker in class PourOverCoffeeMaker cannot be applied to given types;
          local = new PourOverCoffeeMaker();
                  ^
  required: Water
  found: no arguments
  reason: actual and formal argument lists differ in length
/Users/damian/Development/HiltIncrementality/app/build/generated/source/kapt/debug/com/example/hiltincrementality/DaggerApp_HiltComponents_SingletonC.java:89: error: constructor EspressoCoffeeMaker in class EspressoCoffeeMaker cannot be applied to given types;
          local = new EspressoCoffeeMaker();
                  ^
  required: Water
  found: no arguments
  reason: actual and formal argument lists differ in length

Cleaning (either which Gradle or rm -rf app/build) will fix the problem (allowing the app to build again) at the loss of incrementality. The same problem now exists in reverse, if you were to re- comment the val water: Water line .

danysantiago commented 3 years ago

Thanks for the repro sample Damian!

I've enabled kapt verbose and some of javac's processing logs, here is an output of both the first build (successful clean-build.log) and second incremental build (failure incremental-build.log).

The most interesting part here is that kapt in the app recognizes that compiled classes in its dependencies changed (in the library Gradle module), this can be seen in the logs as:

[incremental apt] Changed classpath names: com/example/library/PourOverCoffeeMaker, com/example/library/DripCoffeeMaker_Factory, com/example/library/EspressoCoffeeMaker, com/example/library/DripCoffeeMaker, com/example/library/PourOverCoffeeMaker_Factory, com/example/library/EspressoCoffeeMaker_Factory, com/example/library/DripCoffeeMaker_Factory$InstanceHolder, com/example/library/EspressoCoffeeMaker_Factory$InstanceHolder, com/example/library/PourOverCoffeeMaker_Factory$InstanceHolder, com/example/library/CoffeeMakerModule

However no processor actually runs (seen a few lines later) even though they are found in the compile classpath:

[INFO] Need to discovery annotation processors in the AP classpath
[INFO] Annotation processors: dagger.hilt.processor.internal.root.RootProcessor, dagger.hilt.android.processor.internal.uninstallmodules.UninstallModulesProcessor, dagger.hilt.processor.internal.aggregateddeps.AggregatedDepsProcessor, dagger.hilt.processor.internal.generatesrootinput.GeneratesRootInputProcessor, dagger.hilt.android.processor.internal.bindvalue.BindValueProcessor, dagger.hilt.android.processor.internal.customtestapplication.CustomTestApplicationProcessor, dagger.hilt.android.processor.internal.androidentrypoint.AndroidEntryPointProcessor, dagger.hilt.processor.internal.aliasof.AliasOfProcessor, dagger.hilt.processor.internal.definecomponent.DefineComponentProcessor, dagger.hilt.processor.internal.originatingelement.OriginatingElementProcessor, dagger.internal.codegen.ComponentProcessor
[INFO] Processing java sources with annotation processors: 
warning: The following options were not recognized by any processor: '[dagger.fastInit, dagger.hilt.android.internal.disableAndroidSuperclassValidation, kapt.kotlin.generated]'Round 1:
        input files: {}
        annotations: []
        last round: false
Round 2:
        input files: {}
        annotations: []
        last round: true

Which is weird because because Hilt's root processor is aggregating and based on Gradle's doc of incremental processor, it should always reprocess, and if it does (like its suppose to, I think) then it would generate the Dagger component interface (annotated with @Component) that the Dagger's ComponentProcessor would then pick-up and eventually determine the signature changes in the compiled classes being used as bindings and ultimately generate the correct Dagger component implementation.

@gavra0, can you please confirm my assumption in the last paragraph? That is, should an aggregating processor reprocess even though changes were only observed in compiled classes that are part of the Gradle module's dependencies?

danysantiago commented 3 years ago

An update: I've created the same project in Java (HiltIncrementalityJava.zip) and was not able to reproduce the issue, which is making kapt very suspicious as the possible culprit of this issue. 🧐

gavra0 commented 3 years ago

In incremental KAPT it is possible to have classpath changes and not to run annotation processing. E.g:

In this case no sources are processed. In case there is at least 1 source to process, we'll add all sources annotated with annotations processed by aggregating annotation processors.

Does this explain the behaviour you are seeing?

danysantiago commented 3 years ago

Thanks Ivan! That sort of explains it. My follow up question is if this same logic applies to already generated sources?

The sample project in this issue has the following setup:

If incremental KAPT looks only at the 'normal source set' (app/src/main/java), i.e. it does not considers generated sources in app/build/generated/source/kapt/ when determining if a classpath changes affects the module's sources, then that would explain what we are seeing here. But if it does considers them, then it might be something else.

gavra0 commented 3 years ago

That is correct, if classpath changes only impact the generated sources, KAPT will not launch annotation processing. We can address this on the KAPT side, but please keep in mind that we are unable to analyze Kotlin sources i.e only generated Java sources can be analyzed when calculating the impact of the classpath changes.

danysantiago commented 3 years ago

Hilt & Dagger only generate Java sources, so that's fine by us, it's a bit hard limitation but better than the current situation. Let me know if I should file a YouTrack issue to help get this resolved.

gavra0 commented 3 years ago

Until KSP comes in, only Java sources are supported sadly. Can you please file an issue on YouTrack about this?

danysantiago commented 3 years ago

Done, created an issue with a more minimal sample project: https://youtrack.jetbrains.com/issue/KT-42182

arunkumar9t2 commented 3 years ago

This can be closed since https://youtrack.jetbrains.com/issue/KT-42182 is available since Kotlin 1.4.30?

danysantiago commented 3 years ago

Closing since KT-42182 is marked a fixed.

vinaygaba commented 2 years ago

I'm observing something similar in one of my libraries(https://github.com/airbnb/Showkase) that's generating Kotlin files. The library works very similar to what's being described in this issue :

It's an aggregating processor where the root module depends on other sub modules and picks up the generated files from the classpath. Even though the generated files of the submodule changed, the root module (irrespective of whether I use kapt or ksp) gets skipped and so the final generated class has obsolete data since it wasn't even generated again .

I've tripled checked that my originating elements are being added correctly. Would appreciate any pointers to investigate this further. @gavra0

gavra0 commented 2 years ago

@vinaygaba Can you please file a YouTrack issue with a sample project? Alternatively, you can run ./gradlew :app:cDJWJ --no-daemon -Dorg.gradle.debug=true (works with KAPT using Gradle workers) and set breakpoints at:

(Please make sure to check out the correct tag of the Kotlin repo, matching the version you are using in the project)