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

Add KSP support to InjectConstructorFactory, MembersInjectorCodeGen, and assisted injection #795

Merged
merged 28 commits into from
Dec 4, 2023

Conversation

ZacSweers
Copy link
Collaborator

@ZacSweers ZacSweers commented Nov 24, 2023

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

@ZacSweers ZacSweers mentioned this pull request Nov 24, 2023
15 tasks
@ZacSweers
Copy link
Collaborator Author

Adding some more to this, please hold

@ZacSweers ZacSweers marked this pull request as draft November 24, 2023 23:01
@ZacSweers ZacSweers changed the title Add KSP support to InjectConstructorFactoryCodeGen and MembersInjectorCodeGen Add KSP support to InjectConstructorFactory, MembersInjectorCodeGen, and assisted injection Nov 25, 2023
@ZacSweers ZacSweers marked this pull request as ready for review November 25, 2023 07:22
Comment on lines +131 to +133
constructor.parameters.joinToString(", ", prefix = "(", postfix = ")") { param ->
param.type().asClassReference().shortName
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this opportunistically as the test removes them but it makes it easier to disambiguate in real usages

if (receiver != null) {
count++
}
val functionSimpleName = if (count >= 23) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the significance of 23 here and why do we care about using N vs the number of params? Maybe add a named constant and/or comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functions that have 23 or more parameters are actually converted to a special FunctionN type that has an arity type, so this accounts for that. We use this logic in Moshi as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constructorParameters: List<ConstructorParameter>,
onError: (String) -> Nothing
): FileSpec {
val packageName = clazz.packageName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if ClassName#packageName handles the root case? Basically verifying that we do not need the extra logic from safePackageString here anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so, insofar as it just returns an empty string for root packages. KP allows empty strings for package names

image

/**
* Parameters for configuring [AnvilCompilationMode] and whether to run a full test run or not.
*/
internal fun useDaggerAndKspParams(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 🎉

@JoelWilcox JoelWilcox merged commit 425327d into square:main Dec 4, 2023
20 checks passed
RBusarow added a commit that referenced this pull request Dec 7, 2023
…tegration

* main: (21 commits)
  properly parse `--tests` parameters in `CompositePlugin`
  fix `:gradle-plugin`'s plugin application order
  misc cleanup
  only apply the benchmark plugin to the main build's root
  remove the explicit task delegation from the ktlint plugin
  don't include the benchmark build as part of the main build
  fix project paths in ci workflow
  don't bundle the publishing plugin application into the library plugin
  add kdoc to CompositePlugin
  Put `sample` and `integration-tests` into the delegate build
  introduce `BasePlugin` and `MinimalPlugin` conventions
  fix the `plugin-integration-tests` job's name
  making a better composite build
  add basic Gradle integration test scaffolding
  Revert "Update kotlinpoet to v1.15.3"
  Add KSP support to InjectConstructorFactory, MembersInjectorCodeGen, and assisted injection (#795)
  Update kotlinpoet to v1.15.3
  Update dependency com.rickbusarow.kgx:kotlin-gradle-extensions to v0.1.9
  Update ktlintPlugin to v12
  Apply KtLint to the unpublished modules
  ...

# Conflicts:
#	build-logic/delegate/settings.gradle
#	gradle-plugin/build.gradle
#	gradle/libs.versions.toml
#	sample/app/build.gradle
@ZacSweers ZacSweers deleted the z/kspInjectConstructor branch March 9, 2024 06:52
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