Option value tracking should refer to the option definitions, not just track option name.
UnparsedOptionValueDescription and OptionValueDescription both had redundant information about options, since they tracked options by their names, and not their definition. Remove the redundancy.
For getEffectiveOptions, the old behavior was relying on the fact that all options were given allowmulitple=false, which was wrong, so stop passing in the default value where it would get confused with values that have been created and tracked by the options parser. Instead, passing value=null is the way to indicate that an option is unset.
RELNOTES: None.
PiperOrigin-RevId: 168038067
GitOrigin-RevId: 95c1ee3261c6cd2263a954dc45611248ca4d5ce7
Change-Id: I2e9c6fd2580fcdc3eec2795d8b9c0d95c05905b4
diff --git a/java/com/google/devtools/common/options/OptionsParser.java b/java/com/google/devtools/common/options/OptionsParser.java
index 6ad7bb2..54642a1 100644
--- a/java/com/google/devtools/common/options/OptionsParser.java
+++ b/java/com/google/devtools/common/options/OptionsParser.java
@@ -272,36 +272,64 @@
* by which other option.
*/
public static class OptionValueDescription {
- private final String name;
+ private final OptionDefinition optionDefinition;
+ private final boolean isDefaultValue;
@Nullable private final String originalValueString;
@Nullable private final Object value;
@Nullable private final OptionPriority priority;
@Nullable private final String source;
@Nullable private final String implicitDependant;
@Nullable private final String expandedFrom;
- private final boolean allowMultiple;
- public OptionValueDescription(
- String name,
+ private OptionValueDescription(
+ OptionDefinition optionDefinition,
+ boolean isDefaultValue,
@Nullable String originalValueString,
@Nullable Object value,
@Nullable OptionPriority priority,
@Nullable String source,
@Nullable String implicitDependant,
- @Nullable String expandedFrom,
- boolean allowMultiple) {
- this.name = name;
+ @Nullable String expandedFrom) {
+ this.optionDefinition = optionDefinition;
+ this.isDefaultValue = isDefaultValue;
this.originalValueString = originalValueString;
this.value = value;
this.priority = priority;
this.source = source;
this.implicitDependant = implicitDependant;
this.expandedFrom = expandedFrom;
- this.allowMultiple = allowMultiple;
+ }
+
+ public static OptionValueDescription newOptionValue(
+ OptionDefinition optionDefinition,
+ @Nullable String originalValueString,
+ @Nullable Object value,
+ @Nullable OptionPriority priority,
+ @Nullable String source,
+ @Nullable String implicitDependant,
+ @Nullable String expandedFrom) {
+ return new OptionValueDescription(
+ optionDefinition,
+ false,
+ originalValueString,
+ value,
+ priority,
+ source,
+ implicitDependant,
+ expandedFrom);
+ }
+
+ public static OptionValueDescription newDefaultValue(OptionDefinition optionDefinition) {
+ return new OptionValueDescription(
+ optionDefinition, true, null, null, OptionPriority.DEFAULT, null, null, null);
+ }
+
+ public OptionDefinition getOptionDefinition() {
+ return optionDefinition;
}
public String getName() {
- return name;
+ return optionDefinition.getOptionName();
}
public String getOriginalValueString() {
@@ -312,7 +340,11 @@
// options use unchecked ListMultimaps due to limitations of Java generics.
@SuppressWarnings({"unchecked", "rawtypes"})
public Object getValue() {
- if (allowMultiple) {
+ if (isDefaultValue) {
+ // If no value was present, we want the default value for this option.
+ return optionDefinition.getDefaultValue();
+ }
+ if (getAllowMultiple() && value != null) {
// Sort the results by option priority and return them in a new list.
// The generic type of the list is not known at runtime, so we can't
// use it here. It was already checked in the constructor, so this is
@@ -362,29 +394,38 @@
}
public boolean getAllowMultiple() {
- return allowMultiple;
+ return optionDefinition.allowsMultiple();
}
@Override
public String toString() {
StringBuilder result = new StringBuilder();
- result.append("option '").append(name).append("' ");
- result.append("set to '").append(value).append("' ");
- result.append("with priority ").append(priority);
- if (source != null) {
- result.append(" and source '").append(source).append("'");
+ result.append("option '").append(optionDefinition.getOptionName()).append("' ");
+ if (isDefaultValue) {
+ result
+ .append("set to its default value: '")
+ .append(optionDefinition.getUnparsedDefaultValue())
+ .append("'");
+ return result.toString();
+ } else {
+ result.append("set to '").append(value).append("' ");
+ result.append("with priority ").append(priority);
+ if (source != null) {
+ result.append(" and source '").append(source).append("'");
+ }
+ if (implicitDependant != null) {
+ result.append(" implicitly by ");
+ }
+ return result.toString();
}
- if (implicitDependant != null) {
- result.append(" implicitly by ");
- }
- return result.toString();
}
// Need to suppress unchecked warnings, because the "multiple occurrence"
// options use unchecked ListMultimaps due to limitations of Java generics.
@SuppressWarnings({"unchecked", "rawtypes"})
void addValue(OptionPriority addedPriority, Object addedValue) {
- Preconditions.checkState(allowMultiple);
+ Preconditions.checkState(optionDefinition.allowsMultiple());
+ Preconditions.checkState(!isDefaultValue);
ListMultimap optionValueList = (ListMultimap) value;
if (addedValue instanceof List<?>) {
optionValueList.putAll(addedPriority, (List<?>) addedValue);
@@ -402,21 +443,18 @@
* <p>Note that the unparsed value and the source parameters can both be null.
*/
public static class UnparsedOptionValueDescription {
- private final String name;
private final OptionDefinition optionDefinition;
- private final String unparsedValue;
+ @Nullable private final String unparsedValue;
private final OptionPriority priority;
- private final String source;
+ @Nullable private final String source;
private final boolean explicit;
public UnparsedOptionValueDescription(
- String name,
OptionDefinition optionDefinition,
- String unparsedValue,
+ @Nullable String unparsedValue,
OptionPriority priority,
- String source,
+ @Nullable String source,
boolean explicit) {
- this.name = name;
this.optionDefinition = optionDefinition;
this.unparsedValue = unparsedValue;
this.priority = priority;
@@ -425,7 +463,7 @@
}
public String getName() {
- return name;
+ return optionDefinition.getOptionName();
}
OptionDefinition getOptionDefinition() {
return optionDefinition;
@@ -460,10 +498,6 @@
return optionDefinition.getImplicitRequirements().length > 0;
}
- boolean allowMultiple() {
- return optionDefinition.allowsMultiple();
- }
-
public String getUnparsedValue() {
return unparsedValue;
}
@@ -483,7 +517,7 @@
@Override
public String toString() {
StringBuilder result = new StringBuilder();
- result.append("option '").append(name).append("' ");
+ result.append("option '").append(optionDefinition.getOptionName()).append("' ");
result.append("set to '").append(unparsedValue).append("' ");
result.append("with priority ").append(priority);
if (source != null) {
diff --git a/java/com/google/devtools/common/options/OptionsParserImpl.java b/java/com/google/devtools/common/options/OptionsParserImpl.java
index 6ae8170..0260606 100644
--- a/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -169,21 +169,10 @@
List<OptionValueDescription> asListOfEffectiveOptions() {
List<OptionValueDescription> result = new ArrayList<>();
for (Map.Entry<String, OptionDefinition> mapEntry : optionsData.getAllOptionDefinitions()) {
- String fieldName = mapEntry.getKey();
OptionDefinition optionDefinition = mapEntry.getValue();
OptionValueDescription entry = parsedValues.get(optionDefinition);
if (entry == null) {
- Object value = optionDefinition.getDefaultValue();
- result.add(
- new OptionValueDescription(
- fieldName,
- /*originalValueString=*/ null,
- value,
- OptionPriority.DEFAULT,
- /*source=*/ null,
- /*implicitDependant=*/ null,
- /*expandedFrom=*/ null,
- false));
+ result.add(OptionValueDescription.newDefaultValue(optionDefinition));
} else {
result.add(entry);
}
@@ -262,21 +251,20 @@
// Record the new value:
parsedValues.put(
optionDefinition,
- new OptionValueDescription(
- name, null, value, priority, source, implicitDependant, expandedFrom, false));
+ OptionValueDescription.newOptionValue(
+ optionDefinition, null, value, priority, source, implicitDependant, expandedFrom));
}
} else {
parsedValues.put(
optionDefinition,
- new OptionValueDescription(
- name, null, value, priority, source, implicitDependant, expandedFrom, false));
+ OptionValueDescription.newOptionValue(
+ optionDefinition, null, value, priority, source, implicitDependant, expandedFrom));
maybeAddDeprecationWarning(optionDefinition);
}
}
private void addListValue(
OptionDefinition optionDefinition,
- String originalName,
Object value,
OptionPriority priority,
String source,
@@ -285,15 +273,14 @@
OptionValueDescription entry = parsedValues.get(optionDefinition);
if (entry == null) {
entry =
- new OptionValueDescription(
- originalName,
+ OptionValueDescription.newOptionValue(
+ optionDefinition,
/* originalValueString */ null,
ArrayListMultimap.create(),
priority,
source,
implicitDependant,
- expandedFrom,
- true);
+ expandedFrom);
parsedValues.put(optionDefinition, entry);
maybeAddDeprecationWarning(optionDefinition);
}
@@ -348,15 +335,14 @@
String unparsedFlagExpression = optionsIterator.next();
ParseOptionResult parseResult = parseOption(unparsedFlagExpression, optionsIterator);
builder.add(
- new OptionValueDescription(
- parseResult.optionDefinition.getOptionName(),
+ OptionValueDescription.newOptionValue(
+ parseResult.optionDefinition,
parseResult.value,
/* value */ null,
/* priority */ null,
/* source */ null,
implicitDependant,
- /* expendedFrom */ null,
- parseResult.optionDefinition.allowsMultiple()));
+ /* expendedFrom */ null));
}
return builder.build();
}
@@ -379,15 +365,14 @@
String unparsedFlagExpression = optionsIterator.next();
ParseOptionResult parseResult = parseOption(unparsedFlagExpression, optionsIterator);
builder.add(
- new OptionValueDescription(
- parseResult.optionDefinition.getOptionName(),
+ OptionValueDescription.newOptionValue(
+ parseResult.optionDefinition,
parseResult.value,
/* value */ null,
/* priority */ null,
/* source */ null,
/* implicitDependant */ null,
- flagName,
- parseResult.optionDefinition.allowsMultiple()));
+ flagName));
}
return builder.build();
}
@@ -448,11 +433,10 @@
OptionDefinition optionDefinition = parseOptionResult.optionDefinition;
@Nullable String value = parseOptionResult.value;
- final String originalName = optionDefinition.getOptionName();
-
if (optionDefinition.isWrapperOption()) {
if (value.startsWith("-")) {
- String sourceMessage = "Unwrapped from wrapper option --" + originalName;
+ String sourceMessage =
+ "Unwrapped from wrapper option --" + optionDefinition.getOptionName();
List<String> unparsed =
parse(
priority,
@@ -475,8 +459,14 @@
continue;
} else {
- throw new OptionsParsingException("Invalid --" + originalName + " value format. "
- + "You may have meant --" + originalName + "=--" + value);
+ throw new OptionsParsingException(
+ "Invalid --"
+ + optionDefinition.getOptionName()
+ + " value format. "
+ + "You may have meant --"
+ + optionDefinition.getOptionName()
+ + "=--"
+ + value);
}
}
@@ -486,11 +476,10 @@
// correctly canonicalize flags.
UnparsedOptionValueDescription unparsedOptionValueDescription =
new UnparsedOptionValueDescription(
- originalName,
optionDefinition,
value,
priority,
- sourceFunction.apply(originalName),
+ sourceFunction.apply(optionDefinition.getOptionName()),
expandedFrom == null);
unparsedValues.add(unparsedOptionValueDescription);
if (optionDefinition.allowsMultiple()) {
@@ -506,14 +495,20 @@
ImmutableList<String> expansion =
optionsData.getEvaluatedExpansion(optionDefinition, value);
- String sourceMessage = "expanded from option --"
- + originalName
- + " from "
- + sourceFunction.apply(originalName);
+ String sourceMessage =
+ "expanded from option --"
+ + optionDefinition.getOptionName()
+ + " from "
+ + sourceFunction.apply(optionDefinition.getOptionName());
Function<Object, String> expansionSourceFunction = o -> sourceMessage;
maybeAddDeprecationWarning(optionDefinition);
List<String> unparsed =
- parse(priority, expansionSourceFunction, null, originalName, expansion);
+ parse(
+ priority,
+ expansionSourceFunction,
+ null,
+ optionDefinition.getOptionName(),
+ expansion);
if (!unparsed.isEmpty()) {
// Throw an assertion, because this indicates an error in the code that specified the
// expansion for the current option.
@@ -540,10 +535,10 @@
if (!optionDefinition.allowsMultiple()) {
setValue(
optionDefinition,
- originalName,
+ optionDefinition.getOptionName(),
convertedValue,
priority,
- sourceFunction.apply(originalName),
+ sourceFunction.apply(optionDefinition.getOptionName()),
implicitDependent,
expandedFrom);
} else {
@@ -554,10 +549,9 @@
// for the field declaration.
addListValue(
optionDefinition,
- originalName,
convertedValue,
priority,
- sourceFunction.apply(originalName),
+ sourceFunction.apply(optionDefinition.getOptionName()),
implicitDependent,
expandedFrom);
}
