Split rules module into a module per rule set (#2865)

* Split rules module into a module per rule set

This significantly improves build and test times.

* Directly instantiate a kotlin scripting engine
This commit is contained in:
Artur Bosch
2020-07-13 19:26:57 +02:00
committed by GitHub
parent bf6f979e32
commit b6e88a95a1
419 changed files with 545 additions and 631 deletions

View File

@@ -0,0 +1,4 @@
dependencies {
compileOnly(project(":detekt-api"))
testImplementation(project(":detekt-test"))
}

View File

@@ -0,0 +1,85 @@
package io.gitlab.arturbosch.detekt.rules.performance
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 org.jetbrains.kotlin.psi.KtElement
import org.jetbrains.kotlin.psi.KtNamedFunction
import org.jetbrains.kotlin.psi.KtParameter
import org.jetbrains.kotlin.psi.KtTypeReference
import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType
/**
* Using Array<Primitive> leads to implicit boxing and performance hit. Prefer using Kotlin specialized Array
* Instances.
*
* As stated in the Kotlin [documention](https://kotlinlang.org/docs/reference/basic-types.html#arrays) Kotlin has
* specialized arrays to represent primitive types without boxing overhead, such as `IntArray`, `ByteArray` and so on.
*
* <noncompliant>
* fun function(array: Array<Int>) { }
*
* fun returningFunction(): Array<Double> { }
* </noncompliant>
*
* <compliant>
* fun function(array: IntArray) { }
*
* fun returningFunction(): DoubleArray { }
* </compliant>
*
* @active since v1.2.0
*/
class ArrayPrimitive(config: Config = Config.empty) : Rule(config) {
override val issue = Issue("ArrayPrimitive",
Severity.Performance,
"Using Array<Primitive> leads to implicit boxing and a performance hit",
Debt.FIVE_MINS)
private val primitiveTypes = hashSetOf(
"Int",
"Double",
"Float",
"Short",
"Byte",
"Long",
"Char"
)
override fun visitParameter(parameter: KtParameter) {
val typeReference = parameter.typeReference
if (typeReference != null) {
reportArrayPrimitives(typeReference)
}
super.visitParameter(parameter)
}
override fun visitNamedFunction(function: KtNamedFunction) {
if (function.hasDeclaredReturnType()) {
val typeReference = function.typeReference
if (typeReference != null) {
reportArrayPrimitives(typeReference)
}
}
super.visitNamedFunction(function)
}
private fun reportArrayPrimitives(element: KtElement) {
return element
.collectDescendantsOfType<KtTypeReference> { isArrayPrimitive(it) }
.forEach { report(CodeSmell(issue, Entity.from(it), issue.description)) }
}
private fun isArrayPrimitive(it: KtTypeReference): Boolean {
if (it.text?.startsWith("Array<") == true) {
val genericTypeArguments = it.typeElement?.typeArgumentsAsTypes
return genericTypeArguments?.singleOrNull()?.let { primitiveTypes.contains(it.text) } == true
}
return false
}
}

View File

@@ -0,0 +1,91 @@
package io.gitlab.arturbosch.detekt.rules.performance
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.rules.getIntValueForPsiElement
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.psi.KtBinaryExpression
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtParenthesizedExpression
import org.jetbrains.kotlin.psi.psiUtil.getCallNameExpression
import org.jetbrains.kotlin.psi.psiUtil.getReceiverExpression
/**
* Using the forEach method on ranges has a heavy performance cost. Prefer using simple for loops.
*
* Benchmarks have shown that using forEach on a range can have a huge performance cost in comparison to
* simple for loops. Hence in most contexts a simple for loop should be used instead.
* See more details here: https://sites.google.com/a/athaydes.com/renato-athaydes/posts/kotlinshiddencosts-benchmarks
* To solve this CodeSmell, the forEach usage should be replaced by a for loop.
*
* <noncompliant>
* (1..10).forEach {
* println(it)
* }
* (1 until 10).forEach {
* println(it)
* }
* (10 downTo 1).forEach {
* println(it)
* }
* </noncompliant>
*
* <compliant>
* for (i in 1..10) {
* println(i)
* }
* </compliant>
*
* @active since v1.0.0
*/
class ForEachOnRange(config: Config = Config.empty) : Rule(config) {
override val issue = Issue("ForEachOnRange",
Severity.Performance,
"Using the forEach method on ranges has a heavy performance cost. Prefer using simple for loops.",
Debt.FIVE_MINS)
private val minimumRangeSize = 3
private val rangeOperators = setOf("..", "downTo", "until", "step")
override fun visitCallExpression(expression: KtCallExpression) {
super.visitCallExpression(expression)
expression.getCallNameExpression()?.let {
if (!it.textMatches("forEach")) {
return
}
val parenthesizedExpression = it.getReceiverExpression() as? KtParenthesizedExpression
val binaryExpression = parenthesizedExpression?.expression as? KtBinaryExpression
if (binaryExpression != null && isRangeOperator(binaryExpression)) {
report(CodeSmell(issue, Entity.from(expression), issue.description))
}
}
}
private fun isRangeOperator(binaryExpression: KtBinaryExpression): Boolean {
val range = binaryExpression.children
if (range.size >= minimumRangeSize) {
val hasCorrectLowerValue = hasCorrectLowerValue(range[0])
val hasCorrectUpperValue = getIntValueForPsiElement(range[2]) != null
return hasCorrectLowerValue && hasCorrectUpperValue && rangeOperators.contains(range[1].text)
}
return false
}
private fun hasCorrectLowerValue(element: PsiElement): Boolean {
var lowerValue = getIntValueForPsiElement(element) != null
if (!lowerValue) {
val expression = element as? KtBinaryExpression
if (expression != null) {
lowerValue = isRangeOperator(expression)
}
}
return lowerValue
}
}

View File

@@ -0,0 +1,25 @@
package io.gitlab.arturbosch.detekt.rules.performance
import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.RuleSet
import io.gitlab.arturbosch.detekt.api.internal.DefaultRuleSetProvider
/**
* The performance rule set analyzes code for potential performance problems.
*
* @active since v1.0.0
*/
class PerformanceProvider : DefaultRuleSetProvider {
override val ruleSetId: String = "performance"
override fun instance(config: Config): RuleSet = RuleSet(
ruleSetId,
listOf(
ForEachOnRange(config),
SpreadOperator(config),
UnnecessaryTemporaryInstantiation(config),
ArrayPrimitive(config)
)
)
}

View File

@@ -0,0 +1,91 @@
package io.gitlab.arturbosch.detekt.rules.performance
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 org.jetbrains.kotlin.descriptors.ConstructorDescriptor
import org.jetbrains.kotlin.psi.KtValueArgument
import org.jetbrains.kotlin.psi.KtValueArgumentList
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.CompileTimeConstantUtils
import org.jetbrains.kotlin.resolve.DescriptorUtils
import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
/**
* In most cases using a spread operator causes a full copy of the array to be created before calling a method.
* This has a very high performance penalty. Benchmarks showing this performance penalty can be seen here:
* https://sites.google.com/a/athaydes.com/renato-athaydes/posts/kotlinshiddencosts-benchmarks
*
* The Kotlin compiler since v1.1.60 has an optimization that skips the array copy when an array constructor
* function is used to create the arguments that are passed to the vararg parameter. When type resolution is enabled in
* detekt this case will not be flagged by the rule since it doesn't suffer the performance penalty of an array copy.
*
* <noncompliant>
* val strs = arrayOf("value one", "value two")
* val foo = bar(*strs)
*
* fun bar(vararg strs: String) {
* strs.forEach { println(it) }
* }
* </noncompliant>
*
* <compliant>
* // array copy skipped in this case since Kotlin 1.1.60
* val foo = bar(*arrayOf("value one", "value two"))
*
* // array not passed so no array copy is required
* val foo2 = bar("value one", "value two")
*
* fun bar(vararg strs: String) {
* strs.forEach { println(it) }
* }
* </compliant>
*
* @active since v1.0.0
*/
class SpreadOperator(config: Config = Config.empty) : Rule(config) {
override val issue: Issue = Issue("SpreadOperator", Severity.Performance,
"In most cases using a spread operator causes a full copy of the array to be created before calling a " +
"method which has a very high performance penalty.",
Debt.TWENTY_MINS)
override fun visitValueArgumentList(list: KtValueArgumentList) {
super.visitValueArgumentList(list)
list.arguments
.filter { it.isSpread }
.filterNotNull()
.forEach {
if (bindingContext == BindingContext.EMPTY) {
report(CodeSmell(issue, Entity.from(list), issue.description))
return
}
if (!it.canSkipArrayCopyForSpreadArgument()) {
report(
CodeSmell(
issue,
Entity.from(list),
"Used in this way a spread operator causes a full copy of the array to be created before " +
"calling a method which has a very high performance penalty."
)
)
}
}
}
/**
* Checks if an array copy can be skipped for this usage of the spread operator. If not, an array copy is required
* for this usage of the spread operator, which will have a performance impact.
*/
private fun KtValueArgument.canSkipArrayCopyForSpreadArgument(): Boolean {
val resolvedCall = getArgumentExpression().getResolvedCall(bindingContext) ?: return false
val calleeDescriptor = resolvedCall.resultingDescriptor
return calleeDescriptor is ConstructorDescriptor ||
CompileTimeConstantUtils.isArrayFunctionCall(resolvedCall) ||
DescriptorUtils.getFqName(calleeDescriptor).asString() == "kotlin.arrayOfNulls"
}
}

View File

@@ -0,0 +1,47 @@
package io.gitlab.arturbosch.detekt.rules.performance
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 org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtExpression
/**
* Avoid temporary objects when converting primitive types to String. This has a performance penalty when compared
* to using primitive types directly.
* To solve this issue, remove the wrapping type.
*
* <noncompliant>
* val i = Integer(1).toString() // temporary Integer instantiation just for the conversion
* </noncompliant>
*
* <compliant>
* val i = Integer.toString(1)
* </compliant>
*
* @active since v1.0.0
*/
class UnnecessaryTemporaryInstantiation(config: Config = Config.empty) : Rule(config) {
override val issue: Issue = Issue("UnnecessaryTemporaryInstantiation", Severity.Performance,
"Avoid temporary objects when converting primitive types to String.",
Debt.FIVE_MINS)
private val types: Set<String> = hashSetOf("Boolean", "Byte", "Short", "Integer", "Long", "Float", "Double")
override fun visitCallExpression(expression: KtCallExpression) {
if (isPrimitiveWrapperType(expression.calleeExpression) &&
isToStringMethod(expression.nextSibling?.nextSibling)) {
report(CodeSmell(issue, Entity.from(expression), issue.description))
}
}
private fun isPrimitiveWrapperType(expression: KtExpression?) = types.contains(expression?.text)
private fun isToStringMethod(element: PsiElement?) = element?.text == "toString()"
}

View File

@@ -0,0 +1 @@
io.gitlab.arturbosch.detekt.rules.performance.PerformanceProvider

View File

@@ -0,0 +1,91 @@
package io.gitlab.arturbosch.detekt.rules.performance
import io.gitlab.arturbosch.detekt.test.compileAndLint
import org.assertj.core.api.Assertions.assertThat
import org.spekframework.spek2.Spek
import org.spekframework.spek2.style.specification.describe
class ArrayPrimitiveSpec : Spek({
val subject by memoized { ArrayPrimitive() }
describe("one function parameter") {
it("is an array of primitive type") {
val code = "fun function(array: Array<Int>) {}"
assertThat(subject.compileAndLint(code)).hasSize(1)
}
it("is not an array") {
val code = "fun function(i: Int) {}"
assertThat(subject.compileAndLint(code)).isEmpty()
}
it("is a specialized array") {
val code = "fun function(array: ByteArray) {}"
assertThat(subject.compileAndLint(code)).isEmpty()
}
it("is a star-projected array") {
val code = "fun function(array: Array<*>) {}"
assertThat(subject.compileAndLint(code)).isEmpty()
}
it("is not present") {
val code = "fun function() {}"
assertThat(subject.compileAndLint(code)).isEmpty()
}
it("is an array of a non-primitive type") {
val code = "fun function(array: Array<String>) {}"
assertThat(subject.compileAndLint(code)).isEmpty()
}
it("is an array of an array of a primitive type") {
val code = "fun function(array: Array<Array<Int>>) {}"
assertThat(subject.compileAndLint(code)).hasSize(1)
}
it("is a dictionary with an array of a primitive type as key") {
val code = "fun function(dict: java.util.Dictionary<Int, Array<Int>>) {}"
assertThat(subject.compileAndLint(code)).hasSize(1)
}
}
describe("multiple function parameters") {
it("one is Array<Primitive> and the other is not") {
val code = "fun function(array: Array<Int>, array2: IntArray) {}"
assertThat(subject.compileAndLint(code)).hasSize(1)
}
it("both are arrays of primitive types") {
val code = "fun function(array: Array<Int>, array2: Array<Double>) {}"
assertThat(subject.compileAndLint(code)).hasSize(2)
}
}
describe("return type") {
it("is Array<Primitive>") {
val code = "fun returningFunction(): Array<Float> { return emptyArray() }"
assertThat(subject.compileAndLint(code)).hasSize(1)
}
it("is not an array") {
val code = "fun returningFunction(): Int { return 1 }"
assertThat(subject.compileAndLint(code)).isEmpty()
}
it("is a specialized array") {
val code = "fun returningFunction(): CharArray { return CharArray(0) }"
assertThat(subject.compileAndLint(code)).isEmpty()
}
it("is a star-projected array") {
val code = "fun returningFunction(): Array<*> { return emptyArray<Any>() }"
assertThat(subject.compileAndLint(code)).isEmpty()
}
it("is not explicitly set") {
val code = "fun returningFunction() {}"
assertThat(subject.compileAndLint(code)).isEmpty()
}
}
})

View File

@@ -0,0 +1,64 @@
package io.gitlab.arturbosch.detekt.rules.performance
import io.gitlab.arturbosch.detekt.test.compileAndLint
import org.assertj.core.api.Assertions.assertThat
import org.spekframework.spek2.Spek
import org.spekframework.spek2.style.specification.describe
class ForEachOnRangeSpec : Spek({
describe("ForEachOnRange rule") {
context("a kt file with using a forEach on a range") {
val code = """
fun test() {
(1..10).forEach {
println(it)
}
(1 until 10).forEach {
println(it)
}
(10 downTo 1).forEach {
println(it)
}
(10 downTo 1 step 2).forEach {
println(it)
}
}
"""
it("should report the forEach usage") {
val findings = ForEachOnRange().compileAndLint(code)
assertThat(findings).hasSize(4)
}
}
context("a kt file with using any other method on a range") {
val code = """
fun test() {
(1..10).isEmpty()
}
"""
it("should report not report any issues") {
val findings = ForEachOnRange().compileAndLint(code)
assertThat(findings).isEmpty()
}
}
context("a kt file with using a forEach on a list") {
val code = """
fun test() {
listOf(1, 2, 3).forEach {
println(it)
}
}
"""
it("should report not report any issues") {
val findings = ForEachOnRange().compileAndLint(code)
assertThat(findings).isEmpty()
}
}
}
})

View File

@@ -0,0 +1,178 @@
package io.gitlab.arturbosch.detekt.rules.performance
import io.gitlab.arturbosch.detekt.rules.setupKotlinEnvironment
import io.gitlab.arturbosch.detekt.test.compileAndLint
import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
import org.assertj.core.api.Assertions.assertThat
import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
import org.spekframework.spek2.Spek
import org.spekframework.spek2.style.specification.describe
class SpreadOperatorSpec : Spek({
setupKotlinEnvironment()
val env: KotlinCoreEnvironment by memoized()
val subject by memoized { SpreadOperator() }
describe("SpreadOperator rule") {
/** This rule has different behaviour depending on whether type resolution is enabled in detekt or not. The two
* `context` blocks are there to test behaviour when type resolution is enabled and type resolution is disabled
* as different warning messages are shown in each case.
*/
context("with type resolution") {
val typeResolutionEnabledMessage = "Used in this way a spread operator causes a full copy of the array to" +
" be created before calling a method which has a very high performance penalty."
it("reports when array copy required using named parameters") {
val code = """
val xsArray = intArrayOf(1)
fun foo(vararg xs: Int) {}
val testVal = foo(xs = *xsArray)
"""
val actual = subject.compileAndLintWithContext(env, code)
assertThat(actual).hasSize(1)
assertThat(actual.first().message).isEqualTo(typeResolutionEnabledMessage)
}
it("reports when array copy required without using named parameters") {
val code = """
val xsArray = intArrayOf(1)
fun foo(vararg xs: Int) {}
val testVal = foo(*xsArray)
"""
val actual = subject.compileAndLintWithContext(env, code)
assertThat(actual).hasSize(1)
assertThat(actual.first().message).isEqualTo(typeResolutionEnabledMessage)
}
it("doesn't report when using array constructor with spread operator") {
val code = """
fun foo(vararg xs: Int) {}
val testVal = foo(xs = *intArrayOf(1))
"""
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
}
it("doesn't report when using array constructor with spread operator when varargs parameter comes first") {
val code = """
fun <T> asList(vararg ts: T, stringValue: String): List<Int> = listOf(1,2,3)
val list = asList(-1, 0, *arrayOf(1, 2, 3), 4, stringValue = "5")
"""
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
}
it("doesn't report when passing values directly") {
val code = """
fun <T> asList(vararg ts: T, stringValue: String): List<Int> = listOf(1,2,3)
val list = asList(-1, 0, 1, 2, 3, 4, stringValue = "5")
"""
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
}
it("doesn't report when function doesn't take a vararg parameter") {
val code = """
fun test0(strs: Array<String>) {
test(strs)
}
fun test(strs: Array<String>) {
strs.forEach { println(it) }
}
"""
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
}
it("doesn't report with expression inside params") {
val code = """
fun test0(strs: Array<String>) {
test(2*2)
}
fun test(test : Int) {
println(test)
}
"""
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
}
}
context("without type resolution") {
val typeResolutionDisabledMessage = "In most cases using a spread operator causes a full copy of the " +
"array to be created before calling a method which has a very high performance penalty."
it("reports when array copy required using named parameters") {
val code = """
val xsArray = intArrayOf(1)
fun foo(vararg xs: Int) {}
val testVal = foo(xs = *xsArray)
"""
val actual = subject.compileAndLint(code)
assertThat(actual).hasSize(1)
assertThat(actual.first().message).isEqualTo(typeResolutionDisabledMessage)
}
it("reports when array copy required without using named parameters") {
val code = """
val xsArray = intArrayOf(1)
fun foo(vararg xs: Int) {}
val testVal = foo(*xsArray)
"""
val actual = subject.compileAndLint(code)
assertThat(actual).hasSize(1)
assertThat(actual.first().message).isEqualTo(typeResolutionDisabledMessage)
}
it("doesn't report when using array constructor with spread operator") {
val code = """
fun foo(vararg xs: Int) {}
val testVal = foo(xs = *intArrayOf(1))
"""
val actual = subject.compileAndLint(code)
assertThat(actual).hasSize(1)
assertThat(actual.first().message).isEqualTo(typeResolutionDisabledMessage)
}
it("doesn't report when using array constructor with spread operator when varargs parameter comes first") {
val code = """
fun <T> asList(vararg ts: T, stringValue: String): List<Int> = listOf(1,2,3)
val list = asList(-1, 0, *arrayOf(1, 2, 3), 4, stringValue = "5")
"""
val actual = subject.compileAndLint(code)
assertThat(actual).hasSize(1)
assertThat(actual.first().message).isEqualTo(typeResolutionDisabledMessage)
}
it("doesn't report when passing values directly") {
val code = """
fun <T> asList(vararg ts: T, stringValue: String): List<Int> = listOf(1,2,3)
val list = asList(-1, 0, 1, 2, 3, 4, stringValue = "5")
"""
assertThat(subject.compileAndLint(code)).isEmpty()
}
it("doesn't report when function doesn't take a vararg parameter") {
val code = """
fun test0(strs: Array<String>) {
test(strs)
}
fun test(strs: Array<String>) {
strs.forEach { println(it) }
}
"""
assertThat(subject.compileAndLint(code)).isEmpty()
}
it("doesn't report with expression inside params") {
val code = """
fun test0(strs: Array<String>) {
test(2*2)
}
fun test(test : Int) {
println(test)
}
"""
assertThat(subject.compileAndLint(code)).isEmpty()
}
}
}
})

View File

@@ -0,0 +1,23 @@
package io.gitlab.arturbosch.detekt.rules.performance
import io.gitlab.arturbosch.detekt.test.compileAndLint
import org.assertj.core.api.Assertions.assertThat
import org.spekframework.spek2.Spek
import org.spekframework.spek2.style.specification.describe
class UnnecessaryTemporaryInstantiationSpec : Spek({
val subject by memoized { UnnecessaryTemporaryInstantiation() }
describe("UnnecessaryTemporaryInstantiation rule") {
it("temporary instantiation for conversion") {
val code = "val i = Integer(1).toString()"
assertThat(subject.compileAndLint(code)).hasSize(1)
}
it("right conversion without instantiation") {
val code = "val i = Integer.toString(1)"
assertThat(subject.compileAndLint(code)).isEmpty()
}
}
})