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

Remove dependencies on discouraged-comment-location rule #2371

Merged
merged 7 commits into from
Nov 22, 2023
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
194 changes: 173 additions & 21 deletions documentation/snapshot/docs/rules/standard.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,27 +128,6 @@ Lines in a block comment which (exclusive the indentation) start with a `*` shou

Rule id: `block-comment-initial-star-alignment` (`standard` rule set)

## Discouraged comment location

Detect discouraged comment locations (no autocorrect).

!!! note
Kotlin allows comments to be placed almost everywhere. As this can lead to code which is hard to read, most of them will never be used in practice. Ideally each rule takes comments at all possible locations into account. Sometimes this is really hard and not worth the effort. By explicitly forbidding such comment locations, the development of those rules becomes a bit easier.

=== "[:material-heart-off-outline:](#) Disallowed"

```kotlin
fun <T> /* some comment */ foo(t: T) = "some-result"

fun foo() {
if (true)
// some comment
bar()
}
```

Rule id: `discouraged-comment-location` (`standard` rule set)

## Enum entry

Enum entry names should be uppercase underscore-separated or upper camel-case separated.
Expand Down Expand Up @@ -2022,6 +2001,89 @@ Consistent removal (default) or adding of trailing commas on declaration site.

Rule id: `trailing-comma-on-declaration-site` (`standard` rule set)

## Type argument comment

Disallows comments to be placed at certain locations inside a type argument (list). A KDoc is not allowed.

=== "[:material-heart:](#) Ktlint"

```kotlin
fun Foo<
/* some comment */
out Any
>.foo() {}
fun Foo<
// some comment
out Any
>.foo() {}
```

=== "[:material-heart-off-outline:](#) Disallowed"

```kotlin
fun Foo<out /* some comment */ Any>.foo() {}
fun Foo<
out Any, // some comment
>.foo() {}
val fooBar: FooBar<
/** some comment */
Foo,
Bar
>
```

Note: Although code sample below might look ok, it is semantically and programmatically unclear to which element `some comment 1` refers. As of that comments are only allowed when starting on a separate line.
```kotlin
fun Foo<
out Bar1, // some comment 1
// some comment 2
out Bar2, // some comment
>.foo() {}
```

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

## Type parameter comment

Disallows comments to be placed at certain locations inside a type parameter (list). A KDoc is not allowed.

=== "[:material-heart:](#) Ktlint"

```kotlin
class Foo2<
/* some comment */
out Bar
>
class Foo3<
// some comment
out Bar
>
```

=== "[:material-heart-off-outline:](#) Disallowed"

```kotlin
class Foo1<
/** some comment */
in Bar
>
class Foo2<in /* some comment */ Bar>
class Foo3<
in Bar, // some comment
>
```

Note: Although code sample below might look ok, it is semantically and programmatically unclear to which element `some comment 1` refers. As of that comments are only allowed when starting on a separate line.
```kotlin
class Foo<
out Bar1, // some comment 1
// some comment 2
out Bar2, // some comment
>
```

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

## Unnecessary parenthesis before trailing lambda

An empty parentheses block before a lambda is redundant.
Expand All @@ -2040,6 +2102,96 @@ An empty parentheses block before a lambda is redundant.

Rule id: `unnecessary-parentheses-before-trailing-lambda` (`standard` rule set)

## Value argument comment

Disallows comments to be placed at certain locations inside a value argument (list). A KDoc is not allowed.

=== "[:material-heart:](#) Ktlint"

```kotlin
val foo2 =
foo(
/* some comment */
bar = "bar"
)
val foo3 =
foo(
// some comment
bar = "bar"
)
```

=== "[:material-heart-off-outline:](#) Disallowed"

```kotlin
val foo1 = foo(bar /** some comment */ = "bar")
val foo2 = foo(bar /* some comment */ = "bar")
val foo3 =
foo(
bar = // some comment
"bar"
)
```

Note: Although code sample below might look ok, it is semantically and programmatically unclear to which element `some comment 1` refers. As of that comments are only allowed when starting on a separate line.
```kotlin
class Foo<
out Bar1, // some comment 1
// some comment 2
out Bar2, // some comment
>
```

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

## Value parameter comment

Disallows comments to be placed at certain locations inside a value argument (list). A KDoc is allowed but must start on a separate line.

=== "[:material-heart:](#) Ktlint"

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

=== "[:material-heart-off-outline:](#) Disallowed"

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

Note: Although code sample below might look ok, it is semantically and programmatically unclear to which element `some comment 1` refers. As of that comments are only allowed when starting on a separate line.
```kotlin
class Foo(
bar: Bar1, // some comment 1
// some comment 2
bar2: Bar2, // some comment
)
```

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

## Wrapping

### Argument list wrapping
Expand Down
3 changes: 3 additions & 0 deletions ktlint-rule-engine-core/api/ktlint-rule-engine-core.api
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
public final class com/pinterest/ktlint/rule/engine/core/api/ASTNodeExtensionKt {
public static final fun afterCodeSibling (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/psi/tree/IElementType;)Z
public static final fun beforeCodeSibling (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/psi/tree/IElementType;)Z
public static final fun betweenCodeSiblings (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/psi/tree/IElementType;Lorg/jetbrains/kotlin/com/intellij/psi/tree/IElementType;)Z
public static final fun children (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Lkotlin/sequences/Sequence;
public static final fun findCompositeParentElementOfType (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Lorg/jetbrains/kotlin/com/intellij/psi/tree/IElementType;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;
public static final fun firstChildLeafOrSelf (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -470,3 +470,12 @@ public fun Sequence<ASTNode>.lineLengthWithoutNewlinePrefix(): Int {
.takeWhile { it != '\n' }
.length
}

public fun ASTNode.afterCodeSibling(afterElementType: IElementType): Boolean = prevCodeSibling()?.elementType == afterElementType

public fun ASTNode.beforeCodeSibling(beforeElementType: IElementType): Boolean = nextCodeSibling()?.elementType == beforeElementType

public fun ASTNode.betweenCodeSiblings(
afterElementType: IElementType,
beforeElementType: IElementType,
): Boolean = afterCodeSibling(afterElementType) && beforeCodeSibling(beforeElementType)
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import com.pinterest.ktlint.rule.engine.core.api.ElementType.CLASS
import com.pinterest.ktlint.rule.engine.core.api.ElementType.CLASS_BODY
import com.pinterest.ktlint.rule.engine.core.api.ElementType.ENUM_ENTRY
import com.pinterest.ktlint.rule.engine.core.api.ElementType.FUN
import com.pinterest.ktlint.rule.engine.core.api.ElementType.FUN_KEYWORD
import com.pinterest.ktlint.rule.engine.core.api.ElementType.IDENTIFIER
import com.pinterest.ktlint.rule.engine.core.api.ElementType.LPAR
import com.pinterest.ktlint.rule.engine.core.api.ElementType.MODIFIER_LIST
Expand All @@ -22,6 +23,8 @@ import org.jetbrains.kotlin.com.intellij.lang.FileASTNode
import org.jetbrains.kotlin.psi.psiUtil.leaves
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
import kotlin.reflect.KFunction1

class ASTNodeExtensionTest {
Expand Down Expand Up @@ -722,6 +725,67 @@ class ASTNodeExtensionTest {
)
}

@ParameterizedTest(name = "Text between FUN_KEYWORD and IDENTIFIER: {0}")
@ValueSource(
strings = [
" ",
"\n",
"/* some comment*/",
"// some EOL comment\n",
],
)
fun `Given a function declaration then the IDENTIFIER should be after the FUN_KEYWORD element type`(separator: String) {
val code =
"""
fun${separator}foo() = 42
""".trimIndent()
val actual =
transformCodeToAST(code)
.findChildByType(FUN)
?.findChildByType(IDENTIFIER)
?.afterCodeSibling(FUN_KEYWORD)

assertThat(actual).isTrue()
}

@ParameterizedTest(name = "Text between FUN_KEYWORD and IDENTIFIER: {0}")
@ValueSource(
strings = [
" ",
"\n",
"/* some comment*/",
"// some EOL comment\n",
],
)
fun `Given a function declaration then the FUN_KEYWORD should be before the IDENTIFIER element type`(separator: String) {
val code =
"""
fun${separator}foo() = 42
""".trimIndent()
val actual =
transformCodeToAST(code)
.findChildByType(FUN)
?.findChildByType(FUN_KEYWORD)
?.beforeCodeSibling(IDENTIFIER)

assertThat(actual).isTrue()
}

@Test
fun `Given a function declaration then the IDENTIFIER should be between the FUN_KEYWORD and the VALUE_PARAMETER_LIST element type`() {
val code =
"""
fun foo() = 42
""".trimIndent()
val actual =
transformCodeToAST(code)
.findChildByType(FUN)
?.findChildByType(IDENTIFIER)
?.betweenCodeSiblings(FUN_KEYWORD, VALUE_PARAMETER_LIST)

assertThat(actual).isTrue()
}

private inline fun String.transformAst(block: FileASTNode.() -> Unit): FileASTNode =
transformCodeToAST(this)
.apply(block)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ package com.pinterest.ktlint.rule.engine.internal.rules

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.StandardRuleSetProvider
import com.pinterest.ktlint.ruleset.standard.rules.ArgumentListWrappingRule
import com.pinterest.ktlint.test.KtLintAssertThat.Companion.EOL_CHAR
import com.pinterest.ktlint.test.KtLintAssertThat.Companion.MAX_LINE_LENGTH_MARKER
import com.pinterest.ktlint.test.KtLintAssertThat.Companion.assertThatRule
import com.pinterest.ktlint.test.KtLintAssertThat.Companion.assertThatRuleBuilder
import com.pinterest.ktlint.test.KtlintDocumentationTest
import com.pinterest.ktlint.test.LintViolation
import org.junit.jupiter.api.Nested
Expand All @@ -17,25 +17,21 @@ import org.junit.jupiter.params.provider.ValueSource

class KtlintSuppressionRuleTest {
private val ktlintSuppressionRuleAssertThat =
assertThatRule(
provider = { KtlintSuppressionRule(emptyList()) },
additionalRuleProviders =
setOf(
// Create a dummy rule for each rule id that is used in a ktlint directive or suppression in the tests in this
// class. If no rule provider is added for the rule id, a lint violation is thrown which will bloat the tests too
// much.
//
// Ids of real rules used but for which the real implementation is unwanted as it would modify the formatted code
RuleProvider { DummyRule("standard:no-wildcard-imports") },
RuleProvider { DummyRule("standard:no-multi-spaces") },
RuleProvider { DummyRule("standard:max-line-length") },
RuleProvider { DummyRule("standard:package-name") },
// Ids of fake rules in a custom and the standard rule set
RuleProvider { DummyRule("custom:foo") },
RuleProvider { DummyRule("standard:bar") },
RuleProvider { DummyRule("standard:foo") },
),
)
assertThatRuleBuilder { KtlintSuppressionRule(emptyList()) }
// Create a dummy rule for each rule id that is used in a ktlint directive or suppression in the tests in this
// class. If no rule provider is added for the rule id, a lint violation is thrown which will bloat the tests too
// much.
//
// Ids of real rules used but for which the real implementation is unwanted as it would modify the formatted code
.addAdditionalRuleProvider { DummyRule("standard:no-wildcard-imports") }
.addAdditionalRuleProvider { DummyRule("standard:no-multi-spaces") }
.addAdditionalRuleProvider { DummyRule("standard:max-line-length") }
.addAdditionalRuleProvider { DummyRule("standard:package-name") }
// Ids of fake rules in a custom and the standard rule set
.addAdditionalRuleProvider { DummyRule("custom:foo") }
.addAdditionalRuleProvider { DummyRule("standard:bar") }
.addAdditionalRuleProvider { DummyRule("standard:foo") }
.assertThat()

@Nested
inner class `Given a suppression annotation missing the rule set id prefix` {
Expand Down Expand Up @@ -872,6 +868,7 @@ class KtlintSuppressionRuleTest {
""".trimIndent()
ktlintSuppressionRuleAssertThat(code)
.addAdditionalRuleProvider { ArgumentListWrappingRule() }
.addRequiredRuleProviderDependenciesFrom(StandardRuleSetProvider())
.hasLintViolations(
LintViolation(1, 4, "Directive 'ktlint-disable' is deprecated. Replace with @Suppress annotation"),
LintViolation(4, 54, "Directive 'ktlint-disable' is deprecated. Replace with @Suppress annotation"),
Expand Down Expand Up @@ -1359,6 +1356,7 @@ class KtlintSuppressionRuleTest {
ktlintSuppressionRuleAssertThat(code)
.setMaxLineLength()
.addAdditionalRuleProvider { ArgumentListWrappingRule() }
.addRequiredRuleProviderDependenciesFrom(StandardRuleSetProvider())
.hasLintViolations(
LintViolation(8, 12, "Directive 'ktlint-disable' is deprecated. Replace with @Suppress annotation"),
LintViolation(10, 12, "Directive 'ktlint-enable' is obsolete after migrating to suppress annotations"),
Expand Down
Loading