Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added rule CanBeNonNullable #4379

Merged
merged 8 commits into from Dec 26, 2021
2 changes: 2 additions & 0 deletions config/detekt/detekt.yml
Expand Up @@ -143,6 +143,8 @@ potential-bugs:
active: true

style:
CanBeNonNullableProperty:
active: true
ClassOrdering:
active: true
CollapsibleIfStatements:
Expand Down
2 changes: 2 additions & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
Expand Up @@ -603,6 +603,8 @@ potential-bugs:

style:
active: true
CanBeNonNullableProperty:
active: true
ClassOrdering:
active: false
CollapsibleIfStatements:
Expand Down
@@ -0,0 +1,177 @@
package io.gitlab.arturbosch.detekt.rules.style

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.DetektVisitor
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.ActiveByDefault
import io.gitlab.arturbosch.detekt.rules.isOpen
import org.jetbrains.kotlin.descriptors.PropertyDescriptor
import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.psi.KtBinaryExpression
import org.jetbrains.kotlin.psi.KtConstantExpression
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.psi.KtIfExpression
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.KtPropertyAccessor
import org.jetbrains.kotlin.psi.KtPropertyDelegate
import org.jetbrains.kotlin.psi.KtReturnExpression
import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType
import org.jetbrains.kotlin.psi.psiUtil.isPrivate
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
import org.jetbrains.kotlin.resolve.calls.callUtil.getType
import org.jetbrains.kotlin.resolve.calls.smartcasts.getKotlinTypeForComparison
import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameOrNull
import org.jetbrains.kotlin.types.isNullable

/**
* This rule inspects properties marked as nullable and reports which could be
* declared as non-nullable instead.
*
* <noncompliant>
* class A {
* var a: Int? = 5
*
* fun foo() {
* a = 6
* }
* }
*
* class A {
* val a: Int?
* get() = 5
* }
* </noncompliant>
*
* <compliant>
* class A {
* var a: Int = 5
*
* fun foo() {
* a = 6
* }
* }
*
* class A {
* val a: Int
* get() = 5
* }
* </compliant>
*/
@ActiveByDefault(since = "1.20.0")
BraisGabin marked this conversation as resolved.
Show resolved Hide resolved
class CanBeNonNullableProperty(config: Config = Config.empty) : Rule(config) {
override val issue = Issue(
javaClass.simpleName,
Severity.Style,
"Property can be changed to non-nullable, as it is never set to null.",
cortinico marked this conversation as resolved.
Show resolved Hide resolved
Debt.TEN_MINS
)

override fun visitKtFile(file: KtFile) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing a super.visitKtFile(file) call here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this be necessary? The NonNullableCheckVisitor instance conducts the actual check of the file for the rule, not CanBeNonNullableProperty itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this be necessary?

That's part of the Rule contract and in general follows the visitor design pattern implementation. If by any chance we decide to implement a new feature on detekt that relies on the visitKtFile and lives inside the Rule top level class (or any of its ancestors), this rule will break this behavior.

if (bindingContext == BindingContext.EMPTY) return
NonNullableCheckVisitor().visitKtFile(file)
}

private inner class NonNullableCheckVisitor : DetektVisitor() {
// A list of properties that are marked as nullable during their
// declaration but do not explicitly receive a nullable value in
// the declaration, so they could potentially be marked as non-nullable
// if the file does not encounter these properties being assigned
// a nullable value.
private val candidateProps = mutableMapOf<FqName, KtProperty>()

override fun visitKtFile(file: KtFile) {
super.visitKtFile(file)
// Any candidate properties that were not removed during the inspection
// of the Kotlin file were never assigned nullable values in the code,
// thus they can be converted to non-nullable.
candidateProps.forEach { (_, property) ->
report(
CodeSmell(
issue,
Entity.from(property),
"A nullable property can be made non-nullable."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You could improve this to mention the name of the property as you have it

)
)
}
}

override fun visitProperty(property: KtProperty) {
if (property.getKotlinTypeForComparison(bindingContext)?.isNullable() != true) return
val fqName = property.fqName ?: return
if (property.isCandidate()) {
candidateProps[fqName] = property
}
}

override fun visitBinaryExpression(expression: KtBinaryExpression) {
if (expression.operationToken == KtTokens.EQ) {
val fqName = expression.left
?.getResolvedCall(bindingContext)
?.resultingDescriptor
?.fqNameOrNull()
if (
fqName != null &&
candidateProps.containsKey(fqName) &&
expression.right?.isNullableType() == true
) {
// A candidate property has been assigned a nullable value
// in the file's code, so it can be removed from the map of
// candidates for flagging.
candidateProps.remove(fqName)
}
}
super.visitBinaryExpression(expression)
}

private fun KtProperty.isCandidate(): Boolean {
if (isOpen()) return false
val isSetToNonNullable = initializer?.isNullableType() != true
&& getter?.isNullableType() != true
&& delegate?.returnsNullable() != true
val cannotSetViaNonPrivateMeans = !isVar || (isPrivate() || (setter?.isPrivate() == true))
return isSetToNonNullable && cannotSetViaNonPrivateMeans
}

private fun KtPropertyDelegate?.returnsNullable(): Boolean {
val property = this?.parent as? KtProperty ?: return false
val propertyDescriptor =
bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, property] as? PropertyDescriptor
return listOfNotNull(propertyDescriptor?.getter, propertyDescriptor?.setter).any {
bindingContext[BindingContext.DELEGATED_PROPERTY_RESOLVED_CALL, it]
?.resultingDescriptor
?.returnType
?.isNullable() == true
}
}

private fun KtExpression?.isNullableType(): Boolean {
return when (this) {
is KtConstantExpression -> {
this.text == "null"
}
is KtIfExpression -> {
this.then.isNullableType() || this.`else`.isNullableType()
}
is KtPropertyAccessor -> {
(initializer?.getType(bindingContext)?.isMarkedNullable == true) ||
(
bodyExpression
?.collectDescendantsOfType<KtReturnExpression>()
?.any { it.returnedExpression.isNullableType() } == true
)
}
else -> {
this?.getType(bindingContext)?.isNullable() == true
}
}
}
}
}
Expand Up @@ -23,6 +23,7 @@ class StyleGuideProvider : DefaultRuleSetProvider {
override fun instance(config: Config): RuleSet = RuleSet(
ruleSetId,
listOf(
CanBeNonNullableProperty(config),
ClassOrdering(config),
CollapsibleIfStatements(config),
DestructuringDeclarationWithTooManyEntries(config),
Expand Down