From 323e4816d85aee4bd7b951c4ff4e5cb05a36e24f Mon Sep 17 00:00:00 2001 From: Dominic Zirbel Date: Mon, 20 Jun 2022 00:16:29 -0700 Subject: [PATCH] Add CascadingCallWrapping style rule Add a new rule CascadingCallWrapping which requires that if a chained call is placed on a newline then all subsequent calls must be as well, improving readability of long chains. --- config/detekt/detekt.yml | 2 + .../arturbosch/detekt/api/SplitPattern.kt | 3 +- .../main/resources/default-detekt-config.yml | 3 + .../collection/ConfigurationCollector.kt | 4 +- .../collection/MultiRuleCollector.kt | 3 +- .../generator/collection/RuleVisitor.kt | 6 +- .../detekt/extensions/DetektExtension.kt | 3 +- .../rules/complexity/TooManyFunctions.kt | 3 +- .../SuspendFunWithCoroutineScopeReceiver.kt | 5 +- .../detekt/rules/style/CanBeNonNullable.kt | 10 +- .../rules/style/CascadingCallWrapping.kt | 109 +++++++++++ .../detekt/rules/style/ForbiddenVoid.kt | 3 +- .../rules/style/ObjectLiteralToLambda.kt | 3 +- .../SerialVersionUIDInSerializableClass.kt | 3 +- .../detekt/rules/style/StyleGuideProvider.kt | 1 + .../rules/style/CascadingCallWrappingSpec.kt | 178 ++++++++++++++++++ 16 files changed, 324 insertions(+), 15 deletions(-) create mode 100644 detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CascadingCallWrapping.kt create mode 100644 detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CascadingCallWrappingSpec.kt diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index 3f0b319ce07..be1b32e3e4e 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -155,6 +155,8 @@ potential-bugs: style: CanBeNonNullable: active: true + CascadingCallWrapping: + active: true ClassOrdering: active: true CollapsibleIfStatements: diff --git a/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/SplitPattern.kt b/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/SplitPattern.kt index 08411d63e72..1450f2a1164 100644 --- a/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/SplitPattern.kt +++ b/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/SplitPattern.kt @@ -17,7 +17,8 @@ open class SplitPattern( .mapIf(removeTrailingAsterisks) { seq -> seq.map { it.removePrefix("*") } .map { it.removeSuffix("*") } - }.toList() + } + .toList() private fun Sequence.mapIf( condition: Boolean, diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index f7da171eeb3..2218451948a 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -490,6 +490,9 @@ style: active: true CanBeNonNullable: active: false + CascadingCallWrapping: + active: false + includeElvis: true ClassOrdering: active: false CollapsibleIfStatements: diff --git a/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/ConfigurationCollector.kt b/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/ConfigurationCollector.kt index 59d77b874a8..bc7a4b19dd0 100644 --- a/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/ConfigurationCollector.kt +++ b/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/ConfigurationCollector.kt @@ -171,7 +171,9 @@ class ConfigurationCollector { private fun KtValueArgument.getReferenceIdentifierOrNull(): String? = (getArgumentExpression() as? KtCallableReferenceExpression) - ?.callableReference?.getIdentifier()?.text + ?.callableReference + ?.getIdentifier() + ?.text } private object ConfigWithAndroidVariantsSupport { diff --git a/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/MultiRuleCollector.kt b/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/MultiRuleCollector.kt index aaaffa31213..6c7c4d9e6d0 100644 --- a/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/MultiRuleCollector.kt +++ b/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/MultiRuleCollector.kt @@ -64,7 +64,8 @@ class MultiRuleVisitor : DetektVisitor() { override fun visitSuperTypeList(list: KtSuperTypeList) { val isMultiRule = list.entries ?.mapNotNull { it.typeAsUserType?.referencedName } - ?.any { it == multiRule } ?: false + ?.any { it == multiRule } + ?: false val containingClass = list.containingClass() val className = containingClass?.name diff --git a/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/RuleVisitor.kt b/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/RuleVisitor.kt index 679df61d68a..862eb94c0d4 100644 --- a/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/RuleVisitor.kt +++ b/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/RuleVisitor.kt @@ -63,7 +63,8 @@ internal class RuleVisitor : DetektVisitor() { val isRule = list.entries ?.asSequence() ?.map { it.typeAsUserType?.referencedName } - ?.any { ruleClasses.contains(it) } ?: false + ?.any { ruleClasses.contains(it) } + ?: false val containingClass = list.containingClass() val className = containingClass?.name @@ -138,7 +139,8 @@ internal class RuleVisitor : DetektVisitor() { .singleOrNull { it.name == "issue" } ?.initializer as? KtCallExpression ) - ?.valueArguments.orEmpty() + ?.valueArguments + .orEmpty() if (arguments.size >= ISSUE_ARGUMENT_SIZE) { severity = getArgument(arguments[1], "Severity") diff --git a/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/extensions/DetektExtension.kt b/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/extensions/DetektExtension.kt index cd5e24e1d66..2707e2b2f22 100644 --- a/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/extensions/DetektExtension.kt +++ b/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/extensions/DetektExtension.kt @@ -47,7 +47,8 @@ open class DetektExtension @Inject constructor(objects: ObjectFactory) : CodeQua var baseline: File? = objects.fileProperty() .fileValue(File("detekt-baseline.xml")) - .get().asFile + .get() + .asFile var basePath: String? = null diff --git a/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/TooManyFunctions.kt b/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/TooManyFunctions.kt index ed619afadda..d0ed5b44ad1 100644 --- a/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/TooManyFunctions.kt +++ b/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/TooManyFunctions.kt @@ -156,7 +156,8 @@ class TooManyFunctions(config: Config = Config.empty) : Rule(config) { declarations .filterIsInstance() .count { !isIgnoredFunction(it) } - } ?: 0 + } + ?: 0 private fun isIgnoredFunction(function: KtNamedFunction): Boolean = when { ignoreDeprecated && function.hasAnnotation(DEPRECATED) -> true diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunWithCoroutineScopeReceiver.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunWithCoroutineScopeReceiver.kt index 463b1b064e7..5b18e583756 100644 --- a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunWithCoroutineScopeReceiver.kt +++ b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunWithCoroutineScopeReceiver.kt @@ -67,7 +67,10 @@ class SuspendFunWithCoroutineScopeReceiver(config: Config) : Rule(config) { private fun checkReceiver(function: KtNamedFunction) { val suspendModifier = function.modifierList?.getModifier(KtTokens.SUSPEND_KEYWORD) ?: return val receiver = bindingContext[BindingContext.FUNCTION, function] - ?.extensionReceiverParameter?.value?.type ?: return + ?.extensionReceiverParameter + ?.value + ?.type + ?: return if (receiver.isCoroutineScope()) { report( CodeSmell( diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt index ba14ae34d9b..570f7ce22bd 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt @@ -14,7 +14,6 @@ import io.gitlab.arturbosch.detekt.rules.isNonNullCheck import io.gitlab.arturbosch.detekt.rules.isNullCheck import io.gitlab.arturbosch.detekt.rules.isOpen import io.gitlab.arturbosch.detekt.rules.isOverride -import org.jetbrains.kotlin.com.intellij.codeInsight.NullableNotNullManager.isNullable import org.jetbrains.kotlin.descriptors.CallableDescriptor import org.jetbrains.kotlin.descriptors.DeclarationDescriptor import org.jetbrains.kotlin.descriptors.PropertyDescriptor @@ -137,11 +136,13 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) { function.valueParameters.asSequence() .filter { it.typeReference?.typeElement is KtNullableType - }.mapNotNull { parameter -> + } + .mapNotNull { parameter -> bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, parameter]?.let { it to parameter } - }.forEach { (descriptor, param) -> + } + .forEach { (descriptor, param) -> candidateDescriptors.add(descriptor) nullableParams[descriptor] = NullableParam(param) } @@ -175,7 +176,8 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) { .filter { val onlyNonNullCheck = validSingleChildExpression && it.isNonNullChecked && !it.isNullChecked it.isNonNullForced || onlyNonNullCheck - }.forEach { nullableParam -> + } + .forEach { nullableParam -> report( CodeSmell( issue, diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CascadingCallWrapping.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CascadingCallWrapping.kt new file mode 100644 index 00000000000..3dba019a61a --- /dev/null +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CascadingCallWrapping.kt @@ -0,0 +1,109 @@ +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.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.config +import io.gitlab.arturbosch.detekt.api.internal.Configuration +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtQualifiedExpression +import org.jetbrains.kotlin.psi.KtUnaryExpression + +/** + * Requires that all chained calls are placed on a new line if a preceding one is. + * + * + * foo() + * .bar().baz() + * + * + * + * foo().bar().baz() + * + * foo() + * .bar() + * .baz() + * + */ +class CascadingCallWrapping(config: Config = Config.empty) : Rule(config) { + override val issue = Issue( + id = javaClass.simpleName, + severity = Severity.Style, + description = "If a chained call is wrapped to a new line, subsequent chained calls should be as well.", + debt = Debt.FIVE_MINS, + ) + + @Configuration("require trailing elvis expressions to be wrapped on a new line") + private val includeElvis: Boolean by config(true) + + override fun visitQualifiedExpression(expression: KtQualifiedExpression) { + super.visitQualifiedExpression(expression) + + checkExpression(expression, callExpression = expression.selectorExpression) + } + + override fun visitBinaryExpression(expression: KtBinaryExpression) { + super.visitBinaryExpression(expression) + + if (includeElvis && expression.operationToken == KtTokens.ELVIS) { + checkExpression(expression, callExpression = expression.right) + } + } + + private fun checkExpression(expression: KtExpression, callExpression: KtExpression?) { + if (!expression.containsNewline() && expression.receiverContainsNewline()) { + val callTextOrEmpty = callExpression?.text?.let { " `$it`" }.orEmpty() + report( + CodeSmell( + issue = issue, + entity = Entity.from(expression), + message = "Chained call$callTextOrEmpty should be wrapped to a new line since preceding calls were." + ) + ) + } + } + + @Suppress("ReturnCount") + private fun KtExpression.containsNewline(): Boolean { + val lhs: KtExpression + val rhs: KtExpression + + when (this) { + is KtQualifiedExpression -> { + lhs = receiverExpression + rhs = selectorExpression ?: return false + } + is KtBinaryExpression -> { + if (operationToken != KtTokens.ELVIS) return false + lhs = left ?: return false + rhs = right ?: return false + } + else -> return false + } + + val receiverEnd = lhs.startOffsetInParent + lhs.textLength + val selectorStart = rhs.startOffsetInParent + + return (receiverEnd until selectorStart).any { text[it] == '\n' } + } + + private fun KtExpression.receiverContainsNewline(): Boolean { + val lhs = when (this) { + is KtQualifiedExpression -> receiverExpression + is KtBinaryExpression -> left ?: return false + else -> return false + } + + return when (lhs) { + is KtQualifiedExpression -> lhs.containsNewline() + is KtUnaryExpression -> (lhs.baseExpression as? KtQualifiedExpression)?.containsNewline() == true + else -> false + } + } +} diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenVoid.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenVoid.kt index fb3e6502349..6e67b33309a 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenVoid.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenVoid.kt @@ -80,7 +80,8 @@ class ForbiddenVoid(config: Config = Config.empty) : Rule(config) { private fun KtTypeReference.isPartOfReturnTypeOfFunction() = getStrictParentOfType() ?.typeReference - ?.anyDescendantOfType { it == this } ?: false + ?.anyDescendantOfType { it == this } + ?: false private fun KtTypeReference.isParameterTypeOfFunction() = getStrictParentOfType() != null diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambda.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambda.kt index afe2e62c442..c4bbb262527 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambda.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ObjectLiteralToLambda.kt @@ -102,7 +102,8 @@ class ObjectLiteralToLambda(config: Config = Config.empty) : Rule(config) { if ( declaration.name == null && bindingContext.getType(expression) - ?.singleSuperTypeOrNull()?.couldBeSamInterface == true && + ?.singleSuperTypeOrNull() + ?.couldBeSamInterface == true && declaration.hasConvertibleMethod() ) { report(CodeSmell(issue, Entity.from(expression), issue.description)) diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/SerialVersionUIDInSerializableClass.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/SerialVersionUIDInSerializableClass.kt index 644c95d1566..ad645ea96fe 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/SerialVersionUIDInSerializableClass.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/SerialVersionUIDInSerializableClass.kt @@ -100,7 +100,8 @@ class SerialVersionUIDInSerializableClass(config: Config = Config.empty) : Rule( private fun hasLongAssignment(property: KtProperty): Boolean { val assignmentText = property.children - .singleOrNull { it is KtConstantExpression || it is KtPrefixExpression }?.text + .singleOrNull { it is KtConstantExpression || it is KtPrefixExpression } + ?.text return assignmentText != null && assignmentText.last() == 'L' && assignmentText.substring(0, assignmentText.length - 1).toLongOrNull() != null } diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt index 7d6cbc250cc..981e73134c6 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt @@ -24,6 +24,7 @@ class StyleGuideProvider : DefaultRuleSetProvider { ruleSetId, listOf( CanBeNonNullable(config), + CascadingCallWrapping(config), ClassOrdering(config), CollapsibleIfStatements(config), DestructuringDeclarationWithTooManyEntries(config), diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CascadingCallWrappingSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CascadingCallWrappingSpec.kt new file mode 100644 index 00000000000..a9c141253c9 --- /dev/null +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CascadingCallWrappingSpec.kt @@ -0,0 +1,178 @@ +package io.gitlab.arturbosch.detekt.rules.style + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.test.TestConfig +import io.gitlab.arturbosch.detekt.test.assertThat +import io.gitlab.arturbosch.detekt.test.compileAndLint +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test + +class CascadingCallWrappingSpec { + private val subject = CascadingCallWrapping(Config.empty) + + @Test + fun `reports missing wrapping`() { + val code = """ + val a = 0 + .plus(0).plus(0).plus(0) + """ + + assertThat(subject.compileAndLint(code)) + .hasSize(1) + .hasTextLocations(8 to 30) + .first() + .hasMessage("Chained call `plus(0)` should be wrapped to a new line since preceding calls were.") + } + + @Test + fun `does not report when chained calls are on a single line`() { + val code = """ + val a = 0.plus(0).plus(0) + """ + + assertThat(subject.compileAndLint(code)).isEmpty() + } + + @Test + fun `does not report wrapped calls`() { + val code = """ + val a = 0 + .plus(0) + .plus(0) + """ + + assertThat(subject.compileAndLint(code)).isEmpty() + } + + @Test + fun `does not report unwrapped initial calls`() { + val code = """ + val a = 0.plus(0).plus(0) + .plus(0) + .plus(0) + """ + + assertThat(subject.compileAndLint(code)).isEmpty() + } + + @Test + fun `reports missing wrapping for safe qualified calls`() { + val code = """ + val a = 0 + ?.plus(0)?.plus(0) + """ + + assertThat(subject.compileAndLint(code)).hasSize(1) + } + + @Test + fun `reports missing wrapping for calls with non-null assertions`() { + val code = """ + val a = 0!! + .plus(0)!!.plus(0) + """ + + assertThat(subject.compileAndLint(code)).hasSize(1) + } + + @Test + fun `reports missing wrapping for properties`() { + val code = """ + val a = "" + .plus("").length + + val b = "" + .length.plus(0) + """ + + assertThat(subject.compileAndLint(code)).hasSize(2) + } + + @Nested + inner class `with multiline calls` { + @Test + fun `does not report with wrapping`() { + val code = """ + val a = 0 + .plus( + 0 + ) + .let { + 0 + } + """ + + assertThat(subject.compileAndLint(code)).isEmpty() + } + + @Test + fun `reports missing wrapping`() { + val code = """ + val a = 0 + .plus( + 0 + ) + .let { + 0 + }.plus( + 0 + ) + """ + + assertThat(subject.compileAndLint(code)).hasSize(1) + } + + @Test + fun `does not report when calls are multiline but never wrapped`() { + val code = """ + val a = 0.plus( + 0 + ).let { + 0 + } + """ + + assertThat(subject.compileAndLint(code)).isEmpty() + } + + @Test + fun `does not report for single multiline call`() { + val code = """ + val a = 0.plus( + 0 + ) + """ + + assertThat(subject.compileAndLint(code)).isEmpty() + } + } + + @Nested + inner class `with elvis operators` { + private val subjectIncludingElvis = CascadingCallWrapping(TestConfig(mapOf("includeElvis" to true))) + private val subjectExcludingElvis = CascadingCallWrapping(TestConfig(mapOf("includeElvis" to false))) + + @Test + fun `does not report with wrapping`() { + val code = """ + val a = 0 + .plus(0) + ?: 0 + """ + + assertThat(subjectIncludingElvis.compileAndLint(code)).isEmpty() + assertThat(subjectExcludingElvis.compileAndLint(code)).isEmpty() + } + + @Test + fun `reports missing wrapping`() { + val code = """ + val a = 0 + .plus(0) ?: 0 + """ + + assertThat(subjectIncludingElvis.compileAndLint(code)).hasSize(1) + assertThat(subjectExcludingElvis.compileAndLint(code)).isEmpty() + } + } +}