Disable special "null" value handling for multiple options default value.
Historically multiple options (with allowMultiple = true) has 2 possible values: null (in case a special default value "null" is used) and an empty list in case of anything else.
This change removes this special null case leaving always an empty list. It is the first step to enable real default values support for multiple options.
No user use cases are affected.
RELNOTES: None.
PiperOrigin-RevId: 301162497
diff --git a/src/main/java/com/google/devtools/common/options/OptionDefinition.java b/src/main/java/com/google/devtools/common/options/OptionDefinition.java
index e89234b..29e3e29 100644
--- a/src/main/java/com/google/devtools/common/options/OptionDefinition.java
+++ b/src/main/java/com/google/devtools/common/options/OptionDefinition.java
@@ -253,19 +253,20 @@
/** Returns the evaluated default value for this option & memoizes the result. */
public Object getDefaultValue() {
- if (defaultValue != null || isSpecialNullDefault()) {
+ if (defaultValue != null) {
return defaultValue;
}
- Converter<?> converter = getConverter();
- String defaultValueAsString = getUnparsedDefaultValue();
- boolean allowsMultiple = allowsMultiple();
// If the option allows multiple values then we intentionally return the empty list as
// the default value of this option since it is not always the case that an option
// that allows multiple values will have a converter that returns a list value.
- if (allowsMultiple) {
+ if (allowsMultiple()) {
defaultValue = Collections.emptyList();
+ } else if (isSpecialNullDefault()) {
+ return null;
} else {
// Otherwise try to convert the default value using the converter
+ Converter<?> converter = getConverter();
+ String defaultValueAsString = getUnparsedDefaultValue();
try {
defaultValue = converter.convert(defaultValueAsString);
} catch (OptionsParsingException e) {
diff --git a/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java b/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java
index fc8985d..75f77b5 100644
--- a/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java
@@ -24,6 +24,7 @@
import com.google.devtools.common.options.Converters.StringConverter;
import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
import com.google.devtools.common.options.OptionsParser.ConstructionException;
+import java.util.List;
import java.util.Map;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -174,4 +175,98 @@
verify(mockOptionDef, times(1)).isSpecialNullDefault();
verify(mockOptionDef, times(2)).getUnparsedDefaultValue();
}
+
+ /** Dummy options class, to test defaultValue handling. */
+ private static class DefaultValueTestOptions extends OptionsBase {
+ @Option(
+ name = "null_non_multiple_option",
+ defaultValue = "null",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = OptionEffectTag.NO_OP)
+ public String nullNonMultipleOption;
+
+ @Option(
+ name = "null_multiple_option",
+ allowMultiple = true,
+ defaultValue = "null",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = OptionEffectTag.NO_OP)
+ public List<String> nullMultipleOption;
+
+ @Option(
+ name = "empty_string_multiple_option",
+ allowMultiple = true,
+ defaultValue = "",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = OptionEffectTag.NO_OP)
+ public List<String> emptyStringMultipleOption;
+
+ @Option(
+ name = "non_empty_string_multiple_option",
+ allowMultiple = true,
+ defaultValue = "text",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = OptionEffectTag.NO_OP)
+ public List<String> nonEmptyStringMultipleOption;
+ }
+
+ @Test
+ public void specialDefaultValueForNonMultipleOptionShouldResultInNull() throws Exception {
+ // arrange
+ OptionDefinition optionDef =
+ OptionDefinition.extractOptionDefinition(
+ DefaultValueTestOptions.class.getField("nullNonMultipleOption"));
+
+ // act
+ Object result = optionDef.getDefaultValue();
+
+ // assert
+ assertThat(result).isNull();
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void specialDefaultValueForMultipleOptionShouldResultInEmptyList() throws Exception {
+ // arrange
+ OptionDefinition optionDef =
+ OptionDefinition.extractOptionDefinition(
+ DefaultValueTestOptions.class.getField("nullMultipleOption"));
+
+ // act
+ List<String> result = (List<String>) optionDef.getDefaultValue();
+
+ // assert
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void emptyStringForMultipleOptionShouldResultInEmptyList() throws Exception {
+ // arrange
+ OptionDefinition optionDef =
+ OptionDefinition.extractOptionDefinition(
+ DefaultValueTestOptions.class.getField("emptyStringMultipleOption"));
+
+ // act
+ List<String> result = (List<String>) optionDef.getDefaultValue();
+
+ // assert
+ assertThat(result).isEmpty();
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void nonEmptyStringForMultipleOptionShouldResultInEmptyList() throws Exception {
+ // arrange
+ OptionDefinition optionDef =
+ OptionDefinition.extractOptionDefinition(
+ DefaultValueTestOptions.class.getField("nonEmptyStringMultipleOption"));
+
+ // act
+ List<String> result = (List<String>) optionDef.getDefaultValue();
+
+ // assert
+ // TODO(b/138573276): this is a legacy behavior - assert the value is actually converted
+ assertThat(result).isEmpty();
+ }
}
