From d958fbf5f55294241a82152076e4482e35969f7b Mon Sep 17 00:00:00 2001 From: Dominic Zirbel Date: Mon, 30 May 2022 13:26:55 -0700 Subject: [PATCH] CanBeNonNullable: fix false positives for parameterized types (#4870) CanBeNonNullable reports that properties of parameterized types can be marked as non-nullable even when this is impossible, just because the parameterized type is itself nullable (i.e. does not inherit from a non-nullable type). Instead, the rule should only report cases where a property is explicitly marked nullable but does not need to be. --- .../detekt/rules/style/CanBeNonNullable.kt | 27 ++++++- .../rules/style/CanBeNonNullableSpec.kt | 73 +++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt index 25aae364337..ba14ae34d9b 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt @@ -14,6 +14,7 @@ import io.gitlab.arturbosch.detekt.rules.isNonNullCheck import io.gitlab.arturbosch.detekt.rules.isNullCheck import io.gitlab.arturbosch.detekt.rules.isOpen import io.gitlab.arturbosch.detekt.rules.isOverride +import org.jetbrains.kotlin.com.intellij.codeInsight.NullableNotNullManager.isNullable import org.jetbrains.kotlin.descriptors.CallableDescriptor import org.jetbrains.kotlin.descriptors.DeclarationDescriptor import org.jetbrains.kotlin.descriptors.PropertyDescriptor @@ -52,6 +53,8 @@ import org.jetbrains.kotlin.resolve.calls.smartcasts.getKotlinTypeForComparison import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall import org.jetbrains.kotlin.resolve.calls.util.getType import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameOrNull +import org.jetbrains.kotlin.types.KotlinType +import org.jetbrains.kotlin.types.TypeUtils import org.jetbrains.kotlin.types.isNullable /** @@ -445,7 +448,8 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) { } override fun visitProperty(property: KtProperty) { - if (property.getKotlinTypeForComparison(bindingContext)?.isNullable() == true) { + val type = property.getKotlinTypeForComparison(bindingContext) + if (type?.isNullableAndCanBeNonNullable() == true) { val fqName = property.fqName if (property.isCandidate() && fqName != null) { candidateProps[fqName] = property @@ -474,6 +478,21 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) { super.visitBinaryExpression(expression) } + /** + * Determines whether a type is nullable and can be made non-nullable; for most properties + * this is simply whether they are nullable, but for type parameters they can only be made + * non-nullable when explicitly marked nullable. + * + * Note that [KotlinType.isNullable] for type parameter types is true unless it inherits + * from a non-nullable type, e.g.: + * - nullable: or + * - non-nullable: + * But even if T is nullable, a property `val t: T` cannot be made into a non-nullable type. + */ + private fun KotlinType.isNullableAndCanBeNonNullable(): Boolean { + return if (TypeUtils.isTypeParameter(this)) isMarkedNullable else isNullable() + } + private fun KtProperty.isCandidate(): Boolean { if (isOpen() || isAbstract()) return false val isSetToNonNullable = initializer?.isNullableType() != true && @@ -512,7 +531,11 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) { ) } else -> { - this?.getType(bindingContext)?.isNullable() == true + // only consider types which can be made non-nullable as nullable to warn on + // cases where a field has declared type `T?` but is only assigned as `T`; here + // `T` should not be considered nullable to enforce that the field could be + // declared as just `T` + this?.getType(bindingContext)?.isNullableAndCanBeNonNullable() == true } } } diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt index e5fe215260d..d0e67b42caf 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt @@ -370,6 +370,79 @@ class CanBeNonNullableSpec(val env: KotlinCoreEnvironment) { """ assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() } + + @Test + fun `does not report on non-null properties of a parameterized type`() { + val code = """ + class P(foo: T) { + val a: T = foo + val b: T? = null + } + """.trimIndent() + + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @Test + fun `reports on properties of an unnecessarily nullable parameterized type`() { + val code = """ + class P(foo: T) { + val bar: T? = foo + } + """.trimIndent() + + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `reports on properties of an unnecessarily nullable parameterized type extending Any`() { + val code = """ + class P(foo: T) { + val bar: T? = foo + } + """.trimIndent() + + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `does not report on properties of a parameterized type which must be nullable`() { + val code = """ + class P(private val foo: T) { + val a: T + get() = foo + val b: T? = foo.takeIf { Random.nextBoolean() } + val c: T? + get() = foo.takeIf { Random.nextBoolean() } + } + """.trimIndent() + + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @Test + fun `does not report on properties of a parameterized type which resolve to Nothing`() { + val code = """ + class P { + val foo: T + get() = error("") + } + """.trimIndent() + + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @Test + fun `reports on properties of an unnecessarily nullable parameterized type which resolve to Nothing`() { + val code = """ + class P { + val foo: T? + get() = error("") + } + """.trimIndent() + + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } } @Test