Add CastNullableToNonNullableType rule
#5653
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package io.gitlab.arturbosch.detekt.rules.bugs | ||
|
|
||
| import io.gitlab.arturbosch.detekt.api.CodeSmell | ||
| import io.gitlab.arturbosch.detekt.api.Config | ||
| import io.gitlab.arturbosch.detekt.api.Debt | ||
| import io.gitlab.arturbosch.detekt.api.Entity | ||
| import io.gitlab.arturbosch.detekt.api.Issue | ||
| import io.gitlab.arturbosch.detekt.api.Rule | ||
| import io.gitlab.arturbosch.detekt.api.Severity | ||
| import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution | ||
| import io.gitlab.arturbosch.detekt.rules.isNullable | ||
| import org.jetbrains.kotlin.lexer.KtTokens | ||
| import org.jetbrains.kotlin.psi.KtBinaryExpressionWithTypeRHS | ||
| import org.jetbrains.kotlin.psi.KtNullableType | ||
| import org.jetbrains.kotlin.utils.addToStdlib.ifFalse | ||
| import org.jetbrains.kotlin.utils.addToStdlib.ifTrue | ||
|
|
||
| /** | ||
| * Reports cast of nullable variable to non-null type. Cast like this can hide `null` | ||
| * problems in your code. The compliant code would be that which will correctly check | ||
| * for two things (nullability and type) and not just one (cast). | ||
| * | ||
| * <noncompliant> | ||
| * fun foo(bar: Any?) { | ||
| * val x = bar as String | ||
| * } | ||
| * </noncompliant> | ||
| * | ||
| * <compliant> | ||
| * fun foo(bar: Any?) { | ||
| * val x = checkNotNull(bar) as String | ||
| * } | ||
| * | ||
| * // Alternative | ||
| * fun foo(bar: Any?) { | ||
| * val x = (bar ?: error("null assertion message")) as String | ||
|
There was a problem hiding this comment. If would suggest something like: There was a problem hiding this comment. Hi @cortinico wouldn't
hence there will be no clear and separate handling of @BraisGabin let me know your thoughts as well There was a problem hiding this comment. I would vote to add two alternatives: val x = bar!! as String
val x = bar as String?Or if val x = checkNotNull(bar) as String
val x = bar as String?The idea of this rule is to make clear that there is a cast from nullable to non-nullable. The idea is to avoid it use There was a problem hiding this comment. Hi, @BraisGabin I personally use I don't like There was a problem hiding this comment.
Which might very much be outside the scope of the rule. Your code suggestion is too opinionated in failing on There was a problem hiding this comment. Hi @BraisGabin / @cortinico added below compliant block |
||
| * } | ||
| * </compliant> | ||
| */ | ||
| @RequiresTypeResolution | ||
| class CastNullableToNonNullableType(config: Config = Config.empty) : Rule(config) { | ||
| override val issue: Issue = Issue( | ||
| javaClass.simpleName, | ||
| Severity.Defect, | ||
| "Nullable type to non-null type cast is found. Consider using two assertions, " + | ||
| "`null` assertions and type cast", | ||
| Debt.FIVE_MINS | ||
| ) | ||
|
|
||
| @Suppress("ReturnCount") | ||
| override fun visitBinaryWithTypeRHSExpression(expression: KtBinaryExpressionWithTypeRHS) { | ||
atulgpt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| super.visitBinaryWithTypeRHSExpression(expression) | ||
|
|
||
| val operationReference = expression.operationReference | ||
| if (operationReference.getReferencedNameElementType() != KtTokens.AS_KEYWORD) return | ||
| if (expression.left.text == KtTokens.NULL_KEYWORD.value) return | ||
| val typeElement = expression.right?.typeElement ?: return | ||
| (typeElement is KtNullableType).ifTrue { return } | ||
| val compilerResourcesNonNull = compilerResources ?: return | ||
| expression.left.isNullable( | ||
| bindingContext, | ||
| compilerResourcesNonNull.languageVersionSettings, | ||
| compilerResourcesNonNull.dataFlowValueFactory, | ||
| shouldConsiderPlatformTypeAsNullable = true, | ||
| ).ifFalse { return } | ||
|
|
||
| val message = | ||
| "Use separate `null` assertion and type cast like ('(${expression.left.text} ?: " + | ||
| "error(\"null assertion message\")) as ${typeElement.text}') instead of '${expression.text}'." | ||
| report(CodeSmell(issue, Entity.from(operationReference), message)) | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| package io.gitlab.arturbosch.detekt.rules.bugs | ||
|
|
||
| import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest | ||
| import io.gitlab.arturbosch.detekt.test.assertThat | ||
| import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext | ||
| import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment | ||
| import org.junit.jupiter.api.Test | ||
|
|
||
| @KotlinCoreEnvironmentTest | ||
| class CastNullableToNonNullableTypeSpec(private val env: KotlinCoreEnvironment) { | ||
| private val subject = CastNullableToNonNullableType() | ||
|
|
||
| @Test | ||
| fun `reports casting Nullable type to NonNullable type`() { | ||
| val code = """ | ||
| fun foo(bar: Any?) { | ||
| val x = bar as String | ||
| } | ||
| """.trimIndent() | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).hasSize(1) | ||
| assertThat(findings).hasStartSourceLocation(2, 17) | ||
| assertThat(findings[0]).hasMessage( | ||
| "Use separate `null` assertion and type cast like " + | ||
| "('(bar ?: error(\"null assertion message\")) as String') instead of 'bar as String'." | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `reports casting Nullable value returned from a function call to NonNullable type`() { | ||
| val code = """ | ||
| fun foo(bar: Any?) { | ||
| bar() as Int | ||
| } | ||
|
|
||
| fun bar(): Int? { | ||
| return null | ||
| } | ||
| """.trimIndent() | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).hasSize(1) | ||
| assertThat(findings).hasStartSourceLocation(2, 11) | ||
| assertThat(findings[0]).hasMessage( | ||
| "Use separate `null` assertion and type cast like " + | ||
| "('(bar() ?: error(\"null assertion message\")) as Int') instead of 'bar() as Int'." | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `reports casting of platform type to NonNullable type`() { | ||
| val code = """ | ||
| class Foo { | ||
| fun test() { | ||
| // getSimpleName() is not annotated with nullability information in the JDK, so compiler treats | ||
| // it as a platform type with unknown nullability. | ||
| val y = javaClass.simpleName as String | ||
| } | ||
| } | ||
| """.trimIndent() | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).hasSize(1) | ||
| assertThat(findings).hasStartSourceLocation(5, 38) | ||
| assertThat(findings[0]).hasMessage( | ||
| "Use separate `null` assertion and type cast like " + | ||
| "('(javaClass.simpleName ?: error(\"null assertion message\")) as String') instead of " + | ||
| "'javaClass.simpleName as String'." | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `does not report unnecessary safe check chain to NonNullable type`() { | ||
| val code = """ | ||
| class Foo { | ||
| fun test() { | ||
| val z = 1?.and(2) as Int | ||
| } | ||
| } | ||
| """.trimIndent() | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).isEmpty() | ||
| } | ||
|
|
||
| @Test | ||
| fun `does not report casting of Nullable type to NonNullable expression with assertion to NonNullable type`() { | ||
| val code = """ | ||
| fun foo(bar: Any?) { | ||
| val x = (bar ?: error("null assertion message")) as String | ||
| } | ||
| """.trimIndent() | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).isEmpty() | ||
| } | ||
|
|
||
| @Test | ||
| fun `does not report casting of Nullable type to NonNullable expression with !! assertion to NonNullable type`() { | ||
| val code = """ | ||
| fun foo(bar: Any?) { | ||
| val x = bar!! as String | ||
| } | ||
| """.trimIndent() | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).isEmpty() | ||
| } | ||
|
|
||
| @Test | ||
| fun `does not report casting of Nullable type to NonNullable smart casted variable to NonNullable type`() { | ||
| val code = """ | ||
| fun foo(bar: Any?) { | ||
| val x = bar?.let { bar as String } | ||
| } | ||
| """.trimIndent() | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).isEmpty() | ||
| } | ||
|
|
||
| @Test | ||
| fun `does not report casting of NonNullable type to NonNullable type`() { | ||
| val code = """ | ||
| fun foo(bar: Any?) { | ||
| val x = bar as String? | ||
| } | ||
| """.trimIndent() | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).isEmpty() | ||
| } | ||
|
|
||
| @Test | ||
| fun `does not report safe casting of Nullable type to NonNullable type`() { | ||
| val code = """ | ||
| fun foo(bar: Any?) { | ||
| val x = bar as? String | ||
| } | ||
| """.trimIndent() | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).isEmpty() | ||
| } | ||
|
|
||
| @Test | ||
| fun `does not report as compile error will happen when null to NonNullable type`() { | ||
| val code = """ | ||
| fun foo(bar: Any?) { | ||
| val x = null as String | ||
| } | ||
| """.trimIndent() | ||
| val findings = subject.compileAndLintWithContext(env, code) | ||
| assertThat(findings).isEmpty() | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @cortinico changing the message to
* problems in your code. The compliant code would either us smart-casting or would check the type before performing the cast.doesn't actually reflect the intention of the rule.Original code
bar as Stringasserts two things(nullability as well as type assertions) at once. And suggested fix is to assert both the cases differently or handle both the casesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure I get the point of this rule at this point.
The problem of
bar as Stringis that it will fail at runtime if the two types are incompatible. Using aif (bar is String) ... else ...would not instead.Your rule is instead suggesting to fail for two different scenarios (null or non compatible types) with different error messages, right?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of this rule is that a cast can hide a
!!behind it. That's a potential error and what this rule does is to point it out to ensure that this is not an error but a decission by the devolver. If you really want to cast directly fromAny?toStringyou should be fine withfoo!! as String. If you didn't want to do that, well, then you need to rewrite your code in any other way that doesn't produces a null pointer exception.To me this rule should not be too opinated. Just point to the potential error and its up to the dev to decide how to fix it. If the problem here are the compilant part we can add 3 different ways to make the code compilant because it depends a lot to what the user wants on each case.
If you are casting your code is smelly. This rule at least ensure that the cast doesn't hide any extra surprise.