Skip to content

Commit

Permalink
Do not place an annotation on a value argument, value parameter, type…
Browse files Browse the repository at this point in the history 
… parameter, or type projection in case it is a binary expression (pinterest#2463)

Make creation of the annotation more safe by not using a fake top element in the code which is used to generate the AST.

The 'Suppress' annotation has a parameter, which by default is not allowed to be placed inline (it should start on a new line). Just putting it above the binary expression results in it being applied to the first subexpression only instead of on the entire binary expression.

Closes pinterest#2462
  • Loading branch information
@paul-dingemans
paul-dingemans committed Jan 2, 2024
1 parent 3819251 commit 0a78a47
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 26 deletions.
60 changes: 34 additions & 26 deletions ...ule-engine/src/main/kotlin/com/pinterest/ktlint/rule/engine/internal/KtlintSuppression.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import org.jetbrains.kotlin.com.intellij.psi.tree.TokenSet
import org.jetbrains.kotlin.idea.KotlinLanguage
import org.jetbrains.kotlin.psi.KtAnnotatedExpression
import org.jetbrains.kotlin.psi.KtAnnotationEntry
import org.jetbrains.kotlin.psi.KtBinaryExpression
import org.jetbrains.kotlin.psi.KtBlockExpression
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtClassInitializer
Expand All @@ -43,6 +44,7 @@ import org.jetbrains.kotlin.psi.KtPrimaryConstructor
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.KtPropertyAccessor
import org.jetbrains.kotlin.psi.KtScript
import org.jetbrains.kotlin.psi.KtScriptInitializer
import org.jetbrains.kotlin.psi.KtStringTemplateExpression
import org.jetbrains.kotlin.psi.psiUtil.children
import org.jetbrains.kotlin.psi.psiUtil.findDescendantOfType
Expand Down Expand Up @@ -103,51 +105,59 @@ private fun ASTNode.findParentDeclarationOrExpression(forceFileAnnotation: Boole
return this
}

var targetNode = psi
var targetPsiElement = psi
var isAnnotationForBinaryExpression = false
while (
forceFileAnnotation ||
targetNode is KtClassInitializer ||
targetNode is KtBlockExpression ||
targetNode is KtPrimaryConstructor ||
targetNode is KtFunctionLiteral ||
targetNode is KtLambdaExpression ||
targetNode.parent is KtStringTemplateExpression ||
targetPsiElement is KtClassInitializer ||
targetPsiElement is KtBinaryExpression ||
targetPsiElement is KtBlockExpression ||
targetPsiElement is KtPrimaryConstructor ||
targetPsiElement is KtFunctionLiteral ||
targetPsiElement is KtLambdaExpression ||
targetPsiElement.parent is KtStringTemplateExpression ||
(
targetNode is KtExpression &&
targetNode.parent is KtExpression &&
targetNode.parent !is KtBlockExpression &&
targetNode.parent !is KtDeclaration
targetPsiElement is KtExpression &&
targetPsiElement.parent is KtExpression &&
targetPsiElement.parent !is KtBlockExpression &&
targetPsiElement.parent !is KtDeclaration
) ||
(targetNode !is KtDeclaration && targetNode !is KtExpression)
(targetPsiElement !is KtDeclaration && targetPsiElement !is KtExpression)
) {
targetNode =
if (targetPsiElement is KtBinaryExpression) {
isAnnotationForBinaryExpression = true
}
targetPsiElement =
when {
targetNode.parent == null -> {
targetPsiElement.parent == null -> {
// Prevents null pointer when already at a root node
return targetNode.node
return targetPsiElement.node
}

targetNode.node.elementType in listElementTypeTokenSet -> {
targetPsiElement.node.elementType in listElementTypeTokenSet && !isAnnotationForBinaryExpression -> {
// If a suppression is added to an inner element of a list element, then the annotation should be put on that element
return targetNode.node
// unless this is binary expression. Inserting the annotation on the root of the binary expression would result in
// applying that annotation only on the left hand side of the root expression instead of on the entire binary
// expression.
return targetPsiElement.node
}

targetNode.isIgnorableListElement() -> {
targetPsiElement.isIgnorableListElement() -> {
// If a suppression is added on a direct child of the list type but not inside in a list element then the annotation is
// moved to the next list element. When no such element is found, it will be moved to the parent of the list type
targetNode
targetPsiElement
.node
.nextCodeSibling()
?.firstChildLeafOrSelf()
?.psi
}

else -> {
targetNode
targetPsiElement
}
}?.parent
}
return targetNode.node
return targetPsiElement.node
}

private val listTypeTokenSet = TokenSet.create(TYPE_ARGUMENT_LIST, TYPE_PARAMETER_LIST, VALUE_ARGUMENT_LIST, VALUE_PARAMETER_LIST)
Expand Down Expand Up @@ -229,9 +239,9 @@ private fun ASTNode.suppressionAnnotationTypeOrNull() =
?.let { SuppressAnnotationType.findByIdOrNull(it) }

private fun ASTNode.getValueArguments() =
findChildByType(ElementType.VALUE_ARGUMENT_LIST)
findChildByType(VALUE_ARGUMENT_LIST)
?.children()
?.filter { it.elementType == ElementType.VALUE_ARGUMENT }
?.filter { it.elementType == VALUE_ARGUMENT }
?.map { it.text }
?.toSet()
.orEmpty()
Expand Down Expand Up @@ -363,15 +373,13 @@ private fun ASTNode.createAnnotatedExpression(
.getInstance(psi.project)
.createFileFromText(
KotlinLanguage.INSTANCE,
// Create the annotation for a dummy declaration as the entire code block should be valid Kotlin code
"""
|fun foo() =
|${this.indent(false)}$annotation
|${this.indent(false)}${this.text}
""".trimMargin(),
).getChildOfType<KtScript>()
?.getChildOfType<KtBlockExpression>()
?.getChildOfType<KtNamedFunction>()
?.getChildOfType<KtScriptInitializer>()
?.getChildOfType<KtAnnotatedExpression>()
?: throw IllegalStateException("Can not create annotation '$annotation'")
}
Expand Down
90 changes: 90 additions & 0 deletions ...src/test/kotlin/com/pinterest/ktlint/rule/engine/api/KtlintRuleEngineSuppressionKtTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ package com.pinterest.ktlint.rule.engine.api
import com.pinterest.ktlint.rule.engine.core.api.Rule
import com.pinterest.ktlint.rule.engine.core.api.RuleId
import com.pinterest.ktlint.rule.engine.core.api.RuleProvider
import com.pinterest.ktlint.ruleset.standard.rules.CONDITION_WRAPPING_RULE_ID
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Nested
import org.junit.jupiter.api.Test
import org.junit.jupiter.params.ParameterizedTest
import org.junit.jupiter.params.provider.ValueSource
Expand Down Expand Up @@ -185,6 +187,94 @@ class KtlintRuleEngineSuppressionKtTest {
assertThat(actual).isEqualTo(formattedCode)
}

@Nested
inner class `Issue 2462 - Given binary expression on which a suppression is added` {
@Test
fun `Given a value argument with a multiline condition to which a suppression is to be added`() {
val code =
"""
fun foo() {
require(
true && false ||
true,
)
}
""".trimIndent()
val formattedCode =
"""
fun foo() {
@Suppress("ktlint:standard:condition-wrapping")
require(
true && false ||
true,
)
}
""".trimIndent()
val actual =
ktLintRuleEngine
.insertSuppression(
Code.fromSnippet(code, false),
KtlintSuppressionAtOffset(3, 17, CONDITION_WRAPPING_RULE_ID),
)

assertThat(actual).isEqualTo(formattedCode)
}

@Test
fun `Given a property assignment with a multiline condition to which a suppression is to be added`() {
val code =
"""
val bar =
true && false ||
true
""".trimIndent()
val formattedCode =
"""
@Suppress("ktlint:standard:condition-wrapping")
val bar =
true && false ||
true
""".trimIndent()
val actual =
ktLintRuleEngine
.insertSuppression(
Code.fromSnippet(code, false),
KtlintSuppressionAtOffset(2, 17, CONDITION_WRAPPING_RULE_ID),
)

assertThat(actual).isEqualTo(formattedCode)
}

@Test
fun `Given an if statement with a multiline condition to which a suppression is to be added`() {
val code =
"""
fun foo() {
if (true && false ||
true
) {}
}
""".trimIndent()
val formattedCode =
"""
fun foo() {
@Suppress("ktlint:standard:condition-wrapping")
if (true && false ||
true
) {}
}
""".trimIndent()
val actual =
ktLintRuleEngine
.insertSuppression(
Code.fromSnippet(code, false),
KtlintSuppressionAtOffset(2, 17, CONDITION_WRAPPING_RULE_ID),
)

assertThat(actual).isEqualTo(formattedCode)
}
}

private companion object {
val SOME_RULE_ID = RuleId("standard:some-rule-id")
}
Expand Down

0 comments on commit 0a78a47

Please sign in to comment.