Add OptionDefinition layer between the @Option annotation and its fields and the options parser.

Removes any direct reads of the annotation outside of OptionDefinition. This allows for fewer manual checks for the annotation's existence, unifies error wording, and paves the way for potentially generifying the OptionsParser to accept different @Option-equivalent annotations.

Also allows for cleanup of duplicate code by giving @Option-specific operations a clear home, such as sorts and default logic. In followup changes, we can eliminate some unnecessarily complex caching by instead memoizing values in the OptionDefinition. This will have the positive side effect of making sure reads come from the cached values.

RELNOTES: None.
PiperOrigin-RevId: 166019075

1 files changed, 134 insertions, 145 deletions

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 bd74b67990..e087dbbb56 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -16,7 +16,6 @@ package com.google.devtools.common.options;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Ordering;
 import com.google.devtools.common.options.OptionsParser.ConstructionException;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
@@ -24,13 +23,14 @@ import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
-import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import javax.annotation.concurrent.Immutable;
 
 /**
@@ -57,20 +57,21 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
   private final ImmutableMap<Class<? extends OptionsBase>, Constructor<?>> optionsClasses;
 
   /**
-   * Mapping from option name to {@code @Option}-annotated field. Entries appear ordered first by
-   * their options class (the order in which they were passed to {@link #from(Collection)}, and then
-   * in alphabetic order within each options class.
+   * Mapping from option name to {@code OptionDefinition}. Entries appear ordered first by their
+   * options class (the order in which they were passed to {@link #from(Collection)}, and then in
+   * alphabetic order within each options class.
    */
-  private final ImmutableMap<String, Field> nameToField;
+  private final ImmutableMap<String, OptionDefinition> nameToField;
 
-  /** Mapping from option abbreviation to {@code Option}-annotated field (unordered). */
-  private final ImmutableMap<Character, Field> abbrevToField;
+  /** Mapping from option abbreviation to {@code OptionDefinition} (unordered). */
+  private final ImmutableMap<Character, OptionDefinition> abbrevToField;
 
   /**
-   * Mapping from options class to a list of all {@code Option}-annotated fields in that class. The
-   * map entries are unordered, but the fields in the lists are ordered alphabetically.
+   * Mapping from options class to a list of all {@code OptionFields} in that class. The map entries
+   * are unordered, but the fields in the lists are ordered alphabetically.
    */
-  private final ImmutableMap<Class<? extends OptionsBase>, ImmutableList<Field>> allOptionsFields;
+  private final ImmutableMap<Class<? extends OptionsBase>, ImmutableList<OptionDefinition>>
+      allOptionsFields;
 
   /**
    * Mapping from each {@code Option}-annotated field to the default value for that field
@@ -79,20 +80,20 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
    * <p>(This is immutable like the others, but uses {@code Collections.unmodifiableMap} to support
    * null values.)
    */
-  private final Map<Field, Object> optionDefaults;
+  private final Map<OptionDefinition, Object> optionDefaults;
 
   /**
    * Mapping from each {@code Option}-annotated field to the proper converter (unordered).
    *
    * @see #findConverter
    */
-  private final ImmutableMap<Field, Converter<?>> converters;
+  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<Field, Boolean> allowMultiple;
+  private final ImmutableMap<OptionDefinition, Boolean> allowMultiple;
 
   /**
    * Mapping from each options class to whether or not it has the {@link UsesOnlyCoreTypes}
@@ -105,14 +106,13 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
       "undocumented", "hidden", "internal");
 
   private IsolatedOptionsData(
-      Map<Class<? extends OptionsBase>,
-      Constructor<?>> optionsClasses,
-      Map<String, Field> nameToField,
-      Map<Character, Field> abbrevToField,
-      Map<Class<? extends OptionsBase>, ImmutableList<Field>> allOptionsFields,
-      Map<Field, Object> optionDefaults,
-      Map<Field, Converter<?>> converters,
-      Map<Field, Boolean> allowMultiple,
+      Map<Class<? extends OptionsBase>, Constructor<?>> optionsClasses,
+      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);
@@ -150,7 +150,7 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     return (Constructor<T>) optionsClasses.get(clazz);
   }
 
-  public Field getFieldFromName(String name) {
+  public OptionDefinition getFieldFromName(String name) {
     return nameToField.get(name);
   }
 
@@ -159,11 +159,11 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
    * objects. Entries appear ordered first by their options class (the order in which they were
    * passed to {@link #from(Collection)}, and then in alphabetic order within each options class.
    */
-  public Iterable<Map.Entry<String, Field>> getAllNamedFields() {
+  public Iterable<Map.Entry<String, OptionDefinition>> getAllNamedFields() {
     return nameToField.entrySet();
   }
 
-  public Field getFieldForAbbrev(char abbrev) {
+  public OptionDefinition getFieldForAbbrev(char abbrev) {
     return abbrevToField.get(abbrev);
   }
 
@@ -171,20 +171,21 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
    * Returns a list of all {@link Field} objects for options in the given options class, ordered
    * alphabetically by option name.
    */
-  public ImmutableList<Field> getFieldsForClass(Class<? extends OptionsBase> optionsClass) {
+  public ImmutableList<OptionDefinition> getOptionDefinitionsFromClass(
+      Class<? extends OptionsBase> optionsClass) {
     return allOptionsFields.get(optionsClass);
   }
 
-  public Object getDefaultValue(Field field) {
-    return optionDefaults.get(field);
+  public Object getDefaultValue(OptionDefinition optionDefinition) {
+    return optionDefaults.get(optionDefinition);
   }
 
-  public Converter<?> getConverter(Field field) {
-    return converters.get(field);
+  public Converter<?> getConverter(OptionDefinition optionDefinition) {
+    return converters.get(optionDefinition);
   }
 
-  public boolean getAllowMultiple(Field field) {
-    return allowMultiple.get(field);
+  public boolean getAllowMultiple(OptionDefinition optionDefinition) {
+    return allowMultiple.get(optionDefinition);
   }
 
   public boolean getUsesOnlyCoreTypes(Class<? extends OptionsBase> optionsClass) {
@@ -196,9 +197,9 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
    * that does use it, asserts that the type is a {@code List<T>} and returns its element type
    * {@code T}.
    */
-  private static Type getFieldSingularType(Field field, Option annotation) {
-    Type fieldType = field.getGenericType();
-    if (annotation.allowMultiple()) {
+  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<...>");
@@ -217,43 +218,24 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
    *
    * <p>Can be used for usage help and controlling whether the "no" prefix is allowed.
    */
-  boolean isBooleanField(Field field) {
-    return isBooleanField(field, getConverter(field));
+  boolean isBooleanField(OptionDefinition optionDefinition) {
+    return isBooleanField(optionDefinition, getConverter(optionDefinition));
   }
 
-  private static boolean isBooleanField(Field field, Converter<?> converter) {
-    return field.getType().equals(boolean.class)
-        || field.getType().equals(TriState.class)
+  private static boolean isBooleanField(OptionDefinition optionDefinition, Converter<?> converter) {
+    return optionDefinition.getType().equals(boolean.class)
+        || optionDefinition.getType().equals(TriState.class)
         || converter instanceof BoolOrEnumConverter;
   }
 
-  /** Returns whether a field has Void type. */
-  static boolean isVoidField(Field field) {
-    return field.getType().equals(Void.class);
-  }
-
-  /** Returns whether the arg is an expansion option. */
-  public static boolean isExpansionOption(Option annotation) {
-    return (annotation.expansion().length > 0 || OptionsData.usesExpansionFunction(annotation));
-  }
-
-  /**
-   * Returns whether the arg is an expansion option defined by an expansion function (and not a
-   * constant expansion value).
-   */
-  static boolean usesExpansionFunction(Option annotation) {
-    return annotation.expansionFunction() != ExpansionFunction.class;
-  }
-
   /**
    * 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(Field optionField) {
-    Option annotation = optionField.getAnnotation(Option.class);
-    if (annotation.converter() == Converter.class) {
+  private static Converter<?> findConverter(OptionDefinition optionDefinition) {
+    if (optionDefinition.getProvidedConverter() == Converter.class) {
       // No converter provided, use the default one.
-      Type type = getFieldSingularType(optionField, annotation);
+      Type type = getFieldSingularType(optionDefinition);
       Converter<?> converter = Converters.DEFAULT_CONVERTERS.get(type);
       if (converter == null) {
         throw new ConstructionException(
@@ -261,13 +243,13 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
                 + type
                 + "; possible fix: add "
                 + "converter=... to @Option annotation for "
-                + optionField.getName());
+                + optionDefinition.getField().getName());
       }
       return converter;
     }
     try {
       // Instantiate the given Converter class.
-      Class<?> converter = annotation.converter();
+      Class<?> converter = optionDefinition.getProvidedConverter();
       Constructor<?> constructor = converter.getConstructor();
       return (Converter<?>) constructor.newInstance();
     } catch (Exception e) {
@@ -276,40 +258,32 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
       throw new ConstructionException(e);
     }
   }
-
-  private static final Ordering<Field> fieldOrdering =
-      new Ordering<Field>() {
-    @Override
-    public int compare(Field f1, Field f2) {
-      String n1 = f1.getAnnotation(Option.class).name();
-      String n2 = f2.getAnnotation(Option.class).name();
-      return n1.compareTo(n2);
-    }
-  };
-
-  /**
-   * Return all {@code @Option}-annotated fields, alphabetically ordered by their option name (not
-   * their field name).
-   */
-  private static ImmutableList<Field> getAllAnnotatedFieldsSorted(
+  /** Returns all {@code optionDefinitions}, ordered by their option name (not their field name). */
+  private static ImmutableList<OptionDefinition> getAllOptionDefinitionsSorted(
       Class<? extends OptionsBase> optionsClass) {
-    List<Field> unsortedFields = new ArrayList<>();
-    for (Field field : optionsClass.getFields()) {
-      if (field.isAnnotationPresent(Option.class)) {
-        unsortedFields.add(field);
-      }
-    }
-    return fieldOrdering.immutableSortedCopy(unsortedFields);
+    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;
+          }
+        })
+        .filter(Objects::nonNull)
+        .sorted(OptionDefinition.BY_OPTION_NAME)
+        .collect(ImmutableList.toImmutableList());
   }
 
-  private static Object retrieveDefaultFromAnnotation(Field optionField) {
-    Converter<?> converter = findConverter(optionField);
-    String defaultValueAsString = OptionsParserImpl.getDefaultOptionString(optionField);
+  private static Object retrieveDefaultValue(OptionDefinition optionDefinition) {
+    Converter<?> converter = findConverter(optionDefinition);
+    String defaultValueAsString = optionDefinition.getUnparsedDefaultValue();
     // Special case for "null"
-    if (OptionsParserImpl.isSpecialNullDefault(defaultValueAsString, optionField)) {
+    if (optionDefinition.isSpecialNullDefault()) {
       return null;
     }
-    boolean allowsMultiple = optionField.getAnnotation(Option.class).allowMultiple();
+    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.
@@ -321,17 +295,18 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     try {
       convertedValue = converter.convert(defaultValueAsString);
     } catch (OptionsParsingException e) {
-      throw new IllegalStateException("OptionsParsingException while "
-          + "retrieving default for " + optionField.getName() + ": "
-          + e.getMessage());
+      throw new IllegalStateException(
+          "OptionsParsingException while "
+              + "retrieving default for "
+              + optionDefinition.getField().getName()
+              + ": "
+              + e.getMessage());
     }
     return convertedValue;
   }
 
   private static <A> void checkForCollisions(
-      Map<A, Field> aFieldMap,
-      A optionName,
-      String description) {
+      Map<A, OptionDefinition> aFieldMap, A optionName, String description) {
     if (aFieldMap.containsKey(optionName)) {
       throw new DuplicateOptionDeclarationException(
           "Duplicate option name, due to " + description + ": --" + optionName);
@@ -354,7 +329,7 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
   }
 
   private static void checkAndUpdateBooleanAliases(
-      Map<String, Field> nameToFieldMap,
+      Map<String, OptionDefinition> nameToFieldMap,
       Map<String, String> booleanAliasMap,
       String optionName) {
     // Check that the negating alias does not conflict with existing flags.
@@ -418,13 +393,13 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
   static IsolatedOptionsData from(Collection<Class<? extends OptionsBase>> classes) {
     // Mind which fields have to preserve order.
     Map<Class<? extends OptionsBase>, Constructor<?>> constructorBuilder = new LinkedHashMap<>();
-    Map<Class<? extends OptionsBase>, ImmutableList<Field>> allOptionsFieldsBuilder =
+    Map<Class<? extends OptionsBase>, ImmutableList<OptionDefinition>> allOptionsFieldsBuilder =
         new HashMap<>();
-    Map<String, Field> nameToFieldBuilder = new LinkedHashMap<>();
-    Map<Character, Field> abbrevToFieldBuilder = new HashMap<>();
-    Map<Field, Object> optionDefaultsBuilder = new HashMap<>();
-    Map<Field, Converter<?>> convertersBuilder = new HashMap<>();
-    Map<Field, Boolean> allowMultipleBuilder = 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<>();
@@ -441,44 +416,54 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
         throw new IllegalArgumentException(parsedOptionsClass
             + " lacks an accessible default constructor");
       }
-      ImmutableList<Field> fields = getAllAnnotatedFieldsSorted(parsedOptionsClass);
-      allOptionsFieldsBuilder.put(parsedOptionsClass, fields);
+      ImmutableList<OptionDefinition> optionDefinitions =
+          getAllOptionDefinitionsSorted(parsedOptionsClass);
+      allOptionsFieldsBuilder.put(parsedOptionsClass, optionDefinitions);
 
-      for (Field field : fields) {
-        Option annotation = field.getAnnotation(Option.class);
-        String optionName = annotation.name();
+      for (OptionDefinition optionDefinition : optionDefinitions) {
+        String optionName = optionDefinition.getOptionName();
+
+        // Check that the option makes sense on its own, as defined.
         if (optionName == null) {
           throw new ConstructionException("Option cannot have a null name");
         }
 
-        if (DEPRECATED_CATEGORIES.contains(annotation.category())) {
+        if (DEPRECATED_CATEGORIES.contains(optionDefinition.getOptionCategory())) {
           throw new ConstructionException(
               "Documentation level is no longer read from the option category. Category \""
-                  + annotation.category() + "\" in option \"" + optionName + "\" is disallowed.");
+                  + optionDefinition.getOptionCategory()
+                  + "\" in option \""
+                  + optionName
+                  + "\" is disallowed.");
         }
 
-        checkEffectTagRationality(optionName, annotation.effectTags());
+        checkEffectTagRationality(optionName, optionDefinition.getOptionEffectTags());
         checkMetadataTagAndCategoryRationality(
-            optionName, annotation.metadataTags(), annotation.documentationCategory());
-
-        Type fieldType = getFieldSingularType(field, annotation);
+            optionName,
+            optionDefinition.getOptionMetadataTags(),
+            optionDefinition.getDocumentationCategory());
+        Type fieldType = getFieldSingularType(optionDefinition);
         // For simple, static expansions, don't accept non-Void types.
-        if (annotation.expansion().length != 0 && !isVoidField(field)) {
+        if (optionDefinition.getOptionExpansion().length != 0 && !optionDefinition.isVoidField()) {
           throw new ConstructionException(
               "Option "
-                  + optionName
+                  + optionDefinition.getOptionName()
                   + " is an expansion flag with a static expansion, but does not have Void type.");
         }
 
-        // Get the converter return type.
+        // Get the converter's return type to check that it matches the option type.
         @SuppressWarnings("rawtypes")
-        Class<? extends Converter> converterClass = annotation.converter();
+        Class<? extends Converter> converterClass = optionDefinition.getProvidedConverter();
         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());
+            throw new ConstructionException(
+                "Cannot find converter for field of type "
+                    + optionDefinition.getType()
+                    + " named "
+                    + optionDefinition.getField().getName()
+                    + " in class "
+                    + optionDefinition.getField().getDeclaringClass().getName());
           }
           converterClass = actualConverter.getClass();
         }
@@ -495,10 +480,10 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
           throw new ConstructionException(
               "A known converter object doesn't implement the convert method");
         }
-        Converter<?> converter = findConverter(field);
-        convertersBuilder.put(field, converter);
+        Converter<?> converter = findConverter(optionDefinition);
+        convertersBuilder.put(optionDefinition, converter);
 
-        if (annotation.allowMultiple()) {
+        if (optionDefinition.allowsMultiple()) {
           if (GenericTypeHelper.getRawType(converterResultType) == List.class) {
             Type elementType =
                 ((ParameterizedType) converterResultType).getActualTypeArguments()[0];
@@ -533,47 +518,51 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
           }
         }
 
-        if (isBooleanField(field, converter)) {
+        if (isBooleanField(optionDefinition, converter)) {
           checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, optionName);
         }
 
         checkForCollisions(nameToFieldBuilder, optionName, "option");
         checkForBooleanAliasCollisions(booleanAliasMap, optionName, "option");
-        nameToFieldBuilder.put(optionName, field);
+        nameToFieldBuilder.put(optionName, optionDefinition);
 
-        if (!annotation.oldName().isEmpty()) {
-          String oldName = annotation.oldName();
+        if (!optionDefinition.getOldOptionName().isEmpty()) {
+          String oldName = optionDefinition.getOldOptionName();
           checkForCollisions(nameToFieldBuilder, oldName, "old option name");
           checkForBooleanAliasCollisions(booleanAliasMap, oldName, "old option name");
-          nameToFieldBuilder.put(annotation.oldName(), field);
+          nameToFieldBuilder.put(optionDefinition.getOldOptionName(), optionDefinition);
 
           // If boolean, repeat the alias dance for the old name.
-          if (isBooleanField(field, converter)) {
+          if (isBooleanField(optionDefinition, converter)) {
             checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, oldName);
           }
         }
-        if (annotation.abbrev() != '\0') {
-          checkForCollisions(abbrevToFieldBuilder, annotation.abbrev(), "option abbreviation");
-          abbrevToFieldBuilder.put(annotation.abbrev(), field);
+        if (optionDefinition.getAbbreviation() != '\0') {
+          checkForCollisions(
+              abbrevToFieldBuilder, optionDefinition.getAbbreviation(), "option abbreviation");
+          abbrevToFieldBuilder.put(optionDefinition.getAbbreviation(), optionDefinition);
         }
 
-        optionDefaultsBuilder.put(field, retrieveDefaultFromAnnotation(field));
-
-        allowMultipleBuilder.put(field, annotation.allowMultiple());
-
-        }
+        optionDefaultsBuilder.put(optionDefinition, retrieveDefaultValue(optionDefinition));
+        allowMultipleBuilder.put(optionDefinition, optionDefinition.allowsMultiple());
+      }
 
       boolean usesOnlyCoreTypes = parsedOptionsClass.isAnnotationPresent(UsesOnlyCoreTypes.class);
       if (usesOnlyCoreTypes) {
         // Validate that @UsesOnlyCoreTypes was used correctly.
-        for (Field field : fields) {
+        for (OptionDefinition optionDefinition : optionDefinitions) {
           // The classes in coreTypes are all final. But even if they weren't, we only want to check
           // for exact matches; subclasses would not be considered core types.
-          if (!UsesOnlyCoreTypes.CORE_TYPES.contains(field.getType())) {
+          if (!UsesOnlyCoreTypes.CORE_TYPES.contains(optionDefinition.getType())) {
             throw new ConstructionException(
-                "Options class '" + parsedOptionsClass.getName() + "' is marked as "
-                + "@UsesOnlyCoreTypes, but field '" + field.getName()
-                + "' has type '" + field.getType().getName() + "'");
+                "Options class '"
+                    + parsedOptionsClass.getName()
+                    + "' is marked as "
+                    + "@UsesOnlyCoreTypes, but field '"
+                    + optionDefinition.getField().getName()
+                    + "' has type '"
+                    + optionDefinition.getType().getName()
+                    + "'");
           }
         }
       }