From 5a04d6086c5a2925f13a052a691ceae3e7edbce9 Mon Sep 17 00:00:00 2001 From: Rick Busarow Date: Mon, 26 Feb 2024 14:53:49 -0600 Subject: [PATCH] Revert "[bugfix] Binding supertype which is narrower than return type is wrongly allowed" --- .../conventions/KtlintConventionPlugin.kt | 12 --- compiler-utils/api/compiler-utils.api | 2 - .../internal/reference/ClassReference.kt | 1 - .../internal/reference/TypeReference.kt | 27 ----- .../codegen/dagger/BindsMethodValidator.kt | 102 +++++++++--------- .../dagger/BindsMethodValidatorTest.kt | 44 +------- 6 files changed, 51 insertions(+), 137 deletions(-) diff --git a/build-logic/conventions/src/main/kotlin/com/squareup/anvil/conventions/KtlintConventionPlugin.kt b/build-logic/conventions/src/main/kotlin/com/squareup/anvil/conventions/KtlintConventionPlugin.kt index 71e01b60b..2c7b61b62 100644 --- a/build-logic/conventions/src/main/kotlin/com/squareup/anvil/conventions/KtlintConventionPlugin.kt +++ b/build-logic/conventions/src/main/kotlin/com/squareup/anvil/conventions/KtlintConventionPlugin.kt @@ -13,18 +13,6 @@ open class KtlintConventionPlugin : Plugin { target.extensions.configure(KtlintExtension::class.java) { ktlint -> ktlint.version.set(target.libs.versions.ktlint) - val pathsToExclude = listOf( - "build/generated", - "root-build/generated", - "included-build/generated", - ) - ktlint.filter { - it.exclude { - pathsToExclude.any { excludePath -> - it.file.path.contains(excludePath) - } - } - } ktlint.verbose.set(true) } } diff --git a/compiler-utils/api/compiler-utils.api b/compiler-utils/api/compiler-utils.api index cb4472b3c..00f27d1ff 100644 --- a/compiler-utils/api/compiler-utils.api +++ b/compiler-utils/api/compiler-utils.api @@ -627,8 +627,6 @@ public final class com/squareup/anvil/compiler/internal/reference/TypeReference$ public final class com/squareup/anvil/compiler/internal/reference/TypeReferenceKt { public static final fun AnvilCompilationExceptionTypReference (Lcom/squareup/anvil/compiler/internal/reference/TypeReference;Ljava/lang/String;Ljava/lang/Throwable;)Lcom/squareup/anvil/compiler/api/AnvilCompilationException; public static synthetic fun AnvilCompilationExceptionTypReference$default (Lcom/squareup/anvil/compiler/internal/reference/TypeReference;Ljava/lang/String;Ljava/lang/Throwable;ILjava/lang/Object;)Lcom/squareup/anvil/compiler/api/AnvilCompilationException; - public static final fun allSuperTypeReferences (Lcom/squareup/anvil/compiler/internal/reference/TypeReference;Z)Lkotlin/sequences/Sequence; - public static synthetic fun allSuperTypeReferences$default (Lcom/squareup/anvil/compiler/internal/reference/TypeReference;ZILjava/lang/Object;)Lkotlin/sequences/Sequence; public static final fun toTypeReference (Lorg/jetbrains/kotlin/psi/KtTypeReference;Lcom/squareup/anvil/compiler/internal/reference/ClassReference$Psi;Lcom/squareup/anvil/compiler/internal/reference/AnvilModuleDescriptor;)Lcom/squareup/anvil/compiler/internal/reference/TypeReference$Psi; public static final fun toTypeReference (Lorg/jetbrains/kotlin/types/KotlinType;Lcom/squareup/anvil/compiler/internal/reference/ClassReference;Lcom/squareup/anvil/compiler/internal/reference/AnvilModuleDescriptor;)Lcom/squareup/anvil/compiler/internal/reference/TypeReference$Descriptor; } diff --git a/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/ClassReference.kt b/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/ClassReference.kt index f0536a3e1..a5bdc7139 100644 --- a/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/ClassReference.kt +++ b/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/ClassReference.kt @@ -451,7 +451,6 @@ public fun AnvilCompilationExceptionClassReference( message = message, cause = cause, ) - is Descriptor -> AnvilCompilationException( classDescriptor = classReference.clazz, message = message, diff --git a/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/TypeReference.kt b/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/TypeReference.kt index b9210cae2..965fe91ae 100644 --- a/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/TypeReference.kt +++ b/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/TypeReference.kt @@ -666,30 +666,3 @@ private fun TypeName.lambdaFix(): TypeName { .parameterizedBy(allTypes) .copy(nullable = isNullable) } - -/** - * This will return all super types as [TypeReference], whether they're parsed as [KtTypeReference] - * or [KotlinType]. This will include generated code, assuming it has already been generated. - * The returned sequence will be distinct by FqName, and Psi types are preferred over Descriptors. - * - * The first elements in the returned sequence represent the direct superclass to the receiver. The - * last elements represent the types which are furthest up-stream. - * - * @param includeSelf If true, the receiver class is the first element of the sequence - */ -@ExperimentalAnvilApi -public fun TypeReference.allSuperTypeReferences( - includeSelf: Boolean = false, -): Sequence { - return generateSequence(listOf(this)) { superTypes -> - superTypes - .mapNotNull { it.asClassReferenceOrNull() } - .flatMap { classRef -> - classRef.directSuperTypeReferences() - } - .takeIf { it.isNotEmpty() } - } - .drop(if (includeSelf) 0 else 1) - .flatten() - .distinct() -} diff --git a/compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/BindsMethodValidator.kt b/compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/BindsMethodValidator.kt index 0f350ee51..49e68367d 100644 --- a/compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/BindsMethodValidator.kt +++ b/compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/BindsMethodValidator.kt @@ -7,9 +7,9 @@ import com.squareup.anvil.compiler.codegen.CheckOnlyCodeGenerator import com.squareup.anvil.compiler.daggerBindsFqName import com.squareup.anvil.compiler.daggerModuleFqName import com.squareup.anvil.compiler.internal.reference.AnvilCompilationExceptionFunctionReference +import com.squareup.anvil.compiler.internal.reference.ClassReference import com.squareup.anvil.compiler.internal.reference.MemberFunctionReference -import com.squareup.anvil.compiler.internal.reference.TypeReference -import com.squareup.anvil.compiler.internal.reference.allSuperTypeReferences +import com.squareup.anvil.compiler.internal.reference.allSuperTypeClassReferences import com.squareup.anvil.compiler.internal.reference.classAndInnerClassReferences import com.squareup.anvil.compiler.internal.reference.toTypeReference import org.jetbrains.kotlin.descriptors.ModuleDescriptor @@ -29,29 +29,6 @@ internal class BindsMethodValidator : CheckOnlyCodeGenerator() { override fun isApplicable(context: AnvilContext) = context.generateFactories - internal object Errors { - internal const val BINDS_MUST_BE_ABSTRACT = "@Binds methods must be abstract" - internal const val BINDS_MUST_HAVE_SINGLE_PARAMETER = - "@Binds methods must have exactly one parameter, " + - "whose type is assignable to the return type" - internal const val BINDS_MUST_RETURN_A_VALUE = "@Binds methods must return a value (not void)" - internal fun bindsParameterMustBeAssignable( - paramSuperTypeNames: List, - returnTypeName: String, - parameterType: String?, - ): String { - val superTypesMessage = if (paramSuperTypeNames.isEmpty()) { - "has no supertypes." - } else { - "only has the following supertypes: $paramSuperTypeNames" - } - - return "@Binds methods' parameter type must be assignable to the return type. " + - "Expected binding of type $returnTypeName but impl parameter of type " + - "$parameterType $superTypesMessage" - } - } - override fun checkCode( codeGenDir: File, module: ModuleDescriptor, @@ -77,7 +54,7 @@ internal class BindsMethodValidator : CheckOnlyCodeGenerator() { private fun validateBindsFunction(function: MemberFunctionReference.Psi) { if (!function.isAbstract()) { throw AnvilCompilationExceptionFunctionReference( - message = Errors.BINDS_MUST_BE_ABSTRACT, + message = "@Binds methods must be abstract", functionReference = function, ) } @@ -89,47 +66,64 @@ internal class BindsMethodValidator : CheckOnlyCodeGenerator() { ) } - val bindingParameter = listOfNotNull( - function.singleParameterTypeOrNull(), - function.receiverTypeOrNull(), - ).singleOrNull() - - bindingParameter ?: throw AnvilCompilationExceptionFunctionReference( - message = Errors.BINDS_MUST_HAVE_SINGLE_PARAMETER, - functionReference = function, - ) - - val returnTypeName = function.returnTypeOrNull()?.asTypeName() + val hasSingleBindingParameter = + function.parameters.size == 1 && !function.function.isExtensionDeclaration() + if (!hasSingleBindingParameter) { + throw AnvilCompilationExceptionFunctionReference( + message = "@Binds methods must have exactly one parameter, " + + "whose type is assignable to the return type", + functionReference = function, + ) + } - returnTypeName ?: throw AnvilCompilationExceptionFunctionReference( - message = Errors.BINDS_MUST_RETURN_A_VALUE, + function.returnTypeOrNull() ?: throw AnvilCompilationExceptionFunctionReference( + message = "@Binds methods must return a value (not void)", functionReference = function, ) - val parameterSuperTypes = - bindingParameter - .allSuperTypeReferences(includeSelf = true) - .map { it.asTypeName() } + if (!function.parameterMatchesReturnType() && !function.receiverMatchesReturnType()) { + val returnType = function.returnType().asClassReference().shortName + val paramSuperTypes = (function.parameterSuperTypes() ?: function.receiverSuperTypes())!! + .map { it.shortName } .toList() - if (returnTypeName !in parameterSuperTypes) { - val superTypeNames = parameterSuperTypes.map { it.toString() } + val superTypesMessage = if (paramSuperTypes.size == 1) { + "has no supertypes." + } else { + "only has the following supertypes: ${paramSuperTypes.drop(1)}" + } throw AnvilCompilationExceptionFunctionReference( - message = Errors.bindsParameterMustBeAssignable( - paramSuperTypeNames = superTypeNames.drop(1), - returnTypeName = returnTypeName.toString(), - parameterType = superTypeNames.first(), - ), + message = "@Binds methods' parameter type must be assignable to the return type. " + + "Expected binding of type $returnType but impl parameter of type " + + "${paramSuperTypes.first()} $superTypesMessage", functionReference = function, ) } } - private fun MemberFunctionReference.Psi.singleParameterTypeOrNull(): TypeReference? { - return parameters.singleOrNull()?.type() + private fun MemberFunctionReference.Psi.parameterMatchesReturnType(): Boolean { + return parameterSuperTypes() + ?.contains(returnType().asClassReference()) + ?: false + } + + private fun MemberFunctionReference.Psi.parameterSuperTypes(): Sequence? { + return parameters.singleOrNull() + ?.type() + ?.asClassReference() + ?.allSuperTypeClassReferences(includeSelf = true) + } + + private fun MemberFunctionReference.Psi.receiverMatchesReturnType(): Boolean { + return receiverSuperTypes() + ?.contains(returnType().asClassReference()) + ?: false } - private fun MemberFunctionReference.Psi.receiverTypeOrNull(): TypeReference? { - return function.receiverTypeReference?.toTypeReference(declaringClass, module) + private fun MemberFunctionReference.Psi.receiverSuperTypes(): Sequence? { + return function.receiverTypeReference + ?.toTypeReference(declaringClass, module) + ?.asClassReference() + ?.allSuperTypeClassReferences(includeSelf = true) } } diff --git a/compiler/src/test/java/com/squareup/anvil/compiler/dagger/BindsMethodValidatorTest.kt b/compiler/src/test/java/com/squareup/anvil/compiler/dagger/BindsMethodValidatorTest.kt index 7a15d879b..7bab17050 100644 --- a/compiler/src/test/java/com/squareup/anvil/compiler/dagger/BindsMethodValidatorTest.kt +++ b/compiler/src/test/java/com/squareup/anvil/compiler/dagger/BindsMethodValidatorTest.kt @@ -55,8 +55,8 @@ class BindsMethodValidatorTest( ) if (!useDagger) { assertThat(messages).contains( - "Expected binding of type com.squareup.test.Bar but impl parameter of type com.squareup.test.Foo only has the following " + - "supertypes: [com.squareup.test.Ipsum, com.squareup.test.Lorem]", + "Expected binding of type Bar but impl parameter of type Foo only has the following " + + "supertypes: [Ipsum, Lorem]", ) } } @@ -88,7 +88,7 @@ class BindsMethodValidatorTest( ) if (!useDagger) { assertThat(messages).contains( - "Expected binding of type com.squareup.test.Bar but impl parameter of type com.squareup.test.Foo has no supertypes.", + "Expected binding of type Bar but impl parameter of type Foo has no supertypes.", ) } } @@ -314,44 +314,6 @@ class BindsMethodValidatorTest( } } - @Test - fun `binding which supertype is narrower than return type fails to compile`() { - compile( - """ - package com.squareup.test - - import dagger.Module - import dagger.Binds - - sealed interface ItemDetail { - object DetailTypeA : ItemDetail - } - - interface ItemMapper - - class DetailTypeAItemMapper : ItemMapper - - @Module - interface SomeModule { - @Binds fun shouldBeInvalidComplexBinding(real: DetailTypeAItemMapper): ItemMapper - } - """.trimIndent(), - ) { - assertThat(exitCode).isEqualTo(COMPILATION_ERROR) - assertThat(messages).contains( - "@Binds methods' parameter type must be assignable to the return type", - ) - if (!useDagger) { - assertThat(messages).contains( - "@Binds methods' parameter type must be assignable to the return type. Expected " + - "binding of type com.squareup.test.ItemMapper but impl " + - "parameter of type com.squareup.test.DetailTypeAItemMapper only has the following " + - "supertypes: [com.squareup.test.ItemMapper]", - ) - } - } - } - private fun compile( @Language("kotlin") vararg sources: String, previousCompilationResult: JvmCompilationResult? = null,