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

Support KSP in factory generation #751

Closed
15 tasks done
ZacSweers opened this issue Sep 27, 2023 · 17 comments
Closed
15 tasks done

Support KSP in factory generation #751

ZacSweers opened this issue Sep 27, 2023 · 17 comments
Assignees

Comments

@ZacSweers
Copy link
Collaborator

ZacSweers commented Sep 27, 2023

This issue is to track KSP support in factory gen, and different from #704

@jhperezd
Copy link
Contributor

👋 I'll be taking ComponentDetectorCheck.kt and AnvilMergeAnnotationDetectorCheck.kt to begin with.

@r0adkll
Copy link

r0adkll commented Oct 4, 2023

👋🏻 I'll be taking MembersInjectorGenerator.kt to start with as well

@jhperezd
Copy link
Contributor

👋 PRs for ComponentDetectorCheck.kt (#753) and AnvilMergeAnnotationDetectorCheck.kt (#756) are up. I'll take InjectConstructorFactoryGenerator.kt next.

@ursusursus
Copy link

May I ask?

Is this about not having to use

anvil {
  generateDaggerFactories = true // default is false
}

such that one would then use ksp dagger everywhere instead, therefore avoiding the incremental compiliation issue?

@ZacSweers
Copy link
Collaborator Author

That wouldn't be enough as you still need anvil's hint generation for contributed elements

JoelWilcox pushed a commit that referenced this issue Dec 4, 2023
…and assisted injection (#795)

These were done together as they are linked. Tried to share code where possible, but there are some meaty parts in dagger generation util that I couldn't really easily share much.

Note that one test is disabled in KSP until binding module generation supports KSP, and I felt it best to save that for a later PR since that generator requires some reworking to avoid class merging during generation.

I also fixed a few mis-named files along the way.

Ref: #751
@IlyaGulya
Copy link
Contributor

@ZacSweers
Hello!
Would you accept help from the community to speed up this issue resolution?

@ZacSweers
Copy link
Collaborator Author

Sure! The last two are gnarly, so it make take a bigger lift. I can write up more about them tomorrow

@ZacSweers
Copy link
Collaborator Author

So the main issue is that both of those generates require doing class scanning, which complicates things in KSP due to it processing nodes that are not in the compilation source set.

This is obviously necessary for module/component merging, but ideally shouldn't be necessary for any of the factory generators.

BindingModuleGenerator

This is the one that requires the most change. Currently Anvil will scan within the package and generate one module for all the contributed bindings. This is a greedy aggregating step, and after some discussion with @vRallev we agreed this should be simplified to just generate 1:1 mappings of a @ContributesBinding class to a generated @ContributesTo-annotated binding module, rather than one aggregated one. This refactoring would affect both the existing Anvil PSI support as well as KSP.

The bulk of the work would be to adopt this new model, then implement dual KSP support afterward.

ContributesSubcomponentHandlerGenerator and ContributesSubcomponentGenerator

This is support for @ContributesSubcomponent, which is a fairly involved case and requires doing class scanning. I think ContributesSubcomponentHandlerGenerator can be saved for the dagger-KSP PR as it deals with merging, and ContributesSubcomponentGenerator is just a simple generator. I've updated the task list to remove ContributesSubcomponentHandlerGenerator now that I've looked at it more.

@ZacSweers
Copy link
Collaborator Author

I'm gonna take ContributesSubcomponentGenerator next since it's trivial

@ZacSweers
Copy link
Collaborator Author

ZacSweers commented Dec 24, 2023

BindsMethodValidator should be a simple one to do. There is also a bug with the current implementation in PSI (#750) that may get fixed by KSP since it has built-in assignability APIs

@IlyaGulya
Copy link
Contributor

BindsMethodValidator should be a simple one to do. There is also a bug with the current implementation in PSI (#750) that may get fixed by KSP since it has built-in assignability APIs

Thanks!
Then, I'll take BindsMethodValidator.

@IlyaGulya
Copy link
Contributor

@ZacSweers Is there anything that should be done? I can take another task if there's one 🙂

@ZacSweers
Copy link
Collaborator Author

The last big one will be BindingModuleGenerator. But in this case, anvil needs to change how this works and no longer merge modules to create one large module. Instead, binding module hints need to be updated to generate 1:1 binding -> binding module to simplify things. If you want to tackle that that would be helpful, but you may need more guidance from @vRallev and @JoelWilcox on how that implementation should look. This change would need to work in Embedded too

@ZacSweers
Copy link
Collaborator Author

@IlyaGulya fyi I've actually started working on BindingModuleGenerator after some discussion with the square folks and Ralf

@gildor
Copy link

gildor commented Apr 8, 2024

Does #831 merge means that all points are done now? Maybe there are some other follow-up tasks.
Thanks!

@ZacSweers
Copy link
Collaborator Author

yup!

@ZacSweers
Copy link
Collaborator Author

There's still some work to do around subcomponent contribution, but that's more about the plugin wiring side rather than the impl itself

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

No branches or pull requests

6 participants