[bugfix] Binding supertype which is narrower than return type is wrongly allowed #833

IlyaGulya · 2023-12-31T13:51:37Z

This fixes #750

compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/ClassReference.kt

IlyaGulya · 2024-01-09T07:44:42Z

@ZacSweers
I've fixed ktlint complaints and made TypeReference.isAssignableFrom extension to be class function which is now exposed as public API.
Also I've excluded generated sources from ktlint tasks.

compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/ClassReference.kt

JoelWilcox · 2024-01-10T00:33:25Z

compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/BindsMethodValidator.kt

-  private fun MemberFunctionReference.Psi.parameterMatchesReturnType(): Boolean {
-    return parameterSuperTypes()
-      ?.contains(returnType().asClassReference())
-      ?: false


I would propose keeping the existing structure in the validator and modifying this method to utilize TypeName instead of ClassReference since it contains more of the information that we need in this case. You should be able to use

?.map { it.asTypeName() } ?.contains(returnType().asClassReference().asTypeName())

check for the full type info. Then parameterSuperTypes() would also be updated accordingly to provide TypeReferences.

compiler/src/test/java/com/squareup/anvil/compiler/dagger/BindsMethodValidatorTest.kt

…red fails to compile.

IlyaGulya · 2024-01-12T14:18:52Z

@JoelWilcox I've made all requested fixes. Please take a look.

JoelWilcox · 2024-01-19T02:03:39Z

compiler/src/main/java/com/squareup/anvil/compiler/Utils.kt

+  if (type.isEmpty()) return ""
+
+  val builder = StringBuilder()
+  var current = ""
+  for (i in type.indices) {
+    if (i == type.lastIndex) {
+      builder.append(current + type[i])
+      break
+    }
+    when (val char = type[i]) {
+      '.' -> {
+        current = ""
+      }
+
+      ',', '<', '>' -> {
+        builder.append(current + char)
+        current = ""
+      }
+
+      ' ' -> {
+        builder.append(char)
+        current = ""
+      }
+
+      else -> {
+        current += char
+      }
+    }
+  }
+
+  return builder.toString()


I think you could simplify this using a regex, something like type.replace("([a-z]+\\.)+".toRegex(), ""). Probably needs a little more tinkering to handle nested class cases like com.squareup.Foo.Bar

Removed this completely

JoelWilcox · 2024-01-19T02:04:20Z

...d-logic/conventions/src/main/kotlin/com/squareup/anvil/conventions/KtlintConventionPlugin.kt

+            it.file.path.contains(excludePath)
+          }
+        }
+      }


Is this necessary in the context of the fix? Unclear what the related error/issue is currently

It's not necessary in the context of fix, but without this ktlint checks are failing on generated sources during build:

> Task :gradle-plugin:ktlintGradleTestSourceSetCheck FAILED /Users/ilyagulya/Projects/Community/anvil/gradle-plugin/build/root-build/generated/sources/buildConfig/gradleTest/com/squareup/anvil/plugin/buildProperties/BuildProperties.kt:8:1 Unexpected indentation (4) (should be 2) (standard:indent)

JoelWilcox · 2024-01-19T02:07:50Z

compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/BindsMethodValidator.kt

+    when (returnTypeName) {
+      in parameterSuperTypes -> return
+      in receiverSuperTypes -> return
+      else -> Unit


Nit: since we don't have an else case here it would feel a bit nicer to keep it as an if (condition || condition) { // fail } structure, which is also beneficial over the early returns since that means this check isn't required to stay at the bottom of the validations list as we add more or shift things around.

JoelWilcox · 2024-01-19T02:12:41Z

compiler/src/test/java/com/squareup/anvil/compiler/dagger/BindsMethodValidatorTest.kt

        assertThat(messages).contains(
-          "Expected binding of type Bar but impl parameter of type Foo only has the following " +
-            "supertypes: [Ipsum, Lorem]",
+          Errors.bindsParameterMustBeAssignable(


I'd prefer we keep the hardcoded expected value in this case, since it's possible a bug could be introduced in bindsParameterMustBeAssignable() later on which automatically changes the test to expect an incorrect value.

JoelWilcox · 2024-01-19T02:13:22Z

compiler/src/test/java/com/squareup/anvil/compiler/dagger/BindsMethodValidatorTest.kt

+      )
+      if (!useDagger) {
+        assertThat(messages).contains(
+          Errors.bindsParameterMustBeAssignable(


RBusarow · 2024-01-19T02:22:24Z

...d-logic/conventions/src/main/kotlin/com/squareup/anvil/conventions/KtlintConventionPlugin.kt


    target.extensions.configure(KtlintExtension::class.java) { ktlint ->
      ktlint.version.set(target.libs.versions.ktlint)
+      val pathsToExclude = listOf(


This isn't necessary.

RBusarow · 2024-01-19T03:10:30Z

compiler/src/main/java/com/squareup/anvil/compiler/Utils.kt

+internal fun TypeName.toStringOfSimpleNamesOnly(): String {
+  return removePackageNames(toString())
+}


This extension (and the subsequent String parsing function) ignore all of the work that TypeName has already achieved.

If the type name has simple names, then it's a ClassName or a ParameterizedTypeName. You could do something like:

internal fun TypeName.toStringOfSimpleNamesOnly(): String { return when (this) { is ParameterizedTypeName -> rawType.toStringOfSimpleNamesOnly() is ClassName -> simpleNames.joinToString(separator = ".") is LambdaTypeName, is TypeVariableName, is WildcardTypeName, Dynamic, -> error("???") } }

From the context in which this is being called, the type name should be one of the first two. If I was the one creating this extension, it would be inside the BindsMethodValidator class.

Thanks, I see! But this part is no longer needed since I've removed simple names parsing.

RBusarow · 2024-01-19T03:31:52Z

compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/BindsMethodValidator.kt

+    val parameters = function.parameters
+    val psiFunction = function.function
    val hasSingleBindingParameter =
-      (function.parameters.size == 1 && !function.function.isExtensionDeclaration()) ||
-        (function.parameters.isEmpty() && function.function.isExtensionDeclaration())
+      (parameters.size == 1 && !psiFunction.isExtensionDeclaration()) ||
+        (parameters.isEmpty() && psiFunction.isExtensionDeclaration())


This logic and the new logic below start off by doing the same thing -- trying to figure out what the bound parameter is. You can simplify your logic to something like:

val bindingParameter = listOfNotNull( function.singleParameterTypeOrNull(), function.receiverTypeOrNull() ) .singleOrNull() ?: throw AnvilCompilationExceptionFunctionReference(...) val returnTypeName = function.returnTypeOrNull()?.asClassReference()?.asTypeName() ?: throw AnvilCompilationExceptionFunctionReference(...) val paramSuperTypes = bindingParameter.superTypes() if (returnTypeName !in paramSuperTypes) { throw AnvilCompilationExceptionFunctionReference(...) }

Yeah, that's much cleaner approach, thanks!

RBusarow · 2024-01-19T03:33:46Z

compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/BindsMethodValidator.kt

+    val paramSuperTypes =
+      parameterSuperTypes
+        .ifEmpty { receiverSuperTypes }
+        .map { it.toStringOfSimpleNamesOnly() }


I wouldn't bother trying to parse the simple names at all, since they're only used in the exception message. Just print out the fully qualified name.

Ok, I've just thought it would be nice to have, but sure, I will remove this part

RBusarow · 2024-01-19T03:38:13Z

compiler/src/main/java/com/squareup/anvil/compiler/codegen/dagger/BindsMethodValidator.kt

+  private fun TypeReference?.superTypes(): Sequence<TypeName> {
+    this ?: return emptySequence()
+    return sequence {
+      yield(this@superTypes)
+      yieldAll(
+        this@superTypes
+          .asClassReference()
+          .directSuperTypeReferences(),
+      )
+    }.map { it.asTypeName() }


I might be missing something, but it seems like we should continue to use allSuperTypeClassReferences(includeSelf = true). This version only returns the type and its immediate supertypes, whereas allSuperTypeClassReferences() is recursive.

Unfortunately, allSuperTypeClassReferences lose generic type information (I assume that's because ClassReference can't contain (or should not contain?) information about which types are used as type arguments in specific binding declaration. That's why I'm using TypeReferences here instead of ClassReferences
I will try to make a similar to allSuperTypeClassReferences function which returns TypeReferences instead of ClassReferences

Also, I'm not quite sure that allSuperTypeClassReferences is recursive. Or, at least, I don't see it 🙂

IlyaGulya · 2024-01-24T11:21:54Z

Wow, ok I'll try to get some time to fix this

…h supertype is narrower than return type.

IlyaGulya · 2024-01-27T06:38:07Z

@RBusarow I've made all requested fixes. Please take a look.
I've also introduced new extension TypeReference.allSuperTypeReferences(includeSelf: Boolean = false): Sequence<TypeReference> which works the same as ClassReference.allSuperTypeClassReferences but returns TypeReferences instead.
This is required to keep correct generic type arguments in the TypeName and provide better error message.
If I would use allSuperTypeClassReferences instead, then TypeName will not contain specific type argument which were used in the binding (i.e. ItemMapper<ItemDetail.DetailTypeA>), but instead will contain just generic type variable (i.e. ItemMapper<T : ItemDetail>)

…n-type-wrongly-allowed

IlyaGulya commented Dec 31, 2023

View reviewed changes

compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/ClassReference.kt Outdated Show resolved Hide resolved

IlyaGulya mentioned this pull request Dec 31, 2023

Support KSP in BindsMethodValidator #831

Merged

IlyaGulya force-pushed the bugfix/binding-supertype-narrower-than-return-type-wrongly-allowed branch from 34dce0c to 70e9bbb Compare January 9, 2024 07:41

JoelWilcox reviewed Jan 10, 2024

View reviewed changes

IlyaGulya force-pushed the bugfix/binding-supertype-narrower-than-return-type-wrongly-allowed branch from 659c117 to 823bf4d Compare January 11, 2024 14:40

IlyaGulya changed the title ~~[bugfix] Binding supertype which is narrower than return type is wrongly allowed~~ [WIP][bugfix] Binding supertype which is narrower than return type is wrongly allowed Jan 11, 2024

IlyaGulya force-pushed the bugfix/binding-supertype-narrower-than-return-type-wrongly-allowed branch 3 times, most recently from e2a167d to 2757667 Compare January 12, 2024 12:48

IlyaGulya changed the title ~~[WIP][bugfix] Binding supertype which is narrower than return type is wrongly allowed~~ [bugfix] Binding supertype which is narrower than return type is wrongly allowed Jan 12, 2024

IlyaGulya force-pushed the bugfix/binding-supertype-narrower-than-return-type-wrongly-allowed branch 2 times, most recently from 2c1d874 to ff9405a Compare January 12, 2024 13:52

Add test checking that binding which supertype is narrower than requi…

681b565

…red fails to compile.

IlyaGulya force-pushed the bugfix/binding-supertype-narrower-than-return-type-wrongly-allowed branch 2 times, most recently from 2436aba to 82a4023 Compare January 12, 2024 14:03

IlyaGulya force-pushed the bugfix/binding-supertype-narrower-than-return-type-wrongly-allowed branch from 82a4023 to bac0ac6 Compare January 18, 2024 09:12

JoelWilcox reviewed Jan 19, 2024

View reviewed changes

RBusarow reviewed Jan 19, 2024

View reviewed changes

IlyaGulya force-pushed the bugfix/binding-supertype-narrower-than-return-type-wrongly-allowed branch from bac0ac6 to ab9fe67 Compare January 27, 2024 04:17

Fix BindsMethodValidator in Embedded mode wrongly allows binding whic…

91f19c4

…h supertype is narrower than return type.

IlyaGulya force-pushed the bugfix/binding-supertype-narrower-than-return-type-wrongly-allowed branch from ab9fe67 to 91f19c4 Compare January 27, 2024 06:27

RBusarow approved these changes Feb 12, 2024

View reviewed changes

Merge branch 'main' into bugfix/binding-supertype-narrower-than-retur…

503995e

…n-type-wrongly-allowed

RBusarow enabled auto-merge February 16, 2024 17:38

RBusarow merged commit 1a412a8 into square:main Feb 16, 2024

SteinerOk mentioned this pull request Feb 24, 2024

Strange behavior: @Binds methods' parameter type must be assignable to the return type. #885

Open

This was referenced Feb 26, 2024

Revert "[bugfix] Binding supertype which is narrower than return type is wrongly allowed" #887

Merged

Binding module validation should check type parameters when determining if a bound type is valid #888

Open

[bugfix] Binding supertype which is narrower than return type is wrongly allowed #833

[bugfix] Binding supertype which is narrower than return type is wrongly allowed #833

Uh oh!

Conversation

IlyaGulya commented Dec 31, 2023

Uh oh!

Uh oh!

IlyaGulya commented Jan 9, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IlyaGulya commented Jan 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IlyaGulya Jan 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IlyaGulya commented Jan 24, 2024

Uh oh!

IlyaGulya commented Jan 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

IlyaGulya Jan 27, 2024 •

edited

Loading