Use kotlinx-metadata for more accurate Kotlin checks (#75)

* Shade in kotlin-metadata * Implement SlackJavaEvaluator * Use SlackJavaEvaluator in mockdetector * Fix missing dep in tests * Clean up parseMetadata a bit
slackhq · May 29, 2023 · 2ad70a1 · 2ad70a1
1 parent 6c0dcac
commit 2ad70a1
Show file tree

Hide file tree

Showing 7 changed files with 315 additions and 22 deletions.
diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml
@@ -13,12 +13,14 @@ dokka = { id = "org.jetbrains.dokka", version = "1.8.10" }
 lint = { id = "com.android.lint", version = "8.0.2" }
 ksp = { id = "com.google.devtools.ksp", version = "1.8.21-1.0.11" }
 mavenPublish = { id = "com.vanniktech.maven.publish", version = "0.25.2" }
+mavenShadow = { id = "com.github.johnrengelman.shadow", version = "8.1.1" }
 spotless = { id = "com.diffplug.spotless", version = "6.19.0" }
 
 [libraries]
 autoService-annotations = "com.google.auto.service:auto-service-annotations:1.1.0"
 autoService-ksp = "dev.zacsweers.autoservice:auto-service-ksp:1.0.0"
 junit = "junit:junit:4.13.2"
+kotlin-metadata = { module = "org.jetbrains.kotlinx:kotlinx-metadata-jvm", version = "0.6.0" }
 ktfmt = { module = "com.facebook:ktfmt", version.ref = "ktfmt" }
 eithernet = "com.slack.eithernet:eithernet:1.4.0"
 retrofit = "com.squareup.retrofit2:retrofit:2.9.0"

diff --git a/slack-lint-checks/build.gradle.kts b/slack-lint-checks/build.gradle.kts
@@ -1,5 +1,6 @@
 // Copyright (C) 2021 Slack Technologies, LLC
 // SPDX-License-Identifier: Apache-2.0
+import com.github.jengelman.gradle.plugins.shadow.transformers.ServiceFileTransformer
 import org.jetbrains.kotlin.gradle.dsl.KotlinVersion
 import org.jetbrains.kotlin.gradle.tasks.KotlinCompile
 
@@ -9,6 +10,7 @@ plugins {
   alias(libs.plugins.lint)
   alias(libs.plugins.ksp)
   alias(libs.plugins.mavenPublish)
+  alias(libs.plugins.mavenShadow)
 }
 
 lint {
@@ -20,10 +22,18 @@ lint {
   baseline = file("lint-baseline.xml")
 }
 
+val shade: Configuration = configurations.maybeCreate("compileShaded")
+
+configurations.getByName("compileOnly").extendsFrom(shade)
+
 dependencies {
   compileOnly(libs.bundles.lintApi)
   ksp(libs.autoService.ksp)
   implementation(libs.autoService.annotations)
+  shade(libs.kotlin.metadata) { exclude(group = "org.jetbrains.kotlin", module = "kotlin-stdlib") }
+
+  // Dupe the dep because the shaded version is compileOnly in the eyes of the gradle configurations
+  testImplementation(libs.kotlin.metadata)
   testImplementation(libs.bundles.lintTest)
   testImplementation(libs.junit)
 
@@ -40,3 +50,18 @@ tasks.withType<KotlinCompile>().configureEach {
     languageVersion.set(KotlinVersion.KOTLIN_1_8)
   }
 }
+
+val shadowJar =
+  tasks.shadowJar.apply {
+    configure {
+      archiveClassifier.set("")
+      configurations = listOf(shade)
+      relocate("kotlinx.metadata", "slack.lint.shaded.kotlinx.metadata")
+      transformers.add(ServiceFileTransformer())
+    }
+  }
+
+artifacts {
+  runtimeOnly(shadowJar)
+  archives(shadowJar)
+}
diff --git a/slack-lint-checks/src/main/java/slack/lint/mocking/AbstractMockDetector.kt b/slack-lint-checks/src/main/java/slack/lint/mocking/AbstractMockDetector.kt
@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 package slack.lint.mocking
 
+import com.android.tools.lint.client.api.JavaEvaluator
 import com.android.tools.lint.client.api.UElementHandler
 import com.android.tools.lint.detector.api.Detector
 import com.android.tools.lint.detector.api.JavaContext
@@ -19,6 +20,7 @@ import org.jetbrains.uast.UField
 import org.jetbrains.uast.UReferenceExpression
 import org.jetbrains.uast.UVariable
 import org.jetbrains.uast.getParentOfType
+import slack.lint.util.SlackJavaEvaluator
 
 /**
  * A base detector class for detecting different kinds of mocking behavior. Subclasses can indicate
@@ -39,7 +41,11 @@ abstract class AbstractMockDetector : Detector(), SourceCodeScanner {
   /** Set of annotation FQCNs that should not be mocked */
   open val annotations: Set<String> = emptySet()
 
-  open fun checkType(context: JavaContext, mockedType: PsiClass): Reason? {
+  open fun checkType(
+    context: JavaContext,
+    evaluator: JavaEvaluator,
+    mockedType: PsiClass
+  ): Reason? {
     return null
   }
 
@@ -53,6 +59,7 @@ abstract class AbstractMockDetector : Detector(), SourceCodeScanner {
   override fun getApplicableUastTypes() = listOf(UCallExpression::class.java, UField::class.java)
 
   override fun createUastHandler(context: JavaContext): UElementHandler {
+    val slackEvaluator = SlackJavaEvaluator(context.file.name, context.evaluator)
     return object : UElementHandler() {
 
       // Checks for mock()/spy() calls
@@ -64,22 +71,22 @@ abstract class AbstractMockDetector : Detector(), SourceCodeScanner {
           var argumentType: PsiClass? = null
           if (node.typeArgumentCount == 1) {
             // We can read the type here for the fun <reified T> mock() helpers
-            argumentType = context.evaluator.getTypeClass(node.typeArguments[0])
+            argumentType = slackEvaluator.getTypeClass(node.typeArguments[0])
           } else if (resolvedClass in MOCK_CLASSES && node.valueArgumentCount != 0) {
             when (val firstArg = node.valueArguments[0]) {
               is UClassLiteralExpression -> {
                 // It's Foo.class, we can just use it directly
-                argumentType = context.evaluator.getTypeClass(firstArg.type)
+                argumentType = slackEvaluator.getTypeClass(firstArg.type)
               }
               is UReferenceExpression -> {
                 val type = firstArg.getExpressionType()
                 if (node.methodName == "spy") {
                   // spy takes an instance, so take the type at face value
-                  argumentType = context.evaluator.getTypeClass(type)
+                  argumentType = slackEvaluator.getTypeClass(type)
                 } else if (type is PsiClassType && type.parameterCount == 1) {
                   // If it's a Class and not a "spy" method, assume it's the mock type
                   val classGeneric = type.parameters[0]
-                  argumentType = context.evaluator.getTypeClass(classGeneric)
+                  argumentType = slackEvaluator.getTypeClass(classGeneric)
                 }
               }
             }
@@ -88,7 +95,7 @@ abstract class AbstractMockDetector : Detector(), SourceCodeScanner {
             // Covers cases like `val dynamicMock: TestClass = mock()`
             val variable = node.getParentOfType<UVariable>() ?: return
             nodeToReport = variable
-            argumentType = context.evaluator.getTypeClass(variable.type)
+            argumentType = slackEvaluator.getTypeClass(variable.type)
           }
 
           argumentType?.let { checkMock(nodeToReport, argumentType) }
@@ -100,12 +107,12 @@ abstract class AbstractMockDetector : Detector(), SourceCodeScanner {
         if (isKotlin(node)) {
           val sourcePsi = node.sourcePsi ?: return
           if (sourcePsi is KtProperty && isMockAnnotated(node)) {
-            val type = context.evaluator.getTypeClass(node.type) ?: return
+            val type = slackEvaluator.getTypeClass(node.type) ?: return
             checkMock(node, type)
             return
           }
         } else if (isJava(node) && isMockAnnotated(node)) {
-          val type = context.evaluator.getTypeClass(node.type) ?: return
+          val type = slackEvaluator.getTypeClass(node.type) ?: return
           checkMock(node, type)
           return
         }
@@ -116,7 +123,7 @@ abstract class AbstractMockDetector : Detector(), SourceCodeScanner {
       }
 
       private fun checkMock(node: UElement, type: PsiClass) {
-        val reason = checkType(context, type)
+        val reason = checkType(context, slackEvaluator, type)
         if (reason != null) {
           report(context, type, node, reason)
           return
@@ -138,10 +145,7 @@ abstract class AbstractMockDetector : Detector(), SourceCodeScanner {
   /**
    * @property type a [PsiClass] object representing the class that should not be mocked.
    * @property reason The reason this class should not be mocked, which may be as simple as "it is
-   *
-   * ```
-   *                  annotated to forbid mocking" but may also provide a suggested workaround.
-   * ```
+   *   annotated to forbid mocking" but may also provide a suggested workaround.
    */
   data class Reason(val type: PsiClass, val reason: String)
 }
diff --git a/slack-lint-checks/src/main/java/slack/lint/mocking/DataClassMockDetector.kt b/slack-lint-checks/src/main/java/slack/lint/mocking/DataClassMockDetector.kt
@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 package slack.lint.mocking
 
+import com.android.tools.lint.client.api.JavaEvaluator
 import com.android.tools.lint.detector.api.Category
 import com.android.tools.lint.detector.api.Issue
 import com.android.tools.lint.detector.api.JavaContext
@@ -30,8 +31,12 @@ class DataClassMockDetector : AbstractMockDetector() {
 
   override val annotations: Set<String> = emptySet()
 
-  override fun checkType(context: JavaContext, mockedType: PsiClass): Reason? {
-    return if (context.evaluator.isData(mockedType)) {
+  override fun checkType(
+    context: JavaContext,
+    evaluator: JavaEvaluator,
+    mockedType: PsiClass
+  ): Reason? {
+    return if (evaluator.isData(mockedType)) {
       Reason(
         mockedType,
         "data classes represent pure value classes, so mocking them should not be necessary"

diff --git a/slack-lint-checks/src/main/java/slack/lint/mocking/DoNotMockMockDetector.kt b/slack-lint-checks/src/main/java/slack/lint/mocking/DoNotMockMockDetector.kt
@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 package slack.lint.mocking
 
+import com.android.tools.lint.client.api.JavaEvaluator
 import com.android.tools.lint.detector.api.Category
 import com.android.tools.lint.detector.api.Issue
 import com.android.tools.lint.detector.api.JavaContext
@@ -34,7 +35,11 @@ class DoNotMockMockDetector : AbstractMockDetector() {
     private const val FQCN_EP_DNM = "com.google.errorprone.annotations.DoNotMock"
   }
 
-  override fun checkType(context: JavaContext, mockedType: PsiClass): Reason? {
+  override fun checkType(
+    context: JavaContext,
+    evaluator: JavaEvaluator,
+    mockedType: PsiClass
+  ): Reason? {
     val uMockedType = mockedType.toUElementOfType<UClass>() ?: return null
     val doNotMockAnnotation =
       uMockedType.findAnnotation(FQCN_SLACK_DNM)

diff --git a/slack-lint-checks/src/main/java/slack/lint/util/LintUtils.kt b/slack-lint-checks/src/main/java/slack/lint/util/LintUtils.kt
@@ -42,12 +42,9 @@ import org.jetbrains.uast.tryResolve
 /**
  * @param qualifiedName the qualified name of the desired interface type
  * @param nameFilter an optional name filter, used to check when to stop searching up the type
- *
- * ```
- *                   hierarchy. This is useful if you want to only check direct implementers in
- *                   certain packages. Called with a fully qualified class name; return false if
- *                   you want to stop searching up the type tree, true to continue.
- * ```
+ *   hierarchy. This is useful if you want to only check direct implementers in certain packages.
+ *   Called with a fully qualified class name; return false if you want to stop searching up the
+ *   type tree, true to continue.
  */
 internal fun PsiClass.implements(
   qualifiedName: String,
@@ -310,3 +307,26 @@ internal fun StringOption.loadAsSet(
     .filter(String::isNotBlank)
     .toSet()
 }
+
+internal inline fun <T, reified R> Array<out T>.mapArray(transform: (T) -> R): Array<R> =
+  Array(this.size) { i -> transform(this[i]) }
+
+internal inline fun <T> measureTimeMillisWithResult(block: () -> T): Pair<Long, T> {
+  val start = System.currentTimeMillis()
+  val result = block()
+  return Pair(System.currentTimeMillis() - start, result)
+}
+
+private val logVerbosely by lazy {
+  System.getProperty("slack.lint.logVerbosely", "false").toBoolean()
+}
+
+/**
+ * Logs to std if [logVerbosely] is enabled. Useful for debugging and should not generally be
+ * enabled.
+ */
+internal fun slackLintLog(message: String) {
+  if (logVerbosely) {
+    println("SlackLint: $message")
+  }
+}