From 73cdc6fa9a48edb8518fbc8274c9e30f2c4c2661 Mon Sep 17 00:00:00 2001 From: Alexander Tsay <48321920+aktsay6@users.noreply.github.com> Date: Fri, 19 Mar 2021 18:57:50 +0300 Subject: [PATCH] Bugfix. EMPTY_BLOCK_STRUCTURE_ERROR shouldn't trigger on empty lambdas (#803) ### What's done: * Fixed bug --- .../ruleset/rules/chapter3/EmptyBlock.kt | 37 ++++++++++++++++--- .../cqfn/diktat/ruleset/utils/KotlinParser.kt | 4 +- .../ruleset/chapter3/EmptyBlockWarnTest.kt | 36 ++++++++++++++++++ info/guide/guide-chapter-3.md | 4 +- 4 files changed, 72 insertions(+), 9 deletions(-) diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/EmptyBlock.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/EmptyBlock.kt index f0405440c5..8a1dd69960 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/EmptyBlock.kt +++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/EmptyBlock.kt @@ -9,13 +9,21 @@ import org.cqfn.diktat.ruleset.utils.* import com.pinterest.ktlint.core.ast.ElementType import com.pinterest.ktlint.core.ast.ElementType.CALL_EXPRESSION +import com.pinterest.ktlint.core.ast.ElementType.DOT_QUALIFIED_EXPRESSION +import com.pinterest.ktlint.core.ast.ElementType.FILE import com.pinterest.ktlint.core.ast.ElementType.FUNCTION_LITERAL import com.pinterest.ktlint.core.ast.ElementType.IDENTIFIER +import com.pinterest.ktlint.core.ast.ElementType.LAMBDA_EXPRESSION +import com.pinterest.ktlint.core.ast.ElementType.LPAR import com.pinterest.ktlint.core.ast.ElementType.RBRACE +import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT +import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT_LIST +import com.pinterest.ktlint.core.ast.ElementType.VALUE_PARAMETER import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE import org.jetbrains.kotlin.com.intellij.lang.ASTNode import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.PsiWhiteSpaceImpl +import org.jetbrains.kotlin.psi.psiUtil.parents /** * Rule that checks if empty code blocks (`{ }`) are used and checks their formatting. @@ -41,7 +49,7 @@ class EmptyBlock(configRules: List) : DiktatRule( @Suppress("UnsafeCallOnNullableType", "TOO_LONG_FUNCTION") private fun checkEmptyBlock(node: ASTNode, configuration: EmptyBlockStyleConfiguration) { - if (node.treeParent.isOverridden() || isAnonymousSamClass(node)) { + if (node.treeParent.isOverridden() || isAnonymousSamClass(node) || isLambdaUsedAsFunction(node)) { return } if (node.isBlockEmpty()) { @@ -82,11 +90,11 @@ class EmptyBlock(configRules: List) : DiktatRule( } } - @Suppress("UnsafeCallOnNullableType", "WRONG_WHITESPACE") - private fun isAnonymousSamClass(node: ASTNode) : Boolean = + @Suppress("UnsafeCallOnNullableType") + private fun isAnonymousSamClass(node: ASTNode): Boolean = if (node.elementType == FUNCTION_LITERAL && node.hasParent(CALL_EXPRESSION)) { - // We are checking identifier because it is not class in AST - // , SAM conversions are indistinguishable from lambdas. + // We are checking identifier because it is not class in AST, + // SAM conversions are indistinguishable from lambdas. // So we just verify that identifier is in PascalCase val valueArgument = node.findParentNodeWithSpecificType(CALL_EXPRESSION)!! valueArgument.findLeafWithSpecificType(IDENTIFIER)?.text?.isPascalCase() ?: false @@ -94,6 +102,25 @@ class EmptyBlock(configRules: List) : DiktatRule( false } + @Suppress("UnsafeCallOnNullableType") + private fun isLambdaUsedAsFunction(node: ASTNode): Boolean { + val parents = node.parents() + return when { + parents.any { it.elementType == CALL_EXPRESSION } -> { + val callExpression = parents.find { it.elementType == CALL_EXPRESSION }!! + // excepting cases like list.map { }. In this case call expression will not have value argument list + // And in this case: Parser.parse({}, some, thing) it will have value argument list + callExpression.hasChildOfType(VALUE_ARGUMENT_LIST) + } + parents.any { it.elementType == LAMBDA_EXPRESSION } -> { + val lambdaExpression = parents.find { it.elementType == LAMBDA_EXPRESSION }!! + // cases like A({}). Here Lambda expression is used as a value parameter. + lambdaExpression.treeParent.elementType == VALUE_PARAMETER + } + else -> false + } + } + /** * [RuleConfiguration] for empty blocks formatting */ diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/KotlinParser.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/KotlinParser.kt index 3a994703f7..abeb23661b 100644 --- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/KotlinParser.kt +++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/KotlinParser.kt @@ -10,7 +10,6 @@ import org.jetbrains.kotlin.cli.jvm.compiler.EnvironmentConfigFiles import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment import org.jetbrains.kotlin.com.intellij.lang.ASTNode import org.jetbrains.kotlin.com.intellij.mock.MockProject -import org.jetbrains.kotlin.com.intellij.openapi.Disposable import org.jetbrains.kotlin.com.intellij.openapi.project.Project import org.jetbrains.kotlin.com.intellij.openapi.util.UserDataHolderBase import org.jetbrains.kotlin.com.intellij.pom.PomModel @@ -49,8 +48,7 @@ class KotlinParser { return null } } // I don't really understand what's going on here, but thanks to this, you can use this node in the future - val project = KotlinCoreEnvironment.createForProduction( - Disposable { }, + val project = KotlinCoreEnvironment.createForProduction({}, compilerConfiguration, EnvironmentConfigFiles.JVM_CONFIG_FILES ).project // create project diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/EmptyBlockWarnTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/EmptyBlockWarnTest.kt index 66e82eb25e..c2a87615f0 100644 --- a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/EmptyBlockWarnTest.kt +++ b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/EmptyBlockWarnTest.kt @@ -8,6 +8,8 @@ import org.cqfn.diktat.util.LintTestBase import com.pinterest.ktlint.core.LintError import generated.WarningNames +import org.jetbrains.kotlin.cli.jvm.compiler.EnvironmentConfigFiles +import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment import org.junit.jupiter.api.Tag import org.junit.jupiter.api.Test @@ -204,4 +206,38 @@ class EmptyBlockWarnTest : LintTestBase(::EmptyBlock) { rulesConfigList = rulesConfigListEmptyBlockExist ) } + + @Test + @Tag(WarningNames.EMPTY_BLOCK_STRUCTURE_ERROR) + fun `should not trigger on empty lambdas as a functions`() { + lintMethod( + """ + |fun foo(bar: () -> Unit = {}) + | + |class Some { + | fun bar() { + | A({}) + | } + |} + """.trimMargin(), + rulesConfigList = rulesConfigListEmptyBlockExist + ) + } + + @Test + @Tag(WarningNames.EMPTY_BLOCK_STRUCTURE_ERROR) + fun `should not trigger on empty lambdas as a functions #2`() { + lintMethod( + """ + |fun some() { + | val project = KotlinCoreEnvironment.createForProduction( + | { }, + | compilerConfiguration, + | EnvironmentConfigFiles.JVM_CONFIG_FILES + | ).project + |} + """.trimMargin(), + rulesConfigList = rulesConfigListEmptyBlockExist + ) + } } diff --git a/info/guide/guide-chapter-3.md b/info/guide/guide-chapter-3.md index e764e325af..69d855e7c5 100644 --- a/info/guide/guide-chapter-3.md +++ b/info/guide/guide-chapter-3.md @@ -305,7 +305,7 @@ class A Avoid empty blocks, and ensure braces start on a new line. An empty code block can be closed immediately on the same line and the next line. However, a newline is recommended between opening and closing braces `{}` (see the examples below.) Generally, empty code blocks are prohibited; using them is considered a bad practice (especially for catch block). -They are only appropriate for overridden functions when the base class's functionality is not needed in the class-inheritor. +They are appropriate for overridden functions, when the base class's functionality is not needed in the class-inheritor, for lambdas used as a function and for empty function in implementation of functional interface. ```kotlin override fun foo() { } @@ -318,6 +318,8 @@ fun doNothing() {} fun doNothingElse() { } + +fun foo(bar: () -> Unit = {}) ``` **Invalid examples:**