Skip to content

Commit

Permalink
Only report UnsafeCallOnNullableType on actual nullable types (#1886)
Browse files Browse the repository at this point in the history
* Only check !! used on nullable types

* Use more appropriate method to get a type
  • Loading branch information
3flex authored and schalkms committed Sep 13, 2019
1 parent c2a8fda commit f80fe60
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 17 deletions.
Expand Up @@ -8,10 +8,10 @@ import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import org.jetbrains.kotlin.cfg.WhenChecker
import org.jetbrains.kotlin.codegen.kotlinType
import org.jetbrains.kotlin.psi.KtWhenExpression
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.bindingContextUtil.isUsedAsExpression
import org.jetbrains.kotlin.resolve.calls.callUtil.getType

/**
* Turn on this rule to flag `when` expressions that do not check that all cases are covered when the subject is an enum
Expand Down Expand Up @@ -76,7 +76,7 @@ class MissingWhenCase(config: Config = Config.empty) : Rule(config) {
if (expression.entries.find { it.isElse } != null) return
if (expression.isUsedAsExpression(bindingContext)) return
val subjectExpression = expression.subjectExpression ?: return
val subjectType = subjectExpression.kotlinType(bindingContext)
val subjectType = subjectExpression.getType(bindingContext)
val enumClassDescriptor = WhenChecker.getClassDescriptorOfTypeIfEnum(subjectType)
if (enumClassDescriptor != null) {
val enumMissingCases = WhenChecker.getEnumMissingCases(expression, bindingContext, enumClassDescriptor)
Expand Down
Expand Up @@ -9,12 +9,12 @@ import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import org.jetbrains.kotlin.builtins.KotlinBuiltIns
import org.jetbrains.kotlin.cfg.WhenChecker
import org.jetbrains.kotlin.codegen.kotlinType
import org.jetbrains.kotlin.descriptors.ClassDescriptor
import org.jetbrains.kotlin.load.kotlin.toSourceElement
import org.jetbrains.kotlin.psi.KtWhenExpression
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.BindingContext.DECLARATION_TO_DESCRIPTOR
import org.jetbrains.kotlin.resolve.calls.callUtil.getType
import org.jetbrains.kotlin.resolve.descriptorUtil.module
import org.jetbrains.kotlin.resolve.source.getPsi

Expand Down Expand Up @@ -80,7 +80,7 @@ class RedundantElseInWhen(config: Config = Config.empty) : Rule(config) {
if (bindingContext == BindingContext.EMPTY) return
if (whenExpression.entries.find { it.isElse } == null) return
val subjectExpression = whenExpression.subjectExpression ?: return
val subjectType = subjectExpression.kotlinType(bindingContext) ?: return
val subjectType = subjectExpression.getType(bindingContext) ?: return

if (WhenChecker.getMissingCases(whenExpression, bindingContext).isEmpty()) {
val subjectClass = subjectType.constructor.declarationDescriptor as? ClassDescriptor
Expand Down
Expand Up @@ -9,6 +9,10 @@ import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.psi.KtUnaryExpression
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.callUtil.getType
import org.jetbrains.kotlin.types.typeUtil.TypeNullability
import org.jetbrains.kotlin.types.typeUtil.nullability

/**
* Reports unsafe calls on nullable types. These calls will throw a NullPointerException in case
Expand All @@ -33,8 +37,13 @@ class UnsafeCallOnNullableType(config: Config = Config.empty) : Rule(config) {
"It will throw a NullPointerException at runtime if your nullable value is null.",
Debt.TWENTY_MINS)

@Suppress("ReturnCount")
override fun visitUnaryExpression(expression: KtUnaryExpression) {
super.visitUnaryExpression(expression)
if (bindingContext == BindingContext.EMPTY) return
val type = expression.baseExpression?.getType(bindingContext) ?: return
if (type.nullability() != TypeNullability.NULLABLE) return

if (expression.operationToken == KtTokens.EXCLEXCL) {
report(CodeSmell(issue, Entity.from(expression), "Calling !! on a nullable type will throw a " +
"NullPointerException at runtime in case the value is null. It should be avoided."))
Expand Down
@@ -1,37 +1,55 @@
package io.gitlab.arturbosch.detekt.rules.bugs

import io.gitlab.arturbosch.detekt.test.compileAndLint
import io.gitlab.arturbosch.detekt.test.KtTestCompiler
import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
import org.assertj.core.api.Assertions.assertThat
import org.spekframework.spek2.Spek
import org.spekframework.spek2.style.specification.describe

class UnsafeCallOnNullableTypeSpec : Spek({
val subject by memoized { UnsafeCallOnNullableType() }

val wrapper by memoized(
factory = { KtTestCompiler.createEnvironment() },
destructor = { it.dispose() }
)

describe("check all variants of safe/unsafe calls on nullable types") {

it("reports unsafe call on nullable type") {
val code = """
fun test(str: String?) {
println(str!!.length)
}"""
assertThat(subject.compileAndLint(code)).hasSize(1)
fun test(str: String?) {
println(str!!.length)
}
"""
assertThat(subject.compileAndLintWithContext(wrapper.env, code)).hasSize(1)
}

it("does not report unsafe call on platform type") {
val code = """
import java.util.UUID
val version = UUID.randomUUID()!!
"""
assertThat(subject.compileAndLintWithContext(wrapper.env, code)).isEmpty()
}

it("does not report safe call on nullable type") {
val code = """
fun test(str: String?) {
println(str?.length)
}"""
assertThat(subject.compileAndLint(code)).hasSize(0)
fun test(str: String?) {
println(str?.length)
}
"""
assertThat(subject.compileAndLintWithContext(wrapper.env, code)).isEmpty()
}

it("does not report safe call in combination with the elvis operator") {
val code = """
fun test(str: String?) {
println(str?.length ?: 0)
}"""
assertThat(subject.compileAndLint(code)).hasSize(0)
fun test(str: String?) {
println(str?.length ?: 0)
}
"""
assertThat(subject.compileAndLintWithContext(wrapper.env, code)).isEmpty()
}
}
})

0 comments on commit f80fe60

Please sign in to comment.