Add a type-resolving NamespaceImport lint check

gradle/libs.versions.toml

@@ -249,6 +249,7 @@ zXing = { module = "com.google.zxing:core", version.ref = "zXing" }
 zoomable = { module = "com.mxalbert.zoomable:zoomable", version.ref = "zoomable" }
 # lint dependencies
 lintApi = { module = "com.android.tools.lint:lint-api", version.ref = "lintApi" }
+lintTests = { module = "com.android.tools.lint:lint-tests", version.ref = "lintApi" }
 
 # temporary bug workaround
 androidx-kmp-preview-bug1-workaround = { module = "androidx.customview:customview-poolingcontainer", version = "1.1.0" }

hedvig-lint/build.gradle.kts

@@ -9,6 +9,9 @@ plugins {
 
 dependencies {
   compileOnly(libs.lintApi)
+  testImplementation(libs.junit)
+  testImplementation(libs.lintApi)
+  testImplementation(libs.lintTests)
 }
 
 java {

hedvig-lint/src/main/kotlin/com/hedvig/android/lint/HedvigLintRegistry.kt

@@ -8,6 +8,7 @@ import com.android.tools.lint.detector.api.Issue
 class HedvigLintRegistry : IssueRegistry() {
   override val issues: List<Issue> = listOf(
     Material2Detector.ISSUE,
+    NamespaceImportDetector.ISSUE,
   )
 
   override val api: Int = CURRENT_API

hedvig-lint/src/main/kotlin/com/hedvig/android/lint/NamespaceImportDetector.kt

@@ -0,0 +1,133 @@
+package com.hedvig.android.lint
+
+import com.android.tools.lint.client.api.UElementHandler
+import com.android.tools.lint.detector.api.Category
+import com.android.tools.lint.detector.api.Issue
+import com.android.tools.lint.detector.api.JavaContext
+import com.android.tools.lint.detector.api.Severity
+import com.android.tools.lint.detector.api.SourceCodeScanner
+import com.android.tools.lint.detector.api.StringOption
+import com.android.tools.lint.detector.api.isKotlin
+import com.hedvig.android.lint.config.Priorities
+import com.hedvig.android.lint.util.OptionLoadingDetector
+import com.hedvig.android.lint.util.StringSetLintOption
+import com.hedvig.android.lint.util.sourceImplementation
+import org.jetbrains.uast.UElement
+import org.jetbrains.uast.UImportStatement
+
+private const val NamespaceImportDetectorIssueId = "NamespaceImport"
+
+/**
+ * Reports imports that shorten a qualified reference past the point where the short name still says
+ * what it is, such as `import hedvig.resources.Res.string` turning `Res.string.FOO` into `string.FOO`.
+ *
+ * Importing a type is fine, so a sealed subclass or enum entry (`HomeUiState.Success`) is left alone.
+ * Importing a member off a class owner is not, because the owner is what gave the name its meaning.
+ * A capitalized name that is still meaningless on its own, such as `Clock.System`, is covered by
+ * [DENY_LIST].
+ *
+ * The `hedvig:namespace-import` ktlint rule enforces the same policy across every source set and is
+ * the mechanism of record. This check is additive: it resolves the owner instead of guessing from the
+ * shape of the import path, and it reports inline in the IDE while the import is being typed.
+ * [DEFAULT_DENIED_IMPORTS] therefore has to stay in step with that rule's own denied list.
+ *
+ * It cannot subsume the ktlint rule, because AGP's KMP library plugin registers no task that runs
+ * Android Lint (https://issuetracker.google.com/issues/246751841), which puts every KMP module, the
+ * design system among them, out of reach. Should that gain a runnable lint task, this check becomes
+ * able to cover the whole repository and the ktlint rule becomes the redundant half of the pair.
+ */
+internal class NamespaceImportDetector
+  @JvmOverloads
+  constructor(
+    private val extraDeniedImports: StringSetLintOption = StringSetLintOption(DENY_LIST),
+    private val allowedImports: StringSetLintOption = StringSetLintOption(ALLOW_LIST),
+  ) : OptionLoadingDetector(extraDeniedImports, allowedImports), SourceCodeScanner {
+    override fun getApplicableUastTypes(): List<Class<out UElement>> = listOf<Class<out UElement>>(
+      UImportStatement::class.java,
+    )
+
+    override fun createUastHandler(context: JavaContext): UElementHandler? {
+      val language = context.uastFile?.lang ?: return null
+      if (!isKotlin(language)) return null
+      return object : UElementHandler() {
+        override fun visitImportStatement(node: UImportStatement) {
+          if (node.isOnDemand) return // Wildcards are owned by ktlint's no-wildcard-imports.
+          val importText = node.sourcePsi?.text ?: return
+          // An alias is a deliberate act of renaming, and gives the use site a name of its own.
+          if (importText.contains(" as ")) return
+          val qualifiedName = importText.removePrefix("import").trim()
+
+          val importedName = qualifiedName.substringAfterLast('.')
+          val ownerPath = qualifiedName.substringBeforeLast('.', "")
+          val ownerName = ownerPath.substringAfterLast('.')
+          if (importedName.isEmpty() || ownerName.isEmpty()) return
+          // A lowercase owner is a package, so this is a plain top-level import.
+          if (!ownerName.first().isUpperCase()) return
+          // `Duration.Companion.seconds` and friends exist to enable the `5.seconds` receiver idiom.
+          if (ownerName == "Companion") return
+          // A capitalized path segment is not proof of a class: `platform.Foundation` is a package.
+          if (context.evaluator.findClass(ownerPath) == null) return
+          if (qualifiedName in allowedImports.value) return
+
+          val importsAMember = importedName.first().isLowerCase()
+          val isDeniedByName = qualifiedName in DEFAULT_DENIED_IMPORTS ||
+            qualifiedName in extraDeniedImports.value
+          if (!importsAMember && !isDeniedByName) return
+
+          context.report(
+            issue = ISSUE,
+            location = context.getLocation(node),
+            message = "Import `$ownerName` and write `$ownerName.$importedName` at the use site. " +
+              "On its own, `$importedName` no longer says what it is.",
+          )
+        }
+      }
+    }
+
+    companion object {
+      /**
+       * Imports whose final segment is capitalized, so they read as a type, but which still leave
+       * nothing meaningful behind at the use site. Kept in step with `NamespaceImportRule`, which
+       * applies the same list where this check cannot run.
+       */
+      private val DEFAULT_DENIED_IMPORTS = setOf(
+        "kotlin.time.Clock.System",
+      )
+
+      internal val DENY_LIST = StringOption(
+        "denied-member-imports",
+        "A comma-separated list of fully qualified imports to reject in addition to the built-in ones.",
+        null,
+        "This property should define a comma-separated list of fully qualified imports that must " +
+          "never be used, even though their final segment is capitalized",
+      )
+
+      internal val ALLOW_LIST = StringOption(
+        "allowed-member-imports",
+        "A comma-separated list of fully qualified member imports that should be allowed.",
+        null,
+        "This property should define a comma-separated list of fully qualified member imports that " +
+          "are allowed to shorten their receiver away",
+      )
+
+      val ISSUE = Issue.create(
+        id = NamespaceImportDetectorIssueId,
+        briefDescription = "Importing a member hides the receiver that carries its meaning",
+        explanation = """
+        Import the type, never the namespace. An import may shorten a qualified reference only when \
+        the short name still says what it is to someone reading that line cold.
+
+        Sealed subclasses and enum entries pass that test, so `HomeUiState.Success` may be imported \
+        as `Success`. Members reached through a receiver that carries the meaning do not: \
+        `Res.string.FOO` must not become `string.FOO`, and `Clock.System.now()` must not become \
+        `System.now()`, which additionally reads as `java.lang.System`.
+      """,
+        category = Category.CORRECTNESS,
+        priority = Priorities.NORMAL,
+        severity = Severity.ERROR,
+        implementation = sourceImplementation<NamespaceImportDetector>(),
+      )
+        .setOptions(listOf(DENY_LIST, ALLOW_LIST))
+        .setEnabledByDefault(true)
+    }
+  }

hedvig-lint/src/test/kotlin/com/hedvig/android/lint/NamespaceImportDetectorTest.kt

@@ -0,0 +1,198 @@
+package com.hedvig.android.lint
+
+import com.android.tools.lint.checks.infrastructure.LintDetectorTest
+import com.android.tools.lint.detector.api.Detector
+import com.android.tools.lint.detector.api.Issue
+import org.junit.Test
+
+class NamespaceImportDetectorTest : LintDetectorTest() {
+  override fun getDetector(): Detector = NamespaceImportDetector()
+
+  override fun getIssues(): List<Issue> = listOf(NamespaceImportDetector.ISSUE)
+
+  private val resources = kotlin(
+    """
+    package com.example.res
+    object Res {
+      object string {
+        const val GREETING = "hi"
+      }
+      object drawable
+    }
+    """,
+  ).indented()
+
+  private val designSystem = kotlin(
+    """
+    package com.example.ds
+    object TooltipDefaults {
+      val defaultStyle: Int = 0
+    }
+    """,
+  ).indented()
+
+  private val uiState = kotlin(
+    """
+    package com.example.ui
+    sealed interface HomeUiState {
+      object Success : HomeUiState
+      object Loading : HomeUiState
+    }
+    """,
+  ).indented()
+
+  private val duration = kotlin(
+    """
+    package com.example.time
+    class Duration {
+      companion object {
+        val seconds: Int = 1
+      }
+    }
+    """,
+  ).indented()
+
+  // Kotlin/Native interop packages are capitalized, which a purely textual check misreads as a class.
+  private val capitalizedPackage = kotlin(
+    """
+    package platform.Foundation
+    fun systemLocale(): String = ""
+    """,
+  ).indented()
+
+  @Test
+  fun testReportsLowercaseMemberImport() {
+    lint()
+      .files(
+        resources,
+        kotlin(
+          """
+          package com.example.app
+          import com.example.res.Res.string
+          fun greet() = string.GREETING
+          """,
+        ).indented(),
+      )
+      .issues(NamespaceImportDetector.ISSUE)
+      .run()
+      .expectErrorCount(1)
+      .expectContains("Import Res and write Res.string at the use site")
+  }
+
+  @Test
+  fun testReportsEveryOffendingImportInAFile() {
+    lint()
+      .files(
+        resources,
+        designSystem,
+        kotlin(
+          """
+          package com.example.app
+          import com.example.ds.TooltipDefaults.defaultStyle
+          import com.example.res.Res.drawable
+          import com.example.res.Res.string
+          fun use() = listOf(string, drawable, defaultStyle)
+          """,
+        ).indented(),
+      )
+      .issues(NamespaceImportDetector.ISSUE)
+      .run()
+      .expectErrorCount(3)
+  }
+
+  @Test
+  fun testAllowsSealedSubclassImport() {
+    lint()
+      .files(
+        uiState,
+        kotlin(
+          """
+          package com.example.app
+          import com.example.ui.HomeUiState
+          import com.example.ui.HomeUiState.Loading
+          import com.example.ui.HomeUiState.Success
+          fun describe(state: HomeUiState) = when (state) {
+            Success -> "ok"
+            Loading -> "wait"
+          }
+          """,
+        ).indented(),
+      )
+      .issues(NamespaceImportDetector.ISSUE)
+      .run()
+      .expectClean()
+  }
+
+  @Test
+  fun testAllowsCompanionExtensionImport() {
+    lint()
+      .files(
+        duration,
+        kotlin(
+          """
+          package com.example.app
+          import com.example.time.Duration
+          import com.example.time.Duration.Companion.seconds
+          fun timeout() = Duration.seconds
+          """,
+        ).indented(),
+      )
+      .issues(NamespaceImportDetector.ISSUE)
+      .run()
+      .expectClean()
+  }
+
+  @Test
+  fun testIgnoresCapitalizedPackage() {
+    lint()
+      .files(
+        capitalizedPackage,
+        kotlin(
+          """
+          package com.example.app
+          import platform.Foundation.systemLocale
+          fun locale() = systemLocale()
+          """,
+        ).indented(),
+      )
+      .issues(NamespaceImportDetector.ISSUE)
+      .run()
+      .expectClean()
+  }
+
+  @Test
+  fun testAllowsAliasedImport() {
+    lint()
+      .files(
+        resources,
+        kotlin(
+          """
+          package com.example.app
+          import com.example.res.Res.string as StringResources
+          fun greet() = StringResources.GREETING
+          """,
+        ).indented(),
+      )
+      .issues(NamespaceImportDetector.ISSUE)
+      .run()
+      .expectClean()
+  }
+
+  @Test
+  fun testIgnoresWildcardImport() {
+    lint()
+      .files(
+        resources,
+        kotlin(
+          """
+          package com.example.app
+          import com.example.res.*
+          fun greet() = Res.string.GREETING
+          """,
+        ).indented(),
+      )
+      .issues(NamespaceImportDetector.ISSUE)
+      .run()
+      .expectClean()
+  }
+}