Use cached values for option converter types.
This requires us to have OptionsData for all usage messages, since static functionality is being removed, but this should already have been the case. It was added as an optional argument when the expansion function feature was added, but there is actually no reason not to require it, as the public interface for usage text was already computing the optionsData anyway.
PiperOrigin-RevId: 165386893
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 b4626bc..bd74b67 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -217,10 +217,14 @@
*
* <p>Can be used for usage help and controlling whether the "no" prefix is allowed.
*/
- static boolean isBooleanField(Field field) {
+ boolean isBooleanField(Field field) {
+ return isBooleanField(field, getConverter(field));
+ }
+
+ private static boolean isBooleanField(Field field, Converter<?> converter) {
return field.getType().equals(boolean.class)
|| field.getType().equals(TriState.class)
- || findConverter(field) instanceof BoolOrEnumConverter;
+ || converter instanceof BoolOrEnumConverter;
}
/** Returns whether a field has Void type. */
@@ -245,7 +249,7 @@
* 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.
*/
- static Converter<?> findConverter(Field optionField) {
+ private static Converter<?> findConverter(Field optionField) {
Option annotation = optionField.getAnnotation(Option.class);
if (annotation.converter() == Converter.class) {
// No converter provided, use the default one.
@@ -468,28 +472,31 @@
// Get the converter return type.
@SuppressWarnings("rawtypes")
- Class<? extends Converter> converter = annotation.converter();
- if (converter == Converter.class) {
+ Class<? extends Converter> converterClass = annotation.converter();
+ if (converterClass == Converter.class) {
Converter<?> actualConverter = Converters.DEFAULT_CONVERTERS.get(fieldType);
if (actualConverter == null) {
throw new ConstructionException("Cannot find converter for field of type "
+ field.getType() + " named " + field.getName()
+ " in class " + field.getDeclaringClass().getName());
}
- converter = actualConverter.getClass();
+ converterClass = actualConverter.getClass();
}
- if (Modifier.isAbstract(converter.getModifiers())) {
- throw new ConstructionException("The converter type " + converter
- + " must be a concrete type");
+ if (Modifier.isAbstract(converterClass.getModifiers())) {
+ throw new ConstructionException(
+ "The converter type " + converterClass + " must be a concrete type");
}
Type converterResultType;
try {
- Method convertMethod = converter.getMethod("convert", String.class);
- converterResultType = GenericTypeHelper.getActualReturnType(converter, convertMethod);
+ Method convertMethod = converterClass.getMethod("convert", String.class);
+ converterResultType =
+ GenericTypeHelper.getActualReturnType(converterClass, convertMethod);
} catch (NoSuchMethodException e) {
throw new ConstructionException(
"A known converter object doesn't implement the convert method");
}
+ Converter<?> converter = findConverter(field);
+ convertersBuilder.put(field, converter);
if (annotation.allowMultiple()) {
if (GenericTypeHelper.getRawType(converterResultType) == List.class) {
@@ -526,7 +533,7 @@
}
}
- if (isBooleanField(field)) {
+ if (isBooleanField(field, converter)) {
checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, optionName);
}
@@ -541,7 +548,7 @@
nameToFieldBuilder.put(annotation.oldName(), field);
// If boolean, repeat the alias dance for the old name.
- if (isBooleanField(field)) {
+ if (isBooleanField(field, converter)) {
checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, oldName);
}
}
@@ -552,8 +559,6 @@
optionDefaultsBuilder.put(field, retrieveDefaultFromAnnotation(field));
- convertersBuilder.put(field, findConverter(field));
-
allowMultipleBuilder.put(field, annotation.allowMultiple());
}
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 5ebd4a5..49d1547 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -624,7 +624,7 @@
booleanValue = false;
if (field != null) {
// TODO(bazel-team): Add tests for these cases.
- if (!OptionsData.isBooleanField(field)) {
+ if (!optionsData.isBooleanField(field)) {
throw new OptionsParsingException(
"Illegal use of 'no' prefix on non-boolean option: " + arg, arg);
}
@@ -650,7 +650,7 @@
if (value == null) {
// Special-case boolean to supply value based on presence of "no" prefix.
- if (OptionsData.isBooleanField(field)) {
+ if (optionsData.isBooleanField(field)) {
value = booleanValue ? "1" : "0";
} else if (field.getType().equals(Void.class) && !option.wrapperOption()) {
// This is expected, Void type options have no args (unless they're wrapper options).
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 6971e27..633b6f6 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsUsage.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsUsage.java
@@ -14,6 +14,7 @@
package com.google.devtools.common.options;
import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
@@ -25,9 +26,7 @@
import java.util.List;
import javax.annotation.Nullable;
-/**
- * A renderer for usage messages. For now this is very simple.
- */
+/** A renderer for usage messages for any combination of options classes. */
class OptionsUsage {
private static final Splitter NEWLINE_SPLITTER = Splitter.on('\n');
@@ -44,7 +43,7 @@
List<Field> optionFields = new ArrayList<>(data.getFieldsForClass(optionsClass));
optionFields.sort(BY_NAME);
for (Field optionField : optionFields) {
- getUsage(optionField, usage, OptionsParser.HelpVerbosity.LONG, null);
+ getUsage(optionField, usage, OptionsParser.HelpVerbosity.LONG, data);
}
}
@@ -79,44 +78,31 @@
}
/**
- * Returns the expansion for an option, to the extent known. Precisely, if an {@link OptionsData}
- * object is supplied, the expansion is read from that if the expansion function doesn't take an
- * argument. Otherwise, the annotation is inspected: If the annotation uses {@link
- * Option#expansion} it is returned, and if it uses {@link Option#expansionFunction} null is
- * returned, indicating a lack of definite information. In all cases, when the option is not an
- * expansion option, an empty list is returned.
+ * Returns the expansion for an option, if any, regardless of if the expansion is from a function
+ * or is statically declared in the annotation.
*/
private static @Nullable ImmutableList<String> getExpansionIfKnown(
- Field optionField, Option annotation, @Nullable OptionsData optionsData) {
- if (optionsData != null) {
- try {
- return optionsData.getEvaluatedExpansion(optionField, null);
- } catch (ExpansionNeedsValueException e) {
- return null;
- } catch (OptionsParsingException e) {
- throw new IllegalStateException("Error expanding void expansion function: ", e);
- }
- } else {
- if (OptionsData.usesExpansionFunction(annotation)) {
- return null;
- } else {
- // Empty list if it's not an expansion option.
- return ImmutableList.copyOf(annotation.expansion());
- }
+ Field optionField, OptionsData optionsData) {
+ Preconditions.checkNotNull(optionField);
+ Preconditions.checkNotNull(optionsData);
+ try {
+ return optionsData.getEvaluatedExpansion(optionField, null);
+ } catch (ExpansionNeedsValueException e) {
+ return null;
+ } catch (OptionsParsingException e) {
+ throw new IllegalStateException("Error expanding void expansion function: ", e);
}
+
}
- /**
- * Appends the usage message for a single option-field message to 'usage'. If {@code optionsData}
- * is not supplied, options that use expansion functions won't be fully described.
- */
+ /** Appends the usage message for a single option-field message to 'usage'. */
static void getUsage(
Field optionField,
StringBuilder usage,
OptionsParser.HelpVerbosity helpVerbosity,
- @Nullable OptionsData optionsData) {
- String flagName = getFlagName(optionField);
- String typeDescription = getTypeDescription(optionField);
+ OptionsData optionsData) {
+ String flagName = getFlagName(optionField, optionsData);
+ String typeDescription = getTypeDescription(optionField, optionsData);
Option annotation = optionField.getAnnotation(Option.class);
usage.append(" --").append(flagName);
if (helpVerbosity == OptionsParser.HelpVerbosity.SHORT) { // just the name
@@ -149,7 +135,7 @@
usage.append(paragraphFill(annotation.help(), 4, 80)); // (indent, width)
usage.append('\n');
}
- ImmutableList<String> expansion = getExpansionIfKnown(optionField, annotation, optionsData);
+ ImmutableList<String> expansion = getExpansionIfKnown(optionField, optionsData);
if (expansion == null) {
usage.append(" Expands to unknown options.\n");
} else if (!expansion.isEmpty()) {
@@ -162,20 +148,17 @@
}
}
- /**
- * Append the usage message for a single option-field message to 'usage'. If {@code optionsData}
- * is not supplied, options that use expansion functions won't be fully described.
- */
+ /** Append the usage message for a single option-field message to 'usage'. */
static void getUsageHtml(
- Field optionField, StringBuilder usage, Escaper escaper, @Nullable OptionsData optionsData) {
- String plainFlagName = optionField.getAnnotation(Option.class).name();
- String flagName = getFlagName(optionField);
- String valueDescription = optionField.getAnnotation(Option.class).valueHelp();
- String typeDescription = getTypeDescription(optionField);
+ Field optionField, StringBuilder usage, Escaper escaper, OptionsData optionsData) {
Option annotation = optionField.getAnnotation(Option.class);
+ String plainFlagName = annotation.name();
+ String flagName = getFlagName(optionField, optionsData);
+ String valueDescription = annotation.valueHelp();
+ String typeDescription = getTypeDescription(optionField, optionsData);
usage.append("<dt><code><a name=\"flag--").append(plainFlagName).append("\"></a>--");
usage.append(flagName);
- if (OptionsData.isBooleanField(optionField) || OptionsData.isVoidField(optionField)) {
+ if (optionsData.isBooleanField(optionField) || OptionsData.isVoidField(optionField)) {
// Nothing for boolean, tristate, boolean_or_enum, or void options.
} else if (!valueDescription.isEmpty()) {
usage.append("=").append(escaper.escape(valueDescription));
@@ -207,7 +190,7 @@
usage.append(paragraphFill(escaper.escape(annotation.help()), 0, 80)); // (indent, width)
usage.append('\n');
}
- ImmutableList<String> expansion = getExpansionIfKnown(optionField, annotation, optionsData);
+ ImmutableList<String> expansion = getExpansionIfKnown(optionField, optionsData);
if (expansion == null) {
usage.append(" Expands to unknown options.<br>\n");
} else if (!expansion.isEmpty()) {
@@ -298,13 +281,13 @@
}
};
- private static String getTypeDescription(Field optionsField) {
- return OptionsData.findConverter(optionsField).getTypeDescription();
+ private static String getTypeDescription(Field optionsField, OptionsData optionsData) {
+ return optionsData.getConverter(optionsField).getTypeDescription();
}
- static String getFlagName(Field field) {
+ static String getFlagName(Field field, OptionsData optionsData) {
String name = field.getAnnotation(Option.class).name();
- return OptionsData.isBooleanField(field) ? "[no]" + name : name;
+ return optionsData.isBooleanField(field) ? "[no]" + name : name;
}
}
diff --git a/src/test/java/com/google/devtools/common/options/OptionsTest.java b/src/test/java/com/google/devtools/common/options/OptionsTest.java
index 12ba3e4..e8d7237 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsTest.java
@@ -88,8 +88,11 @@
// Interestingly, the class needs to be public, or else the default constructor ends up not
// being public and the expander can't be instantiated.
- /** SpecialExpansion */
- public static class SpecialExpansion implements ExpansionFunction {
+ /**
+ * Defines an expansion function that looks at other options defined with it and expands to
+ * options that match a pattern.
+ */
+ public static class ExpansionDependsOnOtherOptionDefinitions implements ExpansionFunction {
@Override
public ImmutableList<String> getExpansion(ExpansionContext context) {
TreeSet<String> flags = new TreeSet<>();
@@ -102,8 +105,11 @@
}
}
- /** VariableExpansion */
- public static class VariableExpansion implements ExpansionFunction {
+ /**
+ * Defines an expansion function that adapts its expansion to the value assigned to the original
+ * expansion option.
+ */
+ public static class ExpansionDependsOnFlagValue implements ExpansionFunction {
@Override
public ImmutableList<String> getExpansion(ExpansionContext context)
throws OptionsParsingException {
@@ -140,16 +146,17 @@
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
effectTags = {OptionEffectTag.NO_OP},
defaultValue = "null",
- expansionFunction = SpecialExpansion.class
+ expansionFunction = ExpansionDependsOnOtherOptionDefinitions.class
)
public Void specialExp;
-
@Option(
- name = "dynamicexp",
- documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
- effectTags = {OptionEffectTag.NO_OP},
- defaultValue = "null", expansionFunction = VariableExpansion.class)
+ name = "dynamicexp",
+ documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+ effectTags = {OptionEffectTag.NO_OP},
+ defaultValue = "null",
+ expansionFunction = ExpansionDependsOnFlagValue.class
+ )
public Void variableExpansion;
}
@@ -420,9 +427,9 @@
String usage = Options.getUsage(HttpOptions.class);
assertThat(usage)
.contains(" --special\n Expands to: --host=special.google.com --port=8080");
- // Expansion functions aren't evaluated since we're just grabbing the usage for an OptionsBase
- // subclass and not for a completed parser. The completed case is covered in OptionsParserTest.
- assertThat(usage).contains(" --specialexp\n Expands to unknown options.");
+ // Expect that the usage text contains the expansion appropriate to the options bases that were
+ // loaded into the options parser.
+ assertThat(usage).contains(" --specialexp\n Expands to: --specialexp_bar --specialexp_foo");
}
public static class NullTestOptions extends OptionsBase {
