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.

Sign up for GitHub

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

Jump to bottom

Fix suppression handling when 'formatter:on' not properly specified #2719

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Fix suppression handling when 'formatter:on' not properly specified #2719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Fix suppression handling when 'formatter:on' not properly specified 
If the 'formatter:on' tag is not specified in the same scope as the 'formatter:off' tag, then the IndentationRule might throw an exception as the IndentationContextStack is not properly cleaned.

 Also, cleaned up of `CommentSuppressionHint.EOL` type as it was not used since replacing the EOL suppression hints in `1.0` release.

Closes #2695
  • Loading branch information
@paul-dingemans
paul-dingemans committed Jun 25, 2024
commit 1155d794dd4d0237ebf8460f04b813ca6bf1f659
54 changes: 30 additions & 24 deletions ...ne/src/main/kotlin/com/pinterest/ktlint/rule/engine/internal/SuppressionLocatorBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
package com.pinterest.ktlint.rule.engine.internal

import com.pinterest.ktlint.rule.engine.core.api.ElementType.RBRACE
import com.pinterest.ktlint.rule.engine.core.api.RuleId
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.EditorConfig
import com.pinterest.ktlint.rule.engine.core.api.isWhiteSpaceWithNewline
import com.pinterest.ktlint.rule.engine.core.api.prevLeaf
import com.pinterest.ktlint.rule.engine.core.api.nextSibling
import com.pinterest.ktlint.rule.engine.internal.SuppressionLocatorBuilder.CommentSuppressionHint.Type.BLOCK_END
import com.pinterest.ktlint.rule.engine.internal.SuppressionLocatorBuilder.CommentSuppressionHint.Type.BLOCK_START
import com.pinterest.ktlint.rule.engine.internal.SuppressionLocatorBuilder.CommentSuppressionHint.Type.EOL
import com.pinterest.ktlint.rule.engine.internal.rules.KTLINT_SUPPRESSION_RULE_ID
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.PsiComment
Expand Down Expand Up @@ -99,7 +98,7 @@ internal object SuppressionLocatorBuilder {
}

return suppressionHints.plus(
commentSuppressionsHints.toSuppressionHints(rootNode),
commentSuppressionsHints.toSuppressionHints(),
)
}

Expand Down Expand Up @@ -139,21 +138,11 @@ internal object SuppressionLocatorBuilder {
}
}

private fun MutableList<CommentSuppressionHint>.toSuppressionHints(rootNode: ASTNode): MutableList<SuppressionHint> {
private fun MutableList<CommentSuppressionHint>.toSuppressionHints(): MutableList<SuppressionHint> {
val suppressionHints = mutableListOf<SuppressionHint>()
val blockCommentSuppressionHints = mutableListOf<CommentSuppressionHint>()
forEach { commentSuppressionHint ->
when (commentSuppressionHint.type) {
EOL -> {
val commentNode = commentSuppressionHint.node
suppressionHints.add(
SuppressionHint(
IntRange(commentNode.prevNewLineOffset(), commentNode.startOffset),
commentSuppressionHint.disabledRuleIds,
),
)
}

BLOCK_START -> {
blockCommentSuppressionHints.add(commentSuppressionHint)
}
Expand All @@ -175,20 +164,38 @@ internal object SuppressionLocatorBuilder {
}
}
suppressionHints.addAll(
// Remaining hints were not properly closed
blockCommentSuppressionHints.map {
SuppressionHint(
IntRange(it.node.startOffset, rootNode.textLength),
it.disabledRuleIds,
)
val rbraceOfContainingBlock = it.node.rbraceOfContainingBlock()
if (rbraceOfContainingBlock == null) {
// Apply suppression on next sibling only, when the outer element does not end with a RBRACE
it.node
.nextSibling()
.let { nextSibling ->
SuppressionHint(
IntRange(
it.node.startOffset,
nextSibling?.textRange?.endOffset ?: it.node.textRange.endOffset,
),
it.disabledRuleIds,
)
}
} else {
// Exclude closing curly brace of the containing block
SuppressionHint(
IntRange(it.node.startOffset, rbraceOfContainingBlock.startOffset - 1),
it.disabledRuleIds,
)
}
},
)
return suppressionHints
}

private fun ASTNode.prevNewLineOffset(): Int =
prevLeaf { it.isWhiteSpaceWithNewline() }
?.let { it.startOffset + it.text.lastIndexOf('\n') + 1 }
?: 0
private fun ASTNode.rbraceOfContainingBlock(): ASTNode? =
treeParent
.lastChildNode
?.takeIf { it.elementType == RBRACE }

private fun <T> List<T>.tail() = this.subList(1, this.size)

Expand Down Expand Up @@ -261,7 +268,6 @@ internal object SuppressionLocatorBuilder {
val type: Type,
) {
enum class Type {
EOL,
BLOCK_START,
BLOCK_END,
}
Expand Down
96 changes: 96 additions & 0 deletions ...rc/test/kotlin/com/pinterest/ktlint/rule/engine/internal/SuppressionLocatorBuilderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,58 @@ class SuppressionLocatorBuilderTest {
)
}

@Test
fun `Issue 2695 - Given that the formatter-on tag is not found in a block containing the formatter-off tag then the IndentationRule should not throw an exception`() {
val code =
"""
val fooReported1 = "foo"
fun bar() {
// @formatter:off
val fooNotReported1 = "foo"
val fooNotReported2 = "foo"
}
val fooReported2 = "foo"
""".trimIndent()

val actual =
lint(
code,
editorConfigOverride = EditorConfigOverride.from(FORMATTER_TAGS_ENABLED_PROPERTY to true),
ruleProviders = setOf(RuleProvider { IndentationRule() }),
)
assertThat(actual).containsExactly(
lintError(1, 5, "standard:no-foo-identifier-standard"),
lintError(1, 5, "$NON_STANDARD_RULE_SET_ID:no-foo-identifier"),
lintError(7, 5, "standard:no-foo-identifier-standard"),
lintError(7, 5, "$NON_STANDARD_RULE_SET_ID:no-foo-identifier"),
)
}

@Test
fun `Issue 2695 - Given that the formatter-on tag is applied on a non-block element then the IndentationRule should not throw an exception`() {
val code =
"""
val bar1 = fooReported()
fun bar() =
// @formatter:off
fooNotReported()
val bar2 = fooReported()
""".trimIndent()

val actual =
lint(
code,
editorConfigOverride = EditorConfigOverride.from(FORMATTER_TAGS_ENABLED_PROPERTY to true),
ruleProviders = setOf(RuleProvider { IndentationRule() }),
)
assertThat(actual).containsExactly(
lintError(1, 12, "standard:no-foo-identifier-standard"),
lintError(1, 12, "$NON_STANDARD_RULE_SET_ID:no-foo-identifier"),
lintError(5, 12, "standard:no-foo-identifier-standard"),
lintError(5, 12, "$NON_STANDARD_RULE_SET_ID:no-foo-identifier"),
)
}

@Test
fun `Given that a NoFooIdentifierRule violation is suppressed with custom formatter tags in block comments then do not find a violation in that block`() {
val code =
Expand Down Expand Up @@ -371,6 +423,50 @@ class SuppressionLocatorBuilderTest {
assertThat(actual).isEqualTo(formattedCode)
}

@Test
fun `Issue 2695 - `() {
val code =
"""
fun bar() {
/* ktlint-disable standard:indent */
return mapOf(
1 to " 1 ms",
10 to " 10 ms",
999 to " 999 ms",
1000 to " 1 sec",
)
/* ktlint-enable standard:indent */
}
""".trimIndent()
val formattedCode =
"""
@Suppress("ktlint:standard:indent")
fun bar() {
return mapOf(
1 to " 1 ms",
10 to " 10 ms",
999 to " 999 ms",
1000 to " 1 sec",
)
}
""".trimIndent()

val actual =
KtLintRuleEngine(
ruleProviders =
setOf(
RuleProvider { IndentationRule() },
),
editorConfigOverride =
EMPTY_EDITOR_CONFIG_OVERRIDE
.plus(
STANDARD_NO_FOO_IDENTIFIER_RULE_ID.createRuleExecutionEditorConfigProperty() to RuleExecution.enabled,
),
).format(Code.fromSnippet(code)) { _ -> AutocorrectDecision.ALLOW_AUTOCORRECT }

assertThat(actual).isEqualTo(formattedCode)
}

private class NoFooIdentifierRule(
id: RuleId,
) : Rule(
Expand Down