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
Changes from 7 commits
d4818a5
6f9c65e
b08f1ec
a67703d
1c9c60e
a772919
f6a184b
9202e76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,185 @@ | ||
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.RequiresTypeResolution | ||
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.KtClass | ||
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> | ||
*/ | ||
@RequiresTypeResolution | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're missing a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would this be necessary? The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's part of the |
||
if (bindingContext == BindingContext.EMPTY) return | ||
NonNullableCheckVisitor().visitKtFile(file) | ||
super.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), | ||
"The nullable property '${property.name}' can be made non-nullable." | ||
) | ||
) | ||
} | ||
} | ||
|
||
override fun visitClass(klass: KtClass) { | ||
if (!klass.isInterface()) { | ||
super.visitClass(klass) | ||
} | ||
} | ||
|
||
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)?.isNullable() == true) || | ||
( | ||
bodyExpression | ||
?.collectDescendantsOfType<KtReturnExpression>() | ||
?.any { it.returnedExpression.isNullableType() } == true | ||
) | ||
} | ||
else -> { | ||
this?.getType(bindingContext)?.isNullable() == true | ||
} | ||
} | ||
} | ||
} | ||
} |
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.
I think that it's safe to enable this. Or, if it isn't, remove these lines