Skip to content

Commit 09d7ed4

Browse files
SimonMarquisSimonMarquis
authored
🕵️ Extensive lint detection for nullable/non-nullable values checks (#398)
1 parent 8f62921 commit 09d7ed4

File tree

3 files changed

+596
-324
lines changed

3 files changed

+596
-324
lines changed

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@
6767
- Prefer using `kotlin.test` assertions instead of JUnit's in Kotlin unit tests.
6868
- Prefer using `kotlin.test` assertions instead of `assert` in unit tests. Its execution requires a specific JVM option to be enabled on the JVM.
6969
- Prefer using `assertIs` and `assertIsNot` assertions when checking for types instead of boolean assertions.
70+
- Prefer using `assertNull`/`assertNotNull` assertions when checking for null/non-null values instead of equality assertions.
7071
- Prefer using `assertEquals`/`assertSame` and `assertNotEquals`/`assertNotSame` assertions when checking for equality instead of boolean assertions.
7172

7273
</details>

‎lint/src/main/kotlin/fr/smarquis/playground/lint/AssertionsDetector.kt‎

Lines changed: 183 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -8,112 +8,218 @@ import com.android.tools.lint.detector.api.JavaContext
88
import com.android.tools.lint.detector.api.Scope.JAVA_FILE
99
import com.android.tools.lint.detector.api.Scope.TEST_SOURCES
1010
import com.android.tools.lint.detector.api.Severity
11+
import com.android.tools.lint.detector.api.isJava
1112
import com.intellij.psi.PsiMethod
13+
import fr.smarquis.playground.lint.AssertionsDetector.EqualityAssertionReplacement.Binary
14+
import fr.smarquis.playground.lint.AssertionsDetector.EqualityAssertionReplacement.UnaryNull
15+
import org.jetbrains.kotlin.utils.addToStdlib.UnsafeCastFunction
1216
import org.jetbrains.uast.UBinaryExpression
1317
import org.jetbrains.uast.UBinaryExpressionWithType
1418
import org.jetbrains.uast.UBlockExpression
1519
import org.jetbrains.uast.UCallExpression
20+
import org.jetbrains.uast.UElement
1621
import org.jetbrains.uast.UExpression
1722
import org.jetbrains.uast.ULambdaExpression
1823
import org.jetbrains.uast.UReturnExpression
1924
import org.jetbrains.uast.UastBinaryExpressionWithTypeKind.InstanceCheck
25+
import org.jetbrains.uast.UastBinaryOperator
2026
import org.jetbrains.uast.UastBinaryOperator.Companion.EQUALS
2127
import org.jetbrains.uast.UastBinaryOperator.Companion.IDENTITY_EQUALS
2228
import org.jetbrains.uast.UastBinaryOperator.Companion.IDENTITY_NOT_EQUALS
2329
import org.jetbrains.uast.UastBinaryOperator.Companion.NOT_EQUALS
24-
import org.jetbrains.uast.java.isJava
30+
import org.jetbrains.uast.isNullLiteral
31+
import org.jetbrains.uast.kotlin.KotlinBinaryExpressionWithTypeKinds.NEGATED_INSTANCE_CHECK
2532
import org.jetbrains.uast.skipParenthesizedExprDown
2633
import java.util.EnumSet
2734

2835
public class AssertionsDetector : Detector(), Detector.UastScanner {
2936

30-
override fun getApplicableUastTypes(): List<Class<UCallExpression>> = listOf(UCallExpression::class.java)
37+
override fun getApplicableUastTypes(): List<Class<out UElement>> = listOf(UCallExpression::class.java)
3138

3239
override fun createUastHandler(context: JavaContext): UElementHandler? {
3340
if (!context.isTestSource) return null
3441
return object : UElementHandler() {
3542
override fun visitCallExpression(node: UCallExpression) {
3643
// Avoid enforcing kotlin-test use in java sources
37-
if (isJava(node.javaPsi?.language)) return
44+
if (node.javaPsi?.language?.let(::isJava) == true) return
3845
val psiMethod = node.resolve() ?: return
39-
// Kotlin assert
40-
if (psiMethod.name == "assert" && psiMethod.containingClass?.qualifiedName?.startsWith("kotlin.") == true) {
41-
return context.report(
42-
issue = KOTLIN_ASSERT_ISSUE,
43-
scope = node,
44-
location = context.getLocation(node),
45-
message = "Use `kotlin.test` assertion",
46-
)
47-
}
48-
// JUnit assertions
49-
for (assertionClass in setOf("org.junit.Assert", "junit.framework.Assert")) {
50-
if (context.evaluator.isMemberInClass(psiMethod, assertionClass)) {
51-
return context.report(
52-
issue = JUNIT_ASSERTION_ISSUE,
53-
scope = node,
54-
location = context.getLocation(node),
55-
message = "Use `kotlin.test` assertion",
56-
)
57-
}
58-
}
59-
// Boolean assertion instead of type assertion
60-
typeAssertionMap[node.methodName ?: psiMethod.name]?.let report@{ replacement ->
61-
val assertion = context.computeBooleanAssertion(node, psiMethod)
62-
if (assertion.expression !is UBinaryExpressionWithType) return@report
63-
if (assertion.expression.operationKind !is InstanceCheck) return@report
64-
return context.report(
65-
issue = KOTLIN_TYPE_ASSERTION_ISSUE,
66-
scope = node,
67-
location = context.getLocation(node),
68-
message = "Replace boolean assertion with `${replacement.substringAfterLast(".")}`",
69-
quickfixData = fix()
70-
.replace().all()
71-
.with(
72-
buildString {
73-
append(replacement.substringAfterLast("."))
74-
append("<").append(assertion.expression.type.canonicalText).append(">")
75-
append("(")
76-
append(assertion.expression.operand.asSourceString())
77-
if (assertion.message != null) append(", message = ").append(assertion.message.asSourceString())
78-
append(")")
79-
},
80-
)
81-
.shortenNames(true).reformat(true)
82-
.imports(replacement)
83-
.autoFix().build(),
84-
)
85-
}
86-
// Boolean assertion instead of equality assertion
87-
equalityAssertionMap[node.methodName ?: psiMethod.name]?.let report@{ replacements ->
88-
val assertion = context.computeBooleanAssertion(node, psiMethod)
89-
if (assertion.expression !is UBinaryExpression) return@report
90-
val replacement = replacements[assertion.expression.operator] ?: return@report
91-
return context.report(
92-
issue = KOTLIN_EQUALITY_ASSERTION_ISSUE,
93-
scope = node,
94-
location = context.getLocation(node),
95-
message = "Replace boolean assertion with `${replacement.substringAfterLast(".")}`",
96-
quickfixData = fix()
97-
.replace().all().with(
98-
buildString {
99-
append(replacement.substringAfterLast("."))
100-
append("(expected = ").append(assertion.expression.rightOperand.asSourceString())
101-
append(", actual = ").append(assertion.expression.leftOperand.asSourceString())
102-
if (assertion.message != null) append(", message = ").append(assertion.message.asSourceString())
103-
append(")")
104-
},
105-
)
106-
.shortenNames(true).reformat(true)
107-
.imports(replacement)
108-
.autoFix().build(),
109-
)
110-
}
46+
checkJunitAssertion(context, node, psiMethod)
47+
checkKotlinAssert(context, node, psiMethod)
48+
checkTypeAssertion(context, node, psiMethod)
49+
checkEqualityAssertion(context, node, psiMethod)
11150
}
11251
}
11352
}
11453

54+
private fun checkJunitAssertion(context: JavaContext, node: UCallExpression, psiMethod: PsiMethod) {
55+
for (assertionClass in setOf("org.junit.Assert", "junit.framework.Assert")) {
56+
if (!context.evaluator.isMemberInClass(psiMethod, assertionClass)) continue
57+
context.report(
58+
issue = JUNIT_ASSERTION_ISSUE,
59+
scope = node,
60+
location = context.getLocation(node),
61+
message = "Use `kotlin.test` assertion",
62+
)
63+
}
64+
}
65+
66+
private fun checkKotlinAssert(context: JavaContext, node: UCallExpression, psiMethod: PsiMethod) {
67+
if (psiMethod.name != "assert") return
68+
if (psiMethod.containingClass?.qualifiedName?.startsWith("kotlin.") != true) return
69+
context.report(
70+
issue = KOTLIN_ASSERT_ISSUE,
71+
scope = node,
72+
location = context.getLocation(node),
73+
message = "Use `kotlin.test` assertion",
74+
)
75+
}
76+
77+
@Suppress("UnstableApiUsage")
78+
private fun checkTypeAssertion(context: JavaContext, node: UCallExpression, psiMethod: PsiMethod) {
79+
if (psiMethod.name != "assertFalse" && psiMethod.name != "assertTrue") return
80+
val assertion = context.computeBooleanAssertion(node, psiMethod)
81+
if (assertion.expression !is UBinaryExpressionWithType) return
82+
if (assertion.expression.operationKind !is InstanceCheck) return
83+
val replacement = when (psiMethod.name) {
84+
"assertFalse" -> when (assertion.expression.operationKind) {
85+
InstanceCheck.INSTANCE -> "kotlin.test.assertIsNot"
86+
NEGATED_INSTANCE_CHECK -> "kotlin.test.assertIs"
87+
else -> return
88+
}
89+
90+
"assertTrue" -> when (assertion.expression.operationKind) {
91+
InstanceCheck.INSTANCE -> "kotlin.test.assertIs"
92+
NEGATED_INSTANCE_CHECK -> "kotlin.test.assertIsNot"
93+
else -> return
94+
}
95+
96+
else -> return
97+
}
98+
context.report(
99+
issue = KOTLIN_TYPE_ASSERTION_ISSUE,
100+
scope = node,
101+
location = context.getLocation(node),
102+
message = "Replace boolean assertion with `${replacement.substringAfterLast(".")}`",
103+
quickfixData = fix()
104+
.replace().all()
105+
.with(
106+
buildString {
107+
append(replacement.substringAfterLast("."))
108+
append("<").append(assertion.expression.type.canonicalText).append(">")
109+
append("(")
110+
append(assertion.expression.operand.asSourceString())
111+
if (assertion.message != null) append(", message = ").append(assertion.message.asSourceString())
112+
append(")")
113+
},
114+
)
115+
.imports(replacement)
116+
.shortenNames(true).reformat(true).autoFix()
117+
.build(),
118+
)
119+
}
120+
121+
private fun checkEqualityAssertion(context: JavaContext, node: UCallExpression, psiMethod: PsiMethod) {
122+
val assertion = context.computeBooleanAssertion(node, psiMethod)
123+
if (assertion.expression !is UBinaryExpression) return
124+
val replacement = when {
125+
assertion.expression.leftOperand.isNullLiteral() -> UnaryNull(
126+
fqfn = psiMethod.simplifiedNullableAssertion(assertion.expression.operator) ?: return,
127+
actual = assertion.expression.rightOperand,
128+
)
129+
130+
assertion.expression.rightOperand.isNullLiteral() -> UnaryNull(
131+
fqfn = psiMethod.simplifiedNullableAssertion(assertion.expression.operator) ?: return,
132+
actual = assertion.expression.leftOperand,
133+
)
134+
135+
else -> Binary(
136+
fqfn = psiMethod.simplifiedBinaryAssertion(assertion.expression.operator) ?: return,
137+
expected = assertion.expression.rightOperand,
138+
actual = assertion.expression.leftOperand,
139+
)
140+
}
141+
142+
return context.report(
143+
issue = KOTLIN_EQUALITY_ASSERTION_ISSUE,
144+
scope = node,
145+
location = context.getLocation(node),
146+
message = "Replace boolean assertion with `${replacement.fqfn.substringAfterLast(".")}`",
147+
quickfixData = fix()
148+
.replace().all().with(
149+
buildString {
150+
append(replacement.fqfn.substringAfterLast("."))
151+
when (replacement) {
152+
is UnaryNull -> {
153+
append("(").append(replacement.actual.asSourceString())
154+
}
155+
156+
is Binary -> {
157+
val param = if ("assertNot" in replacement.fqfn) "illegal" else "expected"
158+
append("($param = ").append(replacement.expected.asSourceString())
159+
append(", actual = ").append(replacement.actual.asSourceString())
160+
}
161+
}
162+
if (assertion.message != null) append(", message = ").append(assertion.message.asSourceString())
163+
append(")")
164+
},
165+
)
166+
.imports(replacement.fqfn)
167+
.shortenNames(true).reformat(true).autoFix()
168+
.build(),
169+
)
170+
}
171+
172+
private fun PsiMethod.simplifiedNullableAssertion(operator: UastBinaryOperator): String? = when (name) {
173+
"assertTrue" -> when (operator) {
174+
EQUALS -> "kotlin.test.assertNull"
175+
NOT_EQUALS -> "kotlin.test.assertNotNull"
176+
IDENTITY_EQUALS -> "kotlin.test.assertNull"
177+
IDENTITY_NOT_EQUALS -> "kotlin.test.assertNotNull"
178+
else -> null
179+
}
180+
181+
"assertFalse" -> when (operator) {
182+
EQUALS -> "kotlin.test.assertNotNull"
183+
NOT_EQUALS -> "kotlin.test.assertNull"
184+
IDENTITY_EQUALS -> "kotlin.test.assertNotNull"
185+
IDENTITY_NOT_EQUALS -> "kotlin.test.assertNull"
186+
else -> null
187+
}
188+
189+
else -> null
190+
}
191+
192+
private fun PsiMethod.simplifiedBinaryAssertion(operator: UastBinaryOperator): String? = when (name) {
193+
"assertTrue" -> when (operator) {
194+
EQUALS -> "kotlin.test.assertEquals"
195+
NOT_EQUALS -> "kotlin.test.assertNotEquals"
196+
IDENTITY_EQUALS -> "kotlin.test.assertSame"
197+
IDENTITY_NOT_EQUALS -> "kotlin.test.assertNotSame"
198+
else -> null
199+
}
200+
201+
"assertFalse" -> when (operator) {
202+
EQUALS -> "kotlin.test.assertNotEquals"
203+
NOT_EQUALS -> "kotlin.test.assertEquals"
204+
IDENTITY_EQUALS -> "kotlin.test.assertNotSame"
205+
IDENTITY_NOT_EQUALS -> "kotlin.test.assertSame"
206+
else -> null
207+
}
208+
209+
else -> null
210+
}
211+
212+
public sealed class EqualityAssertionReplacement {
213+
public abstract val fqfn: String
214+
215+
public class UnaryNull(override val fqfn: String, public val actual: UExpression) : EqualityAssertionReplacement()
216+
public class Binary(override val fqfn: String, public val expected: UExpression, public val actual: UExpression) :
217+
EqualityAssertionReplacement()
218+
}
219+
115220
private data class BooleanAssertion(val expression: UExpression?, val message: UExpression?)
116221

222+
@OptIn(UnsafeCastFunction::class)
117223
private fun JavaContext.computeBooleanAssertion(call: UCallExpression, method: PsiMethod) = evaluator
118224
.computeArgumentMapping(call, method)
119225
.entries.associateBy(keySelector = { it.value.name }, valueTransform = { it.key })
@@ -141,7 +247,7 @@ public class AssertionsDetector : Detector(), Detector.UastScanner {
141247
explanation = "Prefer using `kotlin.test` assertions instead of JUnit's in Kotlin unit tests.",
142248
category = Category.CORRECTNESS,
143249
priority = 5,
144-
severity = Severity.WARNING,
250+
severity = Severity.ERROR,
145251
implementation = implementation<AssertionsDetector>(EnumSet.of(JAVA_FILE, TEST_SOURCES)),
146252
)
147253

@@ -164,10 +270,6 @@ public class AssertionsDetector : Detector(), Detector.UastScanner {
164270
severity = Severity.ERROR,
165271
implementation = implementation<AssertionsDetector>(EnumSet.of(JAVA_FILE, TEST_SOURCES)),
166272
)
167-
private val typeAssertionMap = mapOf(
168-
"assertTrue" to "kotlin.test.assertIs",
169-
"assertFalse" to "kotlin.test.assertIsNot",
170-
)
171273

172274
/**
173275
* Based on Kotlin's [`ReplaceAssertBooleanWithAssertEquality`](https://www.jetbrains.com/help/inspectopedia/ReplaceAssertBooleanWithAssertEquality.html), but smarter!
@@ -181,20 +283,6 @@ public class AssertionsDetector : Detector(), Detector.UastScanner {
181283
severity = Severity.ERROR,
182284
implementation = implementation<AssertionsDetector>(EnumSet.of(JAVA_FILE, TEST_SOURCES)),
183285
)
184-
private val equalityAssertionMap = mapOf(
185-
"assertTrue" to mapOf(
186-
EQUALS to "kotlin.test.assertEquals",
187-
NOT_EQUALS to "kotlin.test.assertNotEquals",
188-
IDENTITY_EQUALS to "kotlin.test.assertSame",
189-
IDENTITY_NOT_EQUALS to "kotlin.test.assertNotSame",
190-
),
191-
"assertFalse" to mapOf(
192-
EQUALS to "kotlin.test.assertNotEquals",
193-
NOT_EQUALS to "kotlin.test.assertEquals",
194-
IDENTITY_EQUALS to "kotlin.test.assertNotSame",
195-
IDENTITY_NOT_EQUALS to "kotlin.test.assertSame",
196-
),
197-
)
198-
}
199286

287+
}
200288
}

0 commit comments

Comments
 (0)