From eae894152cc2529ef1abc2db89c50d281043c6f6 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Thu, 4 Jun 2026 13:05:55 -0400 Subject: [PATCH] Add a new ci task with faster lint. --- .github/workflows/android.yml | 4 +- build.gradle.kts | 28 +- fast-lint/build.gradle.kts | 39 +++ .../kotlin/org/signal/fastlint/FastLint.kt | 329 ++++++++++++++++++ .../kotlin/org/signal/fastlint/Finding.kt | 11 + .../main/kotlin/org/signal/fastlint/Lint.kt | 59 ++++ .../main/kotlin/org/signal/fastlint/Rule.kt | 174 +++++++++ .../signal/fastlint/rules/AlertDialogRule.kt | 46 +++ .../org/signal/fastlint/rules/AllRules.kt | 14 + .../fastlint/rules/DatabaseReferenceRule.kt | 75 ++++ .../fastlint/rules/ForegroundServiceRule.kt | 48 +++ .../signal/fastlint/rules/LogNotSignalRule.kt | 69 ++++ .../fastlint/rules/LogTagInlinedRule.kt | 53 +++ .../rules/StringResourceEscapingRule.kt | 76 ++++ .../signal/fastlint/rules/VersionCodeRule.kt | 27 ++ .../kotlin/org/signal/fastlint/TestSupport.kt | 25 ++ .../fastlint/rules/AlertDialogRuleTest.kt | 47 +++ .../rules/DatabaseReferenceRuleTest.kt | 71 ++++ .../rules/ForegroundServiceRuleTest.kt | 51 +++ .../fastlint/rules/LogNotSignalRuleTest.kt | 138 ++++++++ .../fastlint/rules/LogTagInlinedRuleTest.kt | 71 ++++ .../rules/StringResourceEscapingRuleTest.kt | 90 +++++ .../fastlint/rules/VersionCodeRuleTest.kt | 39 +++ gradle/lint-libs.versions.toml | 5 + settings.gradle.kts | 1 + 25 files changed, 1588 insertions(+), 2 deletions(-) create mode 100644 fast-lint/build.gradle.kts create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/FastLint.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/Finding.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/Lint.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/Rule.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/rules/AlertDialogRule.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/rules/AllRules.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/rules/DatabaseReferenceRule.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/rules/ForegroundServiceRule.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/rules/LogNotSignalRule.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/rules/LogTagInlinedRule.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/rules/StringResourceEscapingRule.kt create mode 100644 fast-lint/src/main/kotlin/org/signal/fastlint/rules/VersionCodeRule.kt create mode 100644 fast-lint/src/test/kotlin/org/signal/fastlint/TestSupport.kt create mode 100644 fast-lint/src/test/kotlin/org/signal/fastlint/rules/AlertDialogRuleTest.kt create mode 100644 fast-lint/src/test/kotlin/org/signal/fastlint/rules/DatabaseReferenceRuleTest.kt create mode 100644 fast-lint/src/test/kotlin/org/signal/fastlint/rules/ForegroundServiceRuleTest.kt create mode 100644 fast-lint/src/test/kotlin/org/signal/fastlint/rules/LogNotSignalRuleTest.kt create mode 100644 fast-lint/src/test/kotlin/org/signal/fastlint/rules/LogTagInlinedRuleTest.kt create mode 100644 fast-lint/src/test/kotlin/org/signal/fastlint/rules/StringResourceEscapingRuleTest.kt create mode 100644 fast-lint/src/test/kotlin/org/signal/fastlint/rules/VersionCodeRuleTest.kt diff --git a/.github/workflows/android.yml b/.github/workflows/android.yml index 7a2afbea18..5cf155040c 100644 --- a/.github/workflows/android.yml +++ b/.github/workflows/android.yml @@ -41,13 +41,15 @@ jobs: # Required to persist the Gradle configuration cache across runs. cache-encryption-key: ${{ secrets.GRADLE_ENCRYPTION_KEY }} + # Pull requests run the fast custom linter (ci); pushes to main / 8.x branches run the full + # Android lint (qa). - name: Build with Gradle env: SIGNAL_BUILD_CACHE_URL: ${{ secrets.SIGNAL_BUILD_CACHE_URL }} SIGNAL_BUILD_CACHE_USER: ${{ secrets.SIGNAL_BUILD_CACHE_USER }} SIGNAL_BUILD_CACHE_PASSWORD: ${{ secrets.SIGNAL_BUILD_CACHE_PASSWORD }} SIGNAL_BUILD_CACHE_PUSH: ${{ startsWith(github.ref, 'refs/heads/8.') }} - run: ./gradlew qa + run: ./gradlew ${{ github.event_name == 'pull_request' && 'ci' || 'qa' }} - name: Archive reports for failed build if: ${{ failure() }} diff --git a/build.gradle.kts b/build.gradle.kts index 5589efc75f..87b6da70ac 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -73,7 +73,13 @@ tasks.register("buildQa") { tasks.register("qa") { group = "Verification" - description = "Quality Assurance. Run before pushing." + description = "Quality Assurance. Run before release." + dependsOn("clean") +} + +tasks.register("ci") { + group = "Verification" + description = "Faster version of qa that's intended to be run on PRs. Uses a :fast-lint instead of full lint." dependsOn("clean") } @@ -109,6 +115,26 @@ gradle.projectsEvaluated { } } + tasks.named("ci") { + dependsOn("ktlintCheck") + dependsOn("buildQa") + dependsOn("checkStopship") + + dependsOn(appTestTask) + appCompileInstrumentationTask?.let { dependsOn(it) } + + dependsOn(":fast-lint:fastLint") + + subprojects.forEach { subproject -> + subproject.tasks.findByName("ktlintCheck")?.let { dependsOn(it) } + } + + subprojects.filter { it.name != "Signal-Android" }.forEach { subproject -> + val testTask = subproject.tasks.findByName("testDebugUnitTest") ?: subproject.tasks.findByName("test") + testTask?.let { dependsOn(it) } + } + } + // Ensure clean runs before everything else rootProject.allprojects.forEach { project -> project.tasks.matching { it.name != "clean" }.configureEach { diff --git a/fast-lint/build.gradle.kts b/fast-lint/build.gradle.kts new file mode 100644 index 0000000000..46697594d3 --- /dev/null +++ b/fast-lint/build.gradle.kts @@ -0,0 +1,39 @@ +plugins { + id("java-library") + id("org.jetbrains.kotlin.jvm") +} + +java { + sourceCompatibility = JavaVersion.toVersion(libs.versions.javaVersion.get()) + targetCompatibility = JavaVersion.toVersion(libs.versions.javaVersion.get()) +} + +kotlin { + jvmToolchain { + languageVersion = JavaLanguageVersion.of(libs.versions.kotlinJvmTarget.get()) + } +} + +dependencies { + implementation(lintLibs.intellij.core) + implementation(lintLibs.kotlin.compiler) + implementation(libs.google.guava.android) + + testImplementation(testLibs.junit.junit) +} + +tasks.register("fastLint") { + group = "Verification" + description = "Runs the fast custom AST linter (:fast-lint) over the whole repository. Fails on any finding." + mainClass.set("org.signal.fastlint.Lint") + classpath = sourceSets["main"].runtimeClasspath + maxHeapSize = "2g" + + // Strings resolved at configuration time so the task is configuration-cache compatible. + val repoRoot = rootProject.projectDir.absolutePath + val reportPath = layout.buildDirectory.file("reports/fast-lint/findings.txt").get().asFile.absolutePath + args(repoRoot, "--report=$reportPath") + + // A linter should run every time, not be skipped as up-to-date. + outputs.upToDateWhen { false } +} diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/FastLint.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/FastLint.kt new file mode 100644 index 0000000000..8a7ec2509a --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/FastLint.kt @@ -0,0 +1,329 @@ +package org.signal.fastlint + +import com.intellij.lang.java.JavaLanguage +import com.intellij.lang.java.syntax.JavaElementTypeConverterExtension +import com.intellij.openapi.Disposable +import com.intellij.openapi.util.Disposer +import com.intellij.platform.syntax.psi.CommonElementTypeConverterFactory +import com.intellij.platform.syntax.psi.ElementTypeConverters +import com.intellij.psi.PsiElement +import com.intellij.psi.PsiFileFactory +import com.intellij.psi.PsiJavaFile +import com.intellij.psi.PsiMethodCallExpression +import com.intellij.psi.PsiNewExpression +import com.intellij.psi.PsiReferenceExpression +import com.intellij.psi.JavaRecursiveElementVisitor +import com.intellij.psi.PsiClass +import org.jetbrains.kotlin.K1Deprecation +import org.jetbrains.kotlin.cli.common.messages.MessageCollector +import org.jetbrains.kotlin.cli.jvm.compiler.EnvironmentConfigFiles +import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment +import org.jetbrains.kotlin.config.CommonConfigurationKeys +import org.jetbrains.kotlin.config.CompilerConfiguration +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtClassOrObject +import org.jetbrains.kotlin.psi.KtPsiFactory +import org.jetbrains.kotlin.psi.KtSimpleNameExpression +import org.jetbrains.kotlin.psi.KtTreeVisitorVoid +import org.signal.fastlint.rules.ALL_RULES +import java.io.File +import java.io.StringReader +import javax.xml.stream.XMLInputFactory +import javax.xml.stream.XMLStreamConstants +import javax.xml.stream.XMLStreamException + +/** Aggregate result of a repository scan. */ +data class RunResult( + val kotlinFiles: Int, + val javaFiles: Int, + val xmlFiles: Int, + val findings: List +) + +/** + * The fast-lint engine. Sets up the Kotlin/Java PSI front-ends once, partitions the registered + * [rules] by the node types they handle, and analyzes Kotlin, Java, and XML sources. Reusable across + * many files; not thread-safe (PSI parsing is single-threaded). Close to release the parser. + */ +class FastLint(rules: List = ALL_RULES) : AutoCloseable { + + private val ktCallRules = rules.filterIsInstance() + private val ktNameRules = rules.filterIsInstance() + private val ktClassRules = rules.filterIsInstance() + private val javaCallRules = rules.filterIsInstance() + private val javaNewRules = rules.filterIsInstance() + private val javaReferenceRules = rules.filterIsInstance() + private val javaClassRules = rules.filterIsInstance() + private val xmlElementRules = rules.filterIsInstance() + private val xmlStringResourceRules = rules.filterIsInstance() + + private val disposable: Disposable = Disposer.newDisposable() + private val ktFactory: KtPsiFactory + private val psiFactory: PsiFileFactory + + init { + val env = createKotlinEnvironment(disposable) + registerJavaConverter(disposable) + ktFactory = KtPsiFactory(env.project, markGenerated = false) + psiFactory = PsiFileFactory.getInstance(env.project) + } + + fun run(root: File): RunResult { + var kotlin = 0 + var java = 0 + var xml = 0 + val findings = ArrayList() + for (file in sourceFiles(root)) { + val source = file.readText() + when (file.extension) { + "kt" -> { + findings.addAll(analyzeKotlin(file, source)) + kotlin++ + } + "java" -> { + findings.addAll(analyzeJava(file, source)) + java++ + } + "xml" -> { + findings.addAll(analyzeXml(file, source)) + xml++ + } + } + } + return RunResult(kotlin, java, xml, findings) + } + + fun analyzeKotlin(file: File, source: String): List { + val ktFile = ktFactory.createFile(file.name, source) + val context = KotlinFileContext(file, ktFile) + val findings = ArrayList() + val reporter = SuppressingReporter(file, findings) + + ktFile.accept(object : KtTreeVisitorVoid() { + override fun visitCallExpression(expression: KtCallExpression) { + super.visitCallExpression(expression) + for (rule in ktCallRules) rule.onCall(expression, context, reporter) + } + + override fun visitSimpleNameExpression(expression: KtSimpleNameExpression) { + super.visitSimpleNameExpression(expression) + for (rule in ktNameRules) rule.onName(expression, context, reporter) + } + + override fun visitClassOrObject(classOrObject: KtClassOrObject) { + super.visitClassOrObject(classOrObject) + for (rule in ktClassRules) rule.onClass(classOrObject, context, reporter) + } + }) + return findings + } + + fun analyzeJava(file: File, source: String): List { + val javaFile = psiFactory.createFileFromText(file.name, JavaLanguage.INSTANCE, source) as? PsiJavaFile ?: return emptyList() + val context = JavaFileContext(file, javaFile, source) + val findings = ArrayList() + val reporter = SuppressingReporter(file, findings) + + javaFile.accept(object : JavaRecursiveElementVisitor() { + override fun visitMethodCallExpression(call: PsiMethodCallExpression) { + super.visitMethodCallExpression(call) + for (rule in javaCallRules) rule.onCall(call, context, reporter) + } + + override fun visitNewExpression(expression: PsiNewExpression) { + super.visitNewExpression(expression) + for (rule in javaNewRules) rule.onNew(expression, context, reporter) + } + + override fun visitReferenceExpression(expression: PsiReferenceExpression) { + super.visitReferenceExpression(expression) + for (rule in javaReferenceRules) rule.onReference(expression, context, reporter) + } + + override fun visitClass(aClass: PsiClass) { + super.visitClass(aClass) + for (rule in javaClassRules) rule.onClass(aClass, context, reporter) + } + }) + return findings + } + + fun analyzeXml(file: File, source: String): List { + val path = file.path + val isLayout = "/res/layout" in path + val isValues = VALUES_FILE.containsMatchIn(path) + if (!isLayout && !isValues) { + return emptyList() + } + if (xmlElementRules.isEmpty() && xmlStringResourceRules.isEmpty()) { + return emptyList() + } + + val context = XmlFileContext(file, isLayout, isValues) + val findings = ArrayList() + val ignoreStack = ArrayDeque>() + + val reader = XML_INPUT_FACTORY.createXMLStreamReader(StringReader(source)) + try { + var inString = false + var stringName: String? = null + var stringLine = 0 + var stringIgnores: Set = emptySet() + val stringText = StringBuilder() + + while (reader.hasNext()) { + when (reader.next()) { + XMLStreamConstants.START_ELEMENT -> { + val line = reader.location.lineNumber + val here = HashSet() + val attributes = ArrayList(reader.attributeCount) + for (i in 0 until reader.attributeCount) { + val prefix = reader.getAttributePrefix(i) ?: "" + val local = reader.getAttributeLocalName(i) + val value = reader.getAttributeValue(i) + attributes.add(XmlAttribute(prefix, local, value)) + if (prefix == "tools" && local == "ignore") { + value.split(',').forEach { here.add(it.trim()) } + } + } + + if (xmlElementRules.isNotEmpty()) { + val element = XmlStartElement(reader.localName, line, attributes) + val sink = IgnoringXmlSink(file, findings, here, ignoreStack) + for (rule in xmlElementRules) rule.onStartElement(element, context, sink) + } + + if (isValues && reader.localName == "string" && xmlStringResourceRules.isNotEmpty()) { + inString = true + stringName = attributes.firstOrNull { it.localName == "name" }?.value + stringLine = line + stringIgnores = unionIgnores(here, ignoreStack) + stringText.setLength(0) + } + ignoreStack.addLast(here) + } + + XMLStreamConstants.CHARACTERS -> { + if (inString) { + stringText.append(reader.text) + } + } + + XMLStreamConstants.END_ELEMENT -> { + if (inString && reader.localName == "string") { + val sink = SetIgnoringXmlSink(file, findings, stringIgnores) + for (rule in xmlStringResourceRules) rule.onStringResource(stringName, stringText.toString(), stringLine, context, sink) + inString = false + } + ignoreStack.removeLast() + } + } + } + } catch (e: XMLStreamException) { + // Skip files that aren't well-formed standalone XML (e.g. fragments); lint skips these too. + } finally { + reader.close() + } + return findings + } + + override fun close() = Disposer.dispose(disposable) + + private class SuppressingReporter(val file: File, val out: MutableList) : Reporter { + override fun report(checkId: String, element: PsiElement, line: Int, message: String) { + if (!isSuppressed(element, checkId)) { + out.add(Finding(checkId, file, line, message)) + } + } + } + + private class IgnoringXmlSink( + val file: File, + val out: MutableList, + val here: Set, + val ancestors: ArrayDeque> + ) : XmlSink { + override fun report(checkId: String, line: Int, message: String) { + if (here.contains(checkId) || here.contains("all")) { + return + } + if (ancestors.any { it.contains(checkId) || it.contains("all") }) { + return + } + out.add(Finding(checkId, file, line, message)) + } + } + + private class SetIgnoringXmlSink(val file: File, val out: MutableList, val ignores: Set) : XmlSink { + override fun report(checkId: String, line: Int, message: String) { + if (ignores.contains(checkId) || ignores.contains("all")) { + return + } + out.add(Finding(checkId, file, line, message)) + } + } + + companion object { + private val VALUES_FILE = Regex("/res/values/.*\\.xml$") + private val EXCLUDED_DIRS = setOf("build", ".git", ".gradle", ".idea", "test", "androidTest", "testFixtures") + + // Vendored third-party forks (Glide, PhotoView) are not held to Signal conventions. + private val EXCLUDED_MODULE_PATHS = listOf("lib/glide/", "lib/photoview/") + + private val XML_INPUT_FACTORY: XMLInputFactory = XMLInputFactory.newInstance().apply { + setProperty(XMLInputFactory.IS_NAMESPACE_AWARE, true) + setProperty(XMLInputFactory.SUPPORT_DTD, false) + } + + private fun unionIgnores(here: Set, ancestors: ArrayDeque>): Set { + if (here.isEmpty() && ancestors.all { it.isEmpty() }) { + return emptySet() + } + val result = HashSet(here) + ancestors.forEach { result.addAll(it) } + return result + } + + /** + * KotlinCoreEnvironment registers the Kotlin token->IElementType converter for the new + * platform.syntax framework, but not Java's, so parsing Java source otherwise fails with + * "IElementType for token WHITE_SPACE is missing". Register the common + Java converters ourselves. + */ + private fun registerJavaConverter(disposable: Disposable) { + val converters = ElementTypeConverters.instance + converters.addExplicitExtension(JavaLanguage.INSTANCE, CommonElementTypeConverterFactory(), disposable) + converters.addExplicitExtension(JavaLanguage.INSTANCE, JavaElementTypeConverterExtension(), disposable) + } + + // createForProduction is K1 API (opt-in, slated to become an error in Kotlin 2.3). We + // deliberately use the parse-only K1 PSI environment (as ktlint does); migrating to the + // Analysis API is unnecessary for syntax-only checks. + @OptIn(K1Deprecation::class) + private fun createKotlinEnvironment(disposable: Disposable): KotlinCoreEnvironment { + val config = CompilerConfiguration().apply { + put(CommonConfigurationKeys.MODULE_NAME, "fastlint") + put(CommonConfigurationKeys.MESSAGE_COLLECTOR_KEY, MessageCollector.NONE) + } + return KotlinCoreEnvironment.createForProduction(disposable, config, EnvironmentConfigFiles.JVM_CONFIG_FILES) + } + + fun sourceFiles(root: File): List { + val rootPath = root.path + val out = ArrayList(16384) + root.walkTopDown() + .onEnter { it.name !in EXCLUDED_DIRS } + .forEach { f -> + if (f.isFile && "/src/" in f.path && "/test/" !in f.path && "/androidTest/" !in f.path) { + val relative = f.path.removePrefix(rootPath).trimStart('/') + if (EXCLUDED_MODULE_PATHS.any { relative.startsWith(it) }) { + return@forEach + } + when (f.extension) { + "kt", "java", "xml" -> out.add(f) + } + } + } + return out + } + } +} diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/Finding.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/Finding.kt new file mode 100644 index 0000000000..81c3aef0fd --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/Finding.kt @@ -0,0 +1,11 @@ +package org.signal.fastlint + +import java.io.File + +/** A single issue reported by a [Rule]. */ +data class Finding( + val checkId: String, + val file: File, + val line: Int, + val message: String +) diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/Lint.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/Lint.kt new file mode 100644 index 0000000000..14b9c57245 --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/Lint.kt @@ -0,0 +1,59 @@ +@file:JvmName("Lint") + +package org.signal.fastlint + +import java.io.File +import kotlin.system.exitProcess + +/** + * A lightweight linter that does real AST traversal of Kotlin (PSI) and Java (PSI) plus streaming + * traversal of XML resources, using the same parser front-ends that power lint/ktlint. + * + * Note that this lint is parse-only: there is no symbol resolution or classpath, so receiver classes + * are resolved syntactically via the import table. + * + * Suppression honors @SuppressLint/@SuppressWarnings/@Suppress in code and tools:ignore in XML. + * + * Usage: Lint [--report=] + */ +fun main(args: Array) { + val root = File(args.firstOrNull { !it.startsWith("--") } ?: ".").absoluteFile + val reportPath = args.firstOrNull { it.startsWith("--report=") }?.substringAfter('=') + + val start = System.nanoTime() + val result = FastLint().use { it.run(root) } + val elapsedMs = (System.nanoTime() - start) / 1_000_000 + + val sorted = result.findings.sortedWith(compareBy({ it.file.path }, { it.line }, { it.checkId })) + val rootPrefix = root.path + "/" + + val sb = StringBuilder() + sb.appendLine("fast-lint: scanned ${result.kotlinFiles} Kotlin + ${result.javaFiles} Java + ${result.xmlFiles} XML files in ${elapsedMs}ms") + sb.appendLine() + if (sorted.isEmpty()) { + sb.appendLine("No issues found.") + } else { + sb.appendLine("Issues by check:") + sorted.groupingBy { it.checkId }.eachCount().toSortedMap().forEach { (id, count) -> + sb.appendLine(" %-34s %d".format(id, count)) + } + sb.appendLine(" %-34s %d".format("TOTAL", sorted.size)) + sb.appendLine() + sb.appendLine("Issues:") + sorted.forEach { + sb.appendLine(" ${it.file.path.removePrefix(rootPrefix)}:${it.line}: ${it.checkId}: ${it.message}") + } + } + val output = sb.toString() + print(output) + + if (reportPath != null) { + File(reportPath).apply { parentFile?.mkdirs() }.writeText(output) + } + + if (sorted.isNotEmpty()) { + System.err.println("\nfast-lint found ${sorted.size} issue(s). Suppress legitimate cases with @SuppressLint(\"\") or tools:ignore.") + exitProcess(1) + } + exitProcess(0) +} diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/Rule.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/Rule.kt new file mode 100644 index 0000000000..25292ed46b --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/Rule.kt @@ -0,0 +1,174 @@ +package org.signal.fastlint + +import com.intellij.psi.PsiAnnotation +import com.intellij.psi.PsiClass +import com.intellij.psi.PsiElement +import com.intellij.psi.PsiJavaFile +import com.intellij.psi.PsiMethodCallExpression +import com.intellij.psi.PsiModifierListOwner +import com.intellij.psi.PsiNewExpression +import com.intellij.psi.PsiReferenceExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtClassOrObject +import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtModifierListOwner +import org.jetbrains.kotlin.psi.KtSimpleNameExpression +import java.io.File + +/** + * Marker for a fast-lint rule. A rule implements one or more of the capability interfaces below + * (e.g. [KtCallRule], [JavaClassRule], [XmlElementRule]) for the node types it cares about. The + * engine partitions registered rules by capability, so a rule is only invoked for the node types + * it actually handles. Rules are stateless and registered in [org.signal.fastlint.rules.ALL_RULES]. + */ +interface Rule + +interface KtCallRule : Rule { + fun onCall(call: KtCallExpression, context: KotlinFileContext, reporter: Reporter) +} + +interface KtNameRule : Rule { + fun onName(name: KtSimpleNameExpression, context: KotlinFileContext, reporter: Reporter) +} + +interface KtClassRule : Rule { + fun onClass(klass: KtClassOrObject, context: KotlinFileContext, reporter: Reporter) +} + +interface JavaCallRule : Rule { + fun onCall(call: PsiMethodCallExpression, context: JavaFileContext, reporter: Reporter) +} + +interface JavaNewRule : Rule { + fun onNew(expression: PsiNewExpression, context: JavaFileContext, reporter: Reporter) +} + +interface JavaReferenceRule : Rule { + fun onReference(reference: PsiReferenceExpression, context: JavaFileContext, reporter: Reporter) +} + +interface JavaClassRule : Rule { + fun onClass(klass: PsiClass, context: JavaFileContext, reporter: Reporter) +} + +interface XmlElementRule : Rule { + fun onStartElement(element: XmlStartElement, context: XmlFileContext, sink: XmlSink) +} + +interface XmlStringResourceRule : Rule { + fun onStringResource(name: String?, value: String, line: Int, context: XmlFileContext, sink: XmlSink) +} + +/** Reports findings for code (Kotlin/Java) rules. Handles @Suppress/@SuppressLint suppression. */ +interface Reporter { + fun report(checkId: String, element: PsiElement, line: Int, message: String) +} + +/** Reports findings for XML rules. Handles tools:ignore suppression. */ +interface XmlSink { + fun report(checkId: String, line: Int, message: String) +} + +/** Per-file context for Kotlin rules: the imports table and offset->line / receiver resolution. */ +class KotlinFileContext(val file: File, val ktFile: KtFile) { + val imports: Map = buildKotlinImports(ktFile) + fun lineOf(offset: Int): Int = lineNumberOf(ktFile.text, offset) + fun resolveFqn(receiver: String): String = resolveReceiver(receiver, imports) +} + +/** Per-file context for Java rules. */ +class JavaFileContext(val file: File, val javaFile: PsiJavaFile, val text: CharSequence) { + val imports: Map = buildJavaImports(javaFile) + fun lineOf(offset: Int): Int = lineNumberOf(text, offset) + fun resolveFqn(receiver: String): String = resolveReceiver(receiver, imports) +} + +/** Per-file context for XML rules. */ +class XmlFileContext(val file: File, val isLayout: Boolean, val isValues: Boolean) + +class XmlAttribute(val prefix: String, val localName: String, val value: String) + +class XmlStartElement(val localName: String, val line: Int, val attributes: List) + +// --------------------------------------------------------------------------- +// Shared helpers +// --------------------------------------------------------------------------- + +private val SUPPRESS_ANNOTATIONS = setOf("Suppress", "SuppressLint", "SuppressWarnings") + +internal fun lineNumberOf(text: CharSequence, offset: Int): Int { + var line = 1 + val end = minOf(offset, text.length) + var i = 0 + while (i < end) { + if (text[i] == '\n') { + line++ + } + i++ + } + return line +} + +/** Resolves a receiver like "Log" or "Build.VERSION_CODES" to a fully-qualified name via imports. */ +internal fun resolveReceiver(receiver: String, imports: Map): String { + val head = receiver.substringBefore('.') + val mapped = imports[head] ?: return receiver + return if ('.' in receiver) { + mapped + receiver.substring(receiver.indexOf('.')) + } else { + mapped + } +} + +private fun buildKotlinImports(ktFile: KtFile): Map { + val imports = HashMap() + ktFile.importList?.imports?.forEach { imp -> + val fq = imp.importedFqName?.asString() ?: return@forEach + imports[imp.aliasName ?: fq.substringAfterLast('.')] = fq + } + return imports +} + +private fun buildJavaImports(javaFile: PsiJavaFile): Map { + val imports = HashMap() + javaFile.importList?.importStatements?.forEach { imp -> + val qn = imp.qualifiedName ?: return@forEach + if (!imp.isOnDemand) { + imports[qn.substringAfterLast('.')] = qn + } + } + return imports +} + +/** True if any @Suppress/@SuppressLint/@SuppressWarnings on the element or an ancestor names checkId. */ +internal fun isSuppressed(element: PsiElement, checkId: String): Boolean { + val needle = "\"$checkId\"" + var current: PsiElement? = element + while (current != null) { + when (val node = current) { + is KtModifierListOwner -> + for (entry in node.annotationEntries) { + val name = entry.shortName?.asString() ?: continue + if (name in SUPPRESS_ANNOTATIONS) { + val args = entry.valueArgumentList?.text ?: "" + if (args.contains(needle) || args.contains("\"all\"")) { + return true + } + } + } + + is PsiModifierListOwner -> + for (annotation: PsiAnnotation in node.modifierList?.annotations.orEmpty()) { + val name = annotation.nameReferenceElement?.referenceName ?: continue + if (name in SUPPRESS_ANNOTATIONS) { + val args = annotation.parameterList.text + if (args.contains(needle) || args.contains("\"all\"")) { + return true + } + } + } + } + current = current.parent + } + return false +} diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/rules/AlertDialogRule.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/AlertDialogRule.kt new file mode 100644 index 0000000000..97fee9282a --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/AlertDialogRule.kt @@ -0,0 +1,46 @@ +package org.signal.fastlint.rules + +import com.intellij.psi.PsiNewExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtDotQualifiedExpression +import org.jetbrains.kotlin.psi.KtNameReferenceExpression +import org.jetbrains.kotlin.psi.psiUtil.getQualifiedExpressionForSelector +import org.signal.fastlint.JavaFileContext +import org.signal.fastlint.JavaNewRule +import org.signal.fastlint.KotlinFileContext +import org.signal.fastlint.KtCallRule +import org.signal.fastlint.Reporter + +/** Flags framework/appcompat AlertDialog.Builder usage in favor of MaterialAlertDialogBuilder. */ +object AlertDialogRule : KtCallRule, JavaNewRule { + + private val ALERT_DIALOG_FQNS = setOf("android.app.AlertDialog", "androidx.appcompat.app.AlertDialog") + + private fun message(fqn: String) = "Using $fqn.Builder instead of MaterialAlertDialogBuilder" + + override fun onCall(call: KtCallExpression, context: KotlinFileContext, reporter: Reporter) { + val callee = (call.calleeExpression as? KtNameReferenceExpression)?.getReferencedName() ?: return + if (callee != "Builder") { + return + } + val receiver = (call.getQualifiedExpressionForSelector() as? KtDotQualifiedExpression)?.receiverExpression?.text ?: return + if (!receiver.endsWith("AlertDialog")) { + return + } + val fqn = context.resolveFqn(receiver) + if (fqn in ALERT_DIALOG_FQNS) { + reporter.report("AlertDialogBuilderUsage", call, context.lineOf(call.textOffset), message(fqn)) + } + } + + override fun onNew(expression: PsiNewExpression, context: JavaFileContext, reporter: Reporter) { + val ref = expression.classReference?.text ?: return + if (!ref.endsWith("AlertDialog.Builder")) { + return + } + val fqn = context.resolveFqn(ref.removeSuffix(".Builder")) + if (fqn in ALERT_DIALOG_FQNS) { + reporter.report("AlertDialogBuilderUsage", expression, context.lineOf(expression.textOffset), message(fqn)) + } + } +} diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/rules/AllRules.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/AllRules.kt new file mode 100644 index 0000000000..eb6ada686d --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/AllRules.kt @@ -0,0 +1,14 @@ +package org.signal.fastlint.rules + +import org.signal.fastlint.Rule + +/** Every rule the fast linter runs. Add a new rule here to register it. */ +val ALL_RULES: List = listOf( + LogNotSignalRule, + LogTagInlinedRule, + VersionCodeRule, + ForegroundServiceRule, + AlertDialogRule, + DatabaseReferenceRule, + StringResourceEscapingRule +) diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/rules/DatabaseReferenceRule.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/DatabaseReferenceRule.kt new file mode 100644 index 0000000000..a5bd5ffe36 --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/DatabaseReferenceRule.kt @@ -0,0 +1,75 @@ +package org.signal.fastlint.rules + +import com.intellij.psi.PsiClass +import com.intellij.psi.PsiElement +import com.intellij.psi.PsiField +import com.intellij.psi.util.PsiTreeUtil +import org.jetbrains.kotlin.psi.KtClassOrObject +import org.jetbrains.kotlin.psi.KtProperty +import org.signal.fastlint.JavaClassRule +import org.signal.fastlint.JavaFileContext +import org.signal.fastlint.KotlinFileContext +import org.signal.fastlint.KtClassRule +import org.signal.fastlint.Reporter +import java.util.Locale + +/** + * A class extending "Database" with a String column whose name contains "recipient"/"thread" must + * implement Recipient/ThreadIdDatabaseReference. Type information is approximated syntactically. + */ +object DatabaseReferenceRule : KtClassRule, JavaClassRule { + + override fun onClass(klass: KtClassOrObject, context: KotlinFileContext, reporter: Reporter) { + val fields = klass.body?.properties.orEmpty().filter { it.isStringTyped() }.mapNotNull { it.name } + check(klass, context.lineOf(klass.textOffset), klass.superTypeListEntries.map { it.text }, emptyList(), fields, reporter) + } + + override fun onClass(klass: PsiClass, context: JavaFileContext, reporter: Reporter) { + // Read fields syntactically (direct children only) to avoid building a member cache, which would + // resolve superclasses and route into the (uninitialized) Kotlin resolver. + val fields = PsiTreeUtil.getChildrenOfTypeAsList(klass, PsiField::class.java) + .filter { it.typeElement?.text == "String" || it.typeElement?.text == "java.lang.String" } + .map { it.name } + check( + element = klass, + line = context.lineOf(klass.textOffset), + superTypes = klass.extendsList?.referenceElements?.map { it.text }.orEmpty(), + implementsTypes = klass.implementsList?.referenceElements?.map { it.text }.orEmpty(), + fieldNames = fields, + reporter = reporter + ) + } + + private fun check( + element: PsiElement, + line: Int, + superTypes: List, + implementsTypes: List, + fieldNames: List, + reporter: Reporter + ) { + val all = superTypes + implementsTypes + val extendsDatabase = all.any { it.substringBefore('(').substringAfterLast('.').trim() == "Database" } + if (!extendsDatabase) { + return + } + + val implementsRecipient = all.any { it.contains("RecipientIdDatabaseReference") } + val implementsThread = all.any { it.contains("ThreadIdDatabaseReference") } + + fieldNames.forEach { name -> + val lower = name.lowercase(Locale.US) + if (!implementsRecipient && lower.contains("recipient")) { + reporter.report("RecipientIdDatabaseReferenceUsage", element, line, "References a RecipientId ('$name') without implementing RecipientIdDatabaseReference") + } + if (!implementsThread && lower.contains("thread")) { + reporter.report("ThreadIdDatabaseReferenceUsage", element, line, "References a thread id ('$name') without implementing ThreadIdDatabaseReference") + } + } + } + + private fun KtProperty.isStringTyped(): Boolean { + val typeText = typeReference?.text ?: return false + return typeText == "String" || typeText == "kotlin.String" + } +} diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/rules/ForegroundServiceRule.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/ForegroundServiceRule.kt new file mode 100644 index 0000000000..e555eb50ef --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/ForegroundServiceRule.kt @@ -0,0 +1,48 @@ +package org.signal.fastlint.rules + +import com.intellij.psi.PsiMethodCallExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtDotQualifiedExpression +import org.jetbrains.kotlin.psi.KtNameReferenceExpression +import org.jetbrains.kotlin.psi.psiUtil.getQualifiedExpressionForSelector +import org.signal.fastlint.JavaCallRule +import org.signal.fastlint.JavaFileContext +import org.signal.fastlint.KotlinFileContext +import org.signal.fastlint.KtCallRule +import org.signal.fastlint.Reporter + +/** Flags Context/ContextCompat.startForegroundService outside ForegroundServiceUtil. */ +object ForegroundServiceRule : KtCallRule, JavaCallRule { + + private const val MESSAGE = "Using startForegroundService instead of ForegroundServiceUtil" + + override fun onCall(call: KtCallExpression, context: KotlinFileContext, reporter: Reporter) { + val callee = (call.calleeExpression as? KtNameReferenceExpression)?.getReferencedName() ?: return + if (callee != "startForegroundService" || context.file.name == "ForegroundServiceUtil.kt") { + return + } + val receiver = (call.getQualifiedExpressionForSelector() as? KtDotQualifiedExpression)?.receiverExpression?.text + if (isLikelyContextReceiver(receiver)) { + reporter.report("StartForegroundServiceUsage", call, context.lineOf(call.textOffset), MESSAGE) + } + } + + override fun onCall(call: PsiMethodCallExpression, context: JavaFileContext, reporter: Reporter) { + val callee = call.methodExpression.referenceName ?: return + if (callee != "startForegroundService" || context.file.name == "ForegroundServiceUtil.java") { + return + } + if (isLikelyContextReceiver(call.methodExpression.qualifierExpression?.text)) { + reporter.report("StartForegroundServiceUsage", call, context.lineOf(call.textOffset), MESSAGE) + } + } + + /** + * The rule only targets the framework call (Context / ContextCompat). Without type resolution we + * approximate: a null/lowercase receiver is a context instance, "ContextCompat" is the helper; an + * upper-case qualifier is a class (a Signal wrapper like FcmFetchManager), so skip it. + */ + private fun isLikelyContextReceiver(receiver: String?): Boolean { + return receiver == null || receiver == "ContextCompat" || receiver.firstOrNull()?.isLowerCase() == true + } +} diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/rules/LogNotSignalRule.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/LogNotSignalRule.kt new file mode 100644 index 0000000000..200fa08af6 --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/LogNotSignalRule.kt @@ -0,0 +1,69 @@ +package org.signal.fastlint.rules + +import com.intellij.psi.PsiMethodCallExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtDotQualifiedExpression +import org.jetbrains.kotlin.psi.KtNameReferenceExpression +import org.jetbrains.kotlin.psi.psiUtil.getQualifiedExpressionForSelector +import org.signal.fastlint.JavaCallRule +import org.signal.fastlint.JavaFileContext +import org.signal.fastlint.KotlinFileContext +import org.signal.fastlint.KtCallRule +import org.signal.fastlint.Reporter + +/** + * Flags logging methods (v/d/i/w/e/wtf) called on any receiver other than the app's logger + * (org.signal.core.util.logging.Log). The single-character method names keep this targeted at + * logger-style calls. + * + * Allowed besides Log itself: chains off Log such as Log.internal(), the sensitive-data logger + * SensitiveLog, and any call within the logging package (which implements the logger). + */ +object LogNotSignalRule : KtCallRule, JavaCallRule { + + private val LOG_METHODS = setOf("v", "d", "i", "w", "e", "wtf") + private const val SIGNAL_LOG = "org.signal.core.util.logging.Log" + private const val SENSITIVE_LOG = "org.signal.registration.util.SensitiveLog" + private const val LOGGING_PACKAGE = "org.signal.core.util.logging" + + // Logger implementations delegate to a wrapped logger, so we do not flag them against themselves. + private val LOGGER_IMPLEMENTATION_FILES = setOf("SensitiveLog.kt", "SensitiveLog.java") + + override fun onCall(call: KtCallExpression, context: KotlinFileContext, reporter: Reporter) { + val callee = (call.calleeExpression as? KtNameReferenceExpression)?.getReferencedName() ?: return + if (callee !in LOG_METHODS) { + return + } + if (isLoggerImplementation(context.file.name, context.ktFile.packageFqName.asString())) { + return + } + val receiver = (call.getQualifiedExpressionForSelector() as? KtDotQualifiedExpression)?.receiverExpression?.text ?: return + val fqn = context.resolveFqn(receiver) + if (!isAllowedLogger(fqn)) { + reporter.report("LogNotSignal", call, context.lineOf(call.textOffset), "Using '$fqn' instead of a Signal Logger") + } + } + + override fun onCall(call: PsiMethodCallExpression, context: JavaFileContext, reporter: Reporter) { + val callee = call.methodExpression.referenceName ?: return + if (callee !in LOG_METHODS) { + return + } + if (isLoggerImplementation(context.file.name, context.javaFile.packageName)) { + return + } + val receiver = call.methodExpression.qualifierExpression?.text ?: return + val fqn = context.resolveFqn(receiver) + if (!isAllowedLogger(fqn)) { + reporter.report("LogNotSignal", call, context.lineOf(call.textOffset), "Using '$fqn' instead of a Signal Logger") + } + } + + private fun isAllowedLogger(fqn: String): Boolean { + return fqn == SIGNAL_LOG || fqn.startsWith("$SIGNAL_LOG.") || fqn == SENSITIVE_LOG + } + + private fun isLoggerImplementation(fileName: String, packageName: String): Boolean { + return packageName == LOGGING_PACKAGE || fileName in LOGGER_IMPLEMENTATION_FILES + } +} diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/rules/LogTagInlinedRule.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/LogTagInlinedRule.kt new file mode 100644 index 0000000000..a384834fb0 --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/LogTagInlinedRule.kt @@ -0,0 +1,53 @@ +package org.signal.fastlint.rules + +import com.intellij.psi.PsiMethodCallExpression +import com.intellij.psi.PsiReferenceExpression +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtDotQualifiedExpression +import org.jetbrains.kotlin.psi.KtNameReferenceExpression +import org.jetbrains.kotlin.psi.psiUtil.getQualifiedExpressionForSelector +import org.signal.fastlint.JavaCallRule +import org.signal.fastlint.JavaFileContext +import org.signal.fastlint.KotlinFileContext +import org.signal.fastlint.KtCallRule +import org.signal.fastlint.Reporter + +/** + * Flags calls to the app logger (org.signal.core.util.logging.Log) that pass an inline string tag + * instead of a tag constant. + */ +object LogTagInlinedRule : KtCallRule, JavaCallRule { + + private val LOG_METHODS = setOf("v", "d", "i", "w", "e", "wtf") + private const val SIGNAL_LOG = "org.signal.core.util.logging.Log" + + override fun onCall(call: KtCallExpression, context: KotlinFileContext, reporter: Reporter) { + val callee = (call.calleeExpression as? KtNameReferenceExpression)?.getReferencedName() ?: return + if (callee !in LOG_METHODS) { + return + } + val receiver = (call.getQualifiedExpressionForSelector() as? KtDotQualifiedExpression)?.receiverExpression?.text ?: return + if (context.resolveFqn(receiver) != SIGNAL_LOG) { + return + } + val firstArg = call.valueArguments.firstOrNull()?.getArgumentExpression() + if (firstArg != null && firstArg !is KtNameReferenceExpression && firstArg !is KtDotQualifiedExpression) { + reporter.report("LogTagInlined", call, context.lineOf(call.textOffset), "Not using a tag constant") + } + } + + override fun onCall(call: PsiMethodCallExpression, context: JavaFileContext, reporter: Reporter) { + val callee = call.methodExpression.referenceName ?: return + if (callee !in LOG_METHODS) { + return + } + val receiver = call.methodExpression.qualifierExpression?.text ?: return + if (context.resolveFqn(receiver) != SIGNAL_LOG) { + return + } + val firstArg = call.argumentList.expressions.firstOrNull() + if (firstArg != null && firstArg !is PsiReferenceExpression) { + reporter.report("LogTagInlined", call, context.lineOf(call.textOffset), "Not using a tag constant") + } + } +} diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/rules/StringResourceEscapingRule.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/StringResourceEscapingRule.kt new file mode 100644 index 0000000000..6bcfddba14 --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/StringResourceEscapingRule.kt @@ -0,0 +1,76 @@ +package org.signal.fastlint.rules + +import org.signal.fastlint.XmlFileContext +import org.signal.fastlint.XmlSink +import org.signal.fastlint.XmlStringResourceRule + +/** + * Flags string resource values that aapt rejects: + * - an unescaped apostrophe outside a double-quoted span, or an unbalanced (odd) unescaped double + * quote — a backslash escapes the next character, an unescaped double quote toggles a "quoting" + * span (in which apostrophes are allowed), and an apostrophe outside such a span must be \'; + * - a value starting with '@' or '?' that is not a resource / theme-attribute reference, which aapt + * interprets as a (broken) reference unless escaped as \@ / \?. + */ +object StringResourceEscapingRule : XmlStringResourceRule { + + // A resource reference: @[+][*][package:]type/name, e.g. @string/foo, @+id/foo, @android:id/foo. + private val RESOURCE_REFERENCE = Regex("^@\\+?\\*?(\\w+:)?\\w+/.+") + + override fun onStringResource(name: String?, value: String, line: Int, context: XmlFileContext, sink: XmlSink) { + checkQuotes(value, line, sink) + checkLeadingReferenceCharacter(value, line, sink) + } + + private fun checkQuotes(value: String, line: Int, sink: XmlSink) { + var inQuotedSpan = false + var unescapedDoubleQuotes = 0 + var unescapedApostropheOutsideQuotes = false + + var i = 0 + while (i < value.length) { + when (value[i]) { + '\\' -> i++ // The next character is escaped; skip it. + '"' -> { + unescapedDoubleQuotes++ + inQuotedSpan = !inQuotedSpan + } + '\'' -> if (!inQuotedSpan) { + unescapedApostropheOutsideQuotes = true + } + } + i++ + } + + if (unescapedApostropheOutsideQuotes) { + sink.report("StringResourceEscaping", line, "Apostrophe in a string resource must be escaped as \\' or the value wrapped in double quotes") + } + if (unescapedDoubleQuotes % 2 != 0) { + sink.report("StringResourceEscaping", line, "Unbalanced double quote in a string resource; escape a literal quote as \\\"") + } + } + + private fun checkLeadingReferenceCharacter(value: String, line: Int, sink: XmlSink) { + val trimmed = value.trimStart() + if (trimmed.isEmpty()) { + return + } + when (trimmed[0]) { + '@' -> if (!isResourceReference(trimmed)) { + sink.report("StringResourceEscaping", line, "A string resource starting with '@' must be escaped as \\@ unless it is a resource reference") + } + '?' -> if (!isThemeReference(trimmed)) { + sink.report("StringResourceEscaping", line, "A string resource starting with '?' must be escaped as \\? unless it is a theme attribute reference") + } + } + } + + private fun isResourceReference(value: String): Boolean { + return value == "@null" || value == "@empty" || RESOURCE_REFERENCE.containsMatchIn(value) + } + + private fun isThemeReference(value: String): Boolean { + // ?attr/x, ?android:attr/x, or the ?colorPrimary shorthand: '?' followed by an identifier. + return value.length > 1 && (value[1].isLetter() || value[1] == '_') + } +} diff --git a/fast-lint/src/main/kotlin/org/signal/fastlint/rules/VersionCodeRule.kt b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/VersionCodeRule.kt new file mode 100644 index 0000000000..4847d3240d --- /dev/null +++ b/fast-lint/src/main/kotlin/org/signal/fastlint/rules/VersionCodeRule.kt @@ -0,0 +1,27 @@ +package org.signal.fastlint.rules + +import com.intellij.psi.PsiReferenceExpression +import org.jetbrains.kotlin.psi.KtSimpleNameExpression +import org.signal.fastlint.JavaFileContext +import org.signal.fastlint.JavaReferenceRule +import org.signal.fastlint.KotlinFileContext +import org.signal.fastlint.KtNameRule +import org.signal.fastlint.Reporter + +/** Flags references to Build.VERSION_CODES.* constants; Signal convention is the numeric API level. */ +object VersionCodeRule : KtNameRule, JavaReferenceRule { + + private const val MESSAGE = "Using 'VERSION_CODES' reference instead of the numeric value" + + override fun onName(name: KtSimpleNameExpression, context: KotlinFileContext, reporter: Reporter) { + if (name.getReferencedName() == "VERSION_CODES") { + reporter.report("VersionCodeUsage", name, context.lineOf(name.textOffset), MESSAGE) + } + } + + override fun onReference(reference: PsiReferenceExpression, context: JavaFileContext, reporter: Reporter) { + if (reference.referenceName == "VERSION_CODES") { + reporter.report("VersionCodeUsage", reference, context.lineOf(reference.textOffset), MESSAGE) + } + } +} diff --git a/fast-lint/src/test/kotlin/org/signal/fastlint/TestSupport.kt b/fast-lint/src/test/kotlin/org/signal/fastlint/TestSupport.kt new file mode 100644 index 0000000000..e8e80b9b37 --- /dev/null +++ b/fast-lint/src/test/kotlin/org/signal/fastlint/TestSupport.kt @@ -0,0 +1,25 @@ +package org.signal.fastlint + +import org.signal.fastlint.rules.ALL_RULES +import java.io.File + +/** + * Shared engine for rule tests. Created once per test JVM (PSI environment setup is expensive) and + * reused; tests run sequentially within a JVM so the single instance is safe. + */ +private val ENGINE = FastLint(ALL_RULES) + +fun lintKotlin(code: String, fileName: String = "Test.kt"): List = + ENGINE.analyzeKotlin(File("/repo/app/src/main/java/org/test/$fileName"), code) + +fun lintJava(code: String, fileName: String = "Test.java"): List = + ENGINE.analyzeJava(File("/repo/app/src/main/java/org/test/$fileName"), code) + +fun lintLayout(xml: String): List = + ENGINE.analyzeXml(File("/repo/app/src/main/res/layout/test.xml"), xml) + +fun lintValues(xml: String): List = + ENGINE.analyzeXml(File("/repo/app/src/main/res/values/strings.xml"), xml) + +/** Sorted list of check ids in the findings, for concise assertions. */ +fun List.ids(): List = map { it.checkId }.sorted() diff --git a/fast-lint/src/test/kotlin/org/signal/fastlint/rules/AlertDialogRuleTest.kt b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/AlertDialogRuleTest.kt new file mode 100644 index 0000000000..1b0f8e2f0b --- /dev/null +++ b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/AlertDialogRuleTest.kt @@ -0,0 +1,47 @@ +package org.signal.fastlint.rules + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.signal.fastlint.ids +import org.signal.fastlint.lintJava +import org.signal.fastlint.lintKotlin + +class AlertDialogRuleTest { + + @Test + fun `kotlin appcompat AlertDialog Builder is flagged`() { + val findings = lintKotlin( + """ + import android.content.Context + import androidx.appcompat.app.AlertDialog + class A { fun f(c: Context) { AlertDialog.Builder(c) } } + """.trimIndent() + ) + assertEquals(listOf("AlertDialogBuilderUsage"), findings.ids()) + } + + @Test + fun `java appcompat AlertDialog Builder is flagged`() { + val findings = lintJava( + """ + import android.content.Context; + import androidx.appcompat.app.AlertDialog; + class A { void f(Context c) { new AlertDialog.Builder(c); } } + """.trimIndent() + ) + assertEquals(listOf("AlertDialogBuilderUsage"), findings.ids()) + } + + @Test + fun `MaterialAlertDialogBuilder is not flagged`() { + val findings = lintKotlin( + """ + import android.content.Context + import com.google.android.material.dialog.MaterialAlertDialogBuilder + class A { fun f(c: Context) { MaterialAlertDialogBuilder(c) } } + """.trimIndent() + ) + assertTrue(findings.isEmpty()) + } +} diff --git a/fast-lint/src/test/kotlin/org/signal/fastlint/rules/DatabaseReferenceRuleTest.kt b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/DatabaseReferenceRuleTest.kt new file mode 100644 index 0000000000..bcc72a1854 --- /dev/null +++ b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/DatabaseReferenceRuleTest.kt @@ -0,0 +1,71 @@ +package org.signal.fastlint.rules + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.signal.fastlint.ids +import org.signal.fastlint.lintJava +import org.signal.fastlint.lintKotlin + +class DatabaseReferenceRuleTest { + + @Test + fun `java database with recipient column is flagged`() { + val findings = lintJava( + """ + class MyTable extends Database { + private static final String RECIPIENT_ID = "recipient_id"; + } + """.trimIndent() + ) + assertEquals(listOf("RecipientIdDatabaseReferenceUsage"), findings.ids()) + } + + @Test + fun `java database with thread column is flagged`() { + val findings = lintJava( + """ + class MyTable extends Database { + private static final String THREAD_ID = "thread_id"; + } + """.trimIndent() + ) + assertEquals(listOf("ThreadIdDatabaseReferenceUsage"), findings.ids()) + } + + @Test + fun `database implementing the interface is not flagged`() { + val findings = lintJava( + """ + class MyTable extends Database implements RecipientIdDatabaseReference { + private static final String RECIPIENT_ID = "recipient_id"; + } + """.trimIndent() + ) + assertTrue(findings.isEmpty()) + } + + @Test + fun `non-database class is not flagged`() { + val findings = lintJava( + """ + class Foo { + private static final String RECIPIENT_ID = "recipient_id"; + } + """.trimIndent() + ) + assertTrue(findings.isEmpty()) + } + + @Test + fun `kotlin database with recipient column is flagged`() { + val findings = lintKotlin( + """ + class MyTable : Database() { + val recipientId: String = "" + } + """.trimIndent() + ) + assertEquals(listOf("RecipientIdDatabaseReferenceUsage"), findings.ids()) + } +} diff --git a/fast-lint/src/test/kotlin/org/signal/fastlint/rules/ForegroundServiceRuleTest.kt b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/ForegroundServiceRuleTest.kt new file mode 100644 index 0000000000..56b9116555 --- /dev/null +++ b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/ForegroundServiceRuleTest.kt @@ -0,0 +1,51 @@ +package org.signal.fastlint.rules + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.signal.fastlint.ids +import org.signal.fastlint.lintJava +import org.signal.fastlint.lintKotlin + +class ForegroundServiceRuleTest { + + @Test + fun `context instance call is flagged`() { + val findings = lintKotlin( + """ + import android.content.Context + class A { fun f(context: Context) { context.startForegroundService(null) } } + """.trimIndent() + ) + assertEquals(listOf("StartForegroundServiceUsage"), findings.ids()) + } + + @Test + fun `java ContextCompat call is flagged`() { + val findings = lintJava( + """ + import androidx.core.content.ContextCompat; + class A { void f(android.content.Context c, android.content.Intent i) { ContextCompat.startForegroundService(c, i); } } + """.trimIndent() + ) + assertEquals(listOf("StartForegroundServiceUsage"), findings.ids()) + } + + @Test + fun `static wrapper call on a class is not flagged`() { + val findings = lintKotlin("""class A { fun f() { FcmFetchManager.startForegroundService(null) } }""") + assertTrue(findings.isEmpty()) + } + + @Test + fun `call inside ForegroundServiceUtil is not flagged`() { + val findings = lintKotlin( + """ + import android.content.Context + class A { fun f(context: Context) { context.startForegroundService(null) } } + """.trimIndent(), + fileName = "ForegroundServiceUtil.kt" + ) + assertTrue(findings.isEmpty()) + } +} diff --git a/fast-lint/src/test/kotlin/org/signal/fastlint/rules/LogNotSignalRuleTest.kt b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/LogNotSignalRuleTest.kt new file mode 100644 index 0000000000..6ecdfb5283 --- /dev/null +++ b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/LogNotSignalRuleTest.kt @@ -0,0 +1,138 @@ +package org.signal.fastlint.rules + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.signal.fastlint.ids +import org.signal.fastlint.lintJava +import org.signal.fastlint.lintKotlin + +class LogNotSignalRuleTest { + + @Test + fun `kotlin android Log is flagged`() { + val findings = lintKotlin( + """ + import android.util.Log + class A { fun f() { Log.d("tag", "m") } } + """.trimIndent() + ) + assertEquals(listOf("LogNotSignal"), findings.ids()) + } + + @Test + fun `java android Log is flagged`() { + val findings = lintJava( + """ + import android.util.Log; + class A { void f() { Log.d("tag", "m"); } } + """.trimIndent() + ) + assertEquals(listOf("LogNotSignal"), findings.ids()) + } + + @Test + fun `libsignal server logger is flagged`() { + val findings = lintKotlin( + """ + import org.signal.libsignal.protocol.logging.Log + class A { fun f() { Log.w("tag", "m") } } + """.trimIndent() + ) + assertEquals(listOf("LogNotSignal"), findings.ids()) + } + + @Test + fun `an unrecognized third-party logger is also flagged`() { + val findings = lintKotlin( + """ + import timber.log.Timber + class A { fun f() { Timber.e("m") } } + """.trimIndent() + ) + assertEquals(listOf("LogNotSignal"), findings.ids()) + } + + @Test + fun `app logger is not flagged`() { + val findings = lintKotlin( + """ + import org.signal.core.util.logging.Log + class A { + val tag = "A" + fun f() { Log.d(tag, "m") } + } + """.trimIndent() + ) + assertTrue(findings.isEmpty()) + } + + @Test + fun `a chain off the app logger such as Log internal is not flagged`() { + val findings = lintKotlin( + """ + import org.signal.core.util.logging.Log + class A { + val tag = "A" + fun f() { Log.internal().d(tag, "m") } + } + """.trimIndent() + ) + assertFalse("LogNotSignal" in findings.ids()) + } + + @Test + fun `SensitiveLog is not flagged`() { + val findings = lintKotlin( + """ + import org.signal.registration.util.SensitiveLog + class A { + val tag = "A" + fun f() { SensitiveLog.d(tag, "m") } + } + """.trimIndent() + ) + assertFalse("LogNotSignal" in findings.ids()) + } + + @Test + fun `calls within the logging package are not flagged`() { + val findings = lintKotlin( + """ + package org.signal.core.util.logging + class CompoundLogger { + fun f(logger: Any) { logger.d("t", "m") } + } + """.trimIndent() + ) + assertTrue(findings.isEmpty()) + } + + @Test + fun `SensitiveLog's own delegating implementation is not flagged`() { + val findings = lintKotlin( + """ + package org.signal.registration.util + class SensitiveLog(private val logger: Any) { + fun d(tag: String, message: String) { this.logger.d(tag, message) } + } + """.trimIndent(), + fileName = "SensitiveLog.kt" + ) + assertTrue(findings.isEmpty()) + } + + @Test + fun `SuppressLint silences the rule`() { + val findings = lintKotlin( + """ + import android.annotation.SuppressLint + import android.util.Log + @SuppressLint("LogNotSignal") + class A { fun f() { Log.d("tag", "m") } } + """.trimIndent() + ) + assertTrue(findings.isEmpty()) + } +} diff --git a/fast-lint/src/test/kotlin/org/signal/fastlint/rules/LogTagInlinedRuleTest.kt b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/LogTagInlinedRuleTest.kt new file mode 100644 index 0000000000..9c8e1de38f --- /dev/null +++ b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/LogTagInlinedRuleTest.kt @@ -0,0 +1,71 @@ +package org.signal.fastlint.rules + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.signal.fastlint.ids +import org.signal.fastlint.lintJava +import org.signal.fastlint.lintKotlin + +class LogTagInlinedRuleTest { + + @Test + fun `app logger with inline string tag is flagged`() { + val findings = lintKotlin( + """ + import org.signal.core.util.logging.Log + class A { fun f() { Log.d("literal", "m") } } + """.trimIndent() + ) + assertEquals(listOf("LogTagInlined"), findings.ids()) + } + + @Test + fun `java app logger with inline string tag is flagged`() { + val findings = lintJava( + """ + import org.signal.core.util.logging.Log; + class A { void f() { Log.d("literal", "m"); } } + """.trimIndent() + ) + assertEquals(listOf("LogTagInlined"), findings.ids()) + } + + @Test + fun `app logger with constant tag is not flagged`() { + val findings = lintKotlin( + """ + import org.signal.core.util.logging.Log + class A { + val tag = "A" + fun f() { Log.d(tag, "m") } + } + """.trimIndent() + ) + assertTrue(findings.isEmpty()) + } + + @Test + fun `non-signal logger with inline tag is not flagged as LogTagInlined`() { + val findings = lintKotlin( + """ + import android.util.Log + class A { fun f() { Log.d("literal", "m") } } + """.trimIndent() + ) + assertEquals(listOf("LogNotSignal"), findings.ids()) + } + + @Test + fun `SuppressLint silences the rule`() { + val findings = lintKotlin( + """ + import android.annotation.SuppressLint + import org.signal.core.util.logging.Log + @SuppressLint("LogTagInlined") + class A { fun f() { Log.d("literal", "m") } } + """.trimIndent() + ) + assertTrue(findings.isEmpty()) + } +} diff --git a/fast-lint/src/test/kotlin/org/signal/fastlint/rules/StringResourceEscapingRuleTest.kt b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/StringResourceEscapingRuleTest.kt new file mode 100644 index 0000000000..411828e323 --- /dev/null +++ b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/StringResourceEscapingRuleTest.kt @@ -0,0 +1,90 @@ +package org.signal.fastlint.rules + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.signal.fastlint.ids +import org.signal.fastlint.lintValues + +class StringResourceEscapingRuleTest { + + @Test + fun `unescaped apostrophe is flagged`() { + val findings = lintValues("""Don't""") + assertEquals(listOf("StringResourceEscaping"), findings.ids()) + } + + @Test + fun `escaped apostrophe is not flagged`() { + val findings = lintValues("""Don\'t""") + assertTrue(findings.isEmpty()) + } + + @Test + fun `apostrophe inside a double-quoted span is not flagged`() { + val findings = lintValues(""""Don't"""") + assertTrue(findings.isEmpty()) + } + + @Test + fun `unbalanced double quote is flagged`() { + val findings = lintValues("""5" tall""") + assertEquals(listOf("StringResourceEscaping"), findings.ids()) + } + + @Test + fun `escaped double quote is not flagged`() { + val findings = lintValues("""5\" tall""") + assertTrue(findings.isEmpty()) + } + + @Test + fun `balanced double quotes are not flagged`() { + val findings = lintValues(""""hello world"""") + assertTrue(findings.isEmpty()) + } + + @Test + fun `value starting with a literal at-sign is flagged`() { + val findings = lintValues("""@everyone""") + assertEquals(listOf("StringResourceEscaping"), findings.ids()) + } + + @Test + fun `escaped leading at-sign is not flagged`() { + val findings = lintValues("""\@everyone""") + assertTrue(findings.isEmpty()) + } + + @Test + fun `a resource reference value is not flagged`() { + val findings = lintValues("""@string/foo""") + assertTrue(findings.isEmpty()) + } + + @Test + fun `at-null is not flagged`() { + val findings = lintValues("""@null""") + assertTrue(findings.isEmpty()) + } + + @Test + fun `value starting with a literal question mark is flagged`() { + val findings = lintValues("""???""") + assertEquals(listOf("StringResourceEscaping"), findings.ids()) + } + + @Test + fun `a theme attribute reference is not flagged`() { + val findings = lintValues("""?attr/colorPrimary""") + assertTrue(findings.isEmpty()) + } + + @Test + fun `tools ignore suppresses the rule`() { + val findings = lintValues( + """Don't""" + ) + assertTrue(findings.isEmpty()) + } +} diff --git a/fast-lint/src/test/kotlin/org/signal/fastlint/rules/VersionCodeRuleTest.kt b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/VersionCodeRuleTest.kt new file mode 100644 index 0000000000..fa5574e8be --- /dev/null +++ b/fast-lint/src/test/kotlin/org/signal/fastlint/rules/VersionCodeRuleTest.kt @@ -0,0 +1,39 @@ +package org.signal.fastlint.rules + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.signal.fastlint.ids +import org.signal.fastlint.lintJava +import org.signal.fastlint.lintKotlin + +class VersionCodeRuleTest { + + @Test + fun `kotlin VERSION_CODES reference is flagged`() { + val findings = lintKotlin( + """ + import android.os.Build + class A { fun f() = Build.VERSION.SDK_INT >= Build.VERSION_CODES.O } + """.trimIndent() + ) + assertEquals(listOf("VersionCodeUsage"), findings.ids()) + } + + @Test + fun `java VERSION_CODES reference is flagged`() { + val findings = lintJava( + """ + import android.os.Build; + class A { boolean f() { return Build.VERSION.SDK_INT >= Build.VERSION_CODES.O; } } + """.trimIndent() + ) + assertEquals(listOf("VersionCodeUsage"), findings.ids()) + } + + @Test + fun `VERSION_CODES inside a string literal is not flagged`() { + val findings = lintKotlin("""class A { val s = "see VERSION_CODES.O for details" }""") + assertTrue(findings.isEmpty()) + } +} diff --git a/gradle/lint-libs.versions.toml b/gradle/lint-libs.versions.toml index 653d0f6c32..92c827640a 100644 --- a/gradle/lint-libs.versions.toml +++ b/gradle/lint-libs.versions.toml @@ -8,3 +8,8 @@ lint = "32.1.1" lint-api = { module = "com.android.tools.lint:lint-api", version.ref = "lint" } lint-checks = { module = "com.android.tools.lint:lint-checks", version.ref = "lint" } lint-tests = { module = "com.android.tools.lint:lint-tests", version.ref = "lint" } + +# Parser front-ends used by the fast custom linter (:fast-lint). Same release as lint, so they +# share the "lint" version and are already covered by dependency verification. +intellij-core = { module = "com.android.tools.external.com-intellij:intellij-core", version.ref = "lint" } +kotlin-compiler = { module = "com.android.tools.external.com-intellij:kotlin-compiler", version.ref = "lint" } diff --git a/settings.gradle.kts b/settings.gradle.kts index 925028ed24..a20b051e07 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -145,6 +145,7 @@ include(":demo:apng") // Testing/Lint modules include(":lintchecks") +include(":fast-lint") include(":benchmark") include(":baseline-profile") include(":microbenchmark")