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() + } +}