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

Improve wrapping of binary expressions #2479

Merged
merged 2 commits into from
Jan 4, 2024
Merged

Improve wrapping of binary expressions #2479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45b762d
Wrapping a binary expression takes precedence on wrapping value argum…
paul-dingemans Jan 1, 2024
19dc168
Wrapping a binary expression takes precedence on wrapping value argum…
paul-dingemans Jan 1, 2024
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
6 changes: 1 addition & 5 deletions ...d/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/rules/ArgumentListWrappingRule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import com.pinterest.ktlint.rule.engine.core.api.editorconfig.INDENT_SIZE_PROPER
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.INDENT_STYLE_PROPERTY
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.MAX_LINE_LENGTH_PROPERTY
import com.pinterest.ktlint.rule.engine.core.api.indent
import com.pinterest.ktlint.rule.engine.core.api.isPartOf
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
Expand Down Expand Up @@ -111,10 +110,7 @@ public class ArgumentListWrappingRule :
// skip lambda arguments
node.treeParent?.elementType != FUNCTION_LITERAL &&
// skip if number of arguments is big (we assume it with a magic number of 8)
node.children().count { it.elementType == VALUE_ARGUMENT } <= 8 &&
// skip if part of a value argument list. It depends on the situation whether it is better to wrap the arguments in the list
// or the operators in the binary expression
!node.isPartOf(BINARY_EXPRESSION)
node.children().count { it.elementType == VALUE_ARGUMENT } <= 8
) {
// each argument should be on a separate line if
// - at least one of the arguments is
Expand Down
167 changes: 114 additions & 53 deletions ...c/main/kotlin/com/pinterest/ktlint/ruleset/standard/rules/BinaryExpressionWrappingRule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,35 +2,42 @@ package com.pinterest.ktlint.ruleset.standard.rules

import com.pinterest.ktlint.rule.engine.core.api.ElementType.BINARY_EXPRESSION
import com.pinterest.ktlint.rule.engine.core.api.ElementType.CALL_EXPRESSION
import com.pinterest.ktlint.rule.engine.core.api.ElementType.CONDITION
import com.pinterest.ktlint.rule.engine.core.api.ElementType.ELVIS
import com.pinterest.ktlint.rule.engine.core.api.ElementType.EQ
import com.pinterest.ktlint.rule.engine.core.api.ElementType.FUN
import com.pinterest.ktlint.rule.engine.core.api.ElementType.FUNCTION_LITERAL
import com.pinterest.ktlint.rule.engine.core.api.ElementType.LAMBDA_ARGUMENT
import com.pinterest.ktlint.rule.engine.core.api.ElementType.LAMBDA_EXPRESSION
import com.pinterest.ktlint.rule.engine.core.api.ElementType.LBRACE
import com.pinterest.ktlint.rule.engine.core.api.ElementType.LONG_STRING_TEMPLATE_ENTRY
import com.pinterest.ktlint.rule.engine.core.api.ElementType.OPERATION_REFERENCE
import com.pinterest.ktlint.rule.engine.core.api.ElementType.PROPERTY
import com.pinterest.ktlint.rule.engine.core.api.ElementType.VALUE_ARGUMENT_LIST
import com.pinterest.ktlint.rule.engine.core.api.ElementType.RBRACE
import com.pinterest.ktlint.rule.engine.core.api.ElementType.VALUE_ARGUMENT
import com.pinterest.ktlint.rule.engine.core.api.IndentConfig
import com.pinterest.ktlint.rule.engine.core.api.Rule
import com.pinterest.ktlint.rule.engine.core.api.Rule.VisitorModifier.RunAfterRule
import com.pinterest.ktlint.rule.engine.core.api.Rule.VisitorModifier.RunAfterRule.Mode.REGARDLESS_WHETHER_RUN_AFTER_RULE_IS_LOADED_OR_DISABLED
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.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
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.MAX_LINE_LENGTH_PROPERTY
import com.pinterest.ktlint.rule.engine.core.api.firstChildLeafOrSelf
import com.pinterest.ktlint.rule.engine.core.api.indent
import com.pinterest.ktlint.rule.engine.core.api.isCodeLeaf
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.nextCodeSibling
import com.pinterest.ktlint.rule.engine.core.api.nextLeaf
import com.pinterest.ktlint.rule.engine.core.api.nextSibling
import com.pinterest.ktlint.rule.engine.core.api.noNewLineInClosedRange
import com.pinterest.ktlint.rule.engine.core.api.parent
import com.pinterest.ktlint.rule.engine.core.api.prevLeaf
import com.pinterest.ktlint.rule.engine.core.api.prevSibling
import com.pinterest.ktlint.rule.engine.core.api.upsertWhitespaceAfterMe
import com.pinterest.ktlint.rule.engine.core.api.upsertWhitespaceBeforeMe
import com.pinterest.ktlint.ruleset.standard.StandardRule
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
Expand All @@ -50,10 +57,6 @@ public class BinaryExpressionWrappingRule :
INDENT_STYLE_PROPERTY,
MAX_LINE_LENGTH_PROPERTY,
),
visitorModifiers =
setOf(
RunAfterRule(ARGUMENT_LIST_WRAPPING_RULE_ID, REGARDLESS_WHETHER_RUN_AFTER_RULE_IS_LOADED_OR_DISABLED),
),
),
Rule.Experimental {
private var indentConfig = IndentConfig.DEFAULT_INDENT_CONFIG
Expand All @@ -74,11 +77,11 @@ public class BinaryExpressionWrappingRule :
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
) {
when (node.elementType) {
BINARY_EXPRESSION -> visitExpression(node, emit, autoCorrect)
BINARY_EXPRESSION -> visitBinaryExpression(node, emit, autoCorrect)
}
}

private fun visitExpression(
private fun visitBinaryExpression(
node: ASTNode,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
autoCorrect: Boolean,
Expand All @@ -101,84 +104,142 @@ public class BinaryExpressionWrappingRule :
true,
)
if (autoCorrect) {
expression.upsertWhitespaceBeforeMe(expression.indent().plus(indentConfig.indent))
expression.upsertWhitespaceBeforeMe(indentConfig.childIndentOf(expression))
}
}

node
.takeIf { it.treeParent.elementType == VALUE_ARGUMENT }
?.takeIf { it.causesMaxLineLengthToBeExceeded() }
?.let { expression ->
emit(
expression.startOffset,
"Line is exceeding max line length. Break line before expression",
true,
)
if (autoCorrect) {
expression.upsertWhitespaceBeforeMe(indentConfig.childIndentOf(expression))
}
}

// When left hand side is a call expression which causes the max line length to be exceeded then first wrap that expression
node
.children()
.firstOrNull { !it.isCodeLeaf() }
?.takeIf { it.elementType == CALL_EXPRESSION }
?.takeIf { it.causesMaxLineLengthToBeExceeded() }
?.let { callExpression -> visitCallExpression(callExpression, emit, autoCorrect) }

// The remainder (operation reference plus right hand side) might still cause the max line length to be exceeded
node
.findChildByType(OPERATION_REFERENCE)
.takeIf { node.lastChildNode.causesMaxLineLengthToBeExceeded() || node.isPartOfConditionExceedingMaxLineLength() }
?.findChildByType(OPERATION_REFERENCE)
?.let { operationReference -> visitOperationReference(operationReference, emit, autoCorrect) }
}

private fun ASTNode.isPartOfConditionExceedingMaxLineLength() =
// Checks that when binary expression itself fits on the line, but the closing parenthesis or opening brace does not fit.
// // Suppose that X is the last possible character on the
// // line X
// if (leftHandSideExpression && rightHandSideExpression) {
treeParent
.takeIf { it.elementType == CONDITION }
?.lastChildLeafOrSelf()
?.nextLeaf { it.isWhiteSpaceWithNewline() }
?.prevLeaf()
?.causesMaxLineLengthToBeExceeded()
?: false

private fun visitCallExpression(
node: ASTNode,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
autoCorrect: Boolean,
) {
node
.takeIf { it.elementType == CALL_EXPRESSION }
?.takeIf { it.treeParent.elementType == BINARY_EXPRESSION }
?.let { callExpression ->
// Breaking the lambda expression has priority over breaking value arguments
callExpression
.findChildByType(LAMBDA_ARGUMENT)
?.findChildByType(LAMBDA_EXPRESSION)
?.findChildByType(FUNCTION_LITERAL)
?.let { functionLiteral ->
functionLiteral
.findChildByType(LBRACE)
?.let { lbrace ->
emit(lbrace.startOffset + 1, "Newline expected after '{'", true)
if (autoCorrect) {
lbrace.upsertWhitespaceAfterMe(indentConfig.childIndentOf(lbrace.treeParent))
}
}
functionLiteral
.findChildByType(RBRACE)
?.let { rbrace ->
emit(rbrace.startOffset, "Newline expected before '}'", true)
if (autoCorrect) {
rbrace.upsertWhitespaceBeforeMe(indentConfig.siblingIndentOf(node.treeParent))
}
}
}
}
}

private fun visitOperationReference(
node: ASTNode,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
autoCorrect: Boolean,
) {
node
.takeIf { it.elementType == OPERATION_REFERENCE }
?.takeIf { it.treeParent.elementType == BINARY_EXPRESSION }
?.takeUnless {
// Allow:
// val foo = "string too long to fit on the line" +
// "more text"
it.nextSibling().isWhiteSpaceWithNewline()
}?.takeIf { it.treeParent.elementType == BINARY_EXPRESSION }
?.takeIf { binaryExpression ->
// Ignore binary expression inside raw string literals. Raw string literals are allowed to exceed max-line-length. Wrapping
// (each) binary expression inside such a literal seems to create more chaos than it resolves.
binaryExpression.parent { it.elementType == LONG_STRING_TEMPLATE_ENTRY } == null
}?.takeIf { it.isOnLineExceedingMaxLineLength() }
?.let { operationReference ->
if (node.isCallExpressionFollowedByLambdaArgument() || cannotBeWrappedAtOperationReference(operationReference)) {
// Wrapping after operation reference might not be the best place in case of a call expression or just won't work as
// the left hand side still does not fit on a single line
val offset =
operationReference
.prevLeaf { it.isWhiteSpaceWithNewline() }
?.let { previousNewlineNode ->
previousNewlineNode.startOffset +
previousNewlineNode.text.indexOfLast { it == '\n' } +
1
}?.let { operationReference ->
if (operationReference.firstChildNode.elementType == ELVIS) {
operationReference
.prevLeaf { it.isWhiteSpace() }
.takeUnless { it.isWhiteSpaceWithNewline() }
?.let {
// Wrapping after the elvis operator leads to violating the 'chain-wrapping' rule, so it must wrapped itself
emit(operationReference.startOffset, "Line is exceeding max line length. Break line before '?:'", true)
if (autoCorrect) {
operationReference.upsertWhitespaceBeforeMe(indentConfig.childIndentOf(operationReference))
}
?: operationReference.startOffset
emit(offset, "Line is exceeding max line length", false)
}
} else {
operationReference
.nextSibling()
?.let { nextSibling ->
emit(
nextSibling.startOffset,
"Line is exceeding max line length. Break line after operator in binary expression",
"Line is exceeding max line length. Break line after '${operationReference.text}' in binary expression",
true,
)
if (autoCorrect) {
nextSibling.upsertWhitespaceBeforeMe(operationReference.indent().plus(indentConfig.indent))
nextSibling.upsertWhitespaceBeforeMe(indentConfig.childIndentOf(operationReference))
}
}
}
}
}

private fun ASTNode.isCallExpressionFollowedByLambdaArgument() =
parent { it.elementType == VALUE_ARGUMENT_LIST }
?.takeIf { it.treeParent.elementType == CALL_EXPRESSION }
?.nextCodeSibling()
.let { it?.elementType == LAMBDA_ARGUMENT }

private fun cannotBeWrappedAtOperationReference(operationReference: ASTNode) =
if (operationReference.firstChildNode.elementType == ELVIS) {
true
} else {
operationReference
.takeUnless { it.nextCodeSibling()?.elementType == BINARY_EXPRESSION }
?.let {
val stopAtOperationReferenceLeaf = operationReference.firstChildLeafOrSelf()
maxLineLength <=
it
.leavesOnLine()
.takeWhile { leaf -> leaf != stopAtOperationReferenceLeaf }
.lengthWithoutNewlinePrefix()
}
?: false
}

private fun ASTNode.isOnLineExceedingMaxLineLength() = leavesOnLine().lengthWithoutNewlinePrefix() > maxLineLength

private fun ASTNode.causesMaxLineLengthToBeExceeded() =
lastChildLeafOrSelf().let { lastChildLeaf ->
leavesOnLine()
.takeWhile { it.prevLeaf() != lastChildLeaf }
.lengthWithoutNewlinePrefix()
} > maxLineLength

private fun Sequence<ASTNode>.lengthWithoutNewlinePrefix() =
joinToString(separator = "") { it.text }
.dropWhile { it == '\n' }
Expand Down
77 changes: 70 additions & 7 deletions ...c/test/kotlin/com/pinterest/ktlint/ruleset/standard/rules/ArgumentListWrappingRuleTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,22 +843,46 @@ class ArgumentListWrappingRuleTest {
}

@Test
fun `Given a property assignment with a binary expression for which the left hand side operator is a function call`() {
fun `Given a property assignment with a binary expression for which the left hand side operator is a function call then binary expression wrapping takes precedence`() {
val code =
"""
// $MAX_LINE_LENGTH_MARKER $EOL_CHAR
val foo1 = foobar(foo * bar) + "foo"
val foo2 = foobar("foo", foo * bar) + "foo"
val foo3 = foobar("fooo", foo * bar) + "foo"
val foo2 = foobar(foo * bar) + "fooo"
val foo3 = foobar("fooooooo", bar) + "foo"
val foo4 = foobar("foooooooo", bar) + "foo"
val foo5 = foobar("fooooooooo", bar) + "foo"
val foo6 = foobar("foooo", foo * bar) + "foo"
val foo7 = foobar("fooooooooooo", foo * bar) + "foo"
""".trimIndent()
val formattedCode =
"""
// $MAX_LINE_LENGTH_MARKER $EOL_CHAR
val foo1 = foobar(foo * bar) + "foo"
val foo2 =
foobar(foo * bar) + "fooo"
val foo3 =
foobar("fooooooo", bar) + "foo"
val foo4 =
foobar("foooooooo", bar) + "foo"
val foo5 =
foobar("fooooooooo", bar) +
"foo"
val foo6 =
foobar("foooo", foo * bar) +
"foo"
val foo7 =
foobar(
"fooooooooooo",
foo * bar
) +
"foo"
""".trimIndent()
argumentListWrappingRuleAssertThat(code)
.setMaxLineLength()
.addAdditionalRuleProvider { BinaryExpressionWrappingRule() }
.addAdditionalRuleProvider { WrappingRule() }
// Although the argument-list-wrapping runs before binary-expression-wrapping, it may not wrap the argument values of a
// function call in case that call is part of a binary expression. It might be better to break the line at the operation
// reference instead.
.hasNoLintViolationsExceptInAdditionalRules()
.isFormattedAs(formattedCode)
}

@Test
Expand Down Expand Up @@ -919,4 +943,43 @@ class ArgumentListWrappingRuleTest {
// 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 =
"""
// $MAX_LINE_LENGTH_MARKER $EOL_CHAR
fun foo() {
every { foo.bar(bazbazbazbazbazbazbazbazbaz) } returns bar
}
""".trimIndent()
val formattedCode =
"""
// $MAX_LINE_LENGTH_MARKER $EOL_CHAR
fun foo() {
every {
foo.bar(bazbazbazbazbazbazbazbazbaz)
} returns bar
}
""".trimIndent()
argumentListWrappingRuleAssertThat(code)
.setMaxLineLength()
.addAdditionalRuleProvider { BinaryExpressionWrappingRule() }
.addAdditionalRuleProvider { WrappingRule() }
// Lint violations from BinaryExpressionWrappingRule and WrappingRule are reported during linting only. When formatting, the
// wrapping of the braces of the function literal by the BinaryExpressionWrapping prevents those violations from occurring.
.hasLintViolationsForAdditionalRule(
LintViolation(3, 12, "Newline expected after '{'"),
LintViolation(3, 12, "Missing newline after \"{\""),
LintViolation(3, 50, "Newline expected before '}'"),
// Lint violation below only occurs during linting. Resolving violations above, prevents the next violation from occurring
LintViolation(3, 59, "Line is exceeding max line length. Break line after 'returns' in binary expression"),
)
// The lint violation below is only reported during lint. When formatting, the violation above is resolved first, and as a
// result this violation will no longer occur.
.hasLintViolations(
LintViolation(3, 21, "Argument should be on a separate line (unless all arguments can fit a single line)"),
LintViolation(3, 48, "Missing newline before \")\""),
).isFormattedAs(formattedCode)
}
}
Loading