diff options
Diffstat (limited to 'src/main/java/com')
8 files changed, 185 insertions, 239 deletions
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 72f001fce0..6aa27b22d2 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 @@ package com.google.devtools.build.lib.analysis.config; 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 @@ public final class TransitiveOptionDetails implements Serializable { 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 8d9f93caba..a1602342e8 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.build.lib.windows.WindowsSubprocessFactory; 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 @@ public final class BlazeRuntime { 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 4be2b2327c..e76688c6d0 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 @@ public final class InvocationPolicyEnforcer { } // 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 @@ public final class InvocationPolicyEnforcer { 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 @@ public final class InvocationPolicyEnforcer { 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 @@ public final class InvocationPolicyEnforcer { // 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 @@ public final class InvocationPolicyEnforcer { // 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 @@ public final class InvocationPolicyEnforcer { 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 @@ public final class InvocationPolicyEnforcer { "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 @@ public final class InvocationPolicyEnforcer { 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 406b6cd94b..ca91b1d83d 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 @@ package com.google.devtools.common.options; 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.ParameterizedType; 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { other.nameToField, other.abbrevToField, other.allOptionsFields, - other.optionDefaults, - other.converters, - other.allowMultiple, other.usesOnlyCoreTypes); } @@ -176,135 +144,29 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { + "\" 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { } } - if (isBooleanField(optionDefinition, converter)) { + if (optionDefinition.isBooleanField()) { checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, optionName); } @@ -482,7 +339,7 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 @@ public class IsolatedOptionsData extends OpaqueOptionsData { 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 589208a4f1..d3f0d34648 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 @@ import java.util.Comparator; * 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 @@ public final class OptionDefinition { 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 36d11895b2..9d683208d1 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.ImmutableList; 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 @@ import javax.annotation.Nullable; */ 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 @@ public class OptionsParser implements OptionsProvider { } } - // 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 @@ public class OptionsParser implements OptionsProvider { 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 f8d67789f2..28aeb22b70 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 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { } 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 @@ class OptionsParserImpl { /* source */ null, implicitDependant, /* expendedFrom */ null, - optionsData.getAllowMultiple(parseResult.optionDefinition))); + parseResult.optionDefinition.allowsMultiple())); } return builder.build(); } @@ -390,7 +387,7 @@ class OptionsParserImpl { /* source */ null, /* implicitDependant */ null, flagName, - optionsData.getAllowMultiple(parseResult.optionDefinition))); + parseResult.optionDefinition.allowsMultiple())); } return builder.build(); } @@ -527,7 +524,7 @@ class OptionsParserImpl { + 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 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { 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 @@ class OptionsParserImpl { 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 51cc3c624a..0ab30da2b5 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 @@ class OptionsUsage { 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 @@ class OptionsUsage { 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 @@ class OptionsUsage { } } - 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; } } |