From 435e4990ca815eac996ff559c9ebc5ccadbfd6c2 Mon Sep 17 00:00:00 2001 From: Daria Pleshkova <81246075+diphtongue@users.noreply.github.com> Date: Wed, 27 Dec 2023 15:11:04 +0300 Subject: [PATCH] Fixed false positive findings [PARAMETER_NAME_IN_OUTER_LAMBDA] (#1891) * ### What's done: - Fixed false positive findings when lambda operation at the same level - Added warn tests Closes #1880 --- diktat-analysis.yml | 2 + .../ParameterNameInOuterLambdaRule.kt | 42 +++++++- .../diktat/ruleset/utils/AstNodeUtils.kt | 69 +++++++----- .../main/resources/diktat-analysis-huawei.yml | 2 + .../src/main/resources/diktat-analysis.yml | 2 + .../ParameterNameInOuterLambdaRuleWarnTest.kt | 101 ++++++++++++++++++ 6 files changed, 189 insertions(+), 29 deletions(-) diff --git a/diktat-analysis.yml b/diktat-analysis.yml index 824f904060..76d4e0af68 100644 --- a/diktat-analysis.yml +++ b/diktat-analysis.yml @@ -451,6 +451,8 @@ # Checks that outer lambda has explicit parameter name - name: PARAMETER_NAME_IN_OUTER_LAMBDA enabled: true + configuration: + strictMode: true # don't let outer lambdas have `it` as parameter # Checks that property in constructor doesn't contain comment - name: KDOC_NO_CONSTRUCTOR_PROPERTY enabled: true diff --git a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter5/ParameterNameInOuterLambdaRule.kt b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter5/ParameterNameInOuterLambdaRule.kt index a79e3cb82c..333c908e67 100644 --- a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter5/ParameterNameInOuterLambdaRule.kt +++ b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter5/ParameterNameInOuterLambdaRule.kt @@ -1,10 +1,15 @@ package com.saveourtool.diktat.ruleset.rules.chapter5 +import com.saveourtool.diktat.common.config.rules.RuleConfiguration import com.saveourtool.diktat.common.config.rules.RulesConfig +import com.saveourtool.diktat.common.config.rules.getRuleConfig import com.saveourtool.diktat.ruleset.constants.Warnings.PARAMETER_NAME_IN_OUTER_LAMBDA import com.saveourtool.diktat.ruleset.rules.DiktatRule import com.saveourtool.diktat.ruleset.utils.doesLambdaContainIt import com.saveourtool.diktat.ruleset.utils.findAllDescendantsWithSpecificType +import com.saveourtool.diktat.ruleset.utils.hasExplicitIt +import com.saveourtool.diktat.ruleset.utils.hasItInLambda +import com.saveourtool.diktat.ruleset.utils.hasNoParameters import org.jetbrains.kotlin.KtNodeTypes import org.jetbrains.kotlin.com.intellij.lang.ASTNode @@ -17,6 +22,15 @@ class ParameterNameInOuterLambdaRule(configRules: List) : DiktatRul configRules, listOf(PARAMETER_NAME_IN_OUTER_LAMBDA) ) { + /** + * Configuration for the rule ParameterNameInOuterLambda + */ + private val configuration by lazy { + ParameterNameInOuterLambdaConfiguration( + configRules.getRuleConfig(PARAMETER_NAME_IN_OUTER_LAMBDA)?.configuration + ?: emptyMap()) + } + override fun logic(node: ASTNode) { if (node.elementType == KtNodeTypes.LAMBDA_EXPRESSION) { checkLambda(node) @@ -24,9 +38,18 @@ class ParameterNameInOuterLambdaRule(configRules: List) : DiktatRul } private fun checkLambda(node: ASTNode) { - val hasInnerLambda = node - .findAllDescendantsWithSpecificType(KtNodeTypes.LAMBDA_EXPRESSION, false) - .isNotEmpty() + val strictMode = configuration.strictMode + + val innerLambdaList = node.findAllDescendantsWithSpecificType(KtNodeTypes.LAMBDA_EXPRESSION, false) + val hasInnerLambda = innerLambdaList.isNotEmpty() + + val outerLambdaHasNoParameterName = hasNoParameters(node) || node.hasExplicitIt() + val innerLambdasHasNoIt = !hasInnerLambda || + innerLambdaList.all { innerLambda -> !hasItInLambda(innerLambda) } + + if (!strictMode && outerLambdaHasNoParameterName && innerLambdasHasNoIt) { + return + } if (hasInnerLambda && doesLambdaContainIt(node)) { PARAMETER_NAME_IN_OUTER_LAMBDA.warn( configRules, emitWarn, @@ -35,6 +58,19 @@ class ParameterNameInOuterLambdaRule(configRules: List) : DiktatRul ) } } + + /** + * ParameterNameInOuterLambdaConfiguration used when we need to allow the usage of 'it' in outer lambda + * + * @param config - map of strings with configuration options for a Parameter Name In Outer Lambda rule + */ + class ParameterNameInOuterLambdaConfiguration(config: Map) : RuleConfiguration(config) { + /** + * Flag (when false) allows to use `it` in outer lambda, if in inner lambdas would be no `it` + */ + val strictMode = config["strictMode"]?.toBoolean() ?: true + } + companion object { const val NAME_ID = "parameter-name-in-outer-lambda" } diff --git a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/utils/AstNodeUtils.kt b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/utils/AstNodeUtils.kt index 55f293516f..454560937f 100644 --- a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/utils/AstNodeUtils.kt +++ b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/utils/AstNodeUtils.kt @@ -1014,6 +1014,22 @@ fun ASTNode.isPrivate(): Boolean = this.getFirstChildWithType(MODIFIER_LIST)?.ge */ fun ASTNode.hasSetterOrGetter(): Boolean = this.getFirstChildWithType(KtNodeTypes.PROPERTY_ACCESSOR) != null +/** + * Checks if ASTNode has parameter `it` + * + * @return true if this ASTNode has parameter `it` + */ +fun ASTNode.hasExplicitIt(): Boolean { + require(elementType == LAMBDA_EXPRESSION) { "Method can be called only for lambda" } + val parameterList = findChildByType(FUNCTION_LITERAL) + ?.findChildByType(VALUE_PARAMETER_LIST) + ?.psi + as KtParameterList? + return parameterList?.parameters + ?.any { it.name == "it" } + ?: false +} + private fun ASTNode.isFollowedByNewlineCheck() = this.treeNext.elementType == WHITE_SPACE && this.treeNext.text.contains("\n") @@ -1115,17 +1131,6 @@ private fun ASTNode.calculateLineNumber() = getRootNode() index + 1 } -private fun ASTNode.hasExplicitIt(): Boolean { - require(elementType == LAMBDA_EXPRESSION) { "Method can be called only for lambda" } - val parameterList = findChildByType(FUNCTION_LITERAL) - ?.findChildByType(VALUE_PARAMETER_LIST) - ?.psi - as KtParameterList? - return parameterList?.parameters - ?.any { it.name == "it" } - ?: false -} - /** * Gets list of property nodes * @@ -1163,14 +1168,10 @@ fun countCodeLines(node: ASTNode): Int { /** * Check that lambda contains `it`. * - * Note: it checks only provided lambda and inner lambda with explicit parameters - * - * Note: this method can be called only for lambda + * @param lambdaNode ASTNode with type LAMBDA_EXPRESSION + * @return true if `it` contains in lambdaNode.text */ -@Suppress("FUNCTION_BOOLEAN_PREFIX") -fun doesLambdaContainIt(lambdaNode: ASTNode): Boolean { - require(lambdaNode.elementType == LAMBDA_EXPRESSION) { "Method can be called only for lambda" } - +fun hasItInLambda(lambdaNode: ASTNode): Boolean { val excludeChildrenCondition = { node: ASTNode -> node.elementType == LAMBDA_EXPRESSION && (hasNoParameters(node) || node.hasExplicitIt()) } @@ -1181,7 +1182,30 @@ fun doesLambdaContainIt(lambdaNode: ASTNode): Boolean { .map { it.text } .contains("it") - return hasNoParameters(lambdaNode) && hasIt + return hasIt +} + +/** + * @param lambdaNode + * @return true if lambdaNode has no parameters and has `it` in lambdaNode.text + */ +@Suppress("FUNCTION_BOOLEAN_PREFIX") +fun doesLambdaContainIt(lambdaNode: ASTNode): Boolean { + require(lambdaNode.elementType == LAMBDA_EXPRESSION) { "Method can be called only for lambda" } + return hasNoParameters(lambdaNode) && hasItInLambda(lambdaNode) +} + +/** + * Checks that lambda has no parameters + * + * @param lambdaNode ASTNode with type LAMBDA_EXPRESSION + * @return true if node has no parameters + */ +fun hasNoParameters(lambdaNode: ASTNode): Boolean { + require(lambdaNode.elementType == LAMBDA_EXPRESSION) { "Method can be called only for lambda" } + return null == lambdaNode + .findChildByType(FUNCTION_LITERAL) + ?.findChildByType(VALUE_PARAMETER_LIST) } /** @@ -1254,10 +1278,3 @@ private fun hasAnySuppressorForInspection( foundSuppress || foundIgnoredAnnotation || isCompletelyIgnoredBlock } - -private fun hasNoParameters(lambdaNode: ASTNode): Boolean { - require(lambdaNode.elementType == LAMBDA_EXPRESSION) { "Method can be called only for lambda" } - return null == lambdaNode - .findChildByType(FUNCTION_LITERAL) - ?.findChildByType(VALUE_PARAMETER_LIST) -} diff --git a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml b/diktat-rules/src/main/resources/diktat-analysis-huawei.yml index 4db7d3932c..e9c1e7b54b 100644 --- a/diktat-rules/src/main/resources/diktat-analysis-huawei.yml +++ b/diktat-rules/src/main/resources/diktat-analysis-huawei.yml @@ -448,6 +448,8 @@ # Check that lambda with inner lambda doesn't use implicit parameter - name: PARAMETER_NAME_IN_OUTER_LAMBDA enabled: true + configuration: + strictMode: true # don't let outer lambdas have `it` as parameter # Checks that property in constructor doesn't contains comment - name: KDOC_NO_CONSTRUCTOR_PROPERTY enabled: true diff --git a/diktat-rules/src/main/resources/diktat-analysis.yml b/diktat-rules/src/main/resources/diktat-analysis.yml index 59015a6785..aa9b009323 100644 --- a/diktat-rules/src/main/resources/diktat-analysis.yml +++ b/diktat-rules/src/main/resources/diktat-analysis.yml @@ -455,6 +455,8 @@ # Check that lambda with inner lambda doesn't use implicit parameter - name: PARAMETER_NAME_IN_OUTER_LAMBDA enabled: true + configuration: + strictMode: true # don't let outer lambdas have `it` as parameter # Checks that property in KDoc present in class - name: KDOC_EXTRA_PROPERTY enabled: true diff --git a/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter5/ParameterNameInOuterLambdaRuleWarnTest.kt b/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter5/ParameterNameInOuterLambdaRuleWarnTest.kt index fe6a8c3c29..f950a12761 100644 --- a/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter5/ParameterNameInOuterLambdaRuleWarnTest.kt +++ b/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter5/ParameterNameInOuterLambdaRuleWarnTest.kt @@ -16,6 +16,12 @@ class ParameterNameInOuterLambdaRuleWarnTest : LintTestBase(::ParameterNameInOut private val rulesConfigList: List = listOf( RulesConfig(Warnings.PARAMETER_NAME_IN_OUTER_LAMBDA.name, true) ) + private val rulesConfigParameterNameInOuterLambda = listOf( + RulesConfig( + Warnings.PARAMETER_NAME_IN_OUTER_LAMBDA.name, true, mapOf( + "strictMode" to "false" + )) + ) @Test @Tag(WarningNames.PARAMETER_NAME_IN_OUTER_LAMBDA) @@ -197,4 +203,99 @@ class ParameterNameInOuterLambdaRuleWarnTest : LintTestBase(::ParameterNameInOut rulesConfigList = rulesConfigList ) } + + @Test + @Tag(WarningNames.PARAMETER_NAME_IN_OUTER_LAMBDA) + fun `lambdas at the same level`() { + lintMethod( + """ + |fun testA() { + | val overrideFunctions: List = emptyList() + | overrideFunctions.forEach { functionNameMap.compute(it.getIdentifierName()!!.text) { _, oldValue -> (oldValue ?: 0) + 1 } } + |} + """.trimMargin(), + rulesConfigList = this.rulesConfigParameterNameInOuterLambda + ) + } + + @Test + @Tag(WarningNames.PARAMETER_NAME_IN_OUTER_LAMBDA) + fun `lambdas at the same level 2`() { + lintMethod( + """ + |private fun isCheckNeeded(whiteSpace: PsiWhiteSpace) = + | whiteSpace.parent + | .node + | .elementType + | .let { it == VALUE_PARAMETER_LIST || it == VALUE_ARGUMENT_LIST } && + | whiteSpace.siblings(forward = false, withItself = false).none { it is PsiWhiteSpace && it.textContains('\n') } && + | whiteSpace.siblings(forward = true, withItself = false).any { + | it.node.elementType.run { this == VALUE_ARGUMENT || this == VALUE_PARAMETER } + | } + """.trimMargin(), + rulesConfigList = this.rulesConfigParameterNameInOuterLambda + ) + } + + @Test + @Tag(WarningNames.PARAMETER_NAME_IN_OUTER_LAMBDA) + fun `lambdas at the same level 21`() { + lintMethod( + """ + |private fun isCheckNeeded(w: PsiWhiteSpace, h: PsiWhiteSpace) = + | w.let { it == VALUE_PARAMETER_LIST || it == VALUE_ARGUMENT_LIST } && + | h.none { it is PsiWhiteSpace && it.textContains('\n') } && + | h.any { + | it.node.elementType.run { this == VALUE_ARGUMENT || this == VALUE_PARAMETER } + | } + """.trimMargin(), + rulesConfigList = this.rulesConfigParameterNameInOuterLambda + ) + } + + @Test + @Tag(WarningNames.PARAMETER_NAME_IN_OUTER_LAMBDA) + fun `lambdas at the same level 3`() { + lintMethod( + """ + |private fun checkBlankLineAfterKdoc(node: ASTNode) { + | commentType.forEach { + | val kdoc = node.getFirstChildWithType(it) + | kdoc?.treeNext?.let { nodeAfterKdoc -> + | if (nodeAfterKdoc.elementType == WHITE_SPACE && nodeAfterKdoc.numNewLines() > 1) { + | WRONG_NEWLINES_AROUND_KDOC.warnAndFix(configRules, emitWarn, isFixMode, "redundant blank line after ${'$'}{kdoc.text}", nodeAfterKdoc.startOffset, nodeAfterKdoc) { + | nodeAfterKdoc.leaveOnlyOneNewLine() + | } + | } + | } + | } + |} + """.trimMargin(), + rulesConfigList = this.rulesConfigParameterNameInOuterLambda + ) + } + + @Test + @Tag(WarningNames.PARAMETER_NAME_IN_OUTER_LAMBDA) + fun `lambdas at the same level 4`() { + lintMethod( + """ + |private fun KSAnnotation.getArgumentValue(argumentName: String): String = arguments + | .singleOrNull { it.name?.asString() == argumentName } + | .let { + | requireNotNull(it) { + | "Not found ${'$'}argumentName in ${'$'}this" + | } + | } + | .value + | ?.let { it as? String } + | .let { + | requireNotNull(it) { + | "Not found a value for ${'$'}argumentName in ${'$'}this" + | } + | } + """.trimMargin(), + rulesConfigList = this.rulesConfigParameterNameInOuterLambda + ) + } }