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.

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

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

Closed
mfederczuk opened this issue Feb 29, 2024 · 5 comments
Closed

Comments

@mfederczuk
Copy link
Contributor

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

  • Version of ktlint used: 1.2.0
  • Operating System and version: Fedora Linux 39
@mfederczuk
Copy link
Contributor Author

mfederczuk commented Feb 29, 2024

Seems like the problem was introduced in commit cb17bbf 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
Copy link
Contributor Author

mfederczuk commented Feb 29, 2024

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
Copy link
Collaborator

paul-dingemans commented Feb 29, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

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.

mfederczuk added a commit to mfederczuk/ktlint-gradle-plugin that referenced this issue Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants