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

KSP Incremental Processing: Clean source files should have generated output cache accessible via Resolver. #1555

Closed
bcorso opened this issue Sep 28, 2023 · 1 comment · Fixed by #1563
Milestone

Comments

@bcorso
Copy link

bcorso commented Sep 28, 2023

Dagger recently had two incremental processing issues related to KSP:

google/dagger#4063
google/dagger#4054

Repro Example

As an example, take the following source files:

class B extends A {
  @Inject Foo foo;
}

@SomeOtherAnnotation
class A {
  @Inject Bar bar;
}

Suppose that after a clean build we get an output map like this:

// Clean build
Accumulated source to outputs map
  src/main/java/A.java:
    build/generated/ksp/debug/java/byRounds/1/A_MembersInjector.java
    build/generated/ksp/debug/java/byRounds/1/A_SomeOtherThing.java
  src/main/java/B.java:
    build/generated/ksp/debug/java/byRounds/1/B_MembersInjector.java

Now, suppose that B changes and becomes "dirty". The issue is that Dagger ends up generating A_MembersInjector.java when it inspects the superclass of B because it thinks that A_MembersInjector.java has not been generated yet since it is not found via the Resolver. However, generating A_MembersInjector.java overwrites the output map's cache for A, which is then missing generated sources from other annotations originating from A, e.g. A_SomeOtherThing.java:

// B dirty (results in incorrect outputs for A)
Accumulated source to outputs map
  src/main/java/A.java:
    build/generated/ksp/debug/java/byRounds/1/A_MembersInjector.java
  src/main/java/B.java:
    build/generated/ksp/debug/java/byRounds/1/B_MembersInjector.java
@bcorso
Copy link
Author

bcorso commented Sep 28, 2023

(Note: @ting-yuan and I have discussed this offline, but putting it here for completeness.)

Short-term fix:

There is a short-term fix that we can implement within Dagger to avoid generating A_MembersInjector while inspecting class B when A is a source file in the same compilation unit (which we can check using Origin).

However, I think this can be fixed on the KSP side to make things easier (and more efficient) for developers.

Long-term fix:

I think a better, long-term fix is to change how KSP handles its output cache. In particular, if A is "clean" then none of its outputs should change (i.e. any output which originates from A). In addition, we should make the cached output of A available via the Resolver so that processors can "see" the generated output exists and avoid trying to re-generate it. This approach should be more similar to how things work in Javac, and I think it will also be more intuitive to developers using KSP.

@ting-yuan ting-yuan added this to the Dagger milestone Sep 28, 2023
copybara-service bot pushed a commit to google/dagger that referenced this issue Sep 29, 2023
This change should be temporary until we can get a more robust fix in KSP (google/ksp#1555).

Fixes #4063
Fixes #4054

RELNOTES=N/A
PiperOrigin-RevId: 568629041
copybara-service bot pushed a commit to google/dagger that referenced this issue Sep 29, 2023
This change should be temporary until we can get a more robust fix in KSP (google/ksp#1555).

Fixes #4063
Fixes #4054

RELNOTES=N/A
PiperOrigin-RevId: 568629041
copybara-service bot pushed a commit to google/dagger that referenced this issue Sep 29, 2023
This change should be temporary until we can get a more robust fix in KSP (google/ksp#1555).

Fixes #4063
Fixes #4054

RELNOTES=N/A
PiperOrigin-RevId: 568629041
copybara-service bot pushed a commit to google/dagger that referenced this issue Sep 29, 2023
This change should be temporary until we can get a more robust fix in KSP (google/ksp#1555).

Fixes #4063
Fixes #4054

RELNOTES=N/A
PiperOrigin-RevId: 569608460
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants