Repository: RBusarow/ModuleCheck (kotlin)

Issue:
auto-fix sometimes swallows trailing newline characters
This happens in both Kotlin and Groovy build files.

Given this:

``` kotlin
dependencies {
  implementation(project(path = ":lib1"))

  implementation(project(path = ":lib2"))
  implementation(project(path = ":lib3"))
}
```

The problem happens if the `:lib2` dependency should be switched to `api`.  The important parts in this scenario are:
- There's a blank line above the dependency being changed.
- The dependency being changed is not the first statement (technically anything but a comment) in the `dependencies { ... }` block.
  - Groovy and Kotlin parsing handle the whitespace between the opening curly brace and the first statement differently, so there's already a lot of custom logic for handling that.
- There's another statement (probably another dependency declaration) *after* the the dependency being changed.

The existing logic in `main` or the latest `0.11.3` release will create this after auto-correct:
``` kotlin
dependencies {
  implementation(project(path = ":lib1"))

  api(project(path = ":lib2"))  implementation(project(path = ":lib3"))
}
```

The auto-correct behavior is technically adding the `api(...)` dependency above the existing one, then deleting the existing one. 

The existing logic is basically:
```kotlin
val newDependenciesBlock = oldBlock.replace(oldDependencyStatement + '\n', "")
```
Without the `'\n'`, there would be an extra blank line left behind.

The problem is that for both Kotlin and Groovy, this `\n` belongs to the *next* statement.  This all works out fine if we're just deleting a line, like for an unused dependency.  But when replacing, we don't want to remove it.

Pull request:
fixes #443

Patch:
diff --git a/modulecheck-api/src/main/kotlin/modulecheck/api/finding/dependencies.kt b/modulecheck-api/src/main/kotlin/modulecheck/api/finding/dependencies.kt
index 11d3b7d231..e221d13a68 100644
--- a/modulecheck-api/src/main/kotlin/modulecheck/api/finding/dependencies.kt
+++ b/modulecheck-api/src/main/kotlin/modulecheck/api/finding/dependencies.kt
@@ -31,6 +31,10 @@ fun McProject.addDependency(
   val oldStatement = markerDeclaration.statementWithSurroundingText
   val newStatement = newDeclaration.statementWithSurroundingText
 
+  // the `prefixIfNot("\n")` here is important.
+  // It needs to match what we're doing if we delete a dependency.  Otherwise, we wind up adding
+  // or removing newlines instead of just modifying the dependencies.
+  // See https://github.com/RBusarow/ModuleCheck/issues/443
   val combinedStatement = newStatement.plus(oldStatement.prefixIfNot("\n"))
 
   val buildFileText = buildFile.readText()
@@ -86,7 +90,11 @@ fun McProject.removeDependencyWithDelete(
   val text = buildFile.readText()
 
   buildFile.writeText(
-    text.replaceFirst(declaration.statementWithSurroundingText + '\n', "")
+    // the `prefixIfNot("\n")` here is important.
+    // It needs to match what we're doing if we add a new dependency.  Otherwise, we wind up adding
+    // or removing newlines instead of just modifying the dependencies.
+    // See https://github.com/RBusarow/ModuleCheck/issues/443
+    text.replaceFirst(declaration.statementWithSurroundingText.prefixIfNot("\n"), "")
   )
 
   if (configuredDependency is ConfiguredProjectDependency) {


Test patch:
diff --git a/modulecheck-core/src/test/kotlin/modulecheck/core/MustBeApiTest.kt b/modulecheck-core/src/test/kotlin/modulecheck/core/MustBeApiTest.kt
index 3b98bda988..41d26d2776 100644
--- a/modulecheck-core/src/test/kotlin/modulecheck/core/MustBeApiTest.kt
+++ b/modulecheck-core/src/test/kotlin/modulecheck/core/MustBeApiTest.kt
@@ -19,6 +19,8 @@ import modulecheck.parsing.gradle.ConfigurationName
 import modulecheck.parsing.gradle.SourceSetName
 import modulecheck.runtime.test.ProjectFindingReport.mustBeApi
 import modulecheck.runtime.test.RunnerTest
+import modulecheck.testing.createSafely
+import modulecheck.utils.child
 import org.junit.jupiter.api.Test
 
 class MustBeApiTest : RunnerTest() {
@@ -222,6 +224,268 @@ class MustBeApiTest : RunnerTest() {
     )
   }
 
+  // https://github.com/RBusarow/ModuleCheck/issues/443
+  @Test
+  fun `switching to api dependency after a blank line should preserve all newlines -- kotlin`() {
+
+    val lib1 = project(":lib1") {
+      addSource(
+        "com/modulecheck/lib1/Lib1Class.kt",
+        """
+        package com.modulecheck.lib1
+
+        class Lib1Class
+        """
+      )
+    }
+
+    val lib2 = project(":lib2") {
+
+      buildFile {
+        """
+        plugins {
+          kotlin("jvm")
+        }
+        """
+      }
+
+      addSource(
+        "com/modulecheck/lib2/Lib2Class.kt",
+        """
+        package com.modulecheck.lib2
+
+        class Lib2Class
+        """
+      )
+    }
+
+    val lib3 = project(":lib3") {
+
+      buildFile {
+        """
+        plugins {
+          kotlin("jvm")
+        }
+        """
+      }
+
+      addSource(
+        "com/modulecheck/lib3/Lib3Class.kt",
+        """
+        package com.modulecheck.lib3
+
+        class Lib3Class
+        """
+      )
+    }
+
+    val lib4 = project(":lib4") {
+      addDependency(ConfigurationName.implementation, lib1)
+      addDependency(ConfigurationName.implementation, lib2)
+      addDependency(ConfigurationName.implementation, lib3)
+
+      buildFile {
+        """
+        plugins {
+          kotlin("jvm")
+        }
+
+        dependencies {
+          implementation(project(path = ":lib1"))
+
+          implementation(project(path = ":lib2"))
+          implementation(project(path = ":lib3"))
+        }
+        """
+      }
+
+      addSource(
+        "com/modulecheck/lib4/Lib4Class.kt",
+        """
+        package com.modulecheck.lib4
+
+        import com.modulecheck.lib1.Lib1Class
+        import com.modulecheck.lib2.Lib2Class
+        import com.modulecheck.lib3.Lib3Class
+
+        val lib1Class = Lib1Class()
+        val lib2Class = Lib2Class()
+        val lib3Class = Lib3Class()
+        """
+      )
+    }
+
+    run().isSuccess shouldBe true
+
+    lib4.buildFile shouldHaveText """
+        plugins {
+          kotlin("jvm")
+        }
+
+        dependencies {
+          api(project(path = ":lib1"))
+
+          api(project(path = ":lib2"))
+          api(project(path = ":lib3"))
+        }
+    """
+
+    logger.parsedReport() shouldBe listOf(
+      ":lib4" to listOf(
+        mustBeApi(
+          fixed = true,
+          configuration = "implementation",
+          dependency = ":lib1",
+          position = "6, 3"
+        ),
+        mustBeApi(
+          fixed = true,
+          configuration = "implementation",
+          dependency = ":lib2",
+          position = "8, 3"
+        ),
+        mustBeApi(
+          fixed = true,
+          configuration = "implementation",
+          dependency = ":lib3",
+          position = "9, 3"
+        )
+      )
+    )
+  }
+
+  // https://github.com/RBusarow/ModuleCheck/issues/443
+  @Test
+  fun `switching to api dependency after a blank line should preserve all newlines -- groovy`() {
+
+    val lib1 = project(":lib1") {
+      addSource(
+        "com/modulecheck/lib1/Lib1Class.kt",
+        """
+        package com.modulecheck.lib1
+
+        class Lib1Class
+        """
+      )
+    }
+
+    val lib2 = project(":lib2") {
+
+      buildFile {
+        """
+        plugins {
+          kotlin("jvm")
+        }
+        """
+      }
+
+      addSource(
+        "com/modulecheck/lib2/Lib2Class.kt",
+        """
+        package com.modulecheck.lib2
+
+        class Lib2Class
+        """
+      )
+    }
+
+    val lib3 = project(":lib3") {
+
+      buildFile {
+        """
+        plugins {
+          kotlin("jvm")
+        }
+        """
+      }
+
+      addSource(
+        "com/modulecheck/lib3/Lib3Class.kt",
+        """
+        package com.modulecheck.lib3
+
+        class Lib3Class
+        """
+      )
+    }
+
+    val lib4 = project(":lib4") {
+      addDependency(ConfigurationName.implementation, lib1)
+      addDependency(ConfigurationName.implementation, lib2)
+      addDependency(ConfigurationName.implementation, lib3)
+
+      buildFile.delete()
+      buildFile = projectDir.child("build.gradle")
+        .createSafely(
+          """
+          plugins {
+            id 'org.jetbrains.kotlin.jvm' version '1.6.10'
+          }
+
+          dependencies {
+            implementation project(':lib1')
+
+            implementation project(':lib2')
+            implementation project(':lib3')
+          }
+          """.trimIndent()
+        )
+
+      addSource(
+        "com/modulecheck/lib4/Lib4Class.kt",
+        """
+        package com.modulecheck.lib4
+
+        import com.modulecheck.lib1.Lib1Class
+        import com.modulecheck.lib2.Lib2Class
+        import com.modulecheck.lib3.Lib3Class
+
+        val lib1Class = Lib1Class()
+        val lib2Class = Lib2Class()
+        val lib3Class = Lib3Class()
+        """
+      )
+    }
+
+    run().isSuccess shouldBe true
+
+    lib4.buildFile shouldHaveText """
+        plugins {
+          id 'org.jetbrains.kotlin.jvm' version '1.6.10'
+        }
+
+        dependencies {
+          api project(':lib1')
+
+          api project(':lib2')
+          api project(':lib3')
+        }
+    """
+
+    logger.parsedReport() shouldBe listOf(
+      ":lib4" to listOf(
+        mustBeApi(
+          fixed = true,
+          configuration = "implementation",
+          dependency = ":lib1",
+          position = "6, 3"
+        ),
+        mustBeApi(
+          fixed = true,
+          configuration = "implementation",
+          dependency = ":lib2",
+          position = "8, 3"
+        ),
+        mustBeApi(
+          fixed = true,
+          configuration = "implementation",
+          dependency = ":lib3",
+          position = "9, 3"
+        )
+      )
+    )
+  }
+
   @Test
   fun `private property from implementation with auto-correct should not be changed`() {
 
diff --git a/modulecheck-parsing/psi/src/test/kotlin/modulecheck/parsing/psi/KotlinDependencyBlockParserTest.kt b/modulecheck-parsing/psi/src/test/kotlin/modulecheck/parsing/psi/KotlinDependencyBlockParserTest.kt
index 3242ab1b13..ea834025f9 100644
--- a/modulecheck-parsing/psi/src/test/kotlin/modulecheck/parsing/psi/KotlinDependencyBlockParserTest.kt
+++ b/modulecheck-parsing/psi/src/test/kotlin/modulecheck/parsing/psi/KotlinDependencyBlockParserTest.kt
@@ -188,6 +188,47 @@ internal class KotlinDependencyBlockParserTest : ProjectTest() {
     )
   }
 
+  @Test
+  fun `blank line between dependencies`() {
+    val block = KotlinDependencyBlockParser()
+      .parse(
+        """
+       dependencies {
+          api(testFixtures(project(":lib1")))
+
+          api(testFixtures(project(":lib2")))
+          implementation(testFixtures(project(":lib3")))
+       }
+        """
+      ).single()
+
+    block.settings shouldBe listOf(
+      ModuleDependencyDeclaration(
+        moduleRef = StringRef(":lib1"),
+        moduleAccess = """project(":lib1")""",
+        configName = ConfigurationName.api,
+        declarationText = """api(testFixtures(project(":lib1")))""",
+        statementWithSurroundingText = """   api(testFixtures(project(":lib1")))"""
+      ),
+      ModuleDependencyDeclaration(
+        moduleRef = StringRef(":lib2"),
+        moduleAccess = """project(":lib2")""",
+        configName = ConfigurationName.api,
+        declarationText = """api(testFixtures(project(":lib2")))""",
+        statementWithSurroundingText = """|
+           |   api(testFixtures(project(":lib2")))
+        """.trimMargin()
+      ),
+      ModuleDependencyDeclaration(
+        moduleRef = StringRef(":lib3"),
+        moduleAccess = """project(":lib3")""",
+        configName = ConfigurationName.implementation,
+        declarationText = """implementation(testFixtures(project(":lib3")))""",
+        statementWithSurroundingText = """   implementation(testFixtures(project(":lib3")))"""
+      ),
+    )
+  }
+
   @Test
   fun `string module dependency declaration with testFixtures should be parsed`() {
     val block = KotlinDependencyBlockParser()