Move static converter legality checks to compile time.

The information about whether a converter correctly matches the type of option it is meant to convert strings to is available at compile time. There is no reason to do this check at runtime.

Now, for an option to compile, it will need to have a converter that matches the option's type, taking into account whether the option is expected to accumulate multiple values. If it does not specify its own converter, a matching converter in the Converters.DEFAULT_CONVERTER list must be found, and the default value provided must be parseable by the matching default converter.

Remove tests that were testing failure modes which no longer compile.

RELNOTES: None.
PiperOrigin-RevId: 167092773

11 files changed, 408 insertions, 292 deletions

diff --git a/src/main/java/com/google/devtools/common/options/Converters.java b/src/main/java/com/google/devtools/common/options/Converters.java
index cf26e7177b..35f2da4f93 100644
--- a/src/main/java/com/google/devtools/common/options/Converters.java
+++ b/src/main/java/com/google/devtools/common/options/Converters.java
@@ -15,6 +15,7 @@ package com.google.devtools.common.options;
 
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import java.time.Duration;
 import java.util.Iterator;
@@ -167,7 +168,7 @@ public final class Converters {
   public static class VoidConverter implements Converter<Void> {
     @Override
     public Void convert(String input) throws OptionsParsingException {
-      if (input == null) {
+      if (input == null || input.equals("null")) {
         return null; // expected input, return is unused so null is fine.
       }
       throw new OptionsParsingException("'" + input + "' unexpected");
@@ -220,24 +221,23 @@ public final class Converters {
     }
   }
 
+  // 1:1 correspondence with UsesOnlyCoreTypes.CORE_TYPES.
   /**
    * 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 {
-    // 1:1 correspondence with UsesOnlyCoreTypes.CORE_TYPES.
-    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(Duration.class, new Converters.DurationConverter());
-    DEFAULT_CONVERTERS.put(Void.class, new Converters.VoidConverter());
-  }
+  public static final ImmutableMap<Class<?>, Converter<?>> DEFAULT_CONVERTERS =
+      new ImmutableMap.Builder<Class<?>, Converter<?>>()
+          .put(String.class, new Converters.StringConverter())
+          .put(int.class, new Converters.IntegerConverter())
+          .put(long.class, new Converters.LongConverter())
+          .put(double.class, new Converters.DoubleConverter())
+          .put(boolean.class, new Converters.BooleanConverter())
+          .put(TriState.class, new Converters.TriStateConverter())
+          .put(Duration.class, new Converters.DurationConverter())
+          .put(Void.class, new Converters.VoidConverter())
+          .build();
 
   /**
    * Join a list of words as in English.  Examples:
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 ca91b1d83d..754ca2caa0 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -20,15 +20,10 @@ import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
 import com.google.devtools.common.options.OptionsParser.ConstructionException;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
-import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
-import java.lang.reflect.ParameterizedType;
-import java.lang.reflect.Type;
 import java.util.Arrays;
 import java.util.Collection;
 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;
@@ -250,80 +245,6 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
                   + "\" is disallowed.");
         }
 
-        Type fieldType = optionDefinition.getFieldSingularType();
-        // For simple, static expansions, don't accept non-Void types.
-        if (optionDefinition.getOptionExpansion().length != 0 && !optionDefinition.isVoidField()) {
-          throw new ConstructionException(
-              "Option "
-                  + optionDefinition.getOptionName()
-                  + " is an expansion flag with a static expansion, but does not have Void type.");
-        }
-
-        // Get the converter's return type to check that it matches the option type.
-        @SuppressWarnings("rawtypes")
-        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 "
-                    + optionDefinition.getType()
-                    + " named "
-                    + optionDefinition.getField().getName()
-                    + " in class "
-                    + optionDefinition.getField().getDeclaringClass().getName());
-          }
-          converterClass = actualConverter.getClass();
-        }
-        if (Modifier.isAbstract(converterClass.getModifiers())) {
-          throw new ConstructionException(
-              "The converter type " + converterClass + " must be a concrete type");
-        }
-        Type converterResultType;
-        try {
-          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");
-        }
-
-        if (optionDefinition.allowsMultiple()) {
-          if (GenericTypeHelper.getRawType(converterResultType) == List.class) {
-            Type elementType =
-                ((ParameterizedType) converterResultType).getActualTypeArguments()[0];
-            if (!GenericTypeHelper.isAssignableFrom(fieldType, elementType)) {
-              throw new ConstructionException(
-                  "If the converter return type of a multiple occurrence option is a list, then "
-                      + "the type of list elements ("
-                      + fieldType
-                      + ") must be assignable from the converter list element type ("
-                      + elementType
-                      + ")");
-            }
-          } else {
-            if (!GenericTypeHelper.isAssignableFrom(fieldType, converterResultType)) {
-              throw new ConstructionException(
-                  "Type of list elements ("
-                      + fieldType
-                      + ") for multiple occurrence option must be assignable from the converter "
-                      + "return type ("
-                      + converterResultType
-                      + ")");
-            }
-          }
-        } else {
-          if (!GenericTypeHelper.isAssignableFrom(fieldType, converterResultType)) {
-            throw new ConstructionException(
-                "Type of field ("
-                    + fieldType
-                    + ") must be assignable from the converter return type ("
-                    + converterResultType
-                    + ")");
-          }
-        }
-
         if (optionDefinition.isBooleanField()) {
           checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, optionName);
         }
diff --git a/src/main/java/com/google/devtools/common/options/OptionDefinition.java b/src/main/java/com/google/devtools/common/options/OptionDefinition.java
index d3f0d34648..1b55a8cef2 100644
--- a/src/main/java/com/google/devtools/common/options/OptionDefinition.java
+++ b/src/main/java/com/google/devtools/common/options/OptionDefinition.java
@@ -21,7 +21,6 @@ import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
 import java.util.Collections;
 import java.util.Comparator;
-import java.util.List;
 
 /**
  * Everything the {@link OptionsParser} needs to know about how an option is defined.
@@ -36,7 +35,9 @@ public class OptionDefinition {
   public static class NotAnOptionException extends ConstructionException {
     public NotAnOptionException(Field field) {
       super(
-          "The field " + field + " does not have the right annotation to be considered an option.");
+          "The field "
+              + field.getName()
+              + " does not have the right annotation to be considered an option.");
     }
   }
 
@@ -192,20 +193,9 @@ public class OptionDefinition {
   Type getFieldSingularType() {
     Type fieldType = getField().getGenericType();
     if (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(
-            String.format(
-                "Option %s allows multiple occurrences, so must be of type List<...>",
-                getField().getName()));
-      }
+      // The validity of the converter is checked at compile time. We know the type to be
+      // List<singularType>.
       ParameterizedType pfieldType = (ParameterizedType) fieldType;
-      if (pfieldType.getRawType() != List.class) {
-        throw new ConstructionException(
-            String.format(
-                "Option %s allows multiple occurrences, so must be of type List<...>",
-                getField().getName()));
-      }
       fieldType = pfieldType.getActualTypeArguments()[0];
     }
     return fieldType;
@@ -226,13 +216,6 @@ public class OptionDefinition {
       // No converter provided, use the default one.
       Type type = getFieldSingularType();
       converter = Converters.DEFAULT_CONVERTERS.get(type);
-      if (converter == null) {
-        throw new ConstructionException(
-            String.format(
-                "Option %s expects values of type %s, but no converter was found; possible fix: "
-                    + "add converter=... to its @Option annotation.",
-                getField().getName(), type));
-      }
     } else {
       try {
         // Instantiate the given Converter class.
diff --git a/src/main/java/com/google/devtools/common/options/processor/BUILD b/src/main/java/com/google/devtools/common/options/processor/BUILD
index 4d495e606b..28f480fa57 100644
--- a/src/main/java/com/google/devtools/common/options/processor/BUILD
+++ b/src/main/java/com/google/devtools/common/options/processor/BUILD
@@ -15,9 +15,7 @@ filegroup(
 
 java_plugin(
     name = "options_preprocessor",
-    srcs = [
-        "OptionProcessor.java",
-    ],
+    srcs = glob(["*.java"]),
     processor_class = "com.google.devtools.common.options.processor.OptionProcessor",
     deps = [
         "//src/main/java/com/google/devtools/common/options:options_internal",
diff --git a/src/main/java/com/google/devtools/common/options/processor/OptionProcessor.java b/src/main/java/com/google/devtools/common/options/processor/OptionProcessor.java
index 5afcc39f48..4e54be59b3 100644
--- a/src/main/java/com/google/devtools/common/options/processor/OptionProcessor.java
+++ b/src/main/java/com/google/devtools/common/options/processor/OptionProcessor.java
@@ -14,13 +14,21 @@
 package com.google.devtools.common.options.processor;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableMap.Builder;
+import com.google.devtools.common.options.Converter;
+import com.google.devtools.common.options.Converters;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
 import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParser;
+import com.google.devtools.common.options.OptionsParsingException;
+import java.util.List;
+import java.util.Map.Entry;
 import java.util.Set;
+import java.util.stream.Collectors;
 import javax.annotation.processing.AbstractProcessor;
 import javax.annotation.processing.Messager;
 import javax.annotation.processing.ProcessingEnvironment;
@@ -28,11 +36,17 @@ import javax.annotation.processing.RoundEnvironment;
 import javax.annotation.processing.SupportedAnnotationTypes;
 import javax.annotation.processing.SupportedSourceVersion;
 import javax.lang.model.SourceVersion;
+import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ElementKind;
+import javax.lang.model.element.ExecutableElement;
 import javax.lang.model.element.Modifier;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.element.VariableElement;
+import javax.lang.model.type.DeclaredType;
+import javax.lang.model.type.ExecutableType;
+import javax.lang.model.type.PrimitiveType;
+import javax.lang.model.type.TypeKind;
 import javax.lang.model.type.TypeMirror;
 import javax.lang.model.util.Elements;
 import javax.lang.model.util.Types;
@@ -52,6 +66,8 @@ public final class OptionProcessor extends AbstractProcessor {
   private Types typeUtils;
   private Elements elementUtils;
   private Messager messager;
+  private ImmutableMap<TypeMirror, Converter<?>> defaultConverters;
+  private ImmutableMap<Class<?>, PrimitiveType> primitiveTypeMap;
 
   @Override
   public synchronized void init(ProcessingEnvironment processingEnv) {
@@ -59,15 +75,44 @@ public final class OptionProcessor extends AbstractProcessor {
     typeUtils = processingEnv.getTypeUtils();
     elementUtils = processingEnv.getElementUtils();
     messager = processingEnv.getMessager();
-  }
 
-  private static class OptionProcessorException extends Exception {
-    private Element elementInError;
+    // Because of the discrepancies between the java.lang and javax.lang type models, we can't
+    // directly use the get() method for the default converter map. Instead, we'll convert it once,
+    // to be more usable, and with the boxed type return values of convert() as the keys.
+    ImmutableMap.Builder<TypeMirror, Converter<?>> converterMapBuilder = new Builder<>();
+
+    // Create a link from the primitive Classes to their primitive types. This intentionally
+    // only contains the types in the DEFAULT_CONVERTERS map.
+    ImmutableMap.Builder<Class<?>, PrimitiveType> builder = new Builder<>();
+    builder.put(int.class, typeUtils.getPrimitiveType(TypeKind.INT));
+    builder.put(double.class, typeUtils.getPrimitiveType(TypeKind.DOUBLE));
+    builder.put(boolean.class, typeUtils.getPrimitiveType(TypeKind.BOOLEAN));
+    builder.put(long.class, typeUtils.getPrimitiveType(TypeKind.LONG));
+    primitiveTypeMap = builder.build();
 
-    OptionProcessorException(Element element, String message, Object... args) {
-      super(String.format(message, args));
-      elementInError = element;
+    for (Entry<Class<?>, Converter<?>> entry : Converters.DEFAULT_CONVERTERS.entrySet()) {
+      Class<?> converterClass = entry.getKey();
+      String typeName = converterClass.getCanonicalName();
+      TypeElement typeElement = elementUtils.getTypeElement(typeName);
+      // Check that we can get a type mirror, either through the type element or the primitive type.
+      if (typeElement != null) {
+        converterMapBuilder.put(typeElement.asType(), entry.getValue());
+      } else {
+        if (!primitiveTypeMap.containsKey(converterClass)) {
+          messager.printMessage(
+              Diagnostic.Kind.ERROR,
+              String.format("Can't get a TypeElement for Type %s", typeName));
+          continue;
+        }
+        // Add the primitive types to the map, both in primitive TypeMirror form, and the boxed
+        // classes, such as java.lang.Integer, because primitives must be boxed in collections,
+        // such as allowMultiple options, which have type List<singleOptionType>.
+        PrimitiveType primitiveType = primitiveTypeMap.get(converterClass);
+        converterMapBuilder.put(primitiveType, entry.getValue());
+        converterMapBuilder.put(typeUtils.boxedClass(primitiveType).asType(), entry.getValue());
+      }
     }
+    defaultConverters = converterMapBuilder.build();
   }
 
   /** Check that the Option variables only occur in OptionBase-inheriting classes. */
@@ -107,6 +152,190 @@ public final class OptionProcessor extends AbstractProcessor {
     }
   }
 
+  private ImmutableList<TypeMirror> getAcceptedConverterReturnTypes(VariableElement optionField)
+      throws OptionProcessorException {
+    TypeMirror optionType = optionField.asType();
+    Option annotation = optionField.getAnnotation(Option.class);
+    TypeMirror listType = elementUtils.getTypeElement(List.class.getCanonicalName()).asType();
+    // Options that accumulate multiple mentions in an arglist must have type List<T>, where each
+    // individual mention has type T. Identify type T to use it for checking the converter's return
+    // type.
+    if (annotation.allowMultiple()) {
+      // Check that the option type is in fact a list.
+      if (optionType.getKind() != TypeKind.DECLARED) {
+        throw new OptionProcessorException(
+            optionField,
+            "Option that allows multiple occurrences must be of type %s, but is of type %s",
+            listType,
+            optionType);
+      }
+      DeclaredType optionDeclaredType = (DeclaredType) optionType;
+      // optionDeclaredType.asElement().asType() gets us from List<actualType> to List<E>, so this
+      // is unfortunately necessary.
+      if (!typeUtils.isAssignable(optionDeclaredType.asElement().asType(), listType)) {
+        throw new OptionProcessorException(
+            optionField,
+            "Option that allows multiple occurrences must be of type %s, but is of type %s",
+            listType,
+            optionType);
+      }
+
+      // Check that there is only one generic parameter, and store it as the singular option type.
+      List<? extends TypeMirror> genericParameters = optionDeclaredType.getTypeArguments();
+      if (genericParameters.size() != 1) {
+        throw new OptionProcessorException(
+            optionField,
+            "Option that allows multiple occurrences must be of type %s, "
+                + "where E is the type of an individual command-line mention of this option, "
+                + "but is of type %s",
+            listType,
+            optionType);
+      }
+
+      // For repeated options, we also accept cases where each option itself contains a list, which
+      // are then concatenated into the final single list type. For this reason, we will accept both
+      // converters that return the type of a single option, and List<singleOption>, which,
+      // incidentally, is the original optionType.
+      // Example: --foo=a,b,c --foo=d,e,f could have a final value of type List<Char>,
+      //       value {a,b,c,e,d,f}, instead of requiring a final value of type List<List<Char>>
+      //       value {{a,b,c},{d,e,f}}
+      TypeMirror singularOptionType = genericParameters.get(0);
+
+      return ImmutableList.of(singularOptionType, optionType);
+    } else {
+      return ImmutableList.of(optionField.asType());
+    }
+  }
+
+  private void checkForDefaultConverter(
+      VariableElement optionField,
+      List<TypeMirror> acceptedConverterReturnTypes,
+      String defaultValue)
+      throws OptionProcessorException {
+    for (TypeMirror acceptedConverterReturnType : acceptedConverterReturnTypes) {
+      Converter<?> converterInstance = defaultConverters.get(acceptedConverterReturnType);
+      if (converterInstance == null) {
+        // This return type isn't a match, move on to the next one in case.
+        continue;
+      }
+      TypeElement converter =
+          elementUtils.getTypeElement(converterInstance.getClass().getCanonicalName());
+      try {
+        // For the default converters, it so happens we have access to the convert methods
+        // at compile time, since we already have the OptionsParser source. Take advantage of
+        // this to test that the provided defaultValue is valid.
+        converterInstance.convert(defaultValue);
+      } catch (OptionsParsingException e) {
+        throw new OptionProcessorException(
+            optionField,
+            /* throwable = */ e,
+            "Option lists a default value (%s) that is not parsable by the option's converter "
+                + "(s)",
+            defaultValue,
+            converter);
+      }
+      return; // This one passes the test.
+    }
+
+    // We didn't find a default converter.
+    throw new OptionProcessorException(
+        optionField,
+        "Cannot find valid converter for option of type %s",
+        acceptedConverterReturnTypes.get(0));
+  }
+
+  private void checkProvidedConverter(
+      VariableElement optionField,
+      ImmutableList<TypeMirror> acceptedConverterReturnTypes,
+      TypeElement converterElement)
+      throws OptionProcessorException {
+    if (converterElement.getModifiers().contains(Modifier.ABSTRACT)) {
+      throw new OptionProcessorException(
+          optionField, "The converter type %s must be a concrete type", converterElement.asType());
+    }
+
+    DeclaredType converterType = (DeclaredType) converterElement.asType();
+
+    // Unfortunately, for provided classes, we do not have access to the compiled convert
+    // method at this time, and cannot check that the default value is parseable. We will
+    // instead check that T of Converter<T> matches the option's type, but this is all we can
+    // do.
+    List<ExecutableElement> methodList =
+        elementUtils
+            .getAllMembers(converterElement)
+            .stream()
+            .filter(element -> element.getKind() == ElementKind.METHOD)
+            .map(methodElement -> (ExecutableElement) methodElement)
+            .filter(methodElement -> methodElement.getSimpleName().contentEquals("convert"))
+            .filter(
+                methodElement ->
+                    methodElement.getParameters().size() == 1
+                        && typeUtils.isSameType(
+                            methodElement.getParameters().get(0).asType(),
+                            elementUtils.getTypeElement(String.class.getCanonicalName()).asType()))
+            .collect(Collectors.toList());
+    // Check that there is just the one method
+    if (methodList.size() != 1) {
+      throw new OptionProcessorException(
+          optionField,
+          "Converter %s has methods 'convert(String)': %s",
+          converterElement,
+          methodList.stream().map(Object::toString).collect(Collectors.joining(", ")));
+    }
+
+    ExecutableType convertMethodType =
+        (ExecutableType) typeUtils.asMemberOf(converterType, methodList.get(0));
+    TypeMirror convertMethodResultType = convertMethodType.getReturnType();
+    // Check that the converter's return type is in the accepted list.
+    for (TypeMirror acceptedConverterReturnType : acceptedConverterReturnTypes) {
+      if (typeUtils.isAssignable(convertMethodResultType, acceptedConverterReturnType)) {
+        return; // This one passes the test.
+      }
+    }
+    throw new OptionProcessorException(
+        optionField,
+        "Type of field (%s) must be assignable from the converter's return type (%s)",
+        acceptedConverterReturnTypes.get(0),
+        convertMethodResultType);
+  }
+
+  private void checkConverter(VariableElement optionField) throws OptionProcessorException {
+    TypeMirror optionType = optionField.asType();
+    Option annotation = optionField.getAnnotation(Option.class);
+    ImmutableList<TypeMirror> acceptedConverterReturnTypes =
+        getAcceptedConverterReturnTypes(optionField);
+
+    // For simple, static expansions, don't accept non-Void types.
+    if (annotation.expansion().length != 0
+        && !typeUtils.isSameType(
+            optionType, elementUtils.getTypeElement(Void.class.getCanonicalName()).asType())) {
+      throw new OptionProcessorException(
+          optionField,
+          "Option is an expansion flag with a static expansion, but does not have Void type.");
+    }
+
+    // Obtain the converter for this option.
+    AnnotationMirror optionMirror =
+        ProcessorUtils.getAnnotation(elementUtils, typeUtils, optionField, Option.class);
+    TypeElement defaultConverterElement =
+        elementUtils.getTypeElement(Converter.class.getCanonicalName());
+    TypeElement converterElement =
+        ProcessorUtils.getClassTypeFromAnnotationField(elementUtils, optionMirror, "converter");
+    if (converterElement == null) {
+      throw new OptionProcessorException(optionField, "Null converter found.");
+    }
+
+    if (typeUtils.isSameType(converterElement.asType(), defaultConverterElement.asType())) {
+      // Find a matching converter in the default converter list, and check that it successfully
+      // parses the default value for this option.
+      checkForDefaultConverter(
+          optionField, acceptedConverterReturnTypes, annotation.defaultValue());
+    } else {
+      // Check that the provided converter has an accepted return type.
+      checkProvidedConverter(optionField, acceptedConverterReturnTypes, converterElement);
+    }
+  }
+
   /**
    * Check that the option lists at least one effect, and that no nonsensical combinations are
    * listed, such as having a known effect listed with UNKNOWN.
@@ -172,10 +401,11 @@ public final class OptionProcessor extends AbstractProcessor {
 
         checkModifiers(optionField);
         checkInOptionBase(optionField);
+        checkConverter(optionField);
         checkEffectTagRationality(optionField);
         checkMetadataTagAndCategoryRationality(optionField);
       } catch (OptionProcessorException e) {
-        error(e.elementInError, e.getMessage());
+        error(e.getElementInError(), e.getMessage());
       }
     }
     // Claim all Option annotated fields.
diff --git a/src/main/java/com/google/devtools/common/options/processor/OptionProcessorException.java b/src/main/java/com/google/devtools/common/options/processor/OptionProcessorException.java
new file mode 100644
index 0000000000..0a35f4ccee
--- /dev/null
+++ b/src/main/java/com/google/devtools/common/options/processor/OptionProcessorException.java
@@ -0,0 +1,35 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.common.options.processor;
+
+import javax.lang.model.element.Element;
+
+/** Exception that indicates a problem in the processing of an {@link Option}. */
+class OptionProcessorException extends Exception {
+  private final Element elementInError;
+
+  OptionProcessorException(Element element, String message, Object... args) {
+    super(String.format(message, args));
+    elementInError = element;
+  }
+
+  OptionProcessorException(Element element, Throwable throwable, String message, Object... args) {
+    super(String.format(message, args), throwable);
+    elementInError = element;
+  }
+
+  Element getElementInError() {
+    return elementInError;
+  }
+}
diff --git a/src/main/java/com/google/devtools/common/options/processor/ProcessorUtils.java b/src/main/java/com/google/devtools/common/options/processor/ProcessorUtils.java
new file mode 100644
index 0000000000..8f94fa554f
--- /dev/null
+++ b/src/main/java/com/google/devtools/common/options/processor/ProcessorUtils.java
@@ -0,0 +1,90 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.common.options.processor;
+
+import java.lang.annotation.Annotation;
+import java.util.Map;
+import javax.lang.model.element.AnnotationMirror;
+import javax.lang.model.element.AnnotationValue;
+import javax.lang.model.element.Element;
+import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.element.TypeElement;
+import javax.lang.model.type.DeclaredType;
+import javax.lang.model.type.TypeMirror;
+import javax.lang.model.util.Elements;
+import javax.lang.model.util.Types;
+
+/** Convenient utilities for dealing with the javax.lang.model types. */
+public class ProcessorUtils {
+
+  /** Return the AnnotationMirror for the annotation of the given type on the element provided. */
+  static AnnotationMirror getAnnotation(
+      Elements elementUtils,
+      Types typeUtils,
+      Element element,
+      Class<? extends Annotation> annotation)
+      throws OptionProcessorException {
+    TypeElement annotationElement = elementUtils.getTypeElement(annotation.getCanonicalName());
+    if (annotationElement == null) {
+      // This can happen if the annotation is on the -processorpath but not on the -classpath.
+      throw new OptionProcessorException(
+          element, "Unable to find the type of annotation %s.", annotation);
+    }
+    TypeMirror annotationMirror = annotationElement.asType();
+
+    for (AnnotationMirror annot : element.getAnnotationMirrors()) {
+      if (typeUtils.isSameType(annot.getAnnotationType(), annotationMirror)) {
+        return annot;
+      }
+    }
+    // No annotation of this requested type found.
+    throw new OptionProcessorException(
+        element, "No annotation %s found for this element.", annotation);
+  }
+
+  /**
+   * Returns the contents of a {@code Class}-typed field in an annotation.
+   *
+   * <p>Taken & adapted from AutoValueProcessor.java
+   *
+   * <p>This method is needed because directly reading the value of such a field from an
+   * AnnotationMirror throws:
+   *
+   * <pre>
+   * javax.lang.model.type.MirroredTypeException: Attempt to access Class object for TypeMirror Foo.
+   * </pre>
+   *
+   * @param annotation The annotation to read from.
+   * @param fieldName The name of the field to read, e.g. "exclude".
+   * @return a set of fully-qualified names of classes appearing in 'fieldName' on 'annotation' on
+   *     'element'.
+   */
+  static TypeElement getClassTypeFromAnnotationField(
+      Elements elementUtils, AnnotationMirror annotation, String fieldName)
+      throws OptionProcessorException {
+    for (Map.Entry<? extends ExecutableElement, ? extends AnnotationValue> entry :
+        elementUtils.getElementValuesWithDefaults(annotation).entrySet()) {
+      if (fieldName.contentEquals(entry.getKey().getSimpleName())) {
+        String qualifiedName =
+            ((TypeElement) ((DeclaredType) entry.getValue().getValue()).asElement())
+                .getQualifiedName()
+                .toString();
+        return elementUtils.getTypeElement(qualifiedName);
+      }
+    }
+    // Annotation missing the requested field.
+    throw new OptionProcessorException(
+        null, "No member %s of the %s annotation found for element.", fieldName, annotation);
+  }
+}
diff --git a/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java b/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java
index d369f33639..2467b83eae 100644
--- a/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java
@@ -19,11 +19,12 @@ import static org.junit.Assert.fail;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
+import com.google.devtools.common.options.Converters.AssignmentConverter;
 import com.google.devtools.common.options.Converters.IntegerConverter;
 import com.google.devtools.common.options.Converters.StringConverter;
+import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
 import com.google.devtools.common.options.OptionsParser.ConstructionException;
 import java.util.Map;
-import java.util.Set;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -35,109 +36,52 @@ public class OptionDefinitionTest {
 
   /** Dummy options class, to test various expected failures of the OptionDefinition. */
   public static class BrokenOptions extends OptionsBase {
-    @Option(
-      name = "missing_its_converter",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.NO_OP},
-      defaultValue = "1"
-    )
-    public Map<String, String> noConverter;
+    public String notAnOption;
 
     @Option(
-      name = "multiple_but_not_a_list",
+      name = "assignments",
+      defaultValue = "foo is not an assignment",
+      converter = AssignmentConverter.class,
       documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.NO_OP},
-      defaultValue = "null",
-      allowMultiple = true
+      effectTags = OptionEffectTag.NO_OP
     )
-    public String multipleWithNoList;
-
-    @Option(
-      name = "multiple_with_wrong_collection",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.NO_OP},
-      defaultValue = "1",
-      allowMultiple = true
-    )
-    public Set<String> multipleWithSetType;
-
-    @Option(
-      name = "invalid_default",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.NO_OP},
-      defaultValue = "not a number"
-    )
-    public int invalidDefault;
-  }
-
-  @Test
-  public void errorForMissingOptionConverter() throws Exception {
-    OptionDefinition optionDef =
-        OptionDefinition.extractOptionDefinition(BrokenOptions.class.getField("noConverter"));
-    try {
-      optionDef.getConverter();
-      fail("Missing converter should have caused getConverter to fail.");
-    } catch (ConstructionException e) {
-      assertThat(e)
-          .hasMessageThat()
-          .contains(
-              "Option noConverter expects values of type java.util.Map<java.lang.String, "
-                  + "java.lang.String>, but no converter was found; possible fix: "
-                  + "add converter=... to its @Option annotation.");
-    }
+    public Map.Entry<String, String> assignments;
   }
 
   @Test
-  public void errorForInvalidOptionTypeForRepeatableOptions() throws Exception {
+  public void optionConverterCannotParseDefaultValue() throws Exception {
     OptionDefinition optionDef =
-        OptionDefinition.extractOptionDefinition(
-            BrokenOptions.class.getField("multipleWithNoList"));
+        OptionDefinition.extractOptionDefinition(BrokenOptions.class.getField("assignments"));
     try {
-      optionDef.getConverter();
-      fail("Mistyped allowMultiple option did not fail getConverter().");
-    } catch (ConstructionException e) {
-      assertThat(e)
-          .hasMessageThat()
-          .contains
-              ("Option multipleWithNoList allows multiple occurrences, so must be of type "
-                  + "List<...>");
-    }
-  }
-
-  @Test
-  public void errorForInvalidCollectionOptionConverter() throws Exception {
-    OptionDefinition optionDef =
-        OptionDefinition.extractOptionDefinition(
-            BrokenOptions.class.getField("multipleWithSetType"));
-    try {
-      optionDef.getConverter();
-      fail("Mistyped allowMultiple option did not fail getConverter().");
+      optionDef.getDefaultValue();
+      fail("Incorrect default should have caused getDefaultValue to fail.");
     } catch (ConstructionException e) {
       assertThat(e)
           .hasMessageThat()
           .contains(
-              "Option multipleWithSetType allows multiple occurrences, so must be of type "
-                  + "List<...>");
+              "OptionsParsingException while retrieving the default value for assignments: "
+                  + "Variable definitions must be in the form of a 'name=value' assignment");
     }
   }
 
   @Test
-  public void errorForInvalidDefaultValue() throws Exception {
-    OptionDefinition optionDef =
-        OptionDefinition.extractOptionDefinition(BrokenOptions.class.getField("invalidDefault"));
+  public void optionDefinitionRejectsNonOptions() throws Exception {
     try {
-      optionDef.getDefaultValue();
-      fail("Invalid default value parsed without failure.");
-    } catch (ConstructionException e) {
+      OptionDefinition.extractOptionDefinition(BrokenOptions.class.getField("notAnOption"));
+      fail("notAnOption isn't an Option, and shouldn't be accepted as one.");
+    } catch (NotAnOptionException e) {
       assertThat(e)
           .hasMessageThat()
           .contains(
-              "OptionsParsingException while retrieving the default value for invalidDefault: "
-                  + "'not a number' is not an int");
+              "The field notAnOption does not have the right annotation to be considered an "
+                  + "option.");
     }
   }
 
-  /** The rare valid options. */
+  /**
+   * Dummy options class with valid options for testing the memoization of converters and default
+   * values.
+   */
   public static class ValidOptionUsingDefaultConverterForMocking extends OptionsBase {
     @Option(
       name = "foo",
@@ -154,7 +98,7 @@ public class OptionDefinitionTest {
       effectTags = {OptionEffectTag.NO_OP},
       defaultValue = "strings"
     )
-    public int bar;
+    public String bar;
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
index 1266d2e25d..5a8534f8fd 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
@@ -416,38 +416,6 @@ public class OptionsDataTest {
   }
 
   /** Dummy options class. */
-  public static class InvalidExpansionOptions extends OptionsBase {
-    @Option(
-      name = "foo",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.NO_OP},
-      defaultValue = "1"
-    )
-    public int foo;
-
-    @Option(
-      name = "bar",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.NO_OP},
-      defaultValue = "1",
-      expansion = {"--foo=42"}
-    )
-    public int bar;
-  }
-
-  @Test
-  public void staticExpansionOptionsShouldNotHaveValues() {
-    try {
-      construct(InvalidExpansionOptions.class);
-      fail();
-    } catch (ConstructionException e) {
-      // Expected exception
-      assertThat(e).hasMessageThat().contains(
-          "Option bar is an expansion flag with a static expansion, but does not have Void type.");
-    }
-  }
-
-  /** Dummy options class. */
   public static class ValidExpansionOptions extends OptionsBase {
     @Option(
       name = "foo",
diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
index 726af7a5d6..8b8754bf77 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
@@ -22,7 +22,6 @@ import static org.junit.Assert.fail;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter;
-import com.google.devtools.common.options.OptionsParser.ConstructionException;
 import com.google.devtools.common.options.OptionsParser.OptionValueDescription;
 import com.google.devtools.common.options.OptionsParser.UnparsedOptionValueDescription;
 import java.io.ByteArrayInputStream;
@@ -1688,29 +1687,6 @@ public class OptionsParserTest {
         .isEqualTo(Arrays.asList("one", "two", "three"));
   }
 
-  public static class IllegalListTypeExample extends OptionsBase {
-    @Option(
-      name = "alpha",
-      converter = CommaSeparatedOptionListConverter.class,
-      allowMultiple = true,
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.NO_OP},
-      defaultValue = "null"
-    )
-    public List<Integer> alpha;
-  }
-
-  @Test
-  public void illegalListType() throws Exception {
-    try {
-      OptionsParser.newOptionsParser(IllegalListTypeExample.class);
-    } catch (ConstructionException e) {
-      // Expected exception
-      return;
-    }
-    fail();
-  }
-
   public static class Yesterday extends OptionsBase {
 
     @Option(
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 ac2be6af90..49c5767a76 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsTest.java
@@ -472,37 +472,8 @@ public class OptionsTest {
     assertThat(options.string).isNull();
   }
 
-  public static class K extends OptionsBase {
-    @Option(
-      name = "1",
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.NO_OP},
-      defaultValue = "null"
-    )
-    public int int1;
-  }
-  @Test
-  public void nullDefaultForPrimitiveTypeOption() throws Exception {
-    // defaultValue() = "null" is not treated specially for primitive types, so
-    // we get an NumberFormatException from the converter (not a
-    // ClassCastException from casting null to int), just as we would for any
-    // other non-integer-literal string default.
-    try {
-      Options.parse(K.class, NO_ARGS).getOptions();
-      fail();
-    } catch (OptionsParser.ConstructionException e) {
-      assertThat(e).hasCauseThat().isInstanceOf(OptionsParsingException.class);
-      assertThat(e)
-          .hasMessageThat()
-          .isEqualTo(
-              "OptionsParsingException while retrieving the default value for "
-                  + "int1: 'null' is not an int");
-    }
-  }
-
   @Test
-  public void nullIsntInterpretedSpeciallyExceptAsADefaultValue()
-      throws Exception {
+  public void nullIsNotInterpretedSpeciallyExceptAsADefaultValue() throws Exception {
     HttpOptions options =
         Options.parse(HttpOptions.class,
                       new String[] { "--host", "null" }).getOptions();