Refactor options converter logic

Moved default converters from parser implementation to Converters. Moved other helpers to OptionsData. Also factored out new function getFieldSingularType.

--
PiperOrigin-RevId: 150473455
MOS_MIGRATED_REVID=150473455

GitOrigin-RevId: 097e64c412c6a4162a22880fd435ef4632878406
Change-Id: Ife5702b6f39415a7df3fd8b44c1867145a6ac466

4 files changed, 244 insertions, 195 deletions

diff --git a/java/com/google/devtools/common/options/Converters.java b/java/com/google/devtools/common/options/Converters.java
index c8b4d47..0d19029 100644
--- a/java/com/google/devtools/common/options/Converters.java
+++ b/java/com/google/devtools/common/options/Converters.java
@@ -16,7 +16,6 @@ package com.google.devtools.common.options;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Maps;
-
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -30,6 +29,171 @@ import java.util.regex.PatternSyntaxException;
  */
 public final class Converters {
 
+  /** Standard converter for booleans. Accepts common shorthands/synonyms. */
+  public static class BooleanConverter implements Converter<Boolean> {
+    @Override
+    public Boolean convert(String input) throws OptionsParsingException {
+      if (input == null) {
+        return false;
+      }
+      input = input.toLowerCase();
+      if (input.equals("true")
+          || input.equals("1")
+          || input.equals("yes")
+          || input.equals("t")
+          || input.equals("y")) {
+        return true;
+      }
+      if (input.equals("false")
+          || input.equals("0")
+          || input.equals("no")
+          || input.equals("f")
+          || input.equals("n")) {
+        return false;
+      }
+      throw new OptionsParsingException("'" + input + "' is not a boolean");
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "a boolean";
+    }
+  }
+
+  /** Standard converter for Strings. */
+  public static class StringConverter implements Converter<String> {
+    @Override
+    public String convert(String input) {
+      return input;
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "a string";
+    }
+  }
+
+  /** Standard converter for integers. */
+  public static class IntegerConverter implements Converter<Integer> {
+    @Override
+    public Integer convert(String input) throws OptionsParsingException {
+      try {
+        return Integer.decode(input);
+      } catch (NumberFormatException e) {
+        throw new OptionsParsingException("'" + input + "' is not an int");
+      }
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "an integer";
+    }
+  }
+
+  /** Standard converter for longs. */
+  public static class LongConverter implements Converter<Long> {
+    @Override
+    public Long convert(String input) throws OptionsParsingException {
+      try {
+        return Long.decode(input);
+      } catch (NumberFormatException e) {
+        throw new OptionsParsingException("'" + input + "' is not a long");
+      }
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "a long integer";
+    }
+  }
+
+  /** Standard converter for doubles. */
+  public static class DoubleConverter implements Converter<Double> {
+    @Override
+    public Double convert(String input) throws OptionsParsingException {
+      try {
+        return Double.parseDouble(input);
+      } catch (NumberFormatException e) {
+        throw new OptionsParsingException("'" + input + "' is not a double");
+      }
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "a double";
+    }
+  }
+
+  /** Standard converter for TriState values. */
+  public static class TriStateConverter implements Converter<TriState> {
+    @Override
+    public TriState convert(String input) throws OptionsParsingException {
+      if (input == null) {
+        return TriState.AUTO;
+      }
+      input = input.toLowerCase();
+      if (input.equals("auto")) {
+        return TriState.AUTO;
+      }
+      if (input.equals("true")
+          || input.equals("1")
+          || input.equals("yes")
+          || input.equals("t")
+          || input.equals("y")) {
+        return TriState.YES;
+      }
+      if (input.equals("false")
+          || input.equals("0")
+          || input.equals("no")
+          || input.equals("f")
+          || input.equals("n")) {
+        return TriState.NO;
+      }
+      throw new OptionsParsingException("'" + input + "' is not a boolean");
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "a tri-state (auto, yes, no)";
+    }
+  }
+
+  /**
+   * Standard "converter" for Void. Should not actually be invoked. For instance, expansion flags
+   * are usually Void-typed and do not invoke the converter.
+   */
+  public static class VoidConverter implements Converter<Void> {
+    @Override
+    public Void convert(String input) throws OptionsParsingException {
+      if (input == null) {
+        return null; // expected input, return is unused so null is fine.
+      }
+      throw new OptionsParsingException("'" + input + "' unexpected");
+    }
+
+    @Override
+    public String getTypeDescription() {
+      return "";
+    }
+  }
+
+  /**
+   * The converters that are available to the options parser by default. These are used if the
+   * {@code @Option} annotation does not specify its own {@code converter}, and its type is one of
+   * the following.
+   */
+  static final Map<Class<?>, Converter<?>> DEFAULT_CONVERTERS = Maps.newHashMap();
+
+  static {
+    DEFAULT_CONVERTERS.put(String.class, new Converters.StringConverter());
+    DEFAULT_CONVERTERS.put(int.class, new Converters.IntegerConverter());
+    DEFAULT_CONVERTERS.put(long.class, new Converters.LongConverter());
+    DEFAULT_CONVERTERS.put(double.class, new Converters.DoubleConverter());
+    DEFAULT_CONVERTERS.put(boolean.class, new Converters.BooleanConverter());
+    DEFAULT_CONVERTERS.put(TriState.class, new Converters.TriStateConverter());
+    DEFAULT_CONVERTERS.put(Void.class, new Converters.VoidConverter());
+  }
+
   /**
    * Join a list of words as in English.  Examples:
    * "nothing"
@@ -92,7 +256,7 @@ public final class Converters {
 
   public static class LogLevelConverter implements Converter<Level> {
 
-    public static Level[] LEVELS = new Level[] {
+    public static final Level[] LEVELS = new Level[] {
       Level.OFF, Level.SEVERE, Level.WARNING, Level.INFO, Level.FINE,
       Level.FINER, Level.FINEST
     };
@@ -295,32 +459,4 @@ public final class Converters {
     }
   }
 
-  /**
-   * A converter for boolean values. This is already one of the defaults, so clients
-   * should not typically need to add this.
-   */
-  public static class BooleanConverter implements Converter<Boolean> {
-    @Override
-    public Boolean convert(String input) throws OptionsParsingException {
-      if (input == null) {
-        return false;
-      }
-      input = input.toLowerCase();
-      if (input.equals("true") || input.equals("1") || input.equals("yes") ||
-          input.equals("t") || input.equals("y")) {
-        return true;
-      }
-      if (input.equals("false") || input.equals("0") || input.equals("no") ||
-          input.equals("f") || input.equals("n")) {
-        return false;
-      }
-      throw new OptionsParsingException("'" + input + "' is not a boolean");
-    }
-
-    @Override
-    public String getTypeDescription() {
-      return "a boolean";
-    }
-  }
-
 }
diff --git a/java/com/google/devtools/common/options/OptionsData.java b/java/com/google/devtools/common/options/OptionsData.java
index ac23d63..ae315a4 100644
--- a/java/com/google/devtools/common/options/OptionsData.java
+++ b/java/com/google/devtools/common/options/OptionsData.java
@@ -28,7 +28,6 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-
 import javax.annotation.concurrent.Immutable;
 
 /**
@@ -66,7 +65,7 @@ final class OptionsData extends OpaqueOptionsData {
   /**
    * Mapping from each Option-annotated field to the proper converter.
    *
-   * @see OptionsParserImpl#findConverter
+   * @see #findConverter
    */
   private final Map<Field, Converter<?>> converters;
 
@@ -130,6 +129,73 @@ final class OptionsData extends OpaqueOptionsData {
     return allowMultiple.get(field);
   }
 
+  /**
+   * 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(Field field, Option annotation) {
+    Type fieldType = field.getGenericType();
+    if (annotation.allowMultiple()) {
+      // If the type isn't a List<T>, this is an error in the option's declaration.
+      if (!(fieldType instanceof ParameterizedType)) {
+        throw new AssertionError("Type of multiple occurrence option must be a List<...>");
+      }
+      ParameterizedType pfieldType = (ParameterizedType) fieldType;
+      if (pfieldType.getRawType() != List.class) {
+        throw new AssertionError("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.
+   */
+  static boolean isBooleanField(Field field) {
+    return field.getType().equals(boolean.class)
+        || field.getType().equals(TriState.class)
+        || findConverter(field) instanceof BoolOrEnumConverter;
+  }
+
+  /** Returns whether a field has Void type. */
+  static boolean isVoidField(Field field) {
+    return field.getType().equals(Void.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.
+   */
+  static Converter<?> findConverter(Field optionField) {
+    Option annotation = optionField.getAnnotation(Option.class);
+    if (annotation.converter() == Converter.class) {
+      // No converter provided, use the default one.
+      Type type = getFieldSingularType(optionField, annotation);
+      Converter<?> converter = Converters.DEFAULT_CONVERTERS.get(type);
+      if (converter == null) {
+        throw new AssertionError(
+            "No converter found for "
+                + type
+                + "; possible fix: add "
+                + "converter=... to @Option annotation for "
+                + optionField.getName());
+      }
+      return converter;
+    }
+    try {
+      // Instantiate the given Converter class.
+      Class<?> converter = annotation.converter();
+      Constructor<?> constructor = converter.getConstructor(new Class<?>[0]);
+      return (Converter<?>) constructor.newInstance(new Object[0]);
+    } catch (Exception e) {
+      throw new AssertionError(e);
+    }
+  }
+
   private static List<Field> getAllAnnotatedFields(Class<? extends OptionsBase> optionsClass) {
     List<Field> allFields = Lists.newArrayList();
     for (Field field : optionsClass.getFields()) {
@@ -144,7 +210,7 @@ final class OptionsData extends OpaqueOptionsData {
   }
 
   private static Object retrieveDefaultFromAnnotation(Field optionField) {
-    Converter<?> converter = OptionsParserImpl.findConverter(optionField);
+    Converter<?> converter = findConverter(optionField);
     String defaultValueAsString = OptionsParserImpl.getDefaultOptionString(optionField);
     // Special case for "null"
     if (OptionsParserImpl.isSpecialNullDefault(defaultValueAsString, optionField)) {
@@ -194,27 +260,13 @@ final class OptionsData extends OpaqueOptionsData {
       for (Field field : fields) {
         Option annotation = field.getAnnotation(Option.class);
 
-        // Check that the field type is a List, and that the converter
-        // type matches the element type of the list.
-        Type fieldType = field.getGenericType();
-        if (annotation.allowMultiple()) {
-          if (!(fieldType instanceof ParameterizedType)) {
-            throw new AssertionError("Type of multiple occurrence option must be a List<...>");
-          }
-          ParameterizedType pfieldType = (ParameterizedType) fieldType;
-          if (pfieldType.getRawType() != List.class) {
-            // Throw an assertion, because this indicates an undetected type
-            // error in the code.
-            throw new AssertionError("Type of multiple occurrence option must be a List<...>");
-          }
-          fieldType = pfieldType.getActualTypeArguments()[0];
-        }
+        Type fieldType = getFieldSingularType(field, annotation);
 
         // Get the converter return type.
         @SuppressWarnings("rawtypes")
         Class<? extends Converter> converter = annotation.converter();
         if (converter == Converter.class) {
-          Converter<?> actualConverter = OptionsParserImpl.DEFAULT_CONVERTERS.get(fieldType);
+          Converter<?> actualConverter = Converters.DEFAULT_CONVERTERS.get(fieldType);
           if (actualConverter == null) {
             throw new AssertionError("Cannot find converter for field of type "
                 + field.getType() + " named " + field.getName()
@@ -282,7 +334,7 @@ final class OptionsData extends OpaqueOptionsData {
 
         optionDefaultsBuilder.put(field, retrieveDefaultFromAnnotation(field));
 
-        convertersBuilder.put(field, OptionsParserImpl.findConverter(field));
+        convertersBuilder.put(field, findConverter(field));
 
         allowMultipleBuilder.put(field, annotation.allowMultiple());
       }
diff --git a/java/com/google/devtools/common/options/OptionsParserImpl.java b/java/com/google/devtools/common/options/OptionsParserImpl.java
index c15f927..93c2cdd 100644
--- a/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -32,8 +32,6 @@ 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.lang.reflect.ParameterizedType;
-import java.lang.reflect.Type;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -50,100 +48,6 @@ import java.util.Map;
  */
 class OptionsParserImpl {
 
-  /**
-   * A bunch of default converters in case the user doesn't specify a
-   * different one in the field annotation.
-   */
-  static final Map<Class<?>, Converter<?>> DEFAULT_CONVERTERS = Maps.newHashMap();
-
-  static {
-    DEFAULT_CONVERTERS.put(String.class, new Converter<String>() {
-      @Override
-      public String convert(String input) {
-        return input;
-      }
-      @Override
-      public String getTypeDescription() {
-        return "a string";
-      }});
-    DEFAULT_CONVERTERS.put(int.class, new Converter<Integer>() {
-      @Override
-      public Integer convert(String input) throws OptionsParsingException {
-        try {
-          return Integer.decode(input);
-        } catch (NumberFormatException e) {
-          throw new OptionsParsingException("'" + input + "' is not an int");
-        }
-      }
-      @Override
-      public String getTypeDescription() {
-        return "an integer";
-      }});
-    DEFAULT_CONVERTERS.put(double.class, new Converter<Double>() {
-      @Override
-      public Double convert(String input) throws OptionsParsingException {
-        try {
-          return Double.parseDouble(input);
-        } catch (NumberFormatException e) {
-          throw new OptionsParsingException("'" + input + "' is not a double");
-        }
-      }
-      @Override
-      public String getTypeDescription() {
-        return "a double";
-      }});
-    DEFAULT_CONVERTERS.put(boolean.class, new Converters.BooleanConverter());
-    DEFAULT_CONVERTERS.put(TriState.class, new Converter<TriState>() {
-      @Override
-      public TriState convert(String input) throws OptionsParsingException {
-        if (input == null) {
-          return TriState.AUTO;
-        }
-        input = input.toLowerCase();
-        if (input.equals("auto")) {
-          return TriState.AUTO;
-        }
-        if (input.equals("true") || input.equals("1") || input.equals("yes") ||
-            input.equals("t") || input.equals("y")) {
-          return TriState.YES;
-        }
-        if (input.equals("false") || input.equals("0") || input.equals("no") ||
-            input.equals("f") || input.equals("n")) {
-          return TriState.NO;
-        }
-        throw new OptionsParsingException("'" + input + "' is not a boolean");
-      }
-      @Override
-      public String getTypeDescription() {
-        return "a tri-state (auto, yes, no)";
-      }});
-    DEFAULT_CONVERTERS.put(Void.class, new Converter<Void>() {
-      @Override
-      public Void convert(String input) throws OptionsParsingException {
-        if (input == null) {
-          return null;  // expected input, return is unused so null is fine.
-        }
-        throw new OptionsParsingException("'" + input + "' unexpected");
-      }
-      @Override
-      public String getTypeDescription() {
-        return "";
-      }});
-    DEFAULT_CONVERTERS.put(long.class, new Converter<Long>() {
-      @Override
-      public Long convert(String input) throws OptionsParsingException {
-        try {
-          return Long.decode(input);
-        } catch (NumberFormatException e) {
-          throw new OptionsParsingException("'" + input + "' is not a long");
-        }
-      }
-      @Override
-      public String getTypeDescription() {
-        return "a long integer";
-      }});
-  }
-
   private final OptionsData optionsData;
 
   /**
@@ -701,7 +605,7 @@ class OptionsParserImpl {
         booleanValue = false;
         if (field != null) {
           // TODO(bazel-team): Add tests for these cases.
-          if (!OptionsParserImpl.isBooleanField(field)) {
+          if (!OptionsData.isBooleanField(field)) {
             throw new OptionsParsingException(
                 "Illegal use of 'no' prefix on non-boolean option: " + arg, arg);
           }
@@ -725,7 +629,7 @@ class OptionsParserImpl {
 
     if (value == null) {
       // Special-case boolean to supply value based on presence of "no" prefix.
-      if (OptionsParserImpl.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).
@@ -782,46 +686,7 @@ class OptionsParserImpl {
     return annotation.defaultValue();
   }
 
-  static boolean isBooleanField(Field field) {
-    return field.getType().equals(boolean.class)
-        || field.getType().equals(TriState.class)
-        || findConverter(field) instanceof BoolOrEnumConverter;
-  }
-
-  static boolean isVoidField(Field field) {
-    return field.getType().equals(Void.class);
-  }
-
   static boolean isSpecialNullDefault(String defaultValueString, Field optionField) {
     return defaultValueString.equals("null") && !optionField.getType().isPrimitive();
   }
-
-  static Converter<?> findConverter(Field optionField) {
-    Option annotation = optionField.getAnnotation(Option.class);
-    if (annotation.converter() == Converter.class) {
-      Type type;
-      if (annotation.allowMultiple()) {
-        // The OptionParserImpl already checked that the type is List<T> for some T;
-        // here we extract the type T.
-        type = ((ParameterizedType) optionField.getGenericType()).getActualTypeArguments()[0];
-      } else {
-        type = optionField.getType();
-      }
-      Converter<?> converter = DEFAULT_CONVERTERS.get(type);
-      if (converter == null) {
-        throw new AssertionError("No converter found for "
-            + type + "; possible fix: add "
-            + "converter=... to @Option annotation for "
-            + optionField.getName());
-      }
-      return converter;
-    }
-    try {
-      Class<?> converter = annotation.converter();
-      Constructor<?> constructor = converter.getConstructor(new Class<?>[0]);
-      return (Converter<?>) constructor.newInstance(new Object[0]);
-    } catch (Exception e) {
-      throw new AssertionError(e);
-    }
-  }
 }
diff --git a/java/com/google/devtools/common/options/OptionsUsage.java b/java/com/google/devtools/common/options/OptionsUsage.java
index b8c19df..f3ee4d3 100644
--- a/java/com/google/devtools/common/options/OptionsUsage.java
+++ b/java/com/google/devtools/common/options/OptionsUsage.java
@@ -13,14 +13,11 @@
 // limitations under the License.
 package com.google.devtools.common.options;
 
-import static com.google.devtools.common.options.OptionsParserImpl.findConverter;
-
 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.Collections;
@@ -138,8 +135,7 @@ class OptionsUsage {
     Option annotation = optionField.getAnnotation(Option.class);
     usage.append("<dt><code><a name=\"flag--").append(plainFlagName).append("\"></a>--");
     usage.append(flagName);
-    if (OptionsParserImpl.isBooleanField(optionField)
-        || OptionsParserImpl.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));
@@ -157,7 +153,7 @@ class OptionsUsage {
     } else {
       // Don't call the annotation directly (we must allow overrides to certain defaults).
       String defaultValueString = OptionsParserImpl.getDefaultOptionString(optionField);
-      if (OptionsParserImpl.isVoidField(optionField)) {
+      if (OptionsData.isVoidField(optionField)) {
         // Void options don't have a default.
       } else if (OptionsParserImpl.isSpecialNullDefault(defaultValueString, optionField)) {
         usage.append(" default: see description");
@@ -259,12 +255,12 @@ class OptionsUsage {
   };
 
   private static String getTypeDescription(Field optionsField) {
-    return findConverter(optionsField).getTypeDescription();
+    return OptionsData.findConverter(optionsField).getTypeDescription();
   }
 
   static String getFlagName(Field field) {
     String name = field.getAnnotation(Option.class).name();
-    return OptionsParserImpl.isBooleanField(field) ? "[no]" + name : name;
+    return OptionsData.isBooleanField(field) ? "[no]" + name : name;
   }
 
 }