Repository: square/anvil (kotlin)

Issue:
primary constructor properties are not included in `ClassReference.Psi.properties`
Given something like:

```kotlin
class Subject(
  val name: String
)
```

A `ClassReference.Descriptor` instance will include `name` as a property, but the `ClassReference.Psi` version will not.  `Descriptor`'s version is correct.

Pull request:
fixes #609

Patch:
diff --git a/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/AnnotationArgumentReference.kt b/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/AnnotationArgumentReference.kt
index 5fe0da2b1..a0b86a524 100644
--- a/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/AnnotationArgumentReference.kt
+++ b/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/AnnotationArgumentReference.kt
@@ -143,7 +143,7 @@ public sealed class AnnotationArgumentReference {
           is KtObjectDeclaration -> {
             toClassReference(module)
               .properties
-              .map { it.property }
+              .mapNotNull { it.property as? KtProperty }
               .findConstPropertyWithName(name)
               ?.initializer
               ?.text
@@ -218,7 +218,9 @@ public sealed class AnnotationArgumentReference {
               is PropertyReference.Descriptor ->
                 property.property.compileTimeInitializer?.value
               is PropertyReference.Psi ->
-                property.property.initializer?.let { parsePrimitiveType(it.text) }
+                // A PropertyReference.property may also be a KtParameter if it's in a constructor,
+                // but if we're here we're in an object, so the property must be a KtProperty.
+                (property.property as KtProperty).initializer?.let { parsePrimitiveType(it.text) }
             }
           }
 
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 6815877ae..ec76c83c4 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
@@ -33,6 +33,7 @@ import org.jetbrains.kotlin.psi.KtClassBody
 import org.jetbrains.kotlin.psi.KtClassOrObject
 import org.jetbrains.kotlin.psi.KtObjectDeclaration
 import org.jetbrains.kotlin.psi.allConstructors
+import org.jetbrains.kotlin.psi.psiUtil.isPropertyParameter
 import org.jetbrains.kotlin.psi.psiUtil.parents
 import org.jetbrains.kotlin.psi.psiUtil.visibilityModifierTypeOrDefault
 import org.jetbrains.kotlin.resolve.DescriptorUtils
@@ -146,11 +147,18 @@ public sealed class ClassReference : Comparable<ClassReference>, AnnotatedRefere
     }
 
     override val properties: List<PropertyReference.Psi> by lazy(NONE) {
-      clazz
-        .children
-        .filterIsInstance<KtClassBody>()
-        .flatMap { it.properties }
-        .map { it.toPropertyReference(this) }
+      buildList {
+        // Order kind of matters here, since the Descriptor APIs will list body/member properties
+        // before the constructor properties.
+        clazz.body
+          ?.properties
+          ?.forEach { add(it.toPropertyReference(this@Psi)) }
+
+        clazz.primaryConstructor
+          ?.valueParameters
+          ?.filter { it.isPropertyParameter() }
+          ?.forEach { add(it.toPropertyReference(this@Psi)) }
+      }
     }
 
     override val typeParameters: List<TypeParameterReference.Psi> by lazy(NONE) {
diff --git a/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/PropertyReference.kt b/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/PropertyReference.kt
index eee6ab914..bf4205e81 100644
--- a/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/PropertyReference.kt
+++ b/compiler-utils/src/main/java/com/squareup/anvil/compiler/internal/reference/PropertyReference.kt
@@ -16,7 +16,11 @@ import org.jetbrains.kotlin.descriptors.annotations.AnnotationUseSiteTarget.PROP
 import org.jetbrains.kotlin.descriptors.annotations.AnnotationUseSiteTarget.PROPERTY_SETTER
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.name.FqName
+import org.jetbrains.kotlin.psi.KtCallableDeclaration
+import org.jetbrains.kotlin.psi.KtParameter
 import org.jetbrains.kotlin.psi.KtProperty
+import org.jetbrains.kotlin.psi.KtValVarKeywordOwner
+import org.jetbrains.kotlin.psi.psiUtil.isPropertyParameter
 import org.jetbrains.kotlin.psi.psiUtil.visibilityModifierTypeOrDefault
 import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe
 import kotlin.LazyThreadSafetyMode.NONE
@@ -61,11 +65,16 @@ public sealed class PropertyReference : AnnotatedReference {
     return fqName.hashCode()
   }
 
-  public class Psi internal constructor(
-    public val property: KtProperty,
+  /**
+   * @param property In practice, this is either a [KtProperty], or a [KtParameter] for which
+   *   [KtParameter.isPropertyParameter()] is true. [KtCallableDeclaration] is the most applicable
+   *   common interface, but it's also implemented by other types like `KtConstructor`.
+   */
+  public class Psi private constructor(
+    public val property: KtCallableDeclaration,
     override val declaringClass: ClassReference.Psi,
-    override val fqName: FqName = property.requireFqName(),
-    override val name: String = fqName.shortName().asString()
+    override val fqName: FqName,
+    override val name: String
   ) : PropertyReference() {
 
     override val annotations: List<AnnotationReference.Psi> by lazy(NONE) {
@@ -86,14 +95,14 @@ public sealed class PropertyReference : AnnotatedReference {
     override val setterAnnotations: List<AnnotationReference.Psi> by lazy(NONE) {
       property.annotationEntries
         .filter { it.useSiteTarget?.getAnnotationUseSiteTarget() == PROPERTY_SETTER }
-        .plus(property.setter?.annotationEntries ?: emptyList())
+        .plus((property as? KtProperty)?.setter?.annotationEntries ?: emptyList())
         .map { it.toAnnotationReference(declaringClass, module) }
     }
 
     override val getterAnnotations: List<AnnotationReference.Psi> by lazy(NONE) {
       property.annotationEntries
         .filter { it.useSiteTarget?.getAnnotationUseSiteTarget() == PROPERTY_GETTER }
-        .plus(property.getter?.annotationEntries ?: emptyList())
+        .plus((property as? KtProperty)?.getter?.annotationEntries ?: emptyList())
         .map { it.toAnnotationReference(declaringClass, module) }
     }
 
@@ -109,6 +118,23 @@ public sealed class PropertyReference : AnnotatedReference {
         )
       }
     }
+
+    internal companion object {
+      // There's no single applicable type for a PSI property. The multiple generic bounds prevent
+      // us from creating a property out of a function, constructor, or destructuring declaration.
+      internal operator fun <T> invoke(
+        property: T,
+        declaringClass: ClassReference.Psi,
+        fqName: FqName = property.requireFqName(),
+        name: String = fqName.shortName().asString()
+      ): Psi where T : KtCallableDeclaration,
+                   T : KtValVarKeywordOwner = Psi(
+        property = property,
+        declaringClass = declaringClass,
+        fqName = fqName,
+        name = name
+      )
+    }
   }
 
   public class Descriptor internal constructor(
@@ -159,6 +185,19 @@ public sealed class PropertyReference : AnnotatedReference {
   }
 }
 
+@ExperimentalAnvilApi
+public fun KtParameter.toPropertyReference(
+  declaringClass: ClassReference.Psi
+): Psi {
+  if (!isPropertyParameter()) {
+    throw AnvilCompilationException(
+      element = this,
+      message = "A KtParameter may only be turned into a PropertyReference if it's a val or var."
+    )
+  }
+  return Psi(this, declaringClass)
+}
+
 @ExperimentalAnvilApi
 public fun KtProperty.toPropertyReference(
   declaringClass: ClassReference.Psi


Test patch:
diff --git a/compiler/src/test/java/com/squareup/anvil/compiler/internal/reference/PropertyReferenceTest.kt b/compiler/src/test/java/com/squareup/anvil/compiler/internal/reference/PropertyReferenceTest.kt
new file mode 100644
index 000000000..f97d37f6a
--- /dev/null
+++ b/compiler/src/test/java/com/squareup/anvil/compiler/internal/reference/PropertyReferenceTest.kt
@@ -0,0 +1,619 @@
+package com.squareup.anvil.compiler.internal.reference
+
+import com.google.common.truth.Truth.assertThat
+import com.squareup.anvil.compiler.compile
+import com.squareup.anvil.compiler.internal.testing.simpleCodeGenerator
+import com.tschuchort.compiletesting.KotlinCompilation.ExitCode.OK
+import org.junit.Test
+
+class PropertyReferenceTest {
+
+  @Test fun `primary constructor val properties are properties`() {
+    compile(
+      """
+      package com.squareup.test
+
+      class Subject(
+        val name: String,
+        val age: Int
+      )
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            assertThat(ref.propertyTypeNames())
+              .containsExactly(
+                "name: kotlin.String",
+                "age: kotlin.Int"
+              )
+              .inOrder()
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `primary constructor var properties are properties`() {
+    compile(
+      """
+      package com.squareup.test
+
+      class Subject(
+        var name: String,
+        var age: Int
+      )
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            assertThat(ref.propertyTypeNames())
+              .containsExactly(
+                "name: kotlin.String",
+                "age: kotlin.Int"
+              )
+              .inOrder()
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `member properties are properties`() {
+    compile(
+      """
+      package com.squareup.test
+
+      class Subject {
+        val name: String = ""
+        val age: Int = 5
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            assertThat(ref.propertyTypeNames())
+              .containsExactly(
+                "name: kotlin.String",
+                "age: kotlin.Int"
+              )
+              .inOrder()
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `nested class member properties are not properties of the outer class`() {
+    compile(
+      """
+      package com.squareup.test
+
+      class Subject {
+        val name: String = ""
+        
+        class Nested {
+          val age: Int = 5        
+        }
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          if (psiRef.shortName == "Nested") return@simpleCodeGenerator null
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            assertThat(ref.propertyTypeNames())
+              .containsExactly("name: kotlin.String")
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `nested class constructor properties are not properties of the outer class`() {
+    compile(
+      """
+      package com.squareup.test
+
+      class Subject {
+        val name: String = ""
+        
+        class Nested(
+          val age: Int        
+        )
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          if (psiRef.shortName == "Nested") return@simpleCodeGenerator null
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            assertThat(ref.propertyTypeNames())
+              .containsExactly("name: kotlin.String")
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `mixed constructor and member properties are all properties`() {
+    compile(
+      """
+      package com.squareup.test
+
+      class Subject(
+        val name: String
+      ) {
+        val age: Int = 5
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            assertThat(ref.propertyTypeNames())
+              .containsExactly(
+                "age: kotlin.Int",
+                "name: kotlin.String"
+              )
+              .inOrder()
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `overridden member properties are properties`() {
+    compile(
+      """
+      package com.squareup.test
+
+      interface Named {
+        val name: String
+      }
+
+      class Subject : Named {
+        override val name: String = "" 
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+
+          if (psiRef.shortName == "Named") return@simpleCodeGenerator null
+
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            assertThat(ref.propertyTypeNames())
+              .containsExactly("name: kotlin.String")
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `properties in a super class are not properties`() {
+    compile(
+      """
+      package com.squareup.test
+
+      abstract class Named {
+        val name: String = ""
+      }
+
+      class Subject : Named()
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+
+          if (psiRef.shortName == "Named") return@simpleCodeGenerator null
+
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            assertThat(ref.propertyTypeNames()).isEmpty()
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `member properties can have annotations`() {
+    compile(
+      """
+      package com.squareup.test
+
+      import javax.inject.Inject
+
+      class Subject {
+        @Inject val name: String = ""
+        val age: Int = 5
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            val nameAnnotations = ref.properties.named("name")
+              .annotations
+              .map { it.fqName.asString() }
+
+            assertThat(nameAnnotations).containsExactly("javax.inject.Inject")
+
+            val ageAnnotations = ref.properties.named("age")
+              .annotations
+              .map { it.fqName.asString() }
+
+            assertThat(ageAnnotations).isEmpty()
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `member properties can have annotations on a getter delegate`() {
+    compile(
+      """
+      package com.squareup.test
+
+      import javax.inject.Inject
+
+      class Subject {
+        @get:Inject val name: String = ""
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            val nameGetterAnnotations = ref.properties.named("name")
+              .getterAnnotations
+              .map { it.fqName.asString() }
+
+            assertThat(nameGetterAnnotations).containsExactly("javax.inject.Inject")
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `member property getter annotations are included in the field's annotations`() {
+    compile(
+      """
+      package com.squareup.test
+
+      import javax.inject.Inject
+
+      class Subject {
+        @get:Inject val name: String = ""
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            val nameGetterAnnotations = ref.properties.named("name")
+              .getterAnnotations
+              .map { it.fqName.asString() }
+
+            assertThat(nameGetterAnnotations).containsExactly("javax.inject.Inject")
+
+            val nameAnnotations = ref.properties.named("name")
+              .annotations
+              .map { it.fqName.asString() }
+
+            assertThat(nameAnnotations).containsExactly("javax.inject.Inject")
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `member properties can have annotations on a setter delegate`() {
+    compile(
+      """
+      package com.squareup.test
+
+      import javax.inject.Inject
+
+      class Subject {
+        @set:Inject var name: String = "" 
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            val nameSetterAnnotations = ref.properties.named("name")
+              .setterAnnotations
+              .map { it.fqName.asString() }
+
+            assertThat(nameSetterAnnotations).containsExactly("javax.inject.Inject")
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `member property setter annotations are included in the field's annotations`() {
+    compile(
+      """
+      package com.squareup.test
+
+      import javax.inject.Inject
+
+      class Subject {
+        @set:Inject var name: String = "" 
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            val nameSetterAnnotations = ref.properties.named("name")
+              .setterAnnotations
+              .map { it.fqName.asString() }
+
+            assertThat(nameSetterAnnotations).containsExactly("javax.inject.Inject")
+
+            val nameAnnotations = ref.properties.named("name")
+              .annotations
+              .map { it.fqName.asString() }
+
+            assertThat(nameAnnotations).containsExactly("javax.inject.Inject")
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `primary constructor properties return empty accessor delegate annotations`() {
+    compile(
+      """
+      package com.squareup.test
+
+      import javax.inject.Inject
+
+      class Subject(
+        val name: String
+      )
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            val nameProperty = ref.properties.named("name")
+
+            assertThat(nameProperty.getterAnnotations).isEmpty()
+            assertThat(nameProperty.setterAnnotations).isEmpty()
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `primary constructor properties can have annotations`() {
+    compile(
+      """
+      package com.squareup.test
+
+      import javax.inject.Inject
+
+      class Subject(
+        @Inject var name: String,
+        val age: Int
+      )
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            val nameAnnotations = ref.properties.named("name")
+              .annotations
+              .map { it.fqName.asString() }
+
+            assertThat(nameAnnotations).containsExactly("javax.inject.Inject")
+
+            val ageAnnotations = ref.properties.named("age")
+              .annotations
+              .map { it.fqName.asString() }
+
+            assertThat(ageAnnotations).isEmpty()
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `primary constructor parameters without val or var are not properties`() {
+    compile(
+      """
+      package com.squareup.test
+
+      class Subject(
+        val name: String,
+        val age: Int,
+        notAProperty: Double
+      )
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            assertThat(ref.propertyTypeNames())
+              .containsExactly(
+                "name: kotlin.String",
+                "age: kotlin.Int"
+              )
+              .inOrder()
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  @Test fun `secondary constructor parameters are not properties`() {
+    compile(
+      """
+      package com.squareup.test
+
+      class Subject(
+        val name: String,
+        val age: Int
+      ) { 
+        constructor(
+          name: String,
+          age: Int,
+          notAProperty: Double
+        ): this(name, age)
+      }
+      """,
+      allWarningsAsErrors = false,
+      codeGenerators = listOf(
+        simpleCodeGenerator { psiRef ->
+          val descriptorRef = psiRef.toDescriptorReference()
+
+          listOf(psiRef, descriptorRef).forEach { ref ->
+
+            assertThat(ref.propertyTypeNames())
+              .containsExactly(
+                "name: kotlin.String",
+                "age: kotlin.Int"
+              )
+              .inOrder()
+          }
+
+          null
+        }
+      )
+    ) {
+      assertThat(exitCode).isEqualTo(OK)
+    }
+  }
+
+  private fun ClassReference.propertyTypeNames() = properties
+    .map { "${it.name}: ${it.type().asTypeName()}" }
+
+  private fun List<PropertyReference>.named(name: String) = single { it.name == name }
+}