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

Update to dagger 2.50 #830

Merged
merged 15 commits into from
Feb 6, 2024
Merged

Update to dagger 2.50 #830

merged 15 commits into from
Feb 6, 2024

Conversation

ZacSweers
Copy link
Collaborator

@ZacSweers ZacSweers commented Dec 25, 2023

This updates Dagger to 2.50, which includes two code gen changes.

1. Prohibit extension declarations for @Binds functions

Starting in Dagger 2.48, @Binds functions can no longer be extension functions.

Note that dagger also prohibits these on @Provides functions, but Anvil didn't have any tests or validation for this case. I added some opportunistically.

2. Add support for new dagger.internal.Provider factories

Dagger 2.50 introduced a new Provider type in preparation for its support of jakarta: https:/google/dagger/releases/tag/dagger-2.50. The way this is implemented in AssistedFactory code gen is via a second createFactoryProvider() factory function that returns an instance of this new internal Provider type. This PR updates Dagger and adds support for this new type.

I had to also force a newer Guava version as it seems Dagger updated theirs and uses some newer APIs, though surprisingly this isn't exposed transitively on the classpath.

This was referenced Dec 25, 2023
@ZacSweers ZacSweers changed the title Update to dagger 2.50 + add support for new dagger.internal.Provider factories Update to dagger 2.50 Dec 25, 2023
Comment on lines +576 to +579
.addFunction(createFactory("create", Provider::class.asClassName()))
// New in Dagger 2.50: factories for dagger.internal.Provider
.addFunction(
FunSpec.builder("create")
.jvmStatic()
.addTypeVariables(typeParameters)
.addParameter(DELEGATE_FACTORY_NAME, generatedFactoryTypeName)
.returns(Provider::class.asClassName().parameterizedBy(baseFactoryTypeName))
.addStatement(
"return %T.create(%T($DELEGATE_FACTORY_NAME))",
InstanceFactory::class,
implParameterizedTypeName,
)
.build(),
createFactory("createFactoryProvider", dagger.internal.Provider::class.asClassName()),
Copy link
Member

Choose a reason for hiding this comment

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

Does Dagger always generate both now? Or are they keying off anything to decide on one vs the other? Looks like all the tests passed so it should be fine, but it feels a little surprising

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It always generates both now. The original for backward compatibility, the new one for forward compatibility. They're going to support both going forward

@ZacSweers
Copy link
Collaborator Author

errrr I don't know what changed on main that broke this so much? It was green before

@ZacSweers
Copy link
Collaborator Author

Should be fixed up now 👍

@ZacSweers
Copy link
Collaborator Author

... these pass locally for me. Ugh

@ZacSweers
Copy link
Collaborator Author

nvm found a new issue. I think this is a slight behavior difference between dagger kapt and KSP though, gonna dig in. Weird that these passed before though, curious what changed on main to trigger these in a way that didn't fail previously 🤔

@RBusarow
Copy link
Member

I'm guessing it's related to the KotlinPoet bump. The KSTypeReference.toTypeName() extension (link) used to automatically resolve, but now it doesn't. That would explain the missing variance here:

com.squareup.anvil.compiler.dagger.InjectConstructorFactoryGeneratorTest > a factory class is generated for a generic class with two type parameters and a where clause[Use Dagger: false, mode: Ksp(symbolProcessorProviders=[])] FAILED
    keys with wrong values
    for key       : R
    expected value: [java.util.Set<? extends java.lang.String>]
    but got value : [java.util.Set<java.lang.String>]
    ---
    expected      : {T=[java.lang.Appendable, java.lang.CharSequence], R=[java.util.Set<? extends java.lang.String>]}
    but was       : {T=[java.lang.Appendable, java.lang.CharSequence], R=[java.util.Set<java.lang.String>]}
        at app//com.squareup.anvil.compiler.dagger.InjectConstructorFactoryGeneratorTest$a factory class is generated for a generic class with two type parameters and a where clause$1.invoke(InjectConstructorFactoryGeneratorTest.kt:2354)
        at app//com.squareup.anvil.compiler.dagger.InjectConstructorFactoryGeneratorTest$a factory class is generated for a generic class with two type parameters and a where clause$1.invoke(InjectConstructorFactoryGeneratorTest.kt:2331)
        at app//com.squareup.anvil.compiler.internal.testing.AnvilCompilationKt.compileAnvil(AnvilCompilation.kt:276)
        at app//com.squareup.anvil.compiler.internal.testing.AnvilCompilationKt.compileAnvil$default(AnvilCompilation.kt:230)
        at app//com.squareup.anvil.compiler.dagger.InjectConstructorFactoryGeneratorTest.compile(InjectConstructorFactoryGeneratorTest.kt:2741)
        at app//com.squareup.anvil.compiler.dagger.InjectConstructorFactoryGeneratorTest.compile$default(InjectConstructorFactoryGeneratorTest.kt:2737)
        at app//com.squareup.anvil.compiler.dagger.InjectConstructorFactoryGeneratorTest.a factory class is generated for a generic class with two type parameters and a where clause(InjectConstructorFactoryGeneratorTest.kt:2331)

@ZacSweers
Copy link
Collaborator Author

Sure, but I haven't made any changes to InjectConstructorFactoryCodeGen, so how is this not also failing on main?

@JoelWilcox JoelWilcox enabled auto-merge (squash) February 6, 2024 01:10
@JoelWilcox JoelWilcox merged commit 622e663 into square:main Feb 6, 2024
18 checks passed
@ZacSweers ZacSweers deleted the z/dagger250 branch February 6, 2024 05:36
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.

3 participants