Treat parsed option values differently by option type.
There is a vexingly large world of possible option types, each with its own quirks of how it interfaces with new inputs as they come in: values can be
- overridden (default)
- concatenated (allowMultiple)
- flattened (allowMultiple that accepts list inputs)
- disappear into additional flag inputs (expansion flags)
Or some combination of the above, in the case of flags with implicit dependencies and wrapper options.
Begin removing the error-prone treatment of all option types with conditional branches. This model of the different options will make it much easier to isolate the option-type specific logic with the command-line parsing logic. Flags that affect other flags (implicit requirements, expansions, and wrappers) will be migrated in a later change.
This CL does not change flag parsing semantics, just migrates the current parsing logic to the new class structure.
RELNOTES: None.
PiperOrigin-RevId: 169239182
diff --git a/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java b/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
index 4285f33..742acb6 100644
--- a/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
+++ b/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
@@ -547,7 +547,7 @@
"Keeping value '%s' from source '%s' for flag '%s' "
+ "because the invocation policy specifying the value(s) '%s' is overridable",
valueDescription.getValue(),
- valueDescription.getSource(),
+ valueDescription.getSourceString(),
optionDefinition.getOptionName(),
setValue.getFlagValueList());
} else {
@@ -561,17 +561,17 @@
for (String flagValue : setValue.getFlagValueList()) {
if (valueDescription == null) {
logInApplySetValueOperation(
- "Setting value for flag '%s' from invocation "
- + "policy to '%s', overriding the default value '%s'",
+ "Setting value for flag '%s' from invocation policy to '%s', overriding the "
+ + "default value '%s'",
optionDefinition.getOptionName(), flagValue, optionDefinition.getDefaultValue());
} else {
logInApplySetValueOperation(
- "Setting value for flag '%s' from invocation "
- + "policy to '%s', overriding value '%s' from '%s'",
+ "Setting value for flag '%s' from invocation policy to '%s', overriding "
+ + "value '%s' from '%s'",
optionDefinition.getOptionName(),
flagValue,
valueDescription.getValue(),
- valueDescription.getSource());
+ valueDescription.getSourceString());
}
setFlagValue(parser, optionDefinition, flagValue);
}
@@ -585,8 +585,6 @@
if (clearedValueDescription != null) {
// Log the removed value.
String clearedFlagName = clearedValueDescription.getOptionDefinition().getOptionName();
- String originalValue = clearedValueDescription.getValue().toString();
- String source = clearedValueDescription.getSource();
OptionDescription desc =
parser.getOptionDescription(
@@ -597,9 +595,13 @@
}
logger.info(
String.format(
- "Using default value '%s' for flag '%s' as "
- + "specified by %s invocation policy, overriding original value '%s' from '%s'",
- clearedFlagDefaultValue, clearedFlagName, policyType, originalValue, source));
+ "Using default value '%s' for flag '%s' as specified by %s invocation policy, "
+ + "overriding original value '%s' from '%s'",
+ clearedFlagDefaultValue,
+ clearedFlagName,
+ policyType,
+ clearedValueDescription.getValue(),
+ clearedValueDescription.getSourceString()));
}
}
@@ -724,8 +726,8 @@
// Use the default value from the policy.
logger.info(
String.format(
- "Overriding default value '%s' for flag '%s' with value '%s' "
- + "specified by invocation policy. %sed values are: %s",
+ "Overriding default value '%s' for flag '%s' with value '%s' specified by "
+ + "invocation policy. %sed values are: %s",
optionDefinition.getDefaultValue(),
optionDefinition.getOptionName(),
newValue,
@@ -738,8 +740,7 @@
throw new OptionsParsingException(
String.format(
"Default flag value '%s' for flag '%s' is not allowed by invocation policy, but "
- + "the policy does not provide a new value. "
- + "%sed values are: %s",
+ + "the policy does not provide a new value. %sed values are: %s",
optionDescription.getOptionDefinition().getDefaultValue(),
optionDefinition.getOptionName(),
policyType,
diff --git a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
index 8e16287..ca709dc 100644
--- a/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
+++ b/src/main/java/com/google/devtools/common/options/OptionValueDescription.java
@@ -14,176 +14,373 @@
package com.google.devtools.common.options;
-import com.google.common.base.Preconditions;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ListMultimap;
+import com.google.devtools.common.options.OptionsParser.ConstructionException;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.List;
-import javax.annotation.Nullable;
+import java.util.stream.Collectors;
/**
- * The name and value of an option with additional metadata describing its priority, source, whether
- * it was set via an implicit dependency, and if so, by which other option.
+ * The value of an option.
+ *
+ * <p>This takes care of tracking the final value as multiple instances of an option are parsed. It
+ * also tracks additional metadata describing its priority, source, whether it was set via an
+ * implicit dependency, and if so, by which other option.
*/
-public class OptionValueDescription {
- 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 OptionDefinition implicitDependant;
- @Nullable private final OptionDefinition expandedFrom;
+public abstract class OptionValueDescription {
- private OptionValueDescription(
- OptionDefinition optionDefinition,
- boolean isDefaultValue,
- @Nullable String originalValueString,
- @Nullable Object value,
- @Nullable OptionPriority priority,
- @Nullable String source,
- @Nullable OptionDefinition implicitDependant,
- @Nullable OptionDefinition expandedFrom) {
+ protected final OptionDefinition optionDefinition;
+
+ public OptionValueDescription(OptionDefinition optionDefinition) {
this.optionDefinition = optionDefinition;
- this.isDefaultValue = isDefaultValue;
- this.originalValueString = originalValueString;
- this.value = value;
- this.priority = priority;
- this.source = source;
- this.implicitDependant = implicitDependant;
- this.expandedFrom = expandedFrom;
- }
-
- public static OptionValueDescription newOptionValue(
- OptionDefinition optionDefinition,
- @Nullable String originalValueString,
- @Nullable Object value,
- @Nullable OptionPriority priority,
- @Nullable String source,
- @Nullable OptionDefinition implicitDependant,
- @Nullable OptionDefinition 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 optionDefinition.getOptionName();
+ /** Returns the current or final value of this option. */
+ public abstract Object getValue();
+
+ /** Returns the source(s) of this option, if there were multiple, duplicates are removed. */
+ public abstract String getSourceString();
+
+ // TODO(b/65540004) implicitDependant and expandedFrom are artifacts of an option instance, and
+ // should be in ParsedOptionDescription.
+ abstract void addOptionInstance(
+ ParsedOptionDescription parsedOption,
+ OptionDefinition implicitDependant,
+ OptionDefinition expandedFrom,
+ List<String> warnings)
+ throws OptionsParsingException;
+
+ /**
+ * For the given option, returns the correct type of OptionValueDescription, to which unparsed
+ * values can be added.
+ *
+ * <p>The categories of option types are non-overlapping, an invariant checked by the
+ * OptionProcessor at compile time.
+ */
+ public static OptionValueDescription createOptionValueDescription(OptionDefinition option) {
+ if (option.allowsMultiple()) {
+ return new RepeatableOptionValueDescription(option);
+ } else if (option.isExpansionOption()) {
+ return new ExpansionOptionValueDescription(option);
+ } else if (option.getImplicitRequirements().length > 0) {
+ return new OptionWithImplicitRequirementsValueDescription(option);
+ } else if (option.isWrapperOption()) {
+ return new WrapperOptionValueDescription(option);
+ } else {
+ return new SingleOptionValueDescription(option);
+ }
}
- public String getOriginalValueString() {
- return originalValueString;
+ /**
+ * For options that have not been set, this will return a correct OptionValueDescription for the
+ * default value.
+ */
+ public static OptionValueDescription getDefaultOptionValue(OptionDefinition option) {
+ return new DefaultOptionValueDescription(option);
}
- // Need to suppress unchecked warnings, because the "multiple occurrence"
- // options use unchecked ListMultimaps due to limitations of Java generics.
- @SuppressWarnings({"unchecked", "rawtypes"})
- public Object getValue() {
- if (isDefaultValue) {
- // If no value was present, we want the default value for this option.
+ static class DefaultOptionValueDescription extends OptionValueDescription {
+
+ private DefaultOptionValueDescription(OptionDefinition optionDefinition) {
+ super(optionDefinition);
+ }
+
+ @Override
+ public Object getValue() {
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
- // type-safe.
- List result = new ArrayList<>();
- ListMultimap realValue = (ListMultimap) value;
+
+ @Override
+ public String getSourceString() {
+ return null;
+ }
+
+ @Override
+ void addOptionInstance(
+ ParsedOptionDescription parsedOption,
+ OptionDefinition implicitDependant,
+ OptionDefinition expandedFrom,
+ List<String> warnings) {
+ throw new IllegalStateException(
+ "Cannot add values to the default option value. Create a modifiable "
+ + "OptionValueDescription using createOptionValueDescription() instead.");
+ }
+ }
+
+ /**
+ * The form of a value for a default type of flag, one that does not accumulate multiple values
+ * and has no expansion.
+ */
+ static class SingleOptionValueDescription extends OptionValueDescription {
+ private ParsedOptionDescription effectiveOptionInstance;
+ private Object effectiveValue;
+ private OptionDefinition optionThatDependsOnEffectiveValue;
+ private OptionDefinition optionThatExpandedToEffectiveValue;
+
+ private SingleOptionValueDescription(OptionDefinition optionDefinition) {
+ super(optionDefinition);
+ if (optionDefinition.allowsMultiple()) {
+ throw new ConstructionException("Can't have a single value for an allowMultiple option.");
+ }
+ if (optionDefinition.isExpansionOption()) {
+ throw new ConstructionException("Can't have a single value for an expansion option.");
+ }
+ effectiveOptionInstance = null;
+ effectiveValue = null;
+ optionThatDependsOnEffectiveValue = null;
+ optionThatExpandedToEffectiveValue = null;
+ }
+
+ @Override
+ public Object getValue() {
+ return effectiveValue;
+ }
+
+ @Override
+ public String getSourceString() {
+ return effectiveOptionInstance.getSource();
+ }
+
+ // Warnings should not end with a '.' because the internal reporter adds one automatically.
+ @Override
+ void addOptionInstance(
+ ParsedOptionDescription parsedOption,
+ OptionDefinition implicitDependant,
+ OptionDefinition expandedFrom,
+ List<String> warnings)
+ throws OptionsParsingException {
+ // This might be the first value, in that case, just store it!
+ if (effectiveOptionInstance == null) {
+ effectiveOptionInstance = parsedOption;
+ optionThatDependsOnEffectiveValue = implicitDependant;
+ optionThatExpandedToEffectiveValue = expandedFrom;
+ effectiveValue = effectiveOptionInstance.getConvertedValue();
+ return;
+ }
+
+ // If there was another value, check whether the new one will override it, and if so,
+ // log warnings describing the change.
+ if (parsedOption.getPriority().compareTo(effectiveOptionInstance.getPriority()) >= 0) {
+ // Output warnings:
+ if ((implicitDependant != null) && (optionThatDependsOnEffectiveValue != null)) {
+ if (!implicitDependant.equals(optionThatDependsOnEffectiveValue)) {
+ warnings.add(
+ String.format(
+ "Option '%s' is implicitly defined by both option '%s' and option '%s'",
+ optionDefinition.getOptionName(),
+ optionThatDependsOnEffectiveValue.getOptionName(),
+ implicitDependant.getOptionName()));
+ }
+ } else if ((implicitDependant != null)
+ && parsedOption.getPriority().equals(effectiveOptionInstance.getPriority())) {
+ warnings.add(
+ String.format(
+ "Option '%s' is implicitly defined by option '%s'; the implicitly set value "
+ + "overrides the previous one",
+ optionDefinition.getOptionName(), implicitDependant.getOptionName()));
+ } else if (optionThatDependsOnEffectiveValue != null) {
+ warnings.add(
+ String.format(
+ "A new value for option '%s' overrides a previous implicit setting of that "
+ + "option by option '%s'",
+ optionDefinition.getOptionName(),
+ optionThatDependsOnEffectiveValue.getOptionName()));
+ } else if ((parsedOption.getPriority() == effectiveOptionInstance.getPriority())
+ && ((optionThatExpandedToEffectiveValue == null) && (expandedFrom != null))) {
+ // Create a warning if an expansion option overrides an explicit option:
+ warnings.add(
+ String.format(
+ "The option '%s' was expanded and now overrides a previous explicitly specified "
+ + "option '%s'",
+ expandedFrom.getOptionName(), optionDefinition.getOptionName()));
+ } else if ((optionThatExpandedToEffectiveValue != null) && (expandedFrom != null)) {
+ warnings.add(
+ String.format(
+ "The option '%s' was expanded to from both options '%s' and '%s'",
+ optionDefinition.getOptionName(),
+ optionThatExpandedToEffectiveValue.getOptionName(),
+ expandedFrom.getOptionName()));
+ }
+
+ // Record the new value:
+ effectiveOptionInstance = parsedOption;
+ optionThatDependsOnEffectiveValue = implicitDependant;
+ optionThatExpandedToEffectiveValue = expandedFrom;
+ effectiveValue = parsedOption.getConvertedValue();
+ } else {
+ // The new value does not override the old value, as it has lower priority.
+ warnings.add(
+ String.format(
+ "The lower priority option '%s' does not override the previous value '%s'",
+ parsedOption.getCommandLineForm(), effectiveOptionInstance.getCommandLineForm()));
+ }
+ }
+
+ @VisibleForTesting
+ ParsedOptionDescription getEffectiveOptionInstance() {
+ return effectiveOptionInstance;
+ }
+ }
+
+ /** The form of a value for an option that accumulates multiple values on the command line. */
+ static class RepeatableOptionValueDescription extends OptionValueDescription {
+ ListMultimap<OptionPriority, ParsedOptionDescription> parsedOptions;
+ ListMultimap<OptionPriority, Object> optionValues;
+
+ private RepeatableOptionValueDescription(OptionDefinition optionDefinition) {
+ super(optionDefinition);
+ if (!optionDefinition.allowsMultiple()) {
+ throw new ConstructionException(
+ "Can't have a repeated value for a non-allowMultiple option.");
+ }
+ parsedOptions = ArrayListMultimap.create();
+ optionValues = ArrayListMultimap.create();
+ }
+
+ @Override
+ public String getSourceString() {
+ return parsedOptions
+ .asMap()
+ .values()
+ .stream()
+ .flatMap(Collection::stream)
+ .map(ParsedOptionDescription::getSource)
+ .distinct()
+ .collect(Collectors.joining(", "));
+ }
+
+ @Override
+ public List<Object> getValue() {
+ // 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 type-safe.
+ List<Object> result = new ArrayList<>();
for (OptionPriority priority : OptionPriority.values()) {
// If there is no mapping for this key, this check avoids object creation (because
// ListMultimap has to return a new object on get) and also an unnecessary addAll call.
- if (realValue.containsKey(priority)) {
- result.addAll(realValue.get(priority));
+ if (optionValues.containsKey(priority)) {
+ result.addAll(optionValues.get(priority));
}
}
return result;
}
- return value;
- }
- /**
- * @return the priority of the thing that set this value for this flag
- */
- public OptionPriority getPriority() {
- return priority;
- }
-
- /**
- * @return the thing that set this value for this flag
- */
- public String getSource() {
- return source;
- }
-
- public OptionDefinition getImplicitDependant() {
- return implicitDependant;
- }
-
- public boolean isImplicitDependency() {
- return implicitDependant != null;
- }
-
- public OptionDefinition getExpansionParent() {
- return expandedFrom;
- }
-
- public boolean isExpansion() {
- return expandedFrom != null;
- }
-
- public boolean getAllowMultiple() {
- return optionDefinition.allowsMultiple();
- }
-
- @Override
- public String toString() {
- StringBuilder result = new StringBuilder();
- 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("'");
+ @Override
+ void addOptionInstance(
+ ParsedOptionDescription parsedOption,
+ OptionDefinition implicitDependant,
+ OptionDefinition expandedFrom,
+ List<String> warnings)
+ throws OptionsParsingException {
+ // For repeatable options, we allow flags that take both single values and multiple values,
+ // potentially collapsing them down.
+ Object convertedValue = parsedOption.getConvertedValue();
+ if (convertedValue instanceof List<?>) {
+ optionValues.putAll(parsedOption.getPriority(), (List<?>) convertedValue);
+ } else {
+ optionValues.put(parsedOption.getPriority(), convertedValue);
}
- 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(optionDefinition.allowsMultiple());
- Preconditions.checkState(!isDefaultValue);
- ListMultimap optionValueList = (ListMultimap) value;
- if (addedValue instanceof List<?>) {
- optionValueList.putAll(addedPriority, (List<?>) addedValue);
- } else {
- optionValueList.put(addedPriority, addedValue);
+ /**
+ * The form of a value for an expansion option, one that does not have its own value but expands
+ * in place to other options. This should be used for both flags with a static expansion defined
+ * in {@link Option#expansion()} and flags with an {@link Option#expansionFunction()}.
+ */
+ static class ExpansionOptionValueDescription extends OptionValueDescription {
+
+ private ExpansionOptionValueDescription(OptionDefinition optionDefinition) {
+ super(optionDefinition);
+ if (!optionDefinition.isExpansionOption()) {
+ throw new ConstructionException(
+ "Options without expansions can't be tracked using ExpansionOptionValueDescription");
+ }
+ }
+
+ @Override
+ public Object getValue() {
+ return null;
+ }
+
+ @Override
+ public String getSourceString() {
+ return null;
+ }
+
+ @Override
+ void addOptionInstance(
+ ParsedOptionDescription parsedOption,
+ OptionDefinition implicitDependant,
+ OptionDefinition expandedFrom,
+ List<String> warnings) {
+ // TODO(b/65540004) Deal with expansion options here instead of in parse(), and track their
+ // link to the options they expanded to to.
+ }
+ }
+
+ /** The form of a value for a flag with implicit requirements. */
+ static class OptionWithImplicitRequirementsValueDescription extends SingleOptionValueDescription {
+
+ private OptionWithImplicitRequirementsValueDescription(OptionDefinition optionDefinition) {
+ super(optionDefinition);
+ if (optionDefinition.getImplicitRequirements().length == 0) {
+ throw new ConstructionException(
+ "Options without implicit requirements can't be tracked using "
+ + "OptionWithImplicitRequirementsValueDescription");
+ }
+ }
+
+ @Override
+ void addOptionInstance(
+ ParsedOptionDescription parsedOption,
+ OptionDefinition implicitDependant,
+ OptionDefinition expandedFrom,
+ List<String> warnings)
+ throws OptionsParsingException {
+ // This is a valued flag, its value is handled the same way as a normal
+ // SingleOptionValueDescription.
+ super.addOptionInstance(parsedOption, implicitDependant, expandedFrom, warnings);
+
+ // Now deal with the implicit requirements.
+ // TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(),
+ // and track their link to the options they implicitly expanded to to.
+ }
+ }
+
+ /** Form for options that contain other options in the value text to which they expand. */
+ static final class WrapperOptionValueDescription extends OptionValueDescription {
+
+ WrapperOptionValueDescription(OptionDefinition optionDefinition) {
+ super(optionDefinition);
+ }
+
+ @Override
+ public Object getValue() {
+ return null;
+ }
+
+ @Override
+ public String getSourceString() {
+ return null;
+ }
+
+ @Override
+ void addOptionInstance(
+ ParsedOptionDescription parsedOption,
+ OptionDefinition implicitDependant,
+ OptionDefinition expandedFrom,
+ List<String> warnings)
+ throws OptionsParsingException {
+ // TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(),
+ // and track their link to the options they implicitly expanded to to.
}
}
}
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
index 3c41951..94568be 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -19,7 +19,6 @@
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
-import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterators;
import com.google.common.collect.LinkedHashMultimap;
@@ -177,7 +176,7 @@
OptionDefinition optionDefinition = mapEntry.getValue();
OptionValueDescription optionValue = optionValues.get(optionDefinition);
if (optionValue == null) {
- result.add(OptionValueDescription.newDefaultValue(optionDefinition));
+ result.add(OptionValueDescription.getDefaultOptionValue(optionDefinition));
} else {
result.add(optionValue);
}
@@ -198,119 +197,6 @@
+ (warning.isEmpty() ? "" : ": " + warning));
}
- // Warnings should not end with a '.' because the internal reporter adds one automatically.
- private void setValue(
- ParsedOptionDescription parsedOption,
- OptionDefinition implicitDependant,
- OptionDefinition expandedFrom)
- throws OptionsParsingException {
- OptionDefinition optionDefinition = parsedOption.getOptionDefinition();
- Preconditions.checkArgument(!optionDefinition.allowsMultiple());
- Object convertedValue = parsedOption.getConvertedValue();
- OptionValueDescription optionValue = optionValues.get(parsedOption.getOptionDefinition());
- if (optionValue != null) {
- // Override existing option if the new value has higher or equal priority.
- if (parsedOption.getPriority().compareTo(optionValue.getPriority()) >= 0) {
- // Output warnings:
- if ((implicitDependant != null) && (optionValue.getImplicitDependant() != null)) {
- if (!implicitDependant.equals(optionValue.getImplicitDependant())) {
- warnings.add(
- "Option '"
- + optionDefinition.getOptionName()
- + "' is implicitly defined by both option '"
- + optionValue.getImplicitDependant().getOptionName()
- + "' and option '"
- + implicitDependant.getOptionName()
- + "'");
- }
- } else if ((implicitDependant != null)
- && parsedOption.getPriority().equals(optionValue.getPriority())) {
- warnings.add(
- "Option '"
- + optionDefinition.getOptionName()
- + "' is implicitly defined by option '"
- + implicitDependant.getOptionName()
- + "'; the implicitly set value overrides the previous one");
- } else if (optionValue.getImplicitDependant() != null) {
- warnings.add(
- "A new value for option '"
- + optionDefinition.getOptionName()
- + "' overrides a previous implicit setting of that option by option '"
- + optionValue.getImplicitDependant().getOptionName()
- + "'");
- } else if ((parsedOption.getPriority() == optionValue.getPriority())
- && ((optionValue.getExpansionParent() == null) && (expandedFrom != null))) {
- // Create a warning if an expansion option overrides an explicit option:
- warnings.add(
- "The option '"
- + expandedFrom.getOptionName()
- + "' was expanded and now overrides a "
- + "previous explicitly specified option '"
- + optionDefinition.getOptionName()
- + "'");
- } else if ((optionValue.getExpansionParent() != null) && (expandedFrom != null)) {
- warnings.add(
- "The option '"
- + optionDefinition.getOptionName()
- + "' was expanded to from both options '"
- + optionValue.getExpansionParent().getOptionName()
- + "' and '"
- + expandedFrom.getOptionName()
- + "'");
- }
-
- // Record the new value:
- optionValues.put(
- optionDefinition,
- OptionValueDescription.newOptionValue(
- optionDefinition,
- null,
- convertedValue,
- parsedOption.getPriority(),
- parsedOption.getSource(),
- implicitDependant,
- expandedFrom));
- }
- } else {
- optionValues.put(
- optionDefinition,
- OptionValueDescription.newOptionValue(
- optionDefinition,
- null,
- convertedValue,
- parsedOption.getPriority(),
- parsedOption.getSource(),
- implicitDependant,
- expandedFrom));
- maybeAddDeprecationWarning(optionDefinition);
- }
- }
-
- private void addListValue(
- ParsedOptionDescription parsedOption,
- OptionDefinition implicitDependant,
- OptionDefinition expandedFrom)
- throws OptionsParsingException {
- OptionDefinition optionDefinition = parsedOption.getOptionDefinition();
- Preconditions.checkArgument(optionDefinition.allowsMultiple());
-
- OptionValueDescription optionValue = optionValues.get(optionDefinition);
- if (optionValue == null) {
- optionValue =
- OptionValueDescription.newOptionValue(
- optionDefinition,
- /* originalValueString */ null,
- ArrayListMultimap.create(),
- parsedOption.getPriority(),
- parsedOption.getSource(),
- implicitDependant,
- expandedFrom);
- optionValues.put(optionDefinition, optionValue);
- maybeAddDeprecationWarning(optionDefinition);
- }
- Object convertedValue = parsedOption.getConvertedValue();
- optionValue.addValue(parsedOption.getPriority(), convertedValue);
- }
OptionValueDescription clearValue(OptionDefinition optionDefinition)
throws OptionsParsingException {
@@ -453,6 +339,8 @@
identifyOptionAndPossibleArgument(
arg, argsIterator, priority, sourceFunction, isExplicit);
OptionDefinition optionDefinition = parsedOption.getOptionDefinition();
+ // All options can be deprecated; check and warn before doing any option-type specific work.
+ maybeAddDeprecationWarning(optionDefinition);
@Nullable String unconvertedValue = parsedOption.getUnconvertedValue();
if (optionDefinition.isWrapperOption()) {
@@ -515,7 +403,6 @@
+ " from "
+ sourceFunction.apply(optionDefinition);
Function<OptionDefinition, String> expansionSourceFunction = o -> sourceMessage;
- maybeAddDeprecationWarning(optionDefinition);
List<String> unparsed =
parse(priority, expansionSourceFunction, null, optionDefinition, expansion);
if (!unparsed.isEmpty()) {
@@ -528,15 +415,10 @@
+ Joiner.on(' ').join(unparsed));
}
} else {
- // ...but allow duplicates of single-use options across separate calls to
- // parse(); latest wins:
- if (!optionDefinition.allowsMultiple()) {
- setValue(parsedOption, implicitDependent, expandedFrom);
- } else {
- // But if it's a multiple-use option, then accumulate the values, in the order in which
- // they were seen.
- addListValue(parsedOption, implicitDependent, expandedFrom);
- }
+ OptionValueDescription entry =
+ optionValues.computeIfAbsent(
+ optionDefinition, OptionValueDescription::createOptionValueDescription);
+ entry.addOptionInstance(parsedOption, implicitDependent, expandedFrom, warnings);
}
// Collect any implicit requirements.
@@ -568,6 +450,13 @@
}
}
+ // Go through the final values and make sure they are valid values for their option. Unlike any
+ // checks that happened above, this also checks that flags that were not set have a valid
+ // default value. getValue() will throw if the value is invalid.
+ for (OptionValueDescription valueDescription : asListOfEffectiveOptions()) {
+ valueDescription.getValue();
+ }
+
return unparsedArgs;
}
@@ -690,8 +579,17 @@
}
try {
optionDefinition.getField().set(optionsInstance, value);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalStateException(
+ String.format(
+ "Unable to set option '%s' to value '%s'.",
+ optionDefinition.getOptionName(), value),
+ e);
} catch (IllegalAccessException e) {
- throw new IllegalStateException(e);
+ throw new IllegalStateException(
+ "Could not set the field due to access issues. This is impossible, as the "
+ + "OptionProcessor checks that all options are non-final public fields.",
+ e);
}
}
return optionsInstance;
