Eliminate some middleman methods

Reduce spaghetti code by exposing the parser's OptionsData as package-private, rather than exposing individual methods ad hoc between OptionsParser and OptionsParserImpl.

Also change some calls from static constructors to diamond syntax.

RELNOTES: None
PiperOrigin-RevId: 153457442

3 files changed, 49 insertions, 51 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 3b55c744c0..9574a90162 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -20,16 +20,14 @@ import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ListMultimap;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
 import com.google.common.escape.Escaper;
 import java.lang.reflect.Field;
 import java.nio.file.FileSystem;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -93,7 +91,7 @@ public class OptionsParser implements OptionsProvider {
    * options classes on the classpath.
    */
   private static final Map<ImmutableList<Class<? extends OptionsBase>>, OptionsData> optionsData =
-      Maps.newHashMap();
+      new HashMap<>();
 
   /**
    * Returns {@link OpaqueOptionsData} suitable for passing along to {@link
@@ -104,32 +102,35 @@ public class OptionsParser implements OptionsProvider {
    * construct lots of different {@link OptionsParser} instances).
    */
   public static OpaqueOptionsData getOptionsData(
-      ImmutableList<Class<? extends OptionsBase>> optionsClasses) throws ConstructionException {
+      List<Class<? extends OptionsBase>> optionsClasses) throws ConstructionException {
     return getOptionsDataInternal(optionsClasses);
   }
 
-  private static synchronized OptionsData getOptionsDataInternal(
-      ImmutableList<Class<? extends OptionsBase>> optionsClasses) throws ConstructionException {
-    OptionsData result = optionsData.get(optionsClasses);
+  /**
+   * Returns the {@link OptionsData} associated with the given list of options classes.
+   */
+  static synchronized OptionsData getOptionsDataInternal(
+      List<Class<? extends OptionsBase>> optionsClasses) throws ConstructionException {
+    ImmutableList<Class<? extends OptionsBase>> immutableOptionsClasses =
+        ImmutableList.copyOf(optionsClasses);
+    OptionsData result = optionsData.get(immutableOptionsClasses);
     if (result == null) {
       try {
-        result = OptionsData.from(optionsClasses);
+        result = OptionsData.from(immutableOptionsClasses);
       } catch (Exception e) {
         throw new ConstructionException(e.getMessage(), e);
       }
-      optionsData.put(optionsClasses, result);
+      optionsData.put(immutableOptionsClasses, result);
     }
     return result;
   }
 
   /**
-   * Returns all the annotated fields for the given class, including inherited
-   * ones.
+   * Returns the {@link OptionsData} associated with the given options class.
    */
-  static Collection<Field> getAllAnnotatedFields(Class<? extends OptionsBase> optionsClass) {
-    OptionsData data = getOptionsDataInternal(
-        ImmutableList.<Class<? extends OptionsBase>>of(optionsClass));
-    return data.getFieldsForClass(optionsClass);
+  static OptionsData getOptionsDataInternal(Class<? extends OptionsBase> optionsClass)
+      throws ConstructionException {
+    return getOptionsDataInternal(ImmutableList.<Class<? extends OptionsBase>>of(optionsClass));
   }
 
   /**
@@ -328,7 +329,7 @@ public class OptionsParser implements OptionsProvider {
         // The generic type of the list is not known at runtime, so we can't
         // use it here. It was already checked in the constructor, so this is
         // type-safe.
-        List result = Lists.newArrayList();
+        List result = new ArrayList<>();
         ListMultimap realValue = (ListMultimap) value;
         for (OptionPriority priority : OptionPriority.values()) {
           // If there is no mapping for this key, this check avoids object creation (because
@@ -536,11 +537,12 @@ public class OptionsParser implements OptionsProvider {
    */
   public String describeOptions(
       Map<String, String> categoryDescriptions, HelpVerbosity helpVerbosity) {
+    OptionsData data = impl.getOptionsData();
     StringBuilder desc = new StringBuilder();
-    if (!impl.getOptionsClasses().isEmpty()) {
-      List<Field> allFields = Lists.newArrayList();
-      for (Class<? extends OptionsBase> optionsClass : impl.getOptionsClasses()) {
-        allFields.addAll(impl.getAnnotatedFieldsFor(optionsClass));
+    if (!data.getOptionsClasses().isEmpty()) {
+      List<Field> allFields = new ArrayList<>();
+      for (Class<? extends OptionsBase> optionsClass : data.getOptionsClasses()) {
+        allFields.addAll(data.getFieldsForClass(optionsClass));
       }
       Collections.sort(allFields, OptionsUsage.BY_CATEGORY);
       String prevCategory = null;
@@ -579,11 +581,12 @@ public class OptionsParser implements OptionsProvider {
    *   of the category.
    */
   public String describeOptionsHtml(Map<String, String> categoryDescriptions, Escaper escaper) {
+    OptionsData data = impl.getOptionsData();
     StringBuilder desc = new StringBuilder();
-    if (!impl.getOptionsClasses().isEmpty()) {
-      List<Field> allFields = Lists.newArrayList();
-      for (Class<? extends OptionsBase> optionsClass : impl.getOptionsClasses()) {
-        allFields.addAll(impl.getAnnotatedFieldsFor(optionsClass));
+    if (!data.getOptionsClasses().isEmpty()) {
+      List<Field> allFields = new ArrayList<>();
+      for (Class<? extends OptionsBase> optionsClass : data.getOptionsClasses()) {
+        allFields.addAll(data.getFieldsForClass(optionsClass));
       }
       Collections.sort(allFields, OptionsUsage.BY_CATEGORY);
       String prevCategory = null;
@@ -620,12 +623,13 @@ public class OptionsParser implements OptionsProvider {
    * details on the format for the flag completion.
    */
   public String getOptionsCompletion() {
+    OptionsData data = impl.getOptionsData();
     StringBuilder desc = new StringBuilder();
 
     // List all options
-    List<Field> allFields = Lists.newArrayList();
-    for (Class<? extends OptionsBase> optionsClass : impl.getOptionsClasses()) {
-      allFields.addAll(impl.getAnnotatedFieldsFor(optionsClass));
+    List<Field> allFields = new ArrayList<>();
+    for (Class<? extends OptionsBase> optionsClass : data.getOptionsClasses()) {
+      allFields.addAll(data.getFieldsForClass(optionsClass));
     }
     // Sort field for deterministic ordering
     Collections.sort(allFields, new Comparator<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 918fef1b8d..5c635fb524 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -25,7 +25,6 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Iterators;
 import com.google.common.collect.LinkedHashMultimap;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
 import com.google.devtools.common.options.OptionsParser.OptionDescription;
 import com.google.devtools.common.options.OptionsParser.OptionUsageRestrictions;
@@ -33,10 +32,11 @@ import com.google.devtools.common.options.OptionsParser.OptionValueDescription;
 import com.google.devtools.common.options.OptionsParser.UnparsedOptionValueDescription;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
+import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -61,14 +61,14 @@ class OptionsParserImpl {
    *
    * This map is modified by repeated calls to {@link #parse(OptionPriority,Function,List)}.
    */
-  private final Map<Field, OptionValueDescription> parsedValues = Maps.newHashMap();
+  private final Map<Field, OptionValueDescription> parsedValues = new HashMap<>();
 
   /**
    * We store the pre-parsed, explicit options for each priority in here.
    * We use partially preparsed options, which can be different from the original
    * representation, e.g. "--nofoo" becomes "--foo=0".
    */
-  private final List<UnparsedOptionValueDescription> unparsedValues = Lists.newArrayList();
+  private final List<UnparsedOptionValueDescription> unparsedValues = new ArrayList<>();
 
   /**
    * Unparsed values for use with the canonicalize command are stored separately from
@@ -82,7 +82,7 @@ class OptionsParserImpl {
   private final Multimap<Field, UnparsedOptionValueDescription> canonicalizeValues
       = LinkedHashMultimap.create();
 
-  private final List<String> warnings = Lists.newArrayList();
+  private final List<String> warnings = new ArrayList<>();
 
   private boolean allowSingleDashLongOptions = false;
   
@@ -122,8 +122,10 @@ class OptionsParserImpl {
    * The implementation of {@link OptionsBase#asMap}.
    */
   static Map<String, Object> optionsAsMap(OptionsBase optionsInstance) {
-    Map<String, Object> map = Maps.newHashMap();
-    for (Field field : OptionsParser.getAllAnnotatedFields(optionsInstance.getClass())) {
+    Class<? extends OptionsBase> optionsClass = optionsInstance.getClass();
+    OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
+    Map<String, Object> map = new HashMap<>();
+    for (Field field : data.getFieldsForClass(optionsClass)) {
       try {
         String name = field.getAnnotation(Option.class).name();
         Object value = field.get(optionsInstance);
@@ -135,10 +137,6 @@ class OptionsParserImpl {
     return map;
   }
 
-  List<Field> getAnnotatedFieldsFor(Class<? extends OptionsBase> clazz) {
-    return optionsData.getFieldsForClass(clazz);
-  }
-
   /**
    * Implements {@link OptionsParser#asListOfUnparsedOptions()}.
    */
@@ -200,7 +198,7 @@ class OptionsParserImpl {
       }
     });
 
-    List<String> result = Lists.newArrayList();
+    List<String> result = new ArrayList<>();
     for (UnparsedOptionValueDescription value : processed) {
 
       // Ignore expansion options.
@@ -217,7 +215,7 @@ class OptionsParserImpl {
    * Implements {@link OptionsParser#asListOfEffectiveOptions()}.
    */
   List<OptionValueDescription> asListOfEffectiveOptions() {
-    List<OptionValueDescription> result = Lists.newArrayList();
+    List<OptionValueDescription> result = new ArrayList<>();
     for (Map.Entry<String, Field> mapEntry : optionsData.getAllNamedFields()) {
       String fieldName = mapEntry.getKey();
       Field field = mapEntry.getValue();
@@ -241,10 +239,6 @@ class OptionsParserImpl {
     return result;
   }
 
-  Collection<Class<?  extends OptionsBase>> getOptionsClasses() {
-    return optionsData.getOptionsClasses();
-  }
-
   private void maybeAddDeprecationWarning(Field field) {
     Option option = field.getAnnotation(Option.class);
     // Continue to support the old behavior for @Deprecated options.
@@ -449,8 +443,8 @@ class OptionsParserImpl {
       String expandedFrom,
       List<String> args) throws OptionsParsingException {
 
-    List<String> unparsedArgs = Lists.newArrayList();
-    LinkedHashMap<String, List<String>> implicitRequirements = Maps.newLinkedHashMap();
+    List<String> unparsedArgs = new ArrayList<>();
+    LinkedHashMap<String, List<String>> implicitRequirements = new LinkedHashMap<>();
 
     Iterator<String> argsIterator = argsPreProcessor.preProcess(args).iterator();
     while (argsIterator.hasNext()) {
@@ -712,7 +706,7 @@ class OptionsParserImpl {
       if (constructor == null) {
         return null;
       }
-      optionsInstance = constructor.newInstance(new Object[0]);
+      optionsInstance = constructor.newInstance();
     } catch (Exception e) {
       throw new IllegalStateException(e);
     }
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 b898ae6c28..1c9f5193f2 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsUsage.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsUsage.java
@@ -16,10 +16,10 @@ package com.google.devtools.common.options;
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
-import com.google.common.collect.Lists;
 import com.google.common.escape.Escaper;
 import java.lang.reflect.Field;
 import java.text.BreakIterator;
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
@@ -40,8 +40,8 @@ class OptionsUsage {
    * OptionsBase} subclasses they depend on until a complete parser is constructed).
    */
   static void getUsage(Class<? extends OptionsBase> optionsClass, StringBuilder usage) {
-    List<Field> optionFields =
-        Lists.newArrayList(OptionsParser.getAllAnnotatedFields(optionsClass));
+    OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
+    List<Field> optionFields = new ArrayList<>(data.getFieldsForClass(optionsClass));
     Collections.sort(optionFields, BY_NAME);
     for (Field optionField : optionFields) {
       getUsage(optionField, usage, OptionsParser.HelpVerbosity.LONG, null);