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

Bugfix. EMPTY_BLOCK_STRUCTURE_ERROR shouldn't trigger on empty lambdas #803

Merged
merged 8 commits into from
Mar 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -41,7 +49,7 @@ class EmptyBlock(configRules: List<RulesConfig>) : 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)) {
aktsay6 marked this conversation as resolved.
Show resolved Hide resolved
return
}
if (node.isBlockEmpty()) {
Expand Down Expand Up @@ -82,18 +90,37 @@ class EmptyBlock(configRules: List<RulesConfig>) : 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
} else {
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
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
)
}
}
4 changes: 3 additions & 1 deletion info/guide/guide-chapter-3.md
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
}
Expand All @@ -318,6 +318,8 @@ fun doNothing() {}

fun doNothingElse() {
}

fun foo(bar: () -> Unit = {})
```

**Invalid examples:**
Expand Down