Move default value & converter finding logic to the OptionDefinition class.
Removes some duplicate computation by memoizing the results. Consolidates caching into a single optionDefinition object, instead of having multiple caches that go from the option name to different parts of what defines an option.
Fly-by cleanup of OptionDescription's contents, all contents that are statically defined as part of an option are in OptionDefintion, while expansion data, which depends on the existence of other options, is more clearly stored separately.
Will move the converter-to-option type matching sanity checks to a compile time check in a later change.
RELNOTES: None.
PiperOrigin-RevId: 166912716
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java
index 72f001f..6aa27b2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java
@@ -17,9 +17,9 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.common.options.OptionDefinition;
+import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
import com.google.devtools.common.options.OptionMetadataTag;
import com.google.devtools.common.options.OptionsBase;
-import com.google.devtools.common.options.OptionsParser.ConstructionException;
import java.io.Serializable;
import java.lang.reflect.Field;
import java.util.Map;
@@ -47,7 +47,7 @@
OptionDefinition optionDefinition;
try {
optionDefinition = OptionDefinition.extractOptionDefinition(field);
- } catch (ConstructionException e) {
+ } catch (NotAnOptionException e) {
// Skip non @Option fields.
continue;
}
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
index 8d9f93c..a160234 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -76,11 +76,11 @@
import com.google.devtools.common.options.CommandNameCache;
import com.google.devtools.common.options.InvocationPolicyParser;
import com.google.devtools.common.options.OptionDefinition;
+import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
import com.google.devtools.common.options.OptionPriority;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsClassProvider;
import com.google.devtools.common.options.OptionsParser;
-import com.google.devtools.common.options.OptionsParser.ConstructionException;
import com.google.devtools.common.options.OptionsParsingException;
import com.google.devtools.common.options.OptionsProvider;
import com.google.devtools.common.options.TriState;
@@ -659,7 +659,7 @@
if (field.getType() == boolean.class || field.getType() == TriState.class) {
prefixes.add("--no" + optionDefinition.getOptionName());
}
- } catch (ConstructionException e) {
+ } catch (NotAnOptionException e) {
// Do nothing, just ignore fields that are not actually options. OptionsBases technically
// shouldn't have fields that are not @Options, but this is a requirement that isn't yet
// being enforced, so this should not cause a failure here.
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 4be2b23..e76688c 100644
--- a/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
+++ b/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
@@ -515,7 +515,8 @@
}
// Flag must allow multiple values if multiple values are specified by the policy.
- if (setValue.getFlagValueCount() > 1 && !optionDescription.getAllowMultiple()) {
+ if (setValue.getFlagValueCount() > 1
+ && !optionDescription.getOptionDefinition().allowsMultiple()) {
throw new OptionsParsingException(
String.format(
"SetValue operation from invocation policy sets multiple values for flag '%s' which "
@@ -546,7 +547,7 @@
logInApplySetValueOperation(
"Setting value for flag '%s' from invocation "
+ "policy to '%s', overriding the default value '%s'",
- flagName, flagValue, optionDescription.getDefaultValue());
+ flagName, flagValue, optionDescription.getOptionDefinition().getDefaultValue());
} else {
logInApplySetValueOperation(
"Setting value for flag '%s' from invocation "
@@ -570,7 +571,7 @@
OptionDescription desc = parser.getOptionDescription(clearedFlagName);
Object clearedFlagDefaultValue = null;
if (desc != null) {
- clearedFlagDefaultValue = desc.getDefaultValue();
+ clearedFlagDefaultValue = desc.getOptionDefinition().getDefaultValue();
}
log.info(
String.format(
@@ -640,15 +641,18 @@
// can be arbitrarily complex.
Set<Object> convertedPolicyValues = new HashSet<>();
for (String value : policyValues) {
- Object convertedValue = optionDescription.getConverter().convert(value);
+ Object convertedValue =
+ optionDescription.getOptionDefinition().getConverter().convert(value);
// Some converters return lists, and if the flag is a repeatable flag, the items in the
// list from the converter should be added, and not the list itself. Otherwise the items
// from invocation policy will be compared to lists, which will never work.
// See OptionsParserImpl.ParsedOptionEntry.addValue.
- if (optionDescription.getAllowMultiple() && convertedValue instanceof List<?>) {
+ if (optionDescription.getOptionDefinition().allowsMultiple()
+ && convertedValue instanceof List<?>) {
convertedPolicyValues.addAll((List<?>) convertedValue);
} else {
- convertedPolicyValues.add(optionDescription.getConverter().convert(value));
+ convertedPolicyValues.add(
+ optionDescription.getOptionDefinition().getConverter().convert(value));
}
}
@@ -656,18 +660,17 @@
// does not also set use_default. Otherwise the default value would will still be set if the
// user uses a disallowed value. This doesn't apply to repeatable flags since the default
// value for repeatable flags is always the empty list.
- if (!optionDescription.getAllowMultiple()) {
+ if (!optionDescription.getOptionDefinition().allowsMultiple()) {
boolean defaultValueAllowed =
- isFlagValueAllowed(convertedPolicyValues, optionDescription.getDefaultValue());
+ isFlagValueAllowed(
+ convertedPolicyValues, optionDescription.getOptionDefinition().getDefaultValue());
if (!defaultValueAllowed && useDefault) {
throw new OptionsParsingException(
String.format(
"%sValues policy disallows the default value '%s' for flag '%s' but also "
+ "specifies to use the default value",
- policyType,
- optionDescription.getDefaultValue(),
- flagName));
+ policyType, optionDescription.getOptionDefinition().getDefaultValue(), flagName));
}
}
@@ -706,14 +709,15 @@
Set<Object> convertedPolicyValues)
throws OptionsParsingException {
- if (!isFlagValueAllowed(convertedPolicyValues, optionDescription.getDefaultValue())) {
+ if (!isFlagValueAllowed(
+ convertedPolicyValues, optionDescription.getOptionDefinition().getDefaultValue())) {
if (newValue != null) {
// Use the default value from the policy.
log.info(
String.format(
"Overriding default value '%s' for flag '%s' with value '%s' "
+ "specified by invocation policy. %sed values are: %s",
- optionDescription.getDefaultValue(),
+ optionDescription.getOptionDefinition().getDefaultValue(),
flagName,
newValue,
policyType,
@@ -727,7 +731,7 @@
"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",
- optionDescription.getDefaultValue(),
+ optionDescription.getOptionDefinition().getDefaultValue(),
flagName,
policyType,
policyValues));
@@ -746,7 +750,7 @@
Set<Object> convertedPolicyValues)
throws OptionsParsingException {
- if (optionDescription.getAllowMultiple()) {
+ if (optionDescription.getOptionDefinition().allowsMultiple()) {
// allowMultiple requires that the type of the option be List<T>, so cast from Object
// to List<?>.
List<?> optionValues = (List<?>) valueDescription.getValue();
diff --git a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
index 406b6cd..ca91b1d 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -16,6 +16,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
import com.google.devtools.common.options.OptionsParser.ConstructionException;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
@@ -25,7 +26,6 @@
import java.lang.reflect.Type;
import java.util.Arrays;
import java.util.Collection;
-import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
@@ -74,28 +74,6 @@
allOptionsFields;
/**
- * Mapping from each {@code Option}-annotated field to the default value for that field
- * (unordered).
- *
- * <p>(This is immutable like the others, but uses {@code Collections.unmodifiableMap} to support
- * null values.)
- */
- private final Map<OptionDefinition, Object> optionDefaults;
-
- /**
- * Mapping from each {@code Option}-annotated field to the proper converter (unordered).
- *
- * @see #findConverter
- */
- private final ImmutableMap<OptionDefinition, Converter<?>> converters;
-
- /**
- * Mapping from each {@code Option}-annotated field to a boolean for whether that field allows
- * multiple values (unordered).
- */
- private final ImmutableMap<OptionDefinition, Boolean> allowMultiple;
-
- /**
* Mapping from each options class to whether or not it has the {@link UsesOnlyCoreTypes}
* annotation (unordered).
*/
@@ -110,18 +88,11 @@
Map<String, OptionDefinition> nameToField,
Map<Character, OptionDefinition> abbrevToField,
Map<Class<? extends OptionsBase>, ImmutableList<OptionDefinition>> allOptionsFields,
- Map<OptionDefinition, Object> optionDefaults,
- Map<OptionDefinition, Converter<?>> converters,
- Map<OptionDefinition, Boolean> allowMultiple,
Map<Class<? extends OptionsBase>, Boolean> usesOnlyCoreTypes) {
this.optionsClasses = ImmutableMap.copyOf(optionsClasses);
this.nameToField = ImmutableMap.copyOf(nameToField);
this.abbrevToField = ImmutableMap.copyOf(abbrevToField);
this.allOptionsFields = ImmutableMap.copyOf(allOptionsFields);
- // Can't use an ImmutableMap here because of null values.
- this.optionDefaults = Collections.unmodifiableMap(optionDefaults);
- this.converters = ImmutableMap.copyOf(converters);
- this.allowMultiple = ImmutableMap.copyOf(allowMultiple);
this.usesOnlyCoreTypes = ImmutableMap.copyOf(usesOnlyCoreTypes);
}
@@ -131,9 +102,6 @@
other.nameToField,
other.abbrevToField,
other.allOptionsFields,
- other.optionDefaults,
- other.converters,
- other.allowMultiple,
other.usesOnlyCoreTypes);
}
@@ -176,135 +144,29 @@
return allOptionsFields.get(optionsClass);
}
- public Object getDefaultValue(OptionDefinition optionDefinition) {
- return optionDefaults.get(optionDefinition);
- }
-
- public Converter<?> getConverter(OptionDefinition optionDefinition) {
- return converters.get(optionDefinition);
- }
-
- public boolean getAllowMultiple(OptionDefinition optionDefinition) {
- return allowMultiple.get(optionDefinition);
- }
-
public boolean getUsesOnlyCoreTypes(Class<? extends OptionsBase> optionsClass) {
return usesOnlyCoreTypes.get(optionsClass);
}
- /**
- * For an option that does not use {@link Option#allowMultiple}, returns its type. For an option
- * that does use it, asserts that the type is a {@code List<T>} and returns its element type
- * {@code T}.
- */
- private static Type getFieldSingularType(OptionDefinition optionDefinition) {
- Type fieldType = optionDefinition.getField().getGenericType();
- if (optionDefinition.allowsMultiple()) {
- // If the type isn't a List<T>, this is an error in the option's declaration.
- if (!(fieldType instanceof ParameterizedType)) {
- throw new ConstructionException("Type of multiple occurrence option must be a List<...>");
- }
- ParameterizedType pfieldType = (ParameterizedType) fieldType;
- if (pfieldType.getRawType() != List.class) {
- throw new ConstructionException("Type of multiple occurrence option must be a List<...>");
- }
- fieldType = pfieldType.getActualTypeArguments()[0];
- }
- return fieldType;
- }
-
- /**
- * Returns whether a field should be considered as boolean.
- *
- * <p>Can be used for usage help and controlling whether the "no" prefix is allowed.
- */
- boolean isBooleanField(OptionDefinition optionDefinition) {
- return isBooleanField(optionDefinition, getConverter(optionDefinition));
- }
-
- private static boolean isBooleanField(OptionDefinition optionDefinition, Converter<?> converter) {
- return optionDefinition.getType().equals(boolean.class)
- || optionDefinition.getType().equals(TriState.class)
- || converter instanceof BoolOrEnumConverter;
- }
-
- /**
- * Given an {@code @Option}-annotated field, retrieves the {@link Converter} that will be used,
- * taking into account the default converters if an explicit one is not specified.
- */
- private static Converter<?> findConverter(OptionDefinition optionDefinition) {
- if (optionDefinition.getProvidedConverter() == Converter.class) {
- // No converter provided, use the default one.
- Type type = getFieldSingularType(optionDefinition);
- Converter<?> converter = Converters.DEFAULT_CONVERTERS.get(type);
- if (converter == null) {
- throw new ConstructionException(
- "No converter found for "
- + type
- + "; possible fix: add "
- + "converter=... to @Option annotation for "
- + optionDefinition.getField().getName());
- }
- return converter;
- }
- try {
- // Instantiate the given Converter class.
- Class<?> converter = optionDefinition.getProvidedConverter();
- Constructor<?> constructor = converter.getConstructor();
- return (Converter<?>) constructor.newInstance();
- } catch (Exception e) {
- // This indicates an error in the Converter, and should be discovered the first time it is
- // used.
- throw new ConstructionException(e);
- }
- }
/** Returns all {@code optionDefinitions}, ordered by their option name (not their field name). */
private static ImmutableList<OptionDefinition> getAllOptionDefinitionsSorted(
Class<? extends OptionsBase> optionsClass) {
return Arrays.stream(optionsClass.getFields())
- .map(field -> {
- try {
- return OptionDefinition.extractOptionDefinition(field);
- } catch (ConstructionException e) {
- // Ignore non-@Option annotated fields. Requiring all fields in the OptionsBase to be
- // @Option-annotated requires a depot cleanup.
- return null;
- }
- })
+ .map(
+ field -> {
+ try {
+ return OptionDefinition.extractOptionDefinition(field);
+ } catch (NotAnOptionException e) {
+ // Ignore non-@Option annotated fields. Requiring all fields in the OptionsBase to
+ // be @Option-annotated requires a depot cleanup.
+ return null;
+ }
+ })
.filter(Objects::nonNull)
.sorted(OptionDefinition.BY_OPTION_NAME)
.collect(ImmutableList.toImmutableList());
}
- private static Object retrieveDefaultValue(OptionDefinition optionDefinition) {
- Converter<?> converter = findConverter(optionDefinition);
- String defaultValueAsString = optionDefinition.getUnparsedDefaultValue();
- // Special case for "null"
- if (optionDefinition.isSpecialNullDefault()) {
- return null;
- }
- boolean allowsMultiple = optionDefinition.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) {
- return Collections.emptyList();
- }
- // Otherwise try to convert the default value using the converter
- Object convertedValue;
- try {
- convertedValue = converter.convert(defaultValueAsString);
- } catch (OptionsParsingException e) {
- throw new IllegalStateException(
- "OptionsParsingException while "
- + "retrieving default for "
- + optionDefinition.getField().getName()
- + ": "
- + e.getMessage());
- }
- return convertedValue;
- }
-
private static <A> void checkForCollisions(
Map<A, OptionDefinition> aFieldMap, A optionName, String description) {
if (aFieldMap.containsKey(optionName)) {
@@ -351,9 +213,6 @@
new HashMap<>();
Map<String, OptionDefinition> nameToFieldBuilder = new LinkedHashMap<>();
Map<Character, OptionDefinition> abbrevToFieldBuilder = new HashMap<>();
- Map<OptionDefinition, Object> optionDefaultsBuilder = new HashMap<>();
- Map<OptionDefinition, Converter<?>> convertersBuilder = new HashMap<>();
- Map<OptionDefinition, Boolean> allowMultipleBuilder = new HashMap<>();
// Maps the negated boolean flag aliases to the original option name.
Map<String, String> booleanAliasMap = new HashMap<>();
@@ -391,7 +250,7 @@
+ "\" is disallowed.");
}
- Type fieldType = getFieldSingularType(optionDefinition);
+ Type fieldType = optionDefinition.getFieldSingularType();
// For simple, static expansions, don't accept non-Void types.
if (optionDefinition.getOptionExpansion().length != 0 && !optionDefinition.isVoidField()) {
throw new ConstructionException(
@@ -429,8 +288,6 @@
throw new ConstructionException(
"A known converter object doesn't implement the convert method");
}
- Converter<?> converter = findConverter(optionDefinition);
- convertersBuilder.put(optionDefinition, converter);
if (optionDefinition.allowsMultiple()) {
if (GenericTypeHelper.getRawType(converterResultType) == List.class) {
@@ -467,7 +324,7 @@
}
}
- if (isBooleanField(optionDefinition, converter)) {
+ if (optionDefinition.isBooleanField()) {
checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, optionName);
}
@@ -482,7 +339,7 @@
nameToFieldBuilder.put(optionDefinition.getOldOptionName(), optionDefinition);
// If boolean, repeat the alias dance for the old name.
- if (isBooleanField(optionDefinition, converter)) {
+ if (optionDefinition.isBooleanField()) {
checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, oldName);
}
}
@@ -491,9 +348,6 @@
abbrevToFieldBuilder, optionDefinition.getAbbreviation(), "option abbreviation");
abbrevToFieldBuilder.put(optionDefinition.getAbbreviation(), optionDefinition);
}
-
- optionDefaultsBuilder.put(optionDefinition, retrieveDefaultValue(optionDefinition));
- allowMultipleBuilder.put(optionDefinition, optionDefinition.allowsMultiple());
}
boolean usesOnlyCoreTypes = parsedOptionsClass.isAnnotationPresent(UsesOnlyCoreTypes.class);
@@ -523,9 +377,6 @@
nameToFieldBuilder,
abbrevToFieldBuilder,
allOptionsFieldsBuilder,
- optionDefaultsBuilder,
- convertersBuilder,
- allowMultipleBuilder,
usesOnlyCoreTypesBuilder);
}
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 589208a..d3f0d34 100644
--- a/src/main/java/com/google/devtools/common/options/OptionDefinition.java
+++ b/src/main/java/com/google/devtools/common/options/OptionDefinition.java
@@ -15,8 +15,13 @@
package com.google.devtools.common.options;
import com.google.devtools.common.options.OptionsParser.ConstructionException;
+import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
+import java.lang.reflect.ParameterizedType;
+import java.lang.reflect.Type;
+import java.util.Collections;
import java.util.Comparator;
+import java.util.List;
/**
* Everything the {@link OptionsParser} needs to know about how an option is defined.
@@ -25,23 +30,32 @@
* the {@link Field} that is annotated, and should contain all logic about default settings and
* behavior.
*/
-public final class OptionDefinition {
+public class OptionDefinition {
+
+ // TODO(b/65049598) make ConstructionException checked, which will make this checked as well.
+ public static class NotAnOptionException extends ConstructionException {
+ public NotAnOptionException(Field field) {
+ super(
+ "The field " + field + " does not have the right annotation to be considered an option.");
+ }
+ }
/**
* If the {@code field} is annotated with the appropriate @{@link Option} annotation, returns the
- * {@code OptionDefinition} for that option. Otherwise, throws a {@link ConstructionException}.
+ * {@code OptionDefinition} for that option. Otherwise, throws a {@link NotAnOptionException}.
*/
public static OptionDefinition extractOptionDefinition(Field field) {
Option annotation = field == null ? null : field.getAnnotation(Option.class);
if (annotation == null) {
- throw new ConstructionException(
- "The field " + field + " does not have the right annotation to be considered an option.");
+ throw new NotAnOptionException(field);
}
return new OptionDefinition(field, annotation);
}
private final Field field;
private final Option optionAnnotation;
+ private Converter<?> converter = null;
+ private Object defaultValue = null;
private OptionDefinition(Field field, Option optionAnnotation) {
this.field = field;
@@ -170,6 +184,110 @@
return getExpansionFunction() != ExpansionFunction.class;
}
+ /**
+ * For an option that does not use {@link Option#allowMultiple}, returns its type. For an option
+ * that does use it, asserts that the type is a {@code List<T>} and returns its element type
+ * {@code T}.
+ */
+ Type getFieldSingularType() {
+ Type fieldType = getField().getGenericType();
+ if (allowsMultiple()) {
+ // If the type isn't a List<T>, this is an error in the option's declaration.
+ if (!(fieldType instanceof ParameterizedType)) {
+ throw new ConstructionException(
+ String.format(
+ "Option %s allows multiple occurrences, so must be of type List<...>",
+ getField().getName()));
+ }
+ ParameterizedType pfieldType = (ParameterizedType) fieldType;
+ if (pfieldType.getRawType() != List.class) {
+ throw new ConstructionException(
+ String.format(
+ "Option %s allows multiple occurrences, so must be of type List<...>",
+ getField().getName()));
+ }
+ fieldType = pfieldType.getActualTypeArguments()[0];
+ }
+ return fieldType;
+ }
+
+ /**
+ * Retrieves the {@link Converter} that will be used for this option, taking into account the
+ * default converters if an explicit one is not specified.
+ *
+ * <p>Memoizes the converter-finding logic to avoid repeating the computation.
+ */
+ Converter<?> getConverter() {
+ if (converter != null) {
+ return converter;
+ }
+ Class<? extends Converter> converterClass = getProvidedConverter();
+ if (converterClass == Converter.class) {
+ // No converter provided, use the default one.
+ Type type = getFieldSingularType();
+ converter = Converters.DEFAULT_CONVERTERS.get(type);
+ if (converter == null) {
+ throw new ConstructionException(
+ String.format(
+ "Option %s expects values of type %s, but no converter was found; possible fix: "
+ + "add converter=... to its @Option annotation.",
+ getField().getName(), type));
+ }
+ } else {
+ try {
+ // Instantiate the given Converter class.
+ Constructor<?> constructor = converterClass.getConstructor();
+ converter = (Converter<?>) constructor.newInstance();
+ } catch (SecurityException | IllegalArgumentException | ReflectiveOperationException e) {
+ // This indicates an error in the Converter, and should be discovered the first time it is
+ // used.
+ throw new ConstructionException(
+ String.format("Error in the provided converter for option %s", getField().getName()),
+ e);
+ }
+ }
+ return converter;
+ }
+
+ /**
+ * Returns whether a field should be considered as boolean.
+ *
+ * <p>Can be used for usage help and controlling whether the "no" prefix is allowed.
+ */
+ boolean isBooleanField() {
+ return getType().equals(boolean.class)
+ || getType().equals(TriState.class)
+ || getConverter() instanceof BoolOrEnumConverter;
+ }
+
+ /** Returns the evaluated default value for this option & memoizes the result. */
+ public Object getDefaultValue() {
+ if (defaultValue != null || isSpecialNullDefault()) {
+ 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) {
+ defaultValue = Collections.emptyList();
+ } else {
+ // Otherwise try to convert the default value using the converter
+ try {
+ defaultValue = converter.convert(defaultValueAsString);
+ } catch (OptionsParsingException e) {
+ throw new ConstructionException(
+ String.format(
+ "OptionsParsingException while retrieving the default value for %s: %s",
+ getField().getName(), e.getMessage()),
+ e);
+ }
+ }
+ return defaultValue;
+ }
+
static final Comparator<OptionDefinition> BY_OPTION_NAME =
Comparator.comparing(OptionDefinition::getOptionName);
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index 36d1189..9d68320 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ListMultimap;
import com.google.common.escape.Escaper;
+import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.nio.file.FileSystem;
@@ -67,6 +68,7 @@
*/
public class OptionsParser implements OptionsProvider {
+ // TODO(b/65049598) make ConstructionException checked.
/**
* An unchecked exception thrown when there is a problem constructing a parser, e.g. an error
* while validating an {@link OptionDefinition} in one of its {@link OptionsBase} subclasses.
@@ -228,50 +230,24 @@
}
}
- // TODO(b/64904491) remove this once the converter and default information is in OptionDefinition
- // and cached.
- /** The metadata about an option. */
+ /** The metadata about an option, in the context of this options parser. */
public static final class OptionDescription {
- private final String name;
-
- // For valued flags
- private final Object defaultValue;
- private final Converter<?> converter;
- private final boolean allowMultiple;
-
+ private final OptionDefinition optionDefinition;
private final OptionsData.ExpansionData expansionData;
private final ImmutableList<OptionValueDescription> implicitRequirements;
OptionDescription(
- String name,
- Object defaultValue,
- Converter<?> converter,
- boolean allowMultiple,
+ OptionDefinition definition,
OptionsData.ExpansionData expansionData,
ImmutableList<OptionValueDescription> implicitRequirements) {
- this.name = name;
- this.defaultValue = defaultValue;
- this.converter = converter;
- this.allowMultiple = allowMultiple;
+ this.optionDefinition = definition;
this.expansionData = expansionData;
this.implicitRequirements = implicitRequirements;
}
- public String getName() {
- return name;
- }
-
- public Object getDefaultValue() {
- return defaultValue;
- }
-
- public Converter<?> getConverter() {
- return converter;
- }
-
- public boolean getAllowMultiple() {
- return allowMultiple;
+ public OptionDefinition getOptionDefinition() {
+ return optionDefinition;
}
public ImmutableList<OptionValueDescription> getImplicitRequirements() {
@@ -926,7 +902,7 @@
try {
optionDefinition = OptionDefinition.extractOptionDefinition(field);
extraNamesFromMap.add("'" + optionDefinition.getOptionName() + "'");
- } catch (ConstructionException e) {
+ } catch (NotAnOptionException e) {
extraNamesFromMap.add("<non-Option field>");
}
}
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 f8d6778..28aeb22 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -173,7 +173,7 @@
OptionDefinition optionDefinition = mapEntry.getValue();
OptionValueDescription entry = parsedValues.get(optionDefinition);
if (entry == null) {
- Object value = optionsData.getDefaultValue(optionDefinition);
+ Object value = optionDefinition.getDefaultValue();
result.add(
new OptionValueDescription(
fieldName,
@@ -327,10 +327,7 @@
}
return new OptionDescription(
- name,
- optionsData.getDefaultValue(optionDefinition),
- optionsData.getConverter(optionDefinition),
- optionsData.getAllowMultiple(optionDefinition),
+ optionDefinition,
optionsData.getExpansionDataForField(optionDefinition),
getImplicitDependantDescriptions(
ImmutableList.copyOf(optionDefinition.getImplicitRequirements()), name));
@@ -359,7 +356,7 @@
/* source */ null,
implicitDependant,
/* expendedFrom */ null,
- optionsData.getAllowMultiple(parseResult.optionDefinition)));
+ parseResult.optionDefinition.allowsMultiple()));
}
return builder.build();
}
@@ -390,7 +387,7 @@
/* source */ null,
/* implicitDependant */ null,
flagName,
- optionsData.getAllowMultiple(parseResult.optionDefinition)));
+ parseResult.optionDefinition.allowsMultiple()));
}
return builder.build();
}
@@ -527,7 +524,7 @@
+ Joiner.on(' ').join(unparsed));
}
} else {
- Converter<?> converter = optionsData.getConverter(optionDefinition);
+ Converter<?> converter = optionDefinition.getConverter();
Object convertedValue;
try {
convertedValue = converter.convert(value);
@@ -645,7 +642,7 @@
booleanValue = false;
if (optionDefinition != null) {
// TODO(bazel-team): Add tests for these cases.
- if (!optionsData.isBooleanField(optionDefinition)) {
+ if (!optionDefinition.isBooleanField()) {
throw new OptionsParsingException(
"Illegal use of 'no' prefix on non-boolean option: " + arg, arg);
}
@@ -670,7 +667,7 @@
if (value == null) {
// Special-case boolean to supply value based on presence of "no" prefix.
- if (optionsData.isBooleanField(optionDefinition)) {
+ if (optionDefinition.isBooleanField()) {
value = booleanValue ? "1" : "0";
} else if (optionDefinition.getType().equals(Void.class)
&& !optionDefinition.isWrapperOption()) {
@@ -707,7 +704,7 @@
Object value;
OptionValueDescription entry = parsedValues.get(optionDefinition);
if (entry == null) {
- value = optionsData.getDefaultValue(optionDefinition);
+ value = optionDefinition.getDefaultValue();
} else {
value = entry.getValue();
}
diff --git a/src/main/java/com/google/devtools/common/options/OptionsUsage.java b/src/main/java/com/google/devtools/common/options/OptionsUsage.java
index 51cc3c6..0ab30da 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsUsage.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsUsage.java
@@ -101,8 +101,8 @@
StringBuilder usage,
OptionsParser.HelpVerbosity helpVerbosity,
OptionsData optionsData) {
- String flagName = getFlagName(optionDefinition, optionsData);
- String typeDescription = getTypeDescription(optionDefinition, optionsData);
+ String flagName = getFlagName(optionDefinition);
+ String typeDescription = getTypeDescription(optionDefinition);
usage.append(" --").append(flagName);
if (helpVerbosity == OptionsParser.HelpVerbosity.SHORT) { // just the name
usage.append('\n');
@@ -163,12 +163,12 @@
Escaper escaper,
OptionsData optionsData) {
String plainFlagName = optionDefinition.getOptionName();
- String flagName = getFlagName(optionDefinition, optionsData);
+ String flagName = getFlagName(optionDefinition);
String valueDescription = optionDefinition.getValueTypeHelpText();
- String typeDescription = getTypeDescription(optionDefinition, optionsData);
+ String typeDescription = getTypeDescription(optionDefinition);
usage.append("<dt><code><a name=\"flag--").append(plainFlagName).append("\"></a>--");
usage.append(flagName);
- if (optionsData.isBooleanField(optionDefinition) || optionDefinition.isVoidField()) {
+ if (optionDefinition.isBooleanField() || optionDefinition.isVoidField()) {
// Nothing for boolean, tristate, boolean_or_enum, or void options.
} else if (!valueDescription.isEmpty()) {
usage.append("=").append(escaper.escape(valueDescription));
@@ -279,12 +279,12 @@
}
}
- private static String getTypeDescription(OptionDefinition optionsField, OptionsData optionsData) {
- return optionsData.getConverter(optionsField).getTypeDescription();
+ private static String getTypeDescription(OptionDefinition optionsDefinition) {
+ return optionsDefinition.getConverter().getTypeDescription();
}
- static String getFlagName(OptionDefinition optionDefinition, OptionsData optionsData) {
+ static String getFlagName(OptionDefinition optionDefinition) {
String name = optionDefinition.getOptionName();
- return optionsData.isBooleanField(optionDefinition) ? "[no]" + name : name;
+ return optionDefinition.isBooleanField() ? "[no]" + name : name;
}
}
