Skip to content

Commit

Permalink
Fixed LineLength (#1759)
Browse files Browse the repository at this point in the history 
### What's done:
- Fixed bug when fix-result of rule `LineLength` execution still contained `LineLength` warnings. To fix this, added loop that trying to fix `LineLength` rule warnings until warnings run out.
- Added `MAX_FIX_NUMBER` for limit on fixing to avoid infinite loop. If the limit is reached, then error message is displayed.
- Added `unFix()` method realization inside `FunAndProperty` and `ValueArgumentList` classes to undo incorrect unnecessary fixes.
- Fixed bug for case when long comment followed multiline property initialization.
- Fixed and reworked several fix tests.

It's part of #1737
  • Loading branch information
@DrAlexD
DrAlexD authored Oct 16, 2023
1 parent 71c81d4 commit acb3a1c
Show file tree
Hide file tree
Showing 9 changed files with 162 additions and 42 deletions.
136 changes: 121 additions & 15 deletions diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter3/LineLength.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,23 @@ import com.saveourtool.diktat.ruleset.rules.DiktatRule
import com.saveourtool.diktat.ruleset.utils.KotlinParser
import com.saveourtool.diktat.ruleset.utils.appendNewlineMergingWhiteSpace
import com.saveourtool.diktat.ruleset.utils.calculateLineColByOffset
import com.saveourtool.diktat.ruleset.utils.countCodeLines
import com.saveourtool.diktat.ruleset.utils.findAllNodesWithConditionOnLine
import com.saveourtool.diktat.ruleset.utils.findChildAfter
import com.saveourtool.diktat.ruleset.utils.findChildBefore
import com.saveourtool.diktat.ruleset.utils.findChildrenMatching
import com.saveourtool.diktat.ruleset.utils.findParentNodeWithSpecificType
import com.saveourtool.diktat.ruleset.utils.getAllChildrenWithType
import com.saveourtool.diktat.ruleset.utils.getFirstChildWithType
import com.saveourtool.diktat.ruleset.utils.getLineNumber
import com.saveourtool.diktat.ruleset.utils.hasChildOfType
import com.saveourtool.diktat.ruleset.utils.isChildAfterAnother
import com.saveourtool.diktat.ruleset.utils.isWhiteSpace
import com.saveourtool.diktat.ruleset.utils.isWhiteSpaceWithNewline
import com.saveourtool.diktat.ruleset.utils.nextSibling
import com.saveourtool.diktat.ruleset.utils.prevSibling

import io.github.oshai.kotlinlogging.KotlinLogging
import org.jetbrains.kotlin.KtNodeTypes.BINARY_EXPRESSION
import org.jetbrains.kotlin.KtNodeTypes.BLOCK
import org.jetbrains.kotlin.KtNodeTypes.DOT_QUALIFIED_EXPRESSION
Expand All @@ -38,7 +46,6 @@ import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.PsiWhiteSpaceImpl
import org.jetbrains.kotlin.com.intellij.psi.tree.IElementType
import org.jetbrains.kotlin.kdoc.lexer.KDocTokens
import org.jetbrains.kotlin.kdoc.lexer.KDocTokens.MARKDOWN_INLINE_LINK
import org.jetbrains.kotlin.kdoc.lexer.KDocTokens.TEXT
import org.jetbrains.kotlin.lexer.KtTokens.ANDAND
Expand Down Expand Up @@ -89,42 +96,97 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
private lateinit var positionByOffset: (Int) -> Pair<Int, Int>

override fun logic(node: ASTNode) {
if (node.elementType == KtFileElementType.INSTANCE) {
node.getChildren(null).forEach {
if (it.elementType != PACKAGE_DIRECTIVE && it.elementType != IMPORT_LIST) {
checkLength(it, configuration)
var currentFixNumber = 0
var isFixedSmthInPreviousStep: Boolean

// loop that trying to fix LineLength rule warnings until warnings run out
do {
isFixedSmthInPreviousStep = false
currentFixNumber++

if (node.elementType == KtFileElementType.INSTANCE) {
node.getChildren(null).forEach {
if (it.elementType != PACKAGE_DIRECTIVE && it.elementType != IMPORT_LIST) {
val isFixedSmthInChildNode = checkLength(it, configuration)

if (!isFixedSmthInPreviousStep && isFixedSmthInChildNode) {
isFixedSmthInPreviousStep = true
}
}
}
}
} while (isFixedSmthInPreviousStep && currentFixNumber < MAX_FIX_NUMBER)

if (currentFixNumber == MAX_FIX_NUMBER) {
log.error {
"The limit on the number of consecutive fixes has been reached. There may be a bug causing an endless loop of fixes."
}
}
}

@Suppress("UnsafeCallOnNullableType", "TOO_LONG_FUNCTION")
private fun checkLength(node: ASTNode, configuration: LineLengthConfiguration) {
@Suppress(
"UnsafeCallOnNullableType",
"TOO_LONG_FUNCTION",
"FUNCTION_BOOLEAN_PREFIX"
)
private fun checkLength(node: ASTNode, configuration: LineLengthConfiguration): Boolean {
var isFixedSmthInChildNode = false

var offset = 0
node.text.lines().forEach { line ->
if (line.length > configuration.lineLength) {
val newNode = node.psi.findElementAt(offset + configuration.lineLength.toInt() - 1)!!.node

if ((newNode.elementType != TEXT && newNode.elementType != MARKDOWN_INLINE_LINK) || !isKdocValid(newNode)) {
positionByOffset = node.treeParent.calculateLineColByOffset()

val fixableType = isFixable(newNode, configuration)

LONG_LINE.warnOnlyOrWarnAndFix(
configRules, emitWarn,
"max line length ${configuration.lineLength}, but was ${line.length}",
offset + node.startOffset, node,
shouldBeAutoCorrected = fixableType !is None,
isFixMode,
) {
// we should keep in mind, that in the course of fixing we change the offset
val textBeforeFix = node.text
val textLenBeforeFix = node.textLength
val blankLinesBeforeFix = node.text.lines().size - countCodeLines(node)

fixableType.fix()

val textAfterFix = node.text
val textLenAfterFix = node.textLength
// offset for all next nodes changed to this delta
offset += (textLenAfterFix - textLenBeforeFix)
val blankLinesAfterFix = node.text.lines().size - countCodeLines(node)

// checking that any fix may have been made
isFixedSmthInChildNode = fixableType !is None

// for cases when text doesn't change, and then we need to stop fixes
if (textBeforeFix == textAfterFix) {
isFixedSmthInChildNode = false
}

// in some kernel cases of long lines, when in fix step we adding `\n` to certain place of the line
// and part of the line is transferred to the new line, this part may still be too long,
// and then in next fix step we can start generating unnecessary blank lines,
// to detect this we count blank lines and make unfix, if necessary
if (blankLinesAfterFix > blankLinesBeforeFix) {
isFixedSmthInChildNode = false
fixableType.unFix()
} else {
// we should keep in mind, that in the course of fixing we change the offset
// offset for all next nodes changed to this delta
offset += (textLenAfterFix - textLenBeforeFix)
}
}
}
}

offset += line.length + 1
}

return isFixedSmthInChildNode
}

@Suppress(
Expand Down Expand Up @@ -342,7 +404,7 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(

// fixme json method
private fun isKdocValid(node: ASTNode) = try {
if (node.elementType == KDocTokens.TEXT) {
if (node.elementType == TEXT) {
URL(node.text.split("\\s".toRegex()).last { it.isNotEmpty() })
} else {
URL(node.text.substring(node.text.indexOfFirst { it == ']' } + 2, node.textLength - 1))
Expand Down Expand Up @@ -458,10 +520,18 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
* Abstract fix - fix anything nodes
*/
abstract fun fix()

/**
* Function unFix - unfix incorrect unnecessary fix-changes
*/
@Suppress("EmptyFunctionBlock")
open fun unFix() {
// Nothing to do here by default.
}
}

/**
* Class None show error long line have unidentified type or something else that we can't analyze
* Class None show error that long line have unidentified type or something else that we can't analyze
*/
private class None : LongLineFixableCases(KotlinParser().createNode("ERROR")) {
@Suppress("EmptyFunctionBlock")
Expand Down Expand Up @@ -493,9 +563,19 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
if (node.treePrev.isWhiteSpace()) {
node.treeParent.removeChild(node.treePrev)
}
val newLineNodeOnPreviousLine = node.findAllNodesWithConditionOnLine(node.getLineNumber() - 1) {
it.elementType == WHITE_SPACE && it.textContains('\n')
}?.lastOrNull()

// for cases when property has multiline initialization, and then we need to move comment before first line
val newLineNodeOnPreviousLine = if (node.treeParent.elementType == PROPERTY) {
node.treeParent.treeParent.findChildrenMatching {
it.elementType == WHITE_SPACE && node.treeParent.treeParent.isChildAfterAnother(node.treeParent, it) && it.textContains('\n')
}
.lastOrNull()
} else {
node.findAllNodesWithConditionOnLine(node.getLineNumber() - 1) {
it.elementType == WHITE_SPACE && it.textContains('\n')
}?.lastOrNull()
}

newLineNodeOnPreviousLine?.let {
val parent = node.treeParent
parent.removeChild(node)
Expand Down Expand Up @@ -667,6 +747,18 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
override fun fix() {
node.appendNewlineMergingWhiteSpace(null, node.findChildByType(EQ)?.treeNext)
}

override fun unFix() {
node.findChildAfter(EQ, WHITE_SPACE)?.let { correctWhiteSpace ->
if (correctWhiteSpace.textContains('\n')) {
correctWhiteSpace.nextSibling()?.let { wrongWhiteSpace ->
if (wrongWhiteSpace.textContains('\n')) {
node.removeChild(wrongWhiteSpace)
}
}
}
}
}
}

/**
Expand Down Expand Up @@ -736,6 +828,18 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
}
}

override fun unFix() {
node.findChildBefore(RPAR, WHITE_SPACE)?.let { correctWhiteSpace ->
if (correctWhiteSpace.textContains('\n')) {
correctWhiteSpace.prevSibling()?.let { wrongWhiteSpace ->
if (wrongWhiteSpace.textContains('\n')) {
node.removeChild(wrongWhiteSpace)
}
}
}
}
}

private fun fixFirst(): Int {
val lineLength = maximumLineLength.lineLength
var startOffset = 0
Expand Down Expand Up @@ -768,6 +872,8 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
}

companion object {
private val log = KotlinLogging.logger {}
private const val MAX_FIX_NUMBER = 10
private const val MAX_LENGTH = 120L
const val NAME_ID = "line-length"
}
Expand Down
14 changes: 1 addition & 13 deletions diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter3/LineLengthFixTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import com.saveourtool.diktat.common.config.rules.RulesConfig
import com.saveourtool.diktat.ruleset.constants.Warnings.LONG_LINE
import com.saveourtool.diktat.ruleset.rules.chapter3.LineLength
import com.saveourtool.diktat.util.FixTestBase
import org.junit.jupiter.api.Disabled
import org.junit.jupiter.api.Test

class LineLengthFixTest : FixTestBase("test/paragraph3/long_line", ::LineLength) {
Expand All @@ -29,7 +28,6 @@ class LineLengthFixTest : FixTestBase("test/paragraph3/long_line", ::LineLength)
mapOf("lineLength" to "151"))
)

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `should fix long comment`() {
fixAndCompare("LongLineCommentExpected.kt", "LongLineCommentTest.kt", rulesConfigListLineLength)
Expand All @@ -40,55 +38,46 @@ class LineLengthFixTest : FixTestBase("test/paragraph3/long_line", ::LineLength)
fixAndCompare("LongInlineCommentsExpected.kt", "LongInlineCommentsTest.kt", rulesConfigListLineLength)
}

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `should not fix long comment which located on the line length limit`() {
fun `should fix long comment 2`() {
fixAndCompare("LongLineCommentExpected2.kt", "LongLineCommentTest2.kt", rulesConfigListDefaultLineLength)
}

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `should fix long string template while some fix is already done`() {
fixAndCompare("LongStringTemplateExpected.kt", "LongStringTemplateTest.kt", rulesConfigListDefaultLineLength)
}

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `should fix long binary expression`() {
fixAndCompare("LongLineExpressionExpected.kt", "LongLineExpressionTest.kt", rulesConfigListLineLength)
}

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `should fix complex long binary expressions`() {
fixAndCompare("LongBinaryExpressionExpected.kt", "LongBinaryExpressionTest.kt", rulesConfigListLineLength)
}

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `should fix long function`() {
fixAndCompare("LongLineFunExpected.kt", "LongLineFunTest.kt", rulesConfigListLineLength)
}

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `should fix long right value`() {
fixAndCompare("LongLineRValueExpected.kt", "LongLineRValueTest.kt", rulesConfigListLineLength)
}

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `should fix short long right value`() {
fixAndCompare("LongShortRValueExpected.kt", "LongShortRValueTest.kt", rulesConfigListShortLineLength)
}

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `shouldn't fix`() {
fixAndCompare("LongExpressionNoFixExpected.kt", "LongExpressionNoFixTest.kt", rulesConfigListShortLineLength)
}

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `should fix annotation`() {
fixAndCompare("LongLineAnnotationExpected.kt", "LongLineAnnotationTest.kt", rulesConfigListLineLength)
Expand All @@ -104,7 +93,6 @@ class LineLengthFixTest : FixTestBase("test/paragraph3/long_line", ::LineLength)
fixAndCompare("LongExpressionInConditionExpected.kt", "LongExpressionInConditionTest.kt", rulesConfigListLineLength)
}

@Disabled("https://github.com/saveourtool/diktat/issues/1737")
@Test
fun `fix long Dot Qualified Expression`() {
fixAndCompare("LongDotQualifiedExpressionExpected.kt", "LongDotQualifiedExpressionTest.kt", rulesConfigListLineLength)
Expand Down
6 changes: 4 additions & 2 deletions diktat-rules/src/test/resources/test/paragraph3/long_line/LongBinaryExpressionExpected.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@ fun foo() {
?: 123 + Methood().linelength

val variable =
Methoooooooooooooooooooooooooood() ?: "some loooooong string"
Methoooooooooooooooooooooooooood()
?: "some loooooong string"

val variable = Methooooood()
?: "some looong string"

var headerKdoc = firstCodeNode.prevSibling {
it.elementType == KDOC
} ?: if (firstCodeNode == packageDirectiveNode) importsList?.prevSibling { it.elementType == KDOC } else null
}
?: if (firstCodeNode == packageDirectiveNode) importsList?.prevSibling { it.elementType == KDOC } else null
}
4 changes: 3 additions & 1 deletion diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineAnnotationExpected.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,6 @@ value = "select * from test inner join test_execution on test.id = test_executio
nativeQuery = true
)
fun some(limit: Int, offset: Int, executionId: Long) =
println("testtesttesttesttesttesttesttesttesttesttesttest")
println(
"testtesttesttesttesttesttesttesttesttesttesttest"
)
6 changes: 4 additions & 2 deletions diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineCommentExpected.kt
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package test.paragraph3.long_line

// Hello World! This is a first part of comment.
// This is a very long comment that cannot be split
// This is a very long comment that cannot be
// split
fun foo() {
val namesList = listOf<String>("Jack", "Nick")
namesList.forEach { name ->
if (name == "Nick") {
namesList.map {
// This is another comment inside map
// This is another comment
// inside map
it.subSequence(0, 1)
it.split("this is long regex") // this comment start to the right of max length
}
Expand Down
15 changes: 13 additions & 2 deletions diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineCommentExpected2.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,19 @@
*/
class ImplicitBackingPropertyRuleTest(configRules: List<RulesConfig>) {
private fun validateAccessors(node: ASTNode, propsWithBackSymbol: List<String>) {
val accessors =
node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) } // Comment, which shouldn't be moved
// Comment, which should be moved
val accessors =
node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) }
// Comment, which should be moved
val accessors2 =
node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) }
var accessors3 = node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) }
.forEach { handleGetAccessors(it, node, propsWithBackSymbol) } // Comment, which shouldn't be moved
// Comment, which should be moved
var accessors4 = node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) }
.forEach { handleGetAccessors(it, node, propsWithBackSymbol) }
var accessors5 = node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) }
.forEach { handleGetAccessors(it, node, propsWithBackSymbol) } // Comment, which shouldn't be moved
accessors.filter { it.hasChildOfType(GET_KEYWORD) }.forEach {
handleGetAccessors(it, node, propsWithBackSymbol)
}
Expand Down
7 changes: 6 additions & 1 deletion diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineCommentTest2.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
*/
class ImplicitBackingPropertyRuleTest(configRules: List<RulesConfig>) {
private fun validateAccessors(node: ASTNode, propsWithBackSymbol: List<String>) {
val accessors = node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) } // Comment, which shouldn't be moved
val accessors = node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) } // Comment, which should be moved
val accessors2 =
node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) } // Comment, which should be moved
var accessors3 = node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) }.forEach { handleGetAccessors(it, node, propsWithBackSymbol) } // Comment, which shouldn't be moved
var accessors4 = node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) }.forEach { handleGetAccessors(it, node, propsWithBackSymbol) } // Comment, which should be moved
var accessors5 = node.findAllDescendantsWithSpecificType(PROPERTY_ACCESSOR).filter { it.hasChildOfType(BLOCK) }.forEach { handleGetAccessors(it, node, propsWithBackSymbol) } // Comment, which shouldn't be moved
accessors.filter { it.hasChildOfType(GET_KEYWORD) }.forEach { handleGetAccessors(it, node, propsWithBackSymbol) }
accessors.filter { it.hasChildOfType(SET_KEYWORD) }.forEach { handleSetAccessors(it, node, propsWithBackSymbol) }
}
Expand Down
Loading

0 comments on commit acb3a1c

Please sign in to comment.