From 3cc093c682453a47312688e64a3a121908ef27cb Mon Sep 17 00:00:00 2001 From: stylianosgakis Date: Thu, 27 Aug 2026 16:50:51 +0200 Subject: [PATCH] chore: add a type-resolving NamespaceImport lint check The ktlint rule already enforces the import rule on every source set, so this adds precision and IDE feedback rather than coverage. Two things it does that ktlint cannot. It resolves the owner through context.evaluator.findClass instead of inferring one from the shape of the import path, so platform.Foundation.systemLocale needs no prefix exception to stay unflagged. And Android Lint reports inline in the IDE while the import is being typed, which is where a style rule is cheapest to obey. It cannot run on KMP modules, so the ktlint rule stays the mechanism of record and the two denied lists have to be kept in step. Deny and allow lists are configurable through the shared lint.xml. hedvig-lint had no test source set, so this adds one plus the lint-tests dependency. Seven cases cover both directions of the rule. --- gradle/libs.versions.toml | 1 + hedvig-lint/build.gradle.kts | 3 + .../hedvig/android/lint/HedvigLintRegistry.kt | 1 + .../android/lint/NamespaceImportDetector.kt | 133 ++++++++++++ .../lint/NamespaceImportDetectorTest.kt | 198 ++++++++++++++++++ 5 files changed, 336 insertions(+) create mode 100644 hedvig-lint/src/main/kotlin/com/hedvig/android/lint/NamespaceImportDetector.kt create mode 100644 hedvig-lint/src/test/kotlin/com/hedvig/android/lint/NamespaceImportDetectorTest.kt diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index ef82d6a592..0e33cc1aae 100644 --- a/gradle/libs.versions.toml +++ b/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" } diff --git a/hedvig-lint/build.gradle.kts b/hedvig-lint/build.gradle.kts index 585c1e0326..595239799e 100644 --- a/hedvig-lint/build.gradle.kts +++ b/hedvig-lint/build.gradle.kts @@ -9,6 +9,9 @@ plugins { dependencies { compileOnly(libs.lintApi) + testImplementation(libs.junit) + testImplementation(libs.lintApi) + testImplementation(libs.lintTests) } java { diff --git a/hedvig-lint/src/main/kotlin/com/hedvig/android/lint/HedvigLintRegistry.kt b/hedvig-lint/src/main/kotlin/com/hedvig/android/lint/HedvigLintRegistry.kt index bf5c2ddf08..f719a19c90 100644 --- a/hedvig-lint/src/main/kotlin/com/hedvig/android/lint/HedvigLintRegistry.kt +++ b/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 = listOf( Material2Detector.ISSUE, + NamespaceImportDetector.ISSUE, ) override val api: Int = CURRENT_API diff --git a/hedvig-lint/src/main/kotlin/com/hedvig/android/lint/NamespaceImportDetector.kt b/hedvig-lint/src/main/kotlin/com/hedvig/android/lint/NamespaceImportDetector.kt new file mode 100644 index 0000000000..bba0cc66d8 --- /dev/null +++ b/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> = listOf>( + 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(), + ) + .setOptions(listOf(DENY_LIST, ALLOW_LIST)) + .setEnabledByDefault(true) + } + } diff --git a/hedvig-lint/src/test/kotlin/com/hedvig/android/lint/NamespaceImportDetectorTest.kt b/hedvig-lint/src/test/kotlin/com/hedvig/android/lint/NamespaceImportDetectorTest.kt new file mode 100644 index 0000000000..2e586d1b98 --- /dev/null +++ b/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 = 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() + } +}