Skip to content

Commit

Permalink
Ignore EOL comments in value-argument-comment and `value-parameter-…
Browse files Browse the repository at this point in the history 
…comment` (#2551)

EOL comments which are just before or after a value argument or value parameter do not belong to the VALUE_ARGUMENT or VALUE_PARAMETER AST node but to the out LIST element. So this rule only reports comments that are actually inside the VALUE_ARGUMENT or VALUE_PARAMETER node.

Closes #2519
  • Loading branch information
@paul-dingemans
paul-dingemans committed Feb 14, 2024
1 parent 22ba7c1 commit 33396eb
Show file tree
Hide file tree
Showing 7 changed files with 54 additions and 215 deletions.
30 changes: 9 additions & 21 deletions documentation/snapshot/docs/rules/standard.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2244,16 +2244,6 @@ Disallows comments to be placed at certain locations inside a value argument.
)
```

!!! note
In Ktlint 1.1.x EOL comments like below are disallowed. This will be reverted in Ktlint 1.2.
```kotlin
val foo1 =
foo(
bar1: Bar1, // some comment
bar2: Bar2, // some other comment
)
```

Rule id: `value-argument-comment` (`standard` rule set)

## Value parameter comment
Expand All @@ -2264,10 +2254,14 @@ Disallows comments to be placed at certain locations inside a value argument.

```kotlin
class Foo1(
/* some comment */
/** some kdoc */
bar = "bar"
)
class Foo2(
/* some comment */
bar = "bar"
)
class Foo3(
// some comment
bar = "bar"
)
Expand All @@ -2277,24 +2271,18 @@ Disallows comments to be placed at certain locations inside a value argument.

```kotlin
class Foo1(
bar = /* some comment */ "bar"
bar = /** some kdoc */ "bar"
)
class Foo2(
bar = /* some comment */ "bar"
)
class Foo3(
bar =
// some comment
"bar"
)
```

!!! note
In Ktlint 1.1.x EOL comments like below are disallowed. This will be reverted in Ktlint 1.2.
```kotlin
class Foo(
bar1: Bar1, // some comment
bar2: Bar2, // some other comment
)
```

Rule id: `value-parameter-comment` (`standard` rule set)

## Wrapping
Expand Down
66 changes: 17 additions & 49 deletions ...d/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/rules/ValueArgumentCommentRule.kt
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
package com.pinterest.ktlint.ruleset.standard.rules

import com.pinterest.ktlint.rule.engine.core.api.ElementType.VALUE_ARGUMENT
import com.pinterest.ktlint.rule.engine.core.api.ElementType.VALUE_ARGUMENT_LIST
import com.pinterest.ktlint.rule.engine.core.api.RuleId
import com.pinterest.ktlint.rule.engine.core.api.SinceKtlint
import com.pinterest.ktlint.rule.engine.core.api.SinceKtlint.Status.STABLE
import com.pinterest.ktlint.rule.engine.core.api.isPartOfComment
import com.pinterest.ktlint.rule.engine.core.api.isWhiteSpaceWithNewline
import com.pinterest.ktlint.rule.engine.core.api.prevLeaf
import com.pinterest.ktlint.ruleset.standard.StandardRule
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.tree.TokenSet

/**
* The AST allows comments to be placed anywhere. This however can lead to code which is unnecessarily hard to read. Or, it makes
Expand All @@ -26,53 +22,25 @@ public class ValueArgumentCommentRule : StandardRule("value-argument-comment") {
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
) {
if (node.isPartOfComment() && node.treeParent.elementType in valueArgumentTokenSet) {
if (node.treeParent.elementType == VALUE_ARGUMENT) {
// Disallow:
// val foo = foo(
// bar /* some comment */ = "bar"
// )
// or
// val foo = foo(
// bar =
// // some comment
// "bar"
// )
emit(
node.startOffset,
"A (block or EOL) comment inside or on same line after a 'value_argument' is not allowed. It may be placed " +
"on a separate line above.",
false,
)
} else if (node.treeParent.elementType == VALUE_ARGUMENT_LIST) {
if (node.prevLeaf().isWhiteSpaceWithNewline()) {
// Allow:
// val foo = foo(
// // some comment
// bar = "bar"
// )
} else {
// Disallow
// class Foo(
// val bar1: Bar, // some comment 1
// // some comment 2
// val bar2: Bar,
// )
// It is not clear whether "some comment 2" belongs to bar1 as a continuation of "some comment 1" or that it belongs to
// bar2. Note both comments are direct children of the value_argument_list.
emit(
node.startOffset,
"A comment in a 'value_argument_list' is only allowed when placed on a separate line",
false,
)
}
}
if (node.isPartOfComment() && node.treeParent.elementType == VALUE_ARGUMENT) {
// Disallow:
// val foo = foo(
// bar /* some comment */ = "bar"
// )
// or
// val foo = foo(
// bar =
// // some comment
// "bar"
// )
emit(
node.startOffset,
"A (block or EOL) comment inside or on same line after a 'value_argument' is not allowed. It may be placed " +
"on a separate line above.",
false,
)
}
}

private companion object {
val valueArgumentTokenSet = TokenSet.create(VALUE_ARGUMENT, VALUE_ARGUMENT_LIST)
}
}

public val VALUE_ARGUMENT_COMMENT_RULE_ID: RuleId = ValueArgumentCommentRule().ruleId
78 changes: 25 additions & 53 deletions .../src/main/kotlin/com/pinterest/ktlint/ruleset/standard/rules/ValueParameterCommentRule.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,12 @@ package com.pinterest.ktlint.ruleset.standard.rules

import com.pinterest.ktlint.rule.engine.core.api.ElementType.KDOC
import com.pinterest.ktlint.rule.engine.core.api.ElementType.VALUE_PARAMETER
import com.pinterest.ktlint.rule.engine.core.api.ElementType.VALUE_PARAMETER_LIST
import com.pinterest.ktlint.rule.engine.core.api.RuleId
import com.pinterest.ktlint.rule.engine.core.api.SinceKtlint
import com.pinterest.ktlint.rule.engine.core.api.SinceKtlint.Status.STABLE
import com.pinterest.ktlint.rule.engine.core.api.isPartOfComment
import com.pinterest.ktlint.rule.engine.core.api.isWhiteSpaceWithNewline
import com.pinterest.ktlint.rule.engine.core.api.prevLeaf
import com.pinterest.ktlint.ruleset.standard.StandardRule
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.tree.TokenSet

/**
* The AST allows comments to be placed anywhere. This however can lead to code which is unnecessarily hard to read. Or, it makes
Expand All @@ -27,58 +23,34 @@ public class ValueParameterCommentRule : StandardRule("value-parameter-comment")
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
) {
if (node.isPartOfComment() && node.treeParent.elementType in valueParameterTokenSet) {
if (node.treeParent.elementType == VALUE_PARAMETER) {
if (node.elementType == KDOC && node.treeParent.firstChildNode == node) {
// Allow KDoc to be the first element of a value parameter. EOL and Block comments are not parsed as the first child of
// the value parameter but as a child of the value parameter list
} else {
// Disallow:
// class Foo(
// bar /* some comment */ = "bar"
// )
// or
// class Foo(
// bar =
// // some comment
// "bar"
// )
emit(
node.startOffset,
"A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line " +
"above.",
false,
)
}
} else if (node.treeParent.elementType == VALUE_PARAMETER_LIST) {
if (node.prevLeaf().isWhiteSpaceWithNewline()) {
// Allow:
// class Foo(
// // some comment
// bar = "bar"
// )
} else {
// Disallow
// class Foo(
// val bar1: Bar, // some comment 1
// // some comment 2
// val bar2: Bar,
// )
// It is not clear whether "some comment 2" belongs to bar1 as a continuation of "some comment 1" or that it belongs to
// bar2. Note both comments are direct children of the value_parameter_list.
emit(
node.startOffset,
"A comment in a 'value_parameter_list' is only allowed when placed on a separate line",
false,
)
}
if (node.isPartOfComment() && node.treeParent.elementType == VALUE_PARAMETER) {
if (node.elementType == KDOC && node.treeParent.firstChildNode == node) {
// Allow KDoc to be the first element of a value parameter. EOL and block comments directly before a value parameter are
// a child of the value parameter list, and as of that will never be the first child of the value.
// class Foo(
// /** some kdoc */
// bar = "bar"
// )
} else {
// Disallow:
// class Foo(
// bar /* some comment */ = "bar"
// )
// or
// class Foo(
// bar =
// // some comment
// "bar"
// )
emit(
node.startOffset,
"A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line " +
"above.",
false,
)
}
}
}

private companion object {
val valueParameterTokenSet = TokenSet.create(VALUE_PARAMETER, VALUE_PARAMETER_LIST)
}
}

public val VALUE_PARAMETER_COMMENT_RULE_ID: RuleId = ValueParameterCommentRule().ruleId
20 changes: 0 additions & 20 deletions ...c/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/ArgumentListWrappingRuleTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -944,26 +944,6 @@ class ArgumentListWrappingRuleTest {
).isFormattedAs(formattedCode)
}

@Test
fun `Issue 2445 - Given a value argument followed by EOL comment after comma`() {
val code =
"""
val foo = foo(
bar1 = "bar1", // some comment 1
bar2 = "bar2", // some comment 2
)
""".trimIndent()
@Suppress("ktlint:standard:argument-list-wrapping", "ktlint:standard:max-line-length")
argumentListWrappingRuleAssertThat(code)
.addAdditionalRuleProvider { ValueArgumentCommentRule() }
.hasLintViolationsForAdditionalRule(
LintViolation(2, 20, "A comment in a 'value_argument_list' is only allowed when placed on a separate line", false),
LintViolation(3, 20, "A comment in a 'value_argument_list' is only allowed when placed on a separate line", false),
)
// When ValueArgumentCommentRule is not loaded or enabled
argumentListWrappingRuleAssertThat(code).hasNoLintViolations()
}

@Test
fun `Issue 2462 - Given a call expression with value argument list inside a binary expression, then first wrap the binary expression`() {
val code =
Expand Down
18 changes: 3 additions & 15 deletions .../src/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/FunctionSignatureRuleTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,28 +211,21 @@ class FunctionSignatureRuleTest {
private /* some comment */ fun f1(a: Any, b: Any): String = "some-result"
private fun /* some comment */ f2(a: Any, b: Any): String = "some-result"
private fun f3 /* some comment */ (a: Any, b: Any): String = "some-result"
private fun f4( /* some comment */ a: Any, b: Any): String = "some-result"
private fun f5(a /* some comment */: Any, b: Any): String = "some-result"
private fun f6(a: /* some comment */ Any, b: Any): String = "some-result"
private fun f7(a: Any /* some comment */, b: Any): String = "some-result"
private fun f8(a: Any, b: Any) /* some comment */: String = "some-result"
private fun f9(a: Any, b: Any): /* some comment */ String = "some-result"
private fun f10(a: Any, b: Any): String /* some comment */ = "some-result"
private fun f11(
a: Any, // some-comment
b: Any
): String = "f10"
""".trimIndent()
@Suppress("ktlint:standard:argument-list-wrapping", "ktlint:standard:max-line-length")
functionSignatureWrappingRuleAssertThat(code)
.setMaxLineLength()
.addAdditionalRuleProvider { ValueParameterCommentRule() }
.hasLintViolationsForAdditionalRule(
LintViolation(5, 17, "A comment in a 'value_parameter_list' is only allowed when placed on a separate line", false),
LintViolation(6, 18, "A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above.", false),
LintViolation(7, 19, "A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above.", false),
LintViolation(8, 23, "A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above.", false),
LintViolation(13, 13, "A comment in a 'value_parameter_list' is only allowed when placed on a separate line", false),
LintViolation(5, 18, "A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above.", false),
LintViolation(6, 19, "A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above.", false),
LintViolation(7, 23, "A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above.", false),
).hasNoLintViolationsExceptInAdditionalRules()
}

Expand All @@ -243,10 +236,6 @@ class FunctionSignatureRuleTest {
private fun f5(a /* some comment */: Any, b: Any): String = "some-result"
private fun f6(a: /* some comment */ Any, b: Any): String = "some-result"
private fun f7(a: Any /* some comment */, b: Any): String = "some-result"
private fun f11(
a: Any, // some-comment
b: Any
): String = "f10"
""".trimIndent()
@Suppress("ktlint:standard:argument-list-wrapping", "ktlint:standard:max-line-length")
functionSignatureWrappingRuleAssertThat(code)
Expand All @@ -255,7 +244,6 @@ class FunctionSignatureRuleTest {
LintViolation(1, 18, "A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above.", false),
LintViolation(2, 19, "A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above.", false),
LintViolation(3, 23, "A comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above.", false),
LintViolation(5, 13, "A comment in a 'value_parameter_list' is only allowed when placed on a separate line", false),
).hasNoLintViolationsExceptInAdditionalRules()
// When ValueParameterCommentRule is not loaded or disabled:
functionSignatureWrappingRuleAssertThat(code).hasNoLintViolations()
Expand Down
39 changes: 0 additions & 39 deletions ...c/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/ValueArgumentCommentRuleTest.kt
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.pinterest.ktlint.ruleset.standard.rules

import com.pinterest.ktlint.test.KtLintAssertThat
import com.pinterest.ktlint.test.LintViolation
import org.junit.jupiter.api.Test

class ValueArgumentCommentRuleTest {
Expand Down Expand Up @@ -64,44 +63,6 @@ class ValueArgumentCommentRuleTest {
valueArgumentCommentRuleAssertThat(code).hasNoLintViolations()
}

@Test
fun `Given a comment as last node of value argument ast node`() {
val code =
"""
val foo1 = foo(
"bar" // some comment
)
val foo2 = foo(
"bar" /* some comment */
)
""".trimIndent()
@Suppress("ktlint:standard:argument-list-wrapping", "ktlint:standard:max-line-length")
valueArgumentCommentRuleAssertThat(code)
.hasLintViolationsWithoutAutoCorrect(
LintViolation(2, 11, "A comment in a 'value_argument_list' is only allowed when placed on a separate line"),
LintViolation(5, 11, "A comment in a 'value_argument_list' is only allowed when placed on a separate line"),
)
}

@Test
fun `Given a comment after a comma on the same line as an value argument ast node`() {
val code =
"""
val foo1 = foo(
"bar", // some comment
)
val foo2 = foo(
"bar", /* some comment */
)
""".trimIndent()
@Suppress("ktlint:standard:argument-list-wrapping", "ktlint:standard:max-line-length")
valueArgumentCommentRuleAssertThat(code)
.hasLintViolationsWithoutAutoCorrect(
LintViolation(2, 12, "A comment in a 'value_argument_list' is only allowed when placed on a separate line"),
LintViolation(5, 12, "A comment in a 'value_argument_list' is only allowed when placed on a separate line"),
)
}

@Test
fun `Given a comment as last node of value argument list`() {
val code =
Expand Down

0 comments on commit 33396eb

Please sign in to comment.