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

Fix KtInvalidLifetimeOwnerAccessException with KSP2 #392

Closed
wants to merge 1 commit into from

Conversation

vRallev
Copy link

@vRallev vRallev commented Jun 11, 2024

InjectProcessor remembers deferred symbols across rounds. KSP (KSP 2 in particular) forbids to cache symbols across rounds and verifies the lifecycle. The fix is trying to get new symbols of the deferred classes each round.

For more details: google/ksp#1854

`InjectProcessor` remembers deferred symbols across rounds. KSP (KSP 2 in particular) forbids to cache symbols across rounds and verifies the lifecycle. The fix is trying to get new symbols of the deferred classes each round.

For more details: google/ksp#1854
@vRallev vRallev marked this pull request as ready for review June 11, 2024 07:27
@evant
Copy link
Owner

evant commented Jun 11, 2024

I think we need to do the same thing with deferredFunctions too right?

@evant
Copy link
Owner

evant commented Jun 11, 2024

Might be a little cleaner to only hold on to the qualified names between rounds, that way it has to be looked up again instead of having to remember to.

@evant
Copy link
Owner

evant commented Jun 11, 2024

We really should have some more rounds tests to verify things like this, not going to hold up this pr on that though

@vRallev
Copy link
Author

vRallev commented Jun 11, 2024

I think we need to do the same thing with deferredFunctions too right?

Yes, but I'm not sure what these function cover or how to "refresh" the symbols. Are these only top-level functions? I also couldn't test this whereas I had reproducible steps for classes and wanted to start with that.

Might be a little cleaner to only hold on to the qualified names between rounds, that way it has to be looked up again instead of having to remember to.

I thought about that, but decided against it in the end. Reason is that APIs like qualifiedName are nullable and I'm falling back to the original symbol in case we need to cover anonymous classes or other rare cases. Not all APIs on a symbol throw this exception.

I really believe theres a lack of API support in KSP.

@evant
Copy link
Owner

evant commented Jun 11, 2024

Are these only top-level functions?

Yes, they were added to support KmpComponentCreate

@evant
Copy link
Owner

evant commented Jun 11, 2024

in case we need to cover anonymous classes or other rare cases

I only expect we'll ever be scanning classes & top-level functions which are always supported right? Falling back to the original symbol that is 'unsupported' sounds like a recipe for some rare hard-to-debug issues to me.

@evant
Copy link
Owner

evant commented Jun 11, 2024

#393 is what I mean. You don't need to look up the symbols again except in the finish case which should hopefully be less common as it implies something was never resolved.

@vRallev
Copy link
Author

vRallev commented Jun 11, 2024

I left comments. Let's continue the discussion there and then merge one or the other fix.

@evant
Copy link
Owner

evant commented Jun 11, 2024

Superseded by #393

@evant evant closed this Jun 11, 2024
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.

2 participants