diff --git a/CHANGELOG.md b/CHANGELOG.md index b8235a58..a6bcfa8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,11 @@ Changelog ========= +**Unreleased** +-------------- + +- Open-source `AvoidUsingNotNullOperator`, `InflationInItemDecoration`, and `DoNotCallViewToString` checks. + 0.8.0 ----- diff --git a/slack-lint-checks/src/main/java/slack/lint/NotNullOperatorDetector.kt b/slack-lint-checks/src/main/java/slack/lint/NotNullOperatorDetector.kt new file mode 100644 index 00000000..04cedeb0 --- /dev/null +++ b/slack-lint-checks/src/main/java/slack/lint/NotNullOperatorDetector.kt @@ -0,0 +1,54 @@ +// Copyright (C) 2024 Slack Technologies, LLC +// SPDX-License-Identifier: Apache-2.0 +package slack.lint + +import com.android.tools.lint.client.api.UElementHandler +import com.android.tools.lint.detector.api.Category +import com.android.tools.lint.detector.api.Detector +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 kotlin.jvm.java +import org.jetbrains.uast.UPostfixExpression +import org.jetbrains.uast.kotlin.isKotlin +import slack.lint.util.sourceImplementation + +class NotNullOperatorDetector : Detector(), SourceCodeScanner { + override fun getApplicableUastTypes() = listOf(UPostfixExpression::class.java) + + override fun createUastHandler(context: JavaContext): UElementHandler? { + if (!isKotlin(context.uastFile?.lang)) return null + + return object : UElementHandler() { + override fun visitPostfixExpression(node: UPostfixExpression) { + if (node.operator.text == "!!") { + // Report a warning for the usage of `!!` + context.report( + ISSUE, + node, + context.getLocation(node.operatorIdentifier ?: node), + "Avoid using the `!!` operator", + ) + } + } + } + } + + companion object { + val ISSUE: Issue = + Issue.create( + "AvoidUsingNotNullOperator", + "Avoid using the !! operator in Kotlin", + """ + The `!!` operator is a not-null assertion in Kotlin that will lead to a \ + `NullPointerException` if the value is null. It's better to use safe \ + null-handling mechanisms like `?.`, `?:`, `?.let`, etc. + """, + Category.CORRECTNESS, + 6, + Severity.WARNING, + sourceImplementation(), + ) + } +} diff --git a/slack-lint-checks/src/main/java/slack/lint/SlackIssueRegistry.kt b/slack-lint-checks/src/main/java/slack/lint/SlackIssueRegistry.kt index 372f6291..00ffa51e 100644 --- a/slack-lint-checks/src/main/java/slack/lint/SlackIssueRegistry.kt +++ b/slack-lint-checks/src/main/java/slack/lint/SlackIssueRegistry.kt @@ -19,6 +19,8 @@ import slack.lint.resources.WrongResourceImportAliasDetector import slack.lint.retrofit.RetrofitUsageDetector import slack.lint.rx.RxSubscribeOnMainDetector import slack.lint.text.SpanMarkPointMissingMaskDetector +import slack.lint.ui.DoNotCallViewToString +import slack.lint.ui.ItemDecorationViewBindingDetector @AutoService(IssueRegistry::class) class SlackIssueRegistry : IssueRegistry() { @@ -66,5 +68,8 @@ class SlackIssueRegistry : IssueRegistry() { add(ParcelizeFunctionPropertyDetector.ISSUE) add(ExceptionMessageDetector.ISSUE) add(MustUseNamedParamsDetector.ISSUE) + add(NotNullOperatorDetector.ISSUE) + add(DoNotCallViewToString.ISSUE) + add(ItemDecorationViewBindingDetector.ISSUE) } } diff --git a/slack-lint-checks/src/main/java/slack/lint/ui/DoNotCallViewToString.kt b/slack-lint-checks/src/main/java/slack/lint/ui/DoNotCallViewToString.kt new file mode 100644 index 00000000..206a5547 --- /dev/null +++ b/slack-lint-checks/src/main/java/slack/lint/ui/DoNotCallViewToString.kt @@ -0,0 +1,52 @@ +// Copyright (C) 2024 Slack Technologies, LLC +// SPDX-License-Identifier: Apache-2.0 +package slack.lint.ui + +import com.android.tools.lint.client.api.UElementHandler +import com.android.tools.lint.detector.api.Category +import com.android.tools.lint.detector.api.Detector +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.intellij.psi.util.InheritanceUtil +import org.jetbrains.uast.UCallExpression +import slack.lint.util.sourceImplementation + +class DoNotCallViewToString : Detector(), SourceCodeScanner { + override fun getApplicableUastTypes() = listOf(UCallExpression::class.java) + + override fun createUastHandler(context: JavaContext): UElementHandler { + return object : UElementHandler() { + override fun visitCallExpression(node: UCallExpression) { + if (node.methodName != "toString") return + val method = node.resolve() ?: return + val containingClass = method.containingClass ?: return + if (InheritanceUtil.isInheritor(containingClass, "android.view.View")) { + context.report( + ISSUE, + node, + context.getNameLocation(node), + "Do not call `View.toString()`", + ) + } + } + } + } + + companion object { + val ISSUE: Issue = + Issue.Companion.create( + "DoNotCallViewToString", + "Do not use `View.toString()`", + """ + `View.toString()` and its overrides can often print surprisingly detailed information about \ + the current view state, and has led to PII logging issues in the past. + """, + Category.Companion.SECURITY, + 9, + Severity.ERROR, + sourceImplementation(), + ) + } +} diff --git a/slack-lint-checks/src/main/java/slack/lint/ui/ItemDecorationViewBindingDetector.kt b/slack-lint-checks/src/main/java/slack/lint/ui/ItemDecorationViewBindingDetector.kt new file mode 100644 index 00000000..df2026cc --- /dev/null +++ b/slack-lint-checks/src/main/java/slack/lint/ui/ItemDecorationViewBindingDetector.kt @@ -0,0 +1,77 @@ +// Copyright (C) 2024 Slack Technologies, LLC +// SPDX-License-Identifier: Apache-2.0 +package slack.lint.ui + +import com.android.tools.lint.client.api.UElementHandler +import com.android.tools.lint.detector.api.Category +import com.android.tools.lint.detector.api.Detector +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.intellij.psi.PsiClass +import com.intellij.psi.util.InheritanceUtil.isInheritor +import kotlin.jvm.java +import org.jetbrains.uast.UCallExpression +import org.jetbrains.uast.getContainingUClass +import slack.lint.util.implements +import slack.lint.util.sourceImplementation + +/** + * Lint detector that ensures `ItemDecoration` does not inflate a view. If the view contains + * `TextView`, this textual information cannot be announced to screen readers by TalkBack. + */ +class ItemDecorationViewBindingDetector : Detector(), SourceCodeScanner { + override fun getApplicableUastTypes() = listOf(UCallExpression::class.java) + + override fun createUastHandler(context: JavaContext): UElementHandler = + object : UElementHandler() { + + override fun visitCallExpression(node: UCallExpression) { + val containingClass = node.getContainingUClass() ?: return + if (!containingClass.implements(ITEM_DECORATION_CLASS_NAME)) return + + val method = node.resolve() ?: return + val methodClass = method.containingClass + + if ( + LAYOUT_INFLATER_METHOD_NAME == method.name && methodClass?.isViewBindingClass() == true + ) { + context.report(ISSUE, node, context.getNameLocation(node), ISSUE_DESCRIPTION) + } + } + } + + private fun PsiClass.isViewBindingClass(): Boolean { + return LAYOUT_INFLATER_PACKAGE_NAME == this.qualifiedName || + isInheritor(this, VIEW_BINDING_PACKAGE_NAME) + } + + companion object { + private const val ISSUE_ID = "InflationInItemDecoration" + private const val ITEM_DECORATION_CLASS_NAME = + "androidx.recyclerview.widget.RecyclerView.ItemDecoration" + private const val LAYOUT_INFLATER_METHOD_NAME = "inflate" + private const val LAYOUT_INFLATER_PACKAGE_NAME = "android.view.LayoutInflater" + private const val VIEW_BINDING_PACKAGE_NAME = "androidx.viewbinding.ViewBinding" + + private const val ISSUE_BRIEF_DESCRIPTION = "Avoid inflating a view to display text" + private const val ISSUE_DESCRIPTION = + """ + ViewBinding should not be used in `ItemDecoration`. If an inflated view contains \ + meaningful textual information, it will not be visible to TalkBack. This means \ + that screen reader users will not be able to know what is on the screen. + """ + + val ISSUE = + Issue.create( + id = ISSUE_ID, + briefDescription = ISSUE_BRIEF_DESCRIPTION, + explanation = ISSUE_DESCRIPTION, + category = Category.A11Y, + priority = 10, + severity = Severity.WARNING, + implementation = sourceImplementation(), + ) + } +} diff --git a/slack-lint-checks/src/test/java/slack/lint/NotNullOperatorDetectorTest.kt b/slack-lint-checks/src/test/java/slack/lint/NotNullOperatorDetectorTest.kt new file mode 100644 index 00000000..c308c32d --- /dev/null +++ b/slack-lint-checks/src/test/java/slack/lint/NotNullOperatorDetectorTest.kt @@ -0,0 +1,66 @@ +// Copyright (C) 2024 Slack Technologies, LLC +// SPDX-License-Identifier: Apache-2.0 +package slack.lint + +import kotlin.text.trimIndent +import org.junit.Test + +class NotNullOperatorDetectorTest : BaseSlackLintTest() { + override fun getDetector() = NotNullOperatorDetector() + + override fun getIssues() = listOf(NotNullOperatorDetector.ISSUE) + + @Test + fun testDocumentationExample() { + lint() + .files( + kotlin( + """ + package foo + + class Test { + fun doNothing(t: String?): Boolean { + t/* this is legal */!!.length + return t!!.length == 1 + } + } + """ + ) + ) + .run() + .expect( + """ + src/foo/Test.kt:6: Warning: Avoid using the !! operator [AvoidUsingNotNullOperator] + t/* this is legal */!!.length + ~~ + src/foo/Test.kt:7: Warning: Avoid using the !! operator [AvoidUsingNotNullOperator] + return t!!.length == 1 + ~~ + 0 errors, 2 warnings + """ + .trimIndent() + ) + } + + @Test + fun `test clean`() { + lint() + .files( + kotlin( + """ + package foo + + class Test { + fun doNothing(t: String?): Boolean { + // Should not match despite the !! string in it + "!!".length++ + return t?.length == 1 + } + } + """ + ) + ) + .run() + .expectClean() + } +}