Skip to content

Commit

Permalink
[#1846] Treat char[] as single-value types (Potentially breaking change)
Browse files Browse the repository at this point in the history
  • Loading branch information
@remkop
remkop committed Oct 16, 2022
1 parent 49f389f commit 6099704
Show file tree
Hide file tree
Showing 3 changed files with 125 additions and 23 deletions.
7 changes: 6 additions & 1 deletion picocli-codegen/src/main/java/picocli/codegen/annotation/processing/CompileTimeTypeInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ class CompileTimeTypeInfo implements CommandLine.Model.ITypeInfo {
final boolean isCollection;
final boolean isMap;

final boolean isCharArray;

public CompileTimeTypeInfo(TypeMirror asType) {
typeMirror = asType;

Expand Down Expand Up @@ -68,11 +70,14 @@ public CompileTimeTypeInfo(TypeMirror asType) {
logger.finest("fixed aux (for single type): " + aux);
}
}
isCharArray = false;
} else if (typeMirror.getKind() == TypeKind.ARRAY) {
aux = Arrays.asList(((ArrayType) typeMirror).getComponentType());
actualGenericTypeArguments = Arrays.asList(aux.get(0).toString());
isCharArray = "char".equals(aux.get(0).toString());
} else {
actualGenericTypeArguments = Collections.emptyList();
isCharArray = false;
}
auxTypeMirrors = aux;
typeElement = tempTypeElement;
Expand Down Expand Up @@ -166,7 +171,7 @@ public boolean isMultiValue() {

@Override
public boolean isArray() {
return typeMirror.getKind() == TypeKind.ARRAY;
return typeMirror.getKind() == TypeKind.ARRAY && !isCharArray;
}

@Override
Expand Down
25 changes: 13 additions & 12 deletions src/main/java/picocli/CommandLine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3629,7 +3629,7 @@ public CommandLine setNegatableOptionTransformer(INegatableOptionTransformer tra
private static boolean isBoolean(Class<?>[] types) { return isBoolean(types[0]) || (isOptional(types[0]) && isBoolean(types[1])); }
private static boolean isBoolean(Class<?> type) { return type == Boolean.class || type == Boolean.TYPE; }
private static CommandLine toCommandLine(Object obj, IFactory factory) { return obj instanceof CommandLine ? (CommandLine) obj : new CommandLine(obj, factory, false);}
private static boolean isMultiValue(Class<?> cls) { return cls.isArray() || Collection.class.isAssignableFrom(cls) || Map.class.isAssignableFrom(cls); }
private static boolean isMultiValue(Class<?> cls) { return (cls.isArray() && cls != char[].class) || Collection.class.isAssignableFrom(cls) || Map.class.isAssignableFrom(cls); }
private static boolean isOptional(Class<?> cls) { return cls != null && "java.util.Optional".equals(cls.getName()); } // #1108
private static Object getOptionalEmpty() throws Exception {
return Class.forName("java.util.Optional").getMethod("empty").invoke(null);
Expand Down Expand Up @@ -11112,12 +11112,15 @@ public MethodParam(Method method, int paramIndex) {
public interface ITypeInfo {
/** Returns {@code true} if {@link #getType()} is {@code boolean} or {@code java.lang.Boolean}. */
boolean isBoolean();
/** Returns {@code true} if {@link #getType()} is an array, map or collection. */
/** Returns {@code true} if {@link #getType()} is an array, map or collection.
* Note that from picocli 4.7, {@code char[]} arrays are considered single values (similar to String) and are not treated as arrays.*/
boolean isMultiValue();

/** Returns {@code true} if {@link #getType()} is {@code java.util.Optional}
* @since 4.6 */
boolean isOptional();
/** Returns {@code true} if this type is an array multi-value type.
* Note that from picocli 4.7, {@code char[]} arrays are considered single values (similar to String) and are not treated as arrays.*/
boolean isArray();
boolean isCollection();
boolean isMap();
Expand Down Expand Up @@ -11191,8 +11194,8 @@ public static ITypeInfo create(Class<?> type, Class<?>[] auxiliaryTypes, List<St
}
if (auxiliaryTypes == null || auxiliaryTypes.length == 0) {
if (type.isArray()) {
if (interactive && type.equals(char[].class)) {
auxiliaryTypes = new Class<?>[]{char[].class};
if (type.equals(char[].class)) {
auxiliaryTypes = new Class<?>[]{char[].class}; // TODO is this still needed?
} else {
auxiliaryTypes = new Class<?>[]{type.getComponentType()};
}
Expand Down Expand Up @@ -11279,7 +11282,7 @@ static Class<?>[] extractTypeParameters(ParameterizedType genericType) {

public boolean isBoolean() { return auxiliaryTypes[0] == boolean.class || auxiliaryTypes[0] == Boolean.class; }
public boolean isMultiValue() { return CommandLine.isMultiValue(type); }
public boolean isArray() { return type.isArray(); }
public boolean isArray() { return type.isArray() && type != char[].class; }
public boolean isCollection() { return Collection.class.isAssignableFrom(type); }
public boolean isMap() { return Map.class.isAssignableFrom(type); }
public boolean isOptional() { return CommandLine.isOptional(type); }
Expand Down Expand Up @@ -14065,11 +14068,11 @@ private int applyOption(ArgSpec argSpec,
}

int result;
if (argSpec.type().isArray() && !(argSpec.interactive() && argSpec.type() == char[].class)) {
if (argSpec.typeInfo().isArray()) {
result = applyValuesToArrayField(argSpec, negated, lookBehind, alreadyUnquoted, arity, workingStack, initialized, argDescription);
} else if (Collection.class.isAssignableFrom(argSpec.type())) {
} else if (argSpec.typeInfo().isCollection()) {
result = applyValuesToCollectionField(argSpec, negated, lookBehind, alreadyUnquoted, arity, workingStack, initialized, argDescription);
} else if (Map.class.isAssignableFrom(argSpec.type())) {
} else if (argSpec.typeInfo().isMap()) {
result = applyValuesToMapField(argSpec, lookBehind, alreadyUnquoted, arity, workingStack, initialized, argDescription);
} else {
result = applyValueToSingleValuedField(argSpec, negated, lookBehind, alreadyUnquoted, arity, workingStack, initialized, argDescription);
Expand Down Expand Up @@ -15755,16 +15758,14 @@ static Text concatOptionText(String prefix, Text text, Help.ColorScheme colorSch
Text param = parameterLabelRenderer.renderParameterLabel(option, colorScheme.ansi(), colorScheme.optionParamStyles);
text = text.concat(prefix);

// related: Interpreter#getActualTypeConverter special logic for interactive char[] options... (also: #648)
boolean treatAsSingleValue = char[].class.equals(option.type()) && option.interactive(); // #1834
if (option.required()) { // e.g., -x=VAL
text = text.concat(name).concat(param).concat("");
if (option.isMultiValue() && !treatAsSingleValue) { // e.g., -x=VAL [-x=VAL]...
if (option.isMultiValue()) { // e.g., -x=VAL [-x=VAL]...
text = text.concat(" [").concat(name).concat(param).concat("]...");
}
} else {
text = text.concat("[").concat(name).concat(param).concat("]");
if (option.isMultiValue() && !treatAsSingleValue) { // add ellipsis to show option is repeatable
if (option.isMultiValue()) { // add ellipsis to show option is repeatable
text = text.concat("...");
}
}
Expand Down
116 changes: 106 additions & 10 deletions src/test/java/picocli/ArityTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -480,14 +480,47 @@ class ArrayOptionArity2_nAndParameters {
@Test
public void testArrayOptionArityNConsumeAllArguments() {
class ArrayOptionArityNAndParameters {
@Parameters char[] charParams;
@Parameters int[] intParams;
@Option(names = "-ints", arity = "*") int[] intOptions;
}
ArrayOptionArityNAndParameters
params = CommandLine.populateCommand(new ArrayOptionArityNAndParameters(), "-ints 1 2 3 4".split(" "));
assertArrayEquals(Arrays.toString(params.intOptions),
new int[] {1, 2, 3, 4}, params.intOptions);
assertArrayEquals(null, params.intParams);
}

@Test
public void testCharacterArrayOptionArityNConsumeAllArguments() {
class ArrayOptionArityNAndParameters {
@Parameters Character[] cParams;
@Option(names = "-chars", arity = "*") Character[] cOptions;
}
ArrayOptionArityNAndParameters
params = CommandLine.populateCommand(new ArrayOptionArityNAndParameters(), "-chars a b c d".split(" "));
assertArrayEquals(Arrays.toString(params.cOptions),
new Character[] {'a', 'b', 'c', 'd'}, params.cOptions);
assertArrayEquals(null, params.cParams);
}

@Test
public void testCharArrayOptionArityNConsumeSingleArgDisallowsMultiArgs() {
class ArrayOptionArityNAndParameters {
@Parameters(arity = "0..1") char[] charParams;
@Option(names = "-chars", arity = "*") char[] charOptions;
}
ArrayOptionArityNAndParameters
params = CommandLine.populateCommand(new ArrayOptionArityNAndParameters(), "-chars a b c d".split(" "));
params = CommandLine.populateCommand(new ArrayOptionArityNAndParameters(), "-chars abcd".split(" "));
assertArrayEquals(Arrays.toString(params.charOptions),
new char[] {'a', 'b', 'c', 'd'}, params.charOptions);
new char[] {'a', 'b', 'c', 'd'}, params.charOptions);
assertArrayEquals(null, params.charParams);

try {
CommandLine.populateCommand(new ArrayOptionArityNAndParameters(), "-chars a b c d".split(" "));
fail("expected MissingParameterException");
} catch (UnmatchedArgumentException ex) {
assertEquals("Unmatched arguments from index 3: 'c', 'd'", ex.getMessage());
}
}
@Test
public void testMissingRequiredParams() {
Expand Down Expand Up @@ -1015,19 +1048,82 @@ public void testArrayOptionsWithArity2MayContainMoreThan2Values() {
@Test
public void testArrayOptionWithoutArityConsumesOneArgument() { // #192
class OptionsNoArityAndParameters {
@Parameters char[] charParams;
@Parameters int[] intParams;
@Option(names = "-ints") int[] intOptions;
}
OptionsNoArityAndParameters
params = CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-ints 1 2 3 4".split(" "));
assertArrayEquals(Arrays.toString(params.intOptions),
new int[] {1, }, params.intOptions);
assertArrayEquals(Arrays.toString(params.intParams), new int[] {2, 3, 4}, params.intParams);

// repeated occurrence
params = CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-ints 1 -ints 2 3 4".split(" "));
assertArrayEquals(Arrays.toString(params.intOptions),
new int[] {1, 2, }, params.intOptions);
assertArrayEquals(Arrays.toString(params.intParams), new int[] {3, 4}, params.intParams);

try {
CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-ints".split(" "));
fail("expected MissingParameterException");
} catch (MissingParameterException ok) {
assertEquals("Missing required parameter for option '-ints' (<intOptions>)", ok.getMessage());
assertEquals(1, ok.getMissing().size());
assertTrue(ok.getMissing().get(0).toString(), ok.getMissing().get(0) instanceof Model.OptionSpec);
}
}

@Test
public void testCharacterArrayOptionWithoutArityConsumesOneArgument() { // #192
class OptionsNoArityAndParameters {
@Parameters Character[] cParams;
@Option(names = "-chars") Character[] cOptions;
}
OptionsNoArityAndParameters
params = CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-chars 1 2 3 4".split(" "));
assertArrayEquals(Arrays.toString(params.cOptions),
new Character[] {'1', }, params.cOptions);
assertArrayEquals(Arrays.toString(params.cParams), new Character[] {'2', '3', '4'}, params.cParams);

// repeated occurrence
params = CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-chars 1 -chars 2 3 4".split(" "));
assertArrayEquals(Arrays.toString(params.cOptions),
new Character[] {'1', '2', }, params.cOptions);
assertArrayEquals(Arrays.toString(params.cParams), new Character[] {'3', '4'}, params.cParams);

try {
CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-chars".split(" "));
fail("expected MissingParameterException");
} catch (MissingParameterException ok) {
assertEquals("Missing required parameter for option '-chars' (<cOptions>)", ok.getMessage());
assertEquals(1, ok.getMissing().size());
assertTrue(ok.getMissing().get(0).toString(), ok.getMissing().get(0) instanceof Model.OptionSpec);
}
}

@Test
public void testCharArrayOption() { // #192
class OptionsNoArityAndParameters {
@Parameters(arity = "0..1") char[] charParams;
@Option(names = "-chars") char[] charOptions;
}
OptionsNoArityAndParameters
params = CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-chars a b c d".split(" "));
params = CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-chars abcd".split(" "));
assertArrayEquals(Arrays.toString(params.charOptions),
new char[] {'a', }, params.charOptions);
assertArrayEquals(Arrays.toString(params.charParams), new char[] {'b', 'c', 'd'}, params.charParams);
new char[] {'a', 'b', 'c', 'd'}, params.charOptions);
assertArrayEquals(Arrays.toString(params.charParams), null, params.charParams);

// repeated occurrence
params = CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-chars a -chars b c d".split(" "));
// repeated occurrence disallowed
try {
CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-chars ab -chars cd".split(" "));
fail("Expected exception");
} catch (OverwrittenOptionException ex) {
assertEquals("option '-chars' (<charOptions>) should be specified only once", ex.getMessage());
}

params = CommandLine.populateCommand(new OptionsNoArityAndParameters(), "-chars ab cd".split(" "));
assertArrayEquals(Arrays.toString(params.charOptions),
new char[] {'a', 'b', }, params.charOptions);
new char[] {'a', 'b', }, params.charOptions);
assertArrayEquals(Arrays.toString(params.charParams), new char[] {'c', 'd'}, params.charParams);

try {
Expand Down

0 comments on commit 6099704

Please sign in to comment.