Skip to content

Use KSP2 #350

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

Merged
merged 20 commits into from
Jan 10, 2025
Merged

Use KSP2 #350

merged 20 commits into from
Jan 10, 2025

Conversation

eygraber
Copy link
Collaborator

@eygraber eygraber commented Feb 8, 2024

No description provided.

@eygraber eygraber force-pushed the ksp2 branch 3 times, most recently from bb53268 to 009a7e7 Compare June 17, 2024 17:05
@eygraber eygraber force-pushed the ksp2 branch 2 times, most recently from 761cad1 to f52527a Compare July 11, 2024 21:34
@eygraber eygraber force-pushed the ksp2 branch 2 times, most recently from 8fb90a7 to 82ddb23 Compare November 15, 2024 00:41
@eygraber
Copy link
Collaborator Author

@evant I updated all dependencies to latest versions, but looks like google/ksp#1854 is still an issue, as well as some issues with typealiases which might be related to https://kotlinlang.slack.com/archives/C013BA8EQSE/p1716431911867909. I tested with 2.1.0-RC and both issues are present there as well.

I probably won't have much time to dedicate to this for the next few months, in case there's anyone who'd like to pick it up.

@ting-yuan
Copy link

The exception KtInvalidLifetimeOwnerAccessException: Access to invalid KtAlwaysAccessibleLifetimeToken: PSI has changed since creation, was thrown in the second round in the SymbolProcessor.finish()

invoke:79, AbstractKSDeclarationImpl$containingFile$2 (com.google.devtools.ksp.impl.symbol.kotlin)
invoke:78, AbstractKSDeclarationImpl$containingFile$2 (com.google.devtools.ksp.impl.symbol.kotlin)
lambda 'synchronized' in 'value':74, SynchronizedLazyImpl (kotlin)
getValue:69, SynchronizedLazyImpl (kotlin)
getContainingFile:78, AbstractKSDeclarationImpl (com.google.devtools.ksp.impl.symbol.kotlin)
lambda 'apply' in 'addOriginatingElement':95, KSAstProvider (me.tatarka.kotlin.ast)
addOriginatingElement:94, KSAstProvider (me.tatarka.kotlin.ast)
lambda 'forEach' in 'generate':27, KmpComponentCreateGenerator (me.tatarka.inject.compiler)
forEach:1863, KmpComponentCreateGenerator (me.tatarka.inject.compiler)
lambda 'apply' in 'generate':23, KmpComponentCreateGenerator (me.tatarka.inject.compiler)
lambda 'with' in 'generate':22, KmpComponentCreateGenerator (me.tatarka.inject.compiler)
generate:18, KmpComponentCreateGenerator (me.tatarka.inject.compiler)
lambda 'forEach' in 'generateKmpComponentCreateFiles':40, ProcessKmpComponentCreateKt (me.tatarka.inject.compiler.ksp)
forEach:216, ProcessKmpComponentCreateKt (me.tatarka.inject.compiler.ksp)
generateKmpComponentCreateFiles:39, ProcessKmpComponentCreateKt (me.tatarka.inject.compiler.ksp)
finish:113, InjectProcessor (me.tatarka.inject.compiler.ksp)
lambda 'forEach' in 'execute':586, KotlinSymbolProcessing (com.google.devtools.ksp.impl)
execute$stub_for_inlining$32:1863, KotlinSymbolProcessing (com.google.devtools.ksp.impl)
forEach:1863, KotlinSymbolProcessing (com.google.devtools.ksp.impl)
execute:586, KotlinSymbolProcessing (com.google.devtools.ksp.impl)
...

whereas the corresponding element of createNative was from the first round:

lambda 'with' in 'processKmpComponentCreate':29, ProcessKmpComponentCreateKt (me.tatarka.inject.compiler.ksp)
processKmpComponentCreate:16, ProcessKmpComponentCreateKt (me.tatarka.inject.compiler.ksp)
process$lambda$3:69, InjectProcessor (me.tatarka.inject.compiler.ksp)
invoke:-1, InjectProcessor$$Lambda/0x00007f43f947f670 (me.tatarka.inject.compiler.ksp)
calcNext:171, FilteringSequence$iterator$1 (kotlin.sequences)
hasNext:194, FilteringSequence$iterator$1 (kotlin.sequences)
toList:813, SequencesKt___SequencesKt (kotlin.sequences)
process:70, InjectProcessor (me.tatarka.inject.compiler.ksp)
invoke:552, KotlinSymbolProcessing$execute$1$1 (com.google.devtools.ksp.impl)
invoke:550, KotlinSymbolProcessing$execute$1$1 (com.google.devtools.ksp.impl)
closeFilesOnException:409, IncrementalContextBase (ksp.com.google.devtools.ksp.common)
lambda 'forEach' in 'execute':550, KotlinSymbolProcessing (com.google.devtools.ksp.impl)
forEach:1863, KotlinSymbolProcessing (com.google.devtools.ksp.impl)
execute:549, KotlinSymbolProcessing (com.google.devtools.ksp.impl)
...

Please avoid caching and using the elements across rounds.

@evant
Copy link
Owner

evant commented Nov 19, 2024

@eygraber ^ probably due to

private val kmpComponentCreateFunctionsByComponentType = mutableMapOf<AstClass, MutableList<AstFunction>>()
we should change it to only store the KSName like we do for other deferred symbols.

@eygraber
Copy link
Collaborator Author

Yup, I started working on that. Not sure if I'm doing something wrong because I'm getting other errors but at least the KtInvalidLifetimeOwnerAccessException seems to have stopped 😄

@eygraber
Copy link
Collaborator Author

@evant I believe the latest commit fixes the KtInvalidLifetimeOwnerAccessException issue.

There is still an issue with typealiases (probably related to google/ksp#1921).

Most of the RoundsTests are failing because of a change in dev.zacsweers.kctfork:ksp:0.6.0 where the messages are split between a messages: String property, and a diagnosticMessages: List<DiagnosticMessage>. diagnosticMessages is used in messagesWithSeverity (which is what we call to get the error output), however the errors that we are looking for only appear in messages.

@evant
Copy link
Owner

evant commented Nov 22, 2024

Is it OK if kotlin-inject doesn't work with KSP1 anymore?

I've seen enough issues that I think it would be good to have overlap supporting both for a bit. What breaks here with ksp1? I don't know if it'll easy to detect which one you are using but we could add an additional option to enable it for now

@eygraber
Copy link
Collaborator Author

eygraber commented Nov 22, 2024

Actually it looks like it does might work with KSP1. The integration tests are all passing, but about half of the unit tests fail. Maybe I'm not configuring kotlin-compile-testing correctly, or maybe the tests are actually failing, it's hard to say for sure.

@eygraber
Copy link
Collaborator Author

Got some inspiration from https://github.com/amzn/kotlin-inject-anvil/blob/main/compiler-utils/src/testFixtures/kotlin/software/amazon/lastmile/kotlin/inject/anvil/Compilation.kt and figured it out 😄

The only unit tests failing now are the ones that check for a specific error format that changed from KSP1 to KSP2, so I'm thinking that it is fine.

@eygraber
Copy link
Collaborator Author

Updated the unit and integration tests to run with both KSP1 and KSP2.

@evant
Copy link
Owner

evant commented Nov 23, 2024

CI is blocked by google/ksp#2230

looks like it's already fixed! I'm ok with throwing a clean on ci for now until that's released though

@eygraber
Copy link
Collaborator Author

Updated, hopefully that's all that's needed.

@sahilbajaj
Copy link

@eygraber @evant Eagerly awaiting action.

@eygraber
Copy link
Collaborator Author

@evant anything still needed here (aside from getting past New Years)?

@evant
Copy link
Owner

evant commented Dec 30, 2024

Will take a final pass when I get a chance but don't imagine anything major blocking merge, no

@eygraber
Copy link
Collaborator Author

It looks like function injection isn't working where the typealias points to another typealias. I don't have the time to investigate now, but that wasn't working before these changes either, so I'm guessing not a blocker?

@evant
Copy link
Owner

evant commented Dec 31, 2024

so I'm guessing not a blocker

yeah, if it's not a regression it's not a blocker

@eygraber eygraber requested a review from evant January 10, 2025 00:40
@evant evant merged commit c385b80 into evant:main Jan 10, 2025
2 checks passed
@eygraber eygraber deleted the ksp2 branch January 10, 2025 06:21
@eygraber
Copy link
Collaborator Author

eygraber commented Apr 9, 2025

@evant with KSP 2.0.0 enabling KSP2 by default, can we get a release that includes this?

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 this pull request may close these issues.

5 participants