[#1993] Bugfix: fallbackValue=Option.NULL_VALUE did not work for Collection or array options

Closes #1993

Author: remkop

RELEASE-NOTES.md

@@ -21,6 +21,7 @@ Artifacts in this release are signed by Remko Popma (6601 E5C0 8DCC BB96).
 
 ## <a name="4.7.2-fixes"></a> Fixed issues
 * [#1959] API: Add ability to enable loading resource bundles in annotation processor for tests.
+* [#1993] Bugfix: `fallbackValue=Option.NULL_VALUE` did not work for `Collection` or array options. Thanks to [Jiri Daněk](https://github.com/jiridanek) for raising this.
 * [#1975][#1976] Enhancement: Fixed `isJansiConsoleInstalled` performance issue. Thanks to [ChrisTrenkamp](https://github.com/ChrisTrenkamp) for the pull request.
 * [#1932] Enhancement: Move System-Rules tests to Java 5 test module; move System-Lambda tests to Java 8+ test module. Facilitate testing with recent JRE's.
 * [#1984] Enhancement (Kotlin): improve `paramLabel` string auto-generated from Kotlin `internal` methods which have mangled names with embedded "$". Thanks to [Ken Yee](https://github.com/kenyee) for raising this.

src/main/java/picocli/CommandLine.java

@@ -14551,7 +14551,8 @@ private List<Object> consumeArguments(ArgSpec argSpec,
             String fallback = consumed == 0 && argSpec.isOption() && !OptionSpec.DEFAULT_FALLBACK_VALUE.equals(((OptionSpec) argSpec).fallbackValue())
                     ? ((OptionSpec) argSpec).fallbackValue()
                     : null;
-            if (fallback != null && (args.isEmpty() || !varargCanConsumeNextValue(argSpec, args.peek())
+            boolean hasFallback = fallback != null || (argSpec.isOption() && Option.NULL_VALUE.equals(((OptionSpec) argSpec).originalFallbackValue));
+            if (hasFallback && (args.isEmpty() || !varargCanConsumeNextValue(argSpec, args.peek())
                     || (!canConsumeOneArgument(argSpec, lookBehind, alreadyUnquoted, arity, consumed, args.peek(), argDescription)))) {
                 args.push(fallback);
             }

src/test/java/picocli/FallbackTest.java

@@ -11,6 +11,7 @@
 import picocli.CommandLine.ParameterException;
 import picocli.CommandLine.Parameters;
 
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -185,4 +186,45 @@ public void testIssue1904CollectionFallback_positional_map() {
         assertEquals(Collections.singletonList(Issue1904.DebugFacility.DEFAULT), obj.facilities);
         assertEquals(Collections.singletonMap(Issue1904.DebugFacility.FALLBACK, "1"), obj.map);
     }
+
+    @Command
+    static class Issue1993 {
+        @Option(names = {"--list"}, arity = "0..1", fallbackValue = CommandLine.Option.NULL_VALUE)
+        List<String> list;
+
+        @Option(names = {"--array"}, arity = "0..1", fallbackValue = CommandLine.Option.NULL_VALUE)
+        String[] array;
+
+        @Option(names = {"--map"}, arity = "0..1", fallbackValue = "KEY="+CommandLine.Option.NULL_VALUE)
+        Map<String, String> map;
+    }
+
+    @Test
+    public void testIssue1993List() {
+        Issue1993 main = new Issue1993();
+        CommandLine commandLine = new CommandLine(main);
+        commandLine.parseArgs("--list", "--list", "pepa");
+
+        assertEquals(Arrays.asList(null, "pepa"), main.list);
+    }
+
+    @Test
+    public void testIssue1993Array() {
+        Issue1993 main = new Issue1993();
+        CommandLine commandLine = new CommandLine(main);
+        commandLine.parseArgs("--array", "--array", "FOO");
+
+        assertArrayEquals(new String[]{null, "FOO"}, main.array);
+    }
+
+    @Test
+    public void testIssue1993Map() {
+        Issue1993 main = new Issue1993();
+        CommandLine commandLine = new CommandLine(main);
+        commandLine.parseArgs("--map", "--map", "FOO=123");
+
+        // Should this sentinel value be replaced with Java `null`?
+        Map<String, String> expected = TestUtil.mapOf("KEY", CommandLine.Option.NULL_VALUE, "FOO", "123");
+        assertEquals(expected, main.map);
+    }
 }