Skip to content

Commit

Permalink
Exclude EOL-comments when check whether max line length is exceeded i…
Browse files Browse the repository at this point in the history
…n rules other than max-line-length (pinterest#2565)

Follow up on pinterest#2450:
- fix exception in `function-literal` in case the literal block just contains an EOL-comment only
- Change function `dropTrailingComment` to private (it was not yet exposed in a released version before)
  • Loading branch information
paul-dingemans authored Feb 27, 2024
1 parent 60b6b8f commit 913887d
Show file tree
Hide file tree
Showing 11 changed files with 111 additions and 36 deletions.
2 changes: 1 addition & 1 deletion ktlint-rule-engine-core/api/ktlint-rule-engine-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ public final class com/pinterest/ktlint/rule/engine/core/api/ASTNodeExtensionKt
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 dropTrailingEolComment (Lkotlin/sequences/Sequence;)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;
public static final fun getColumn (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)I
Expand All @@ -29,6 +28,7 @@ public final class com/pinterest/ktlint/rule/engine/core/api/ASTNodeExtensionKt
public static final fun leavesIncludingSelf (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Z)Lkotlin/sequences/Sequence;
public static synthetic fun leavesIncludingSelf$default (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;ZILjava/lang/Object;)Lkotlin/sequences/Sequence;
public static final fun leavesOnLine (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;)Lkotlin/sequences/Sequence;
public static final fun leavesOnLine (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Z)Lkotlin/sequences/Sequence;
public static final fun lineLength (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;Z)I
public static synthetic fun lineLength$default (Lorg/jetbrains/kotlin/com/intellij/lang/ASTNode;ZILjava/lang/Object;)I
public static final fun lineLengthWithoutNewlinePrefix (Lkotlin/sequences/Sequence;)I
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -438,20 +438,32 @@ public fun ASTNode.leavesIncludingSelf(forward: Boolean = true): Sequence<ASTNod
* Get all leaves on the same line as the given node including the whitespace indentation. Note that the whitespace indentation may start
* with zero or more newline characters.
*/
public fun ASTNode.leavesOnLine(): Sequence<ASTNode> {
@Deprecated(
message =
"Marked for removal in Ktlint 2.x. Rules should not modify code in case the EOL comment causes the max_line_length to be exceeded.",
replaceWith = ReplaceWith("leavesOnLine(excludeEolComment = false)"),
)
public fun ASTNode.leavesOnLine(): Sequence<ASTNode> = leavesOnLine(excludeEolComment = false)

/**
* Get all leaves on the same line as the given node including the whitespace indentation. Note that the whitespace indentation may start
* with zero or more newline characters.
*/
public fun ASTNode.leavesOnLine(excludeEolComment: Boolean): Sequence<ASTNode> {
val lastLeafOnLineOrNull = getLastLeafOnLineOrNull()
return getFirstLeafOnLineOrSelf()
.leavesIncludingSelf()
.applyIf(excludeEolComment) { dropTrailingEolComment() }
.takeWhile { lastLeafOnLineOrNull == null || it.prevLeaf() != lastLeafOnLineOrNull }
}

/**
* Take all nodes preceding the whitespace before the EOL comment
*/
public fun Sequence<ASTNode>.dropTrailingEolComment(): Sequence<ASTNode> =
private fun Sequence<ASTNode>.dropTrailingEolComment(): Sequence<ASTNode> =
takeWhile {
!(it.isWhiteSpace() && it.nextLeaf()?.elementType == EOL_COMMENT) &&
// But if EOL-comment not preceded by whitespace than take all node before the EOL comment
!(it.isWhiteSpaceWithoutNewline() && it.nextLeaf()?.elementType == EOL_COMMENT) &&
// But if EOL-comment not preceded by whitespace than take all nodes before the EOL comment
it.elementType != EOL_COMMENT
}

Expand All @@ -473,7 +485,7 @@ internal fun ASTNode.getLastLeafOnLineOrNull() = nextLeaf { it.textContains('\n'
"Marked for removal in Ktlint 2.x. Rules should not modify code in case the EOL comment causes the max_line_length to be exceeded.",
replaceWith = ReplaceWith("lineLength(excludeEolComment = false)"),
)
public fun ASTNode.lineLengthWithoutNewlinePrefix(): Int = leavesOnLine().lineLengthWithoutNewlinePrefix()
public fun ASTNode.lineLengthWithoutNewlinePrefix(): Int = leavesOnLine(excludeEolComment = false).lineLengthWithoutNewlinePrefix()

/**
* Get the total length of all leaves on the same line as the given node including the whitespace indentation but excluding all leading
Expand All @@ -482,18 +494,16 @@ public fun ASTNode.lineLengthWithoutNewlinePrefix(): Int = leavesOnLine().lineLe
* max-line-length rule report this violation so that the developer can choose whether the comment can be shortened or that it can be placed
* on a separate line.
*/
public fun ASTNode.lineLength(excludeEolComment: Boolean = false): Int =
leavesOnLine()
.applyIf(excludeEolComment) { dropTrailingEolComment() }
.lineLengthWithoutNewlinePrefix()
public fun ASTNode.lineLength(excludeEolComment: Boolean = false): Int = leavesOnLine(excludeEolComment).lineLengthWithoutNewlinePrefix()

/**
* Get the total length of all leaves in the sequence including the whitespace indentation but excluding all leading newline characters in
* the whitespace indentation. The first leaf node in the sequence must be a white space starting with at least one newline.
*/
public fun Sequence<ASTNode>.lineLengthWithoutNewlinePrefix(): Int {
require(first().text.startsWith('\n') || first().prevLeaf() == null) {
"First node in sequence must be a whitespace containing a newline"
val first = firstOrNull() ?: return 0
require(first.text.startsWith('\n') || first.prevLeaf() == null) {
"First node in non-empty sequence must be a whitespace containing a newline"
}
return joinToString(separator = "") { it.text }
// If a line is preceded by a blank line then the ident contains multiple newline chars
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,7 @@ class ASTNodeExtensionTest {
)
}

@Suppress("DEPRECATION")
@Test
fun `Given some line containing identifiers at different indentation levels then check that all leaves on those line are found`() {
val code =
Expand Down Expand Up @@ -665,9 +666,9 @@ class ASTNodeExtensionTest {
)
}

@Suppress("DEPRECATION")
@Nested
inner class LineLengthWithoutNewlinePrefix {
@Suppress("DEPRECATION")
@Test
fun `Given some lines containing identifiers at different indentation levels then get line length exclusive the leading newline characters`() {
val code =
Expand Down Expand Up @@ -697,6 +698,7 @@ class ASTNodeExtensionTest {
)
}

@Suppress("DEPRECATION")
@Test
fun `Given some lines containing identifiers and EOL comment then get line length exclusive the leading newline characters and exclusive EOL comment`() {
val code =
Expand Down Expand Up @@ -726,6 +728,7 @@ class ASTNodeExtensionTest {
)
}

@Suppress("DEPRECATION")
@Test
fun `Given some lines containing identifiers at different indentation levels then get line length exclusive the leading newline characters until and including the identifier`() {
val code =
Expand Down Expand Up @@ -848,6 +851,24 @@ class ASTNodeExtensionTest {
" val foo4 = \"foo4\"".length,
)
}

@Test
fun `Given a line only containing an EOL-comment`() {
val code =
"""
fun bar() {
// no-op
}
""".trimIndent()
val actual =
transformCodeToAST(code)
.firstChildLeafOrSelf()
.leavesIncludingSelf()
.first { it.elementType == ElementType.EOL_COMMENT }
.lineLength(true)

assertThat(actual).isEqualTo(4)
}
}

@ParameterizedTest(name = "Text between FUN_KEYWORD and IDENTIFIER: {0}")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ 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.EXPERIMENTAL
import com.pinterest.ktlint.rule.engine.core.api.children
import com.pinterest.ktlint.rule.engine.core.api.dropTrailingEolComment
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.EditorConfig
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.INDENT_SIZE_PROPERTY
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.INDENT_STYLE_PROPERTY
Expand All @@ -32,6 +31,7 @@ import com.pinterest.ktlint.rule.engine.core.api.isWhiteSpace
import com.pinterest.ktlint.rule.engine.core.api.isWhiteSpaceWithNewline
import com.pinterest.ktlint.rule.engine.core.api.lastChildLeafOrSelf
import com.pinterest.ktlint.rule.engine.core.api.leavesOnLine
import com.pinterest.ktlint.rule.engine.core.api.lineLength
import com.pinterest.ktlint.rule.engine.core.api.lineLengthWithoutNewlinePrefix
import com.pinterest.ktlint.rule.engine.core.api.nextLeaf
import com.pinterest.ktlint.rule.engine.core.api.nextSibling
Expand Down Expand Up @@ -248,15 +248,11 @@ public class BinaryExpressionWrappingRule :
}
}

private fun ASTNode.isOnLineExceedingMaxLineLength() =
maxLineLength <
leavesOnLine()
.dropTrailingEolComment()
.lineLengthWithoutNewlinePrefix()
private fun ASTNode.isOnLineExceedingMaxLineLength() = maxLineLength < lineLength(excludeEolComment = true)

private fun ASTNode.causesMaxLineLengthToBeExceeded() =
lastChildLeafOrSelf().let { lastChildLeaf ->
leavesOnLine()
leavesOnLine(excludeEolComment = true)
.takeWhile { it.prevLeaf() != lastChildLeaf }
.lineLengthWithoutNewlinePrefix()
} > maxLineLength
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ public class ChainMethodContinuationRule :
.last()
.startOfLambdaArgumentInCallExpressionOrNull()
?: lastChildLeafOrSelf().nextLeaf()
leavesOnLine()
leavesOnLine(excludeEolComment = true)
.takeWhile { it != stopAtLeaf }
.lineLengthWithoutNewlinePrefix() > maxLineLength
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ public class FunctionLiteralRule :
require(elementType == VALUE_PARAMETER_LIST && treeParent.elementType == FUNCTION_LITERAL)
val lbrace = treeParent.findChildByType(LBRACE)!!
return lbrace
.leavesOnLine()
.leavesOnLine(excludeEolComment = true)
.takeWhile { it.prevLeaf() != lbrace }
.lineLengthWithoutNewlinePrefix()
}
Expand Down Expand Up @@ -210,7 +210,7 @@ public class FunctionLiteralRule :
.first { it.elementType == VALUE_PARAMETER }
.lastChildLeafOrSelf()
.nextLeaf { !it.isWhiteSpaceWithoutNewline() && !it.isPartOfComment() }
leavesOnLine()
leavesOnLine(excludeEolComment = true)
.takeWhile { it.prevLeaf() != stopAtLeaf }
.lineLengthWithoutNewlinePrefix()
.let { it > maxLineLength }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,14 @@ public class MaxLineLengthRule :
?.takeUnless { it.isPartOfRawMultiLineString() }
?.takeUnless { it.isLineOnlyContainingSingleTemplateString() }
?.takeUnless { it.isLineOnlyContainingComment() }
?.let { lastNodeOnLine ->
?.let {
// Calculate the offset at the last possible position at which the newline should be inserted on the line
val offset = node.leavesOnLine().first().startOffset + maxLineLength + 1
val offset =
node
.leavesOnLine(excludeEolComment = false)
.first()
.startOffset
.plus(maxLineLength + 1)
emit(
offset,
"Exceeded max line length ($maxLineLength)",
Expand All @@ -95,7 +100,7 @@ public class MaxLineLengthRule :
}

private fun ASTNode.lineLength() =
leavesOnLine()
leavesOnLine(excludeEolComment = false)
.sumOf {
when {
it.isWhiteSpaceWithNewline() -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ 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.column
import com.pinterest.ktlint.rule.engine.core.api.dropTrailingEolComment
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.CODE_STYLE_PROPERTY
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.CodeStyleValue.ktlint_official
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.EditorConfig
Expand All @@ -27,7 +26,7 @@ import com.pinterest.ktlint.rule.engine.core.api.indent
import com.pinterest.ktlint.rule.engine.core.api.isPartOfComment
import com.pinterest.ktlint.rule.engine.core.api.isWhiteSpace
import com.pinterest.ktlint.rule.engine.core.api.isWhiteSpaceWithNewline
import com.pinterest.ktlint.rule.engine.core.api.leavesIncludingSelf
import com.pinterest.ktlint.rule.engine.core.api.leavesOnLine
import com.pinterest.ktlint.rule.engine.core.api.nextLeaf
import com.pinterest.ktlint.rule.engine.core.api.prevCodeLeaf
import com.pinterest.ktlint.rule.engine.core.api.prevLeaf
Expand Down Expand Up @@ -304,14 +303,11 @@ public class ParameterListWrappingRule :
private fun ASTNode.isOnLineExceedingMaxLineLength(): Boolean {
val stopLeaf = nextLeaf { it.textContains('\n') }?.nextLeaf()
val lineContent =
prevLeaf { it.textContains('\n') }
?.leavesIncludingSelf()
?.takeWhile { it.prevLeaf() != stopLeaf }
?.dropTrailingEolComment()
?.joinToString(separator = "") { it.text }
?.substringAfter('\n')
?.substringBefore('\n')
.orEmpty()
leavesOnLine(excludeEolComment = true)
.takeWhile { it.prevLeaf() != stopLeaf }
.joinToString(separator = "") { it.text }
.substringAfter('\n')
.substringBefore('\n')
return lineContent.length > maxLineLength
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -478,4 +478,17 @@ class BinaryExpressionWrappingRuleTest {
LintViolation(5, 62, "Exceeded max line length (61)", false),
).hasNoLintViolationsExceptInAdditionalRules()
}

@Test
fun `Issue 2450 - Given a binary expression including an EOL-comment that causes the max line length to be exceeded then ignore the EOL-comment`() {
val code =
"""
// $MAX_LINE_LENGTH_MARKER $EOL_CHAR
val foo1 = foo() ?: "foooooooooooooooooo" + // some comment
"bar"
""".trimIndent()
binaryExpressionWrappingRuleAssertThat(code)
.setMaxLineLength()
.hasNoLintViolations()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -990,4 +990,25 @@ class ChainMethodContinuationRuleTest {
LintViolation(5, 60, "Exceeded max line length (59)", false),
).hasNoLintViolationsExceptInAdditionalRules()
}

@Test
fun `Issue 2450 - Given a chained method and a chain contains an EOL comment that causes max line length to be exceeded then only report violation via max-line-length rule`() {
val code =
"""
// $MAX_LINE_LENGTH_MARKER $EOL_CHAR
val foo2 =
"foo"
.filter {
it
.uppercase() // Some comment
.isUpperCase()
}.lowercase()
""".trimIndent()
chainMethodContinuationRuleAssertThat(code)
.setMaxLineLength()
.addAdditionalRuleProvider { MaxLineLengthRule() }
.hasLintViolationsForAdditionalRule(
LintViolation(6, 46, "Exceeded max line length (45)", false),
).hasNoLintViolationsExceptInAdditionalRules()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -493,4 +493,17 @@ class FunctionLiteralRuleTest {
""".trimIndent()
functionLiteralRuleAssertThat(code).hasNoLintViolations()
}

@Test
fun `Issue 2450 - Given function literal with an EOL-comment as body then do not throw an exception`() {
val code =
"""
fun foo() {
shouldFail<IllegalArgumentException>(sinceKotlin = "255.255.255") {
// no-op
}
}
""".trimIndent()
functionLiteralRuleAssertThat(code).hasNoLintViolations()
}
}

0 comments on commit 913887d

Please sign in to comment.