pinterest / ktlint

An anti-bikeshedding Kotlin linter with built-in formatter
https://pinterest.github.io/ktlint/
MIT License
6.19k stars 505 forks source link

Passing the option `--patterns-from-stdin=` throws an IllegalArgumentException #2578

Closed mfederczuk closed 7 months ago

mfederczuk commented 7 months ago

Since version 1.2.0, passing the option --patterns-from-stdin= (empty string as argument) will cause an IllegalArgumentException to be thrown.

Expected Behavior

Executing the command ktlint --patterns-from-stdin= should not throw an exception.

Observed Behavior

Executing the command ktlint --patterns-from-stdin= throws the following exception:

$ ktlint --patterns-from-stdin=
Exception in thread "main" java.lang.IllegalArgumentException: Failed requirement.
    at com.pinterest.ktlint.cli.internal.KtlintCommandLine.readPatternsFromStdin(KtlintCommandLine.kt:641)
    at com.pinterest.ktlint.cli.internal.KtlintCommandLine.replaceWithPatternsFromStdinOrDefaultPatternsWhenEmpty(KtlintCommandLine.kt:340)
    at com.pinterest.ktlint.cli.internal.KtlintCommandLine.lintOrFormat(KtlintCommandLine.kt:252)
    at com.pinterest.ktlint.cli.internal.KtlintCommandLine.run(KtlintCommandLine.kt:244)
    at com.github.ajalt.clikt.parsers.Parser.parse(Parser.kt:279)
    at com.github.ajalt.clikt.parsers.Parser.parse(Parser.kt:41)
    at com.github.ajalt.clikt.core.CliktCommand.parse(CliktCommand.kt:457)
    at com.github.ajalt.clikt.core.CliktCommand.parse$default(CliktCommand.kt:454)
    at com.github.ajalt.clikt.core.CliktCommand.main(CliktCommand.kt:474)
    at com.github.ajalt.clikt.core.CliktCommand.main(CliktCommand.kt:481)
    at com.pinterest.ktlint.Main.main(Main.kt:20)

Steps to Reproduce

Simply execute the command ktlint --patterns-from-stdin=.

Your Environment

mfederczuk commented 7 months ago

Seems like the problem was introduced in commit cb17bbfc6ec3cb8503228c108ec82b679488065f when switching from picocli to clikt: diff

diff --git ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
index 36e84c38..039e4746 100644
--- a/ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
+++ b/ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
[…]
@@ -338,42 +334,46 @@ internal class KtlintCommandLine {
         return EditorConfigDefaults.load(fullyExpandedEditorConfigPath, ruleProviders.propertyTypes())
     }

-    private fun List<String>.replaceWithPatternsFromStdinOrDefaultPatternsWhenEmpty(): List<String> {
-        val localStdinDelimiter: String? = stdinDelimiter
-        return when {
-            localStdinDelimiter != null -> {
-                val stdinPatterns: Set<String> = readPatternsFromStdin(localStdinDelimiter.ifEmpty { "\u0000" })
-                if (isNotEmpty() && stdinPatterns.isNotEmpty()) {
-                    logger.warn {
-                        "Patterns specified at command line ($this) and patterns from 'stdin' due to flag '--patterns-from-stdin' " +
-                            "($stdinPatterns) are merged"
+    private fun List<String>.replaceWithPatternsFromStdinOrDefaultPatternsWhenEmpty(): List<String> =
+        when {
+            patternsFromStdin != null -> {
+                readPatternsFromStdin(patternsFromStdin!!)
+                    .let { stdinPatterns ->
+                        if (stdinPatterns.isNotEmpty()) {
+                            if (isEmpty()) {
+                                logger.debug { "Patterns read from 'stdin' due to flag '--patterns-from-stdin': $stdinPatterns" }
+                            } else {
+                                logger.warn {
+                                    "Patterns specified at command line ($this) and patterns from 'stdin' due to flag " +
+                                        "'--patterns-from-stdin' ($stdinPatterns) are merged"
+                                }
+                            }
+                        }
+                        // Note: it is okay in case both the original patterns and the patterns from stdin are empty
+                        this.plus(stdinPatterns)
                     }
-                }
-                // Note: it is okay in case both the original patterns and the patterns from stdin are empty
-                this.plus(stdinPatterns)
             }
+
             this.isEmpty() -> {
                 logger.info { "Enable default patterns $DEFAULT_PATTERNS" }
                 DEFAULT_PATTERNS
             }
+
             else -> {
                 // Keep original patterns
                 this
             }
         }
-    }
[…]
mfederczuk commented 7 months ago

Either of these changes should fix the issue: (both untested)

diff --git ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
index 8aab056c..184376a2 100644
--- a/ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
+++ b/ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
@@ -337,7 +337,7 @@ internal class KtlintCommandLine :
     private fun List<String>.replaceWithPatternsFromStdinOrDefaultPatternsWhenEmpty(): List<String> =
         when {
             patternsFromStdin != null -> {
-                readPatternsFromStdin(patternsFromStdin!!)
+                readPatternsFromStdin(patternsFromStdin!!.ifEmpty { "\u0000" })
                     .let { stdinPatterns ->
                         if (stdinPatterns.isNotEmpty()) {
                             if (isEmpty()) {
@@ -638,7 +638,9 @@ internal class KtlintCommandLine :
         }

     private fun readPatternsFromStdin(delimiter: String): Set<String> {
-        require(delimiter.isNotEmpty())
+        require(delimiter.isNotEmpty()) {
+            "Stdin delimiter must not be empty"
+        }

         return String(System.`in`.readBytes())
             .split(delimiter)

diff --git ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
index 8aab056c..ae640534 100644
--- a/ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
+++ b/ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
@@ -638,10 +638,8 @@ internal class KtlintCommandLine :
         }

     private fun readPatternsFromStdin(delimiter: String): Set<String> {
-        require(delimiter.isNotEmpty())
-
         return String(System.`in`.readBytes())
-            .split(delimiter)
+            .split(delimiter.ifEmpty { "\u0000" })
             .let { patterns: List<String> ->
                 patterns.filterTo(LinkedHashSet(patterns.size), String::isNotEmpty)
             }
paul-dingemans commented 7 months ago

clikt handles the options a bit differently from picocli. This should have been documented better (sorry). Please run command ktlint --patterns-from-stdin

Example:

$ ktlint-1.2.0 --patterns-from-stdin
**/*.kt
src/main/kotlin/FooComposable.kt:7:5: Function name should start with a lowercase letter (except factory methods) and use camel case (standard:function-naming)
src/main/kotlin/FooComposable.kt:13:5: Function name should start with a lowercase letter (except factory methods) and use camel case (standard:function-naming)
src/main/kotlin/Foo.kt:45:9: Line must not end with "?:" (standard:chain-wrapping)
src/main/kotlin/Foo.kt:46:1: Unexpected indentation (12) (should be 8) (standard:indent)
src/main/kotlin/Foo.kt:48:1: Unexpected indent of multiline string closing quotes (standard:indent)
18:10:59.813 [main] WARN com.pinterest.ktlint.cli.internal.KtlintCommandLine -- Lint has found errors than can be autocorrected using 'ktlint --format'

Summary error count (descending) by rule:
  standard:function-naming: 2
  standard:indent: 2
  standard:chain-wrapping: 1
mfederczuk commented 7 months ago

The previous behavior was that passing the option --patterns-from-stdin with an empty argument (i.e.: a trailing = with nothing else after) would lead to the pattern delimiter being the NUL byte:

$ ktlint --version
1.1.1

$ printf 'foo\0bar\0' | ktlint --patterns-from-stdin=
<timestamp> [main] WARN com.pinterest.ktlint.cli.internal.KtlintCommandLine -- No files matched [foo, bar]

$ printf 'foo\nbar\n' | ktlint --patterns-from-stdin
<timestamp> [main] WARN com.pinterest.ktlint.cli.internal.KtlintCommandLine -- No files matched [foo, bar]

The behavior with no/omitted argument (i.e.: no = at all) is still the same; the newline character is used as default/fallback value for the pattern delimiter:

$ ktlint --version
1.2.0

$ printf 'foo\nbar\n' | ktlint --patterns-from-stdin
<timestamp> [main] WARN com.pinterest.ktlint.cli.internal.KtlintCommandLine -- No files matched [foo, bar]

But I was expecting that the behavior with an empty argument (NUL byte as pattern delimiter) would be the same.

paul-dingemans commented 7 months ago

Yes, I just found the problem is caused by the default value I have provided. I think that I previously did not understand the real behavior of this option.

Your second solution does work:

diff --git ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
index 8aab056c..ae640534 100644
--- a/ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
+++ b/ktlint-cli/src/main/kotlin/com/pinterest/ktlint/cli/internal/KtlintCommandLine.kt
@@ -638,10 +638,8 @@ internal class KtlintCommandLine :
         }

     private fun readPatternsFromStdin(delimiter: String): Set<String> {
-        require(delimiter.isNotEmpty())
-
         return String(System.`in`.readBytes())
-            .split(delimiter)
+            .split(delimiter.ifEmpty { "\u0000" })
             .let { patterns: List<String> ->
                 patterns.filterTo(LinkedHashSet(patterns.size), String::isNotEmpty)
             }

I will restore the old behavior.