Repository: cqfn/diKTat
Pull request #983 (2021-07-16 09:42:59)

Bugfix of NPE in complex binary expressions in long line rule
### What's done:
* Fix NPE in long binary expressions: previously in complex cases `binList` could hold incorrect nodes, which had no `binary expression` as first common parent
* Fix out of range error in long string templates
* Add tests

codecov[bot]: # [Codecov](https://codecov.io/gh/cqfn/diKTat/pull/983?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn) Report
> Merging [#983](https://codecov.io/gh/cqfn/diKTat/pull/983?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn) (56cf4fa) into [master](https://codecov.io/gh/cqfn/diKTat/commit/a4ce78f85c92104f2d09af32f69582d17e5349a9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn) (a4ce78f) will **decrease** coverage by `0.00%`.
> The diff coverage is `80.00%`.

[![Impacted file tree graph](https://codecov.io/gh/cqfn/diKTat/pull/983/graphs/tree.svg?width=650&height=150&src=pr&token=P2quRDffpt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn)](https://codecov.io/gh/cqfn/diKTat/pull/983?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn)

```diff
@@             Coverage Diff              @@
##             master     #983      +/-   ##
============================================
- Coverage     83.81%   83.81%   -0.01%     
- Complexity     2410     2413       +3     
============================================
  Files           101      101              
  Lines          6087     6091       +4     
  Branches       1801     1803       +2     
============================================
+ Hits           5102     5105       +3     
  Misses          269      269              
- Partials        716      717       +1     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `83.81% <80.00%> (-0.01%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/cqfn/diKTat/pull/983?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn) | Coverage Δ | |
|---|---|---|
| [...g/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt](https://codecov.io/gh/cqfn/diKTat/pull/983/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn#diff-ZGlrdGF0LXJ1bGVzL3NyYy9tYWluL2tvdGxpbi9vcmcvY3Fmbi9kaWt0YXQvcnVsZXNldC9ydWxlcy9jaGFwdGVyMy9MaW5lTGVuZ3RoLmt0) | `92.09% <80.00%> (-0.28%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/cqfn/diKTat/pull/983?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/cqfn/diKTat/pull/983?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn). Last update [a4ce78f...56cf4fa](https://codecov.io/gh/cqfn/diKTat/pull/983?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cqfn).

kgevorkyan: Despite the fact, that for now we get rid of NPE and cover this case by tests, we still have issue on related functionality https://github.com/cqfn/diKTat/issues/984

I think, we can consider it in future PR

Patch:
diff --git a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt
index 4b948826..6c7d9430 100644
--- a/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt
+++ b/diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter3/LineLength.kt
@@ -30,6 +30,7 @@ import com.pinterest.ktlint.core.ast.ElementType.KDOC_TEXT
 import com.pinterest.ktlint.core.ast.ElementType.LITERAL_STRING_TEMPLATE_ENTRY
 import com.pinterest.ktlint.core.ast.ElementType.LONG_STRING_TEMPLATE_ENTRY
 import com.pinterest.ktlint.core.ast.ElementType.LPAR
+import com.pinterest.ktlint.core.ast.ElementType.NULL
 import com.pinterest.ktlint.core.ast.ElementType.OPERATION_REFERENCE
 import com.pinterest.ktlint.core.ast.ElementType.PACKAGE_DIRECTIVE
 import com.pinterest.ktlint.core.ast.ElementType.PARENTHESIZED
@@ -177,12 +178,15 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
             return LongLineFixableCases.None
         }
         // minus 2 here as we are inserting ` +` and we don't want it to exceed line length
-        val shouldAddTwoSpaces = multiLineOffset == 0 && leftOffset + delimiterIndex > configuration.lineLength.toInt() - 2
+        val shouldAddTwoSpaces = (multiLineOffset == 0) && (leftOffset + delimiterIndex > configuration.lineLength.toInt() - 2)
         val correcterDelimiter = if (shouldAddTwoSpaces) {
             node.text.substring(0, delimiterIndex - 2).lastIndexOf(' ')
         } else {
             delimiterIndex
         }
+        if (correcterDelimiter == -1) {
+            return LongLineFixableCases.None
+        }
         return LongLineFixableCases.StringTemplate(node, correcterDelimiter, multiLineOffset == 0)
     }
 
@@ -330,7 +334,7 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
         binList.forEachIndexed { index, astNode ->
             binaryText += findAllText(astNode)
             if (leftOffset + binaryText.length > wrongBinaryExpression.maximumLineLength && index != 0) {
-                val commonParent = astNode.parent({ it in binList[index - 1].parents() })!!
+                val commonParent = astNode.parent({ it.elementType == BINARY_EXPRESSION && it in binList[index - 1].parents() })!!
                 val nextNode = commonParent.findChildByType(OPERATION_REFERENCE)!!.treeNext
                 if (!nextNode.text.contains("\n")) {
                     commonParent.appendNewlineMergingWhiteSpace(nextNode, nextNode)
@@ -429,10 +433,18 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
         }
     }
 
+    /**
+     * Collect by Depth-first search (DFS) all children to the right side of the equal sign with specific type [propertyList],
+     * by which we can split expression.
+     * Such logic needed, because AST representation of complex conditions is quite loaded
+     *
+     * @param node target node to be processed
+     * @param binList where to store the corresponding results
+     */
     private fun dfsForProperty(node: ASTNode, binList: MutableList<ASTNode>) {
         node.getChildren(null).forEach {
             if (it.elementType in propertyList) {
-                if (it.elementType == REFERENCE_EXPRESSION && it.treeParent.elementType == CALL_EXPRESSION) {
+                if (it.elementType == REFERENCE_EXPRESSION && it.treeParent?.elementType == CALL_EXPRESSION) {
                     binList.add(it.treeParent)
                 } else {
                     binList.add(it)
@@ -479,7 +491,7 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
 
         /**
          * @property node node
-         * @property hasNewLineBefore flag to handle type of comment: ordinary comment(long part of which should be moved to the next line)
+         * @property hasNewLineBefore flag to handle type of comment: ordinary comment (long part of which should be moved to the next line)
          * and inline comments (which should be moved entirely to the previous line)
          * @property indexLastSpace index of last space to substring comment
          */
@@ -525,6 +537,6 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
         private const val STRING_PART_OFFSET = 4
         private val propertyList = listOf(INTEGER_CONSTANT, LITERAL_STRING_TEMPLATE_ENTRY, FLOAT_CONSTANT,
             CHARACTER_CONSTANT, REFERENCE_EXPRESSION, BOOLEAN_CONSTANT, LONG_STRING_TEMPLATE_ENTRY,
-            SHORT_STRING_TEMPLATE_ENTRY)
+            SHORT_STRING_TEMPLATE_ENTRY, NULL)
     }
 }


Test patch:
diff --git a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/LineLengthFixTest.kt b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/LineLengthFixTest.kt
index 6325ed13..3b987b50 100644
--- a/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/LineLengthFixTest.kt
+++ b/diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/LineLengthFixTest.kt
@@ -40,6 +40,11 @@ class LineLengthFixTest : FixTestBase("test/paragraph3/long_line", ::LineLength)
         fixAndCompare("LongLineExpressionExpected.kt", "LongLineExpressionTest.kt", rulesConfigListLineLength)
     }
 
+    @Test
+    fun `should fix complex long binary expressions`() {
+        fixAndCompare("LongBinaryExpressionExpected.kt", "LongBinaryExpressionTest.kt", rulesConfigListLineLength)
+    }
+
     @Test
     fun `should fix long function`() {
         fixAndCompare("LongLineFunExpected.kt", "LongLineFunTest.kt", rulesConfigListLineLength)
diff --git a/diktat-rules/src/test/resources/test/paragraph3/long_line/LongBinaryExpressionExpected.kt b/diktat-rules/src/test/resources/test/paragraph3/long_line/LongBinaryExpressionExpected.kt
new file mode 100644
index 00000000..94c5f6f8
--- /dev/null
+++ b/diktat-rules/src/test/resources/test/paragraph3/long_line/LongBinaryExpressionExpected.kt
@@ -0,0 +1,35 @@
+package test.paragraph3.long_line
+
+fun foo() {
+    val veryLongExpression = Methoooooooooooooooood() +
+ 12345
+
+    val veryLongExpression = Methoooooooooooooooood() ?:
+ null
+
+    val veryLongExpression = a.Methooooood() +
+ b.field
+
+    val variable = someField.filter { it.elementType == KDOC }
+
+    // limit at the left side
+    val variable = a?.filter { it.elementType == KDOC } ?:
+ null
+
+    // limit at the right side
+    val variable = bar?.filter { it.b == c } ?:
+ null
+
+    // limit at the operation reference
+    val variable = field?.filter { bar == foo } ?:
+ null
+
+    val variable = 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
+}
diff --git a/diktat-rules/src/test/resources/test/paragraph3/long_line/LongBinaryExpressionTest.kt b/diktat-rules/src/test/resources/test/paragraph3/long_line/LongBinaryExpressionTest.kt
new file mode 100644
index 00000000..2f143861
--- /dev/null
+++ b/diktat-rules/src/test/resources/test/paragraph3/long_line/LongBinaryExpressionTest.kt
@@ -0,0 +1,26 @@
+package test.paragraph3.long_line
+
+fun foo() {
+    val veryLongExpression = Methoooooooooooooooood() + 12345
+
+    val veryLongExpression = Methoooooooooooooooood() ?: null
+
+    val veryLongExpression = a.Methooooood() + b.field
+
+    val variable = someField.filter { it.elementType == KDOC }
+
+    // limit at the left side
+    val variable = a?.filter { it.elementType == KDOC } ?: null
+
+    // limit at the right side
+    val variable = bar?.filter { it.b == c } ?: null
+
+    // limit at the operation reference
+    val variable = field?.filter { bar == foo } ?: null
+
+    val variable = 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
+}
diff --git a/diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineRValueExpected.kt b/diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineRValueExpected.kt
index a2c047bb..2fd0dfcf 100644
--- a/diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineRValueExpected.kt
+++ b/diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineRValueExpected.kt
@@ -24,6 +24,8 @@ fun foo() {
     val longStringExpression = "First part" +
 "second Part"
 
+    val longStringExpression = "First" + "second Part"
+
     val longStringExpression = "First very long" +
 " part" + "second Part"
 
diff --git a/diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineRValueTest.kt b/diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineRValueTest.kt
index 346e4838..fa6d6c18 100644
--- a/diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineRValueTest.kt
+++ b/diktat-rules/src/test/resources/test/paragraph3/long_line/LongLineRValueTest.kt
@@ -19,6 +19,8 @@ fun foo() {
 
     val longStringExpression = "First part" + "second Part"
 
+    val longStringExpression = "First" + "second Part"
+
     val longStringExpression = "First very long part" + "second Part"
 
     val longStringExpression2 =                   "String starts at the line len limit"