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, 119 insertions, 87 deletions

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 d4779fecec..d4e430509a 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -14,8 +14,6 @@
 
 package com.google.devtools.common.options;
 
-import static java.util.Comparator.comparing;
-
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
@@ -28,12 +26,15 @@ import java.nio.file.FileSystem;
 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.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
 /**
@@ -66,7 +67,7 @@ public class OptionsParser implements OptionsProvider {
 
   /**
    * An unchecked exception thrown when there is a problem constructing a parser, e.g. an error
-   * while validating an {@link Option} field in one of its {@link OptionsBase} subclasses.
+   * while validating an {@link OptionDefinition} in one of its {@link OptionsBase} subclasses.
    *
    * <p>This exception is unchecked because it generally indicates an internal error affecting all
    * invocations of the program. I.e., any such error should be immediately obvious to the
@@ -225,9 +226,9 @@ public class OptionsParser implements OptionsProvider {
     }
   }
 
-  /**
-   * The metadata about an option.
-   */
+  // TODO(b/64904491) remove this once the converter and default information is in OptionDefinition
+  // and cached.
+  /** The metadata about an option. */
   public static final class OptionDescription {
 
     private final String name;
@@ -423,16 +424,21 @@ public class OptionsParser implements OptionsProvider {
    */
   public static class UnparsedOptionValueDescription {
     private final String name;
-    private final Field field;
+    private final OptionDefinition optionDefinition;
     private final String unparsedValue;
     private final OptionPriority priority;
     private final String source;
     private final boolean explicit;
 
-    public UnparsedOptionValueDescription(String name, Field field, String unparsedValue,
-        OptionPriority priority, String source, boolean explicit) {
+    public UnparsedOptionValueDescription(
+        String name,
+        OptionDefinition optionDefinition,
+        String unparsedValue,
+        OptionPriority priority,
+        String source,
+        boolean explicit) {
       this.name = name;
-      this.field = field;
+      this.optionDefinition = optionDefinition;
       this.unparsedValue = unparsedValue;
       this.priority = priority;
       this.source = source;
@@ -443,20 +449,20 @@ public class OptionsParser implements OptionsProvider {
       return name;
     }
 
-    Field getField() {
-      return field;
+    OptionDefinition getOptionDefinition() {
+      return optionDefinition;
     }
 
     public boolean isBooleanOption() {
-      return field.getType().equals(boolean.class);
+      return optionDefinition.getType().equals(boolean.class);
     }
 
     private OptionDocumentationCategory documentationCategory() {
-      return field.getAnnotation(Option.class).documentationCategory();
+      return optionDefinition.getDocumentationCategory();
     }
 
     private ImmutableList<OptionMetadataTag> metadataTags() {
-      return ImmutableList.copyOf(field.getAnnotation(Option.class).metadataTags());
+      return ImmutableList.copyOf(optionDefinition.getOptionMetadataTags());
     }
 
     public boolean isDocumented() {
@@ -469,17 +475,15 @@ public class OptionsParser implements OptionsProvider {
     }
 
     boolean isExpansion() {
-      return OptionsData.isExpansionOption(field.getAnnotation(Option.class));
+      return optionDefinition.isExpansionOption();
     }
 
     boolean isImplicitRequirement() {
-      Option option = field.getAnnotation(Option.class);
-      return option.implicitRequirements().length > 0;
+      return optionDefinition.getImplicitRequirements().length > 0;
     }
 
     boolean allowMultiple() {
-      Option option = field.getAnnotation(Option.class);
-      return option.allowMultiple();
+      return optionDefinition.allowsMultiple();
     }
 
     public String getUnparsedValue() {
@@ -535,18 +539,18 @@ public class OptionsParser implements OptionsProvider {
     OptionsData data = impl.getOptionsData();
     StringBuilder desc = new StringBuilder();
     if (!data.getOptionsClasses().isEmpty()) {
-      List<Field> allFields = new ArrayList<>();
+      List<OptionDefinition> allFields = new ArrayList<>();
       for (Class<? extends OptionsBase> optionsClass : data.getOptionsClasses()) {
-        allFields.addAll(data.getFieldsForClass(optionsClass));
+        allFields.addAll(data.getOptionDefinitionsFromClass(optionsClass));
       }
-      allFields.sort(OptionsUsage.BY_CATEGORY);
+      Collections.sort(allFields, OptionDefinition.BY_CATEGORY);
       String prevCategory = null;
 
-      for (Field optionField : allFields) {
-        Option option = optionField.getAnnotation(Option.class);
-        String category = option.category();
+      for (OptionDefinition optionDefinition : allFields) {
+        String category = optionDefinition.getOptionCategory();
         if (!category.equals(prevCategory)
-            && option.documentationCategory() != OptionDocumentationCategory.UNDOCUMENTED) {
+            && optionDefinition.getDocumentationCategory()
+                != OptionDocumentationCategory.UNDOCUMENTED) {
           String description = categoryDescriptions.get(category);
           if (description == null) {
             description = "Options category '" + category + "'";
@@ -555,8 +559,9 @@ public class OptionsParser implements OptionsProvider {
           prevCategory = category;
         }
 
-        if (option.documentationCategory() != OptionDocumentationCategory.UNDOCUMENTED) {
-          OptionsUsage.getUsage(optionField, desc, helpVerbosity, impl.getOptionsData());
+        if (optionDefinition.getDocumentationCategory()
+            != OptionDocumentationCategory.UNDOCUMENTED) {
+          OptionsUsage.getUsage(optionDefinition, desc, helpVerbosity, impl.getOptionsData());
         }
       }
     }
@@ -578,18 +583,18 @@ public class OptionsParser implements OptionsProvider {
     OptionsData data = impl.getOptionsData();
     StringBuilder desc = new StringBuilder();
     if (!data.getOptionsClasses().isEmpty()) {
-      List<Field> allFields = new ArrayList<>();
+      List<OptionDefinition> allFields = new ArrayList<>();
       for (Class<? extends OptionsBase> optionsClass : data.getOptionsClasses()) {
-        allFields.addAll(data.getFieldsForClass(optionsClass));
+        allFields.addAll(data.getOptionDefinitionsFromClass(optionsClass));
       }
-      allFields.sort(OptionsUsage.BY_CATEGORY);
+      Collections.sort(allFields, OptionDefinition.BY_CATEGORY);
       String prevCategory = null;
 
-      for (Field optionField : allFields) {
-        Option option = optionField.getAnnotation(Option.class);
-        String category = option.category();
+      for (OptionDefinition optionDefinition : allFields) {
+        String category = optionDefinition.getOptionCategory();
         if (!category.equals(prevCategory)
-            && option.documentationCategory() != OptionDocumentationCategory.UNDOCUMENTED) {
+            && optionDefinition.getDocumentationCategory()
+                != OptionDocumentationCategory.UNDOCUMENTED) {
           String description = categoryDescriptions.get(category);
           if (description == null) {
             description = "Options category '" + category + "'";
@@ -602,8 +607,9 @@ public class OptionsParser implements OptionsProvider {
           prevCategory = category;
         }
 
-        if (option.documentationCategory() != OptionDocumentationCategory.UNDOCUMENTED) {
-          OptionsUsage.getUsageHtml(optionField, desc, escaper, impl.getOptionsData());
+        if (optionDefinition.getDocumentationCategory()
+            != OptionDocumentationCategory.UNDOCUMENTED) {
+          OptionsUsage.getUsageHtml(optionDefinition, desc, escaper, impl.getOptionsData());
         }
       }
       desc.append("</dl>\n");
@@ -613,8 +619,8 @@ public class OptionsParser implements OptionsProvider {
 
   /**
    * Returns a string listing the possible flag completion for this command along with the command
-   * completion if any. See {@link OptionsUsage#getCompletion(Field, StringBuilder)} for more
-   * details on the format for the flag completion.
+   * completion if any. See {@link OptionsUsage#getCompletion(OptionDefinition, StringBuilder)} for
+   * more details on the format for the flag completion.
    */
   public String getOptionsCompletion() {
     OptionsData data = impl.getOptionsData();
@@ -623,14 +629,14 @@ public class OptionsParser implements OptionsProvider {
     data.getOptionsClasses()
         // List all options
         .stream()
-        .flatMap(optionsClass -> data.getFieldsForClass(optionsClass).stream())
+        .flatMap(optionsClass -> data.getOptionDefinitionsFromClass(optionsClass).stream())
         // Sort field for deterministic ordering
-        .sorted(comparing(optionField -> optionField.getAnnotation(Option.class).name()))
+        .sorted(OptionDefinition.BY_OPTION_NAME)
         .filter(
-            optionField ->
-                optionField.getAnnotation(Option.class).documentationCategory()
+            optionDefinition ->
+                optionDefinition.getDocumentationCategory()
                     != OptionDocumentationCategory.UNDOCUMENTED)
-        .forEach(optionField -> OptionsUsage.getCompletion(optionField, desc));
+        .forEach(optionDefinition -> OptionsUsage.getCompletion(optionDefinition, desc));
 
     return desc.toString();
   }
@@ -780,9 +786,9 @@ public class OptionsParser implements OptionsProvider {
   }
 
   /** Returns all options fields of the given options class, in alphabetic order. */
-  public static Collection<Field> getFields(Class<? extends OptionsBase> optionsClass) {
+  public static Collection<OptionDefinition> getFields(Class<? extends OptionsBase> optionsClass) {
     OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
-    return data.getFieldsForClass(optionsClass);
+    return data.getOptionDefinitionsFromClass(optionsClass);
   }
 
   /**
@@ -798,21 +804,26 @@ public class OptionsParser implements OptionsProvider {
    * Returns a mapping from each option {@link Field} in {@code optionsClass} (including inherited
    * ones) to its value in {@code options}.
    *
-   * <p>The map is a mutable copy; changing the map won't affect {@code options} and vice versa.
-   * The map entries appear sorted alphabetically by option name.
+   * <p>To save space, the map directly stores {@code Fields} instead of the {@code
+   * OptionDefinitions}.
    *
-   * If {@code options} is an instance of a subclass of {@code optionsClass}, any options defined
-   * by the subclass are not included in the map.
+   * <p>The map is a mutable copy; changing the map won't affect {@code options} and vice versa. The
+   * map entries appear sorted alphabetically by option name.
    *
-   * @throws IllegalArgumentException if {@code options} is not an instance of {@code optionsClass}
+   * <p>If {@code options} is an instance of a subclass of {@link OptionsBase}, any options defined
+   * by the subclass are not included in the map, only the options declared in the provided class
+   * are included.
+   *
+   * @throws IllegalArgumentException if {@code options} is not an instance of {@link OptionsBase}
    */
   public static <O extends OptionsBase> Map<Field, Object> toMap(Class<O> optionsClass, O options) {
     OptionsData data = getOptionsDataInternal(optionsClass);
-    // Alphabetized due to getFieldsForClass()'s order.
+    // Alphabetized due to getOptionDefinitionsFromClass()'s order.
     Map<Field, Object> map = new LinkedHashMap<>();
-    for (Field field : data.getFieldsForClass(optionsClass)) {
+    for (OptionDefinition optionDefinition : data.getOptionDefinitionsFromClass(optionsClass)) {
       try {
-        map.put(field, field.get(options));
+        // Get the object value of the optionDefinition and place in map.
+        map.put(optionDefinition.getField(), optionDefinition.getField().get(options));
       } catch (IllegalAccessException e) {
         // All options fields of options classes should be public.
         throw new IllegalStateException(e);
@@ -828,6 +839,9 @@ public class OptionsParser implements OptionsProvider {
    * Given a mapping as returned by {@link #toMap}, and the options class it that its entries
    * correspond to, this constructs the corresponding instance of the options class.
    *
+   * @param map Field to Object, expecting an entry for each field in the optionsClass. This
+   *     directly refers to the Field, without wrapping it in an OptionDefinition, see {@link
+   *     #toMap}.
    * @throws IllegalArgumentException if {@code map} does not contain exactly the fields of {@code
    *     optionsClass}, with values of the appropriate type
    */
@@ -843,15 +857,15 @@ public class OptionsParser implements OptionsProvider {
       throw new IllegalStateException("Error while instantiating options class", e);
     }
 
-    List<Field> fields = data.getFieldsForClass(optionsClass);
+    List<OptionDefinition> optionDefinitions = data.getOptionDefinitionsFromClass(optionsClass);
     // Ensure all fields are covered, no extraneous fields.
-    validateFieldsSets(new LinkedHashSet<>(fields), new LinkedHashSet<>(map.keySet()));
+    validateFieldsSets(data, optionsClass, new LinkedHashSet<Field>(map.keySet()));
     // Populate the instance.
-    for (Field field : fields) {
+    for (OptionDefinition optionDefinition : optionDefinitions) {
       // Non-null as per above check.
-      Object value = map.get(field);
+      Object value = map.get(optionDefinition.getField());
       try {
-        field.set(optionsInstance, value);
+        optionDefinition.getField().set(optionsInstance, value);
       } catch (IllegalAccessException e) {
         throw new IllegalStateException(e);
       }
@@ -861,41 +875,59 @@ public class OptionsParser implements OptionsProvider {
   }
 
   /**
-   * Raises a pretty {@link IllegalArgumentException} if the two sets of fields are not equal.
+   * Raises a pretty {@link IllegalArgumentException} if the provided set of fields is a complete
+   * set for the optionsClass.
    *
    * <p>The entries in {@code fieldsFromMap} may be ill formed by being null or lacking an {@link
-   * Option} annotation. (This isn't done for {@code fieldsFromClass} because they come from an
-   * {@link OptionsData} object.)
+   * Option} annotation.
    */
   private static void validateFieldsSets(
-      LinkedHashSet<Field> fieldsFromClass, LinkedHashSet<Field> fieldsFromMap) {
-    if (!fieldsFromClass.equals(fieldsFromMap)) {
-      List<String> extraNamesFromClass = new ArrayList<>();
-      List<String> extraNamesFromMap = new ArrayList<>();
-      for (Field field : fieldsFromClass) {
-        if (!fieldsFromMap.contains(field)) {
-          extraNamesFromClass.add("'" + field.getAnnotation(Option.class).name() + "'");
-        }
+      OptionsData data,
+      Class<? extends OptionsBase> optionsClass,
+      LinkedHashSet<Field> fieldsFromMap) {
+    ImmutableList<OptionDefinition> optionFieldsFromClasses =
+        data.getOptionDefinitionsFromClass(optionsClass);
+    Set<Field> fieldsFromClass =
+        optionFieldsFromClasses
+            .stream()
+            .map(optionField -> optionField.getField())
+            .collect(Collectors.toSet());
+
+    if (fieldsFromClass.equals(fieldsFromMap)) {
+      // They are already equal, avoid additional checks.
+      return;
+    }
+
+    List<String> extraNamesFromClass = new ArrayList<>();
+    List<String> extraNamesFromMap = new ArrayList<>();
+    for (OptionDefinition optionDefinition : optionFieldsFromClasses) {
+      if (!fieldsFromMap.contains(optionDefinition.getField())) {
+        extraNamesFromClass.add("'" + optionDefinition.getOptionName() + "'");
       }
-      for (Field field : fieldsFromMap) {
-        // Extra validation on the map keys since they don't come from OptionsData.
-        if (!fieldsFromClass.contains(field)) {
-          if (field == null) {
-            extraNamesFromMap.add("<null field>");
-          } else {
-            Option annotation = field.getAnnotation(Option.class);
-            if (annotation == null) {
-              extraNamesFromMap.add("<non-Option field>");
-            } else {
-              extraNamesFromMap.add("'" + annotation.name() + "'");
-            }
+    }
+    for (Field field : fieldsFromMap) {
+      // Extra validation on the map keys since they don't come from OptionsData.
+      if (!fieldsFromClass.contains(field)) {
+        if (field == null) {
+          extraNamesFromMap.add("<null field>");
+        } else {
+
+          OptionDefinition optionDefinition = null;
+          try {
+            optionDefinition = OptionDefinition.extractOptionDefinition(field);
+            extraNamesFromMap.add("'" + optionDefinition.getOptionName() + "'");
+          } catch (ConstructionException e) {
+            extraNamesFromMap.add("<non-Option field>");
           }
         }
       }
-      throw new IllegalArgumentException(
-          "Map keys do not match fields of options class; extra map keys: {"
-          + Joiner.on(", ").join(extraNamesFromMap) + "}; extra options class options: {"
-          + Joiner.on(", ").join(extraNamesFromClass) + "}");
     }
+    throw new IllegalArgumentException(
+        "Map keys do not match fields of options class; extra map keys: {"
+            + Joiner.on(", ").join(extraNamesFromMap)
+            + "}; extra options class options: {"
+            + Joiner.on(", ").join(extraNamesFromClass)
+            + "}");
   }
 }
+