Treat parsed option values differently by option type.

There is a vexingly large world of possible option types, each with its own quirks of how it interfaces with new inputs as they come in: values can be
- overridden (default)
- concatenated (allowMultiple)
- flattened (allowMultiple that accepts list inputs)
- disappear into additional flag inputs (expansion flags)
Or some combination of the above, in the case of flags with implicit dependencies and wrapper options.

Begin removing the error-prone treatment of all option types with conditional branches. This model of the different options will make it much easier to isolate the option-type specific logic with the command-line parsing logic. Flags that affect other flags (implicit requirements, expansions, and wrappers) will be migrated in a later change.

This CL does not change flag parsing semantics, just migrates the current parsing logic to the new class structure.

RELNOTES: None.
PiperOrigin-RevId: 169239182
GitOrigin-RevId: 3ab171a1b03861d40fcf49fb56e2d8df87db25ab
Change-Id: I2933cb6d69e3d5b3c605bc6d9308f8be84dd3714

3 files changed, 364 insertions, 268 deletions

diff --git a/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java b/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
index 4285f33..742acb6 100644
--- a/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
+++ b/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
@@ -547,7 +547,7 @@ public final class InvocationPolicyEnforcer {
           "Keeping value '%s' from source '%s' for flag '%s' "
               + "because the invocation policy specifying the value(s) '%s' is overridable",
           valueDescription.getValue(),
-          valueDescription.getSource(),
+          valueDescription.getSourceString(),
           optionDefinition.getOptionName(),
           setValue.getFlagValueList());
     } else {
@@ -561,17 +561,17 @@ public final class InvocationPolicyEnforcer {
       for (String flagValue : setValue.getFlagValueList()) {
         if (valueDescription == null) {
           logInApplySetValueOperation(
-              "Setting value for flag '%s' from invocation "
-                  + "policy to '%s', overriding the default value '%s'",
+              "Setting value for flag '%s' from invocation policy to '%s', overriding the "
+                  + "default value '%s'",
               optionDefinition.getOptionName(), flagValue, optionDefinition.getDefaultValue());
         } else {
           logInApplySetValueOperation(
-              "Setting value for flag '%s' from invocation "
-                  + "policy to '%s', overriding value '%s' from '%s'",
+              "Setting value for flag '%s' from invocation policy to '%s', overriding "
+                  + "value '%s' from '%s'",
               optionDefinition.getOptionName(),
               flagValue,
               valueDescription.getValue(),
-              valueDescription.getSource());
+              valueDescription.getSourceString());
         }
         setFlagValue(parser, optionDefinition, flagValue);
       }
@@ -585,8 +585,6 @@ public final class InvocationPolicyEnforcer {
     if (clearedValueDescription != null) {
       // Log the removed value.
       String clearedFlagName = clearedValueDescription.getOptionDefinition().getOptionName();
-      String originalValue = clearedValueDescription.getValue().toString();
-      String source = clearedValueDescription.getSource();
 
       OptionDescription desc =
           parser.getOptionDescription(
@@ -597,9 +595,13 @@ public final class InvocationPolicyEnforcer {
       }
       logger.info(
           String.format(
-              "Using default value '%s' for flag '%s' as "
-                  + "specified by %s invocation policy, overriding original value '%s' from '%s'",
-              clearedFlagDefaultValue, clearedFlagName, policyType, originalValue, source));
+              "Using default value '%s' for flag '%s' as specified by %s invocation policy, "
+                  + "overriding original value '%s' from '%s'",
+              clearedFlagDefaultValue,
+              clearedFlagName,
+              policyType,
+              clearedValueDescription.getValue(),
+              clearedValueDescription.getSourceString()));
     }
   }
 
@@ -724,8 +726,8 @@ public final class InvocationPolicyEnforcer {
           // Use the default value from the policy.
           logger.info(
               String.format(
-                  "Overriding default value '%s' for flag '%s' with value '%s' "
-                      + "specified by invocation policy. %sed values are: %s",
+                  "Overriding default value '%s' for flag '%s' with value '%s' specified by "
+                      + "invocation policy. %sed values are: %s",
                   optionDefinition.getDefaultValue(),
                   optionDefinition.getOptionName(),
                   newValue,
@@ -738,8 +740,7 @@ public final class InvocationPolicyEnforcer {
           throw new OptionsParsingException(
               String.format(
                   "Default flag value '%s' for flag '%s' is not allowed by invocation policy, but "
-                      + "the policy does not provide a new value. "
-                      + "%sed values are: %s",
+                      + "the policy does not provide a new value. %sed values are: %s",
                   optionDescription.getOptionDefinition().getDefaultValue(),
                   optionDefinition.getOptionName(),
                   policyType,
diff --git a/java/com/google/devtools/common/options/OptionValueDescription.java b/java/com/google/devtools/common/options/OptionValueDescription.java
index 8e16287..ca709dc 100644
--- a/java/com/google/devtools/common/options/OptionValueDescription.java
+++ b/java/com/google/devtools/common/options/OptionValueDescription.java
@@ -14,176 +14,373 @@
 
 package com.google.devtools.common.options;
 
-import com.google.common.base.Preconditions;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ListMultimap;
+import com.google.devtools.common.options.OptionsParser.ConstructionException;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
-import javax.annotation.Nullable;
+import java.util.stream.Collectors;
 
 /**
- * The name and value of an option with additional metadata describing its priority, source, whether
- * it was set via an implicit dependency, and if so, by which other option.
+ * The value of an option.
+ *
+ * <p>This takes care of tracking the final value as multiple instances of an option are parsed. It
+ * also tracks additional metadata describing its priority, source, whether it was set via an
+ * implicit dependency, and if so, by which other option.
  */
-public class OptionValueDescription {
-  private final OptionDefinition optionDefinition;
-  private final boolean isDefaultValue;
-  @Nullable private final String originalValueString;
-  @Nullable private final Object value;
-  @Nullable private final OptionPriority priority;
-  @Nullable private final String source;
-  @Nullable private final OptionDefinition implicitDependant;
-  @Nullable private final OptionDefinition expandedFrom;
-
-  private OptionValueDescription(
-      OptionDefinition optionDefinition,
-      boolean isDefaultValue,
-      @Nullable String originalValueString,
-      @Nullable Object value,
-      @Nullable OptionPriority priority,
-      @Nullable String source,
-      @Nullable OptionDefinition implicitDependant,
-      @Nullable OptionDefinition expandedFrom) {
-    this.optionDefinition = optionDefinition;
-    this.isDefaultValue = isDefaultValue;
-    this.originalValueString = originalValueString;
-    this.value = value;
-    this.priority = priority;
-    this.source = source;
-    this.implicitDependant = implicitDependant;
-    this.expandedFrom = expandedFrom;
-  }
+public abstract class OptionValueDescription {
 
-  public static OptionValueDescription newOptionValue(
-      OptionDefinition optionDefinition,
-      @Nullable String originalValueString,
-      @Nullable Object value,
-      @Nullable OptionPriority priority,
-      @Nullable String source,
-      @Nullable OptionDefinition implicitDependant,
-      @Nullable OptionDefinition expandedFrom) {
-    return new OptionValueDescription(
-        optionDefinition,
-        false,
-        originalValueString,
-        value,
-        priority,
-        source,
-        implicitDependant,
-        expandedFrom);
-  }
+  protected final OptionDefinition optionDefinition;
 
-  public static OptionValueDescription newDefaultValue(OptionDefinition optionDefinition) {
-    return new OptionValueDescription(
-        optionDefinition, true, null, null, OptionPriority.DEFAULT, null, null, null);
+  public OptionValueDescription(OptionDefinition optionDefinition) {
+    this.optionDefinition = optionDefinition;
   }
 
   public OptionDefinition getOptionDefinition() {
     return optionDefinition;
   }
 
-  public String getName() {
-    return optionDefinition.getOptionName();
+  /** Returns the current or final value of this option. */
+  public abstract Object getValue();
+
+  /** Returns the source(s) of this option, if there were multiple, duplicates are removed. */
+  public abstract String getSourceString();
+
+  // TODO(b/65540004) implicitDependant and expandedFrom are artifacts of an option instance, and
+  // should be in ParsedOptionDescription.
+  abstract void addOptionInstance(
+      ParsedOptionDescription parsedOption,
+      OptionDefinition implicitDependant,
+      OptionDefinition expandedFrom,
+      List<String> warnings)
+      throws OptionsParsingException;
+
+  /**
+   * For the given option, returns the correct type of OptionValueDescription, to which unparsed
+   * values can be added.
+   *
+   * <p>The categories of option types are non-overlapping, an invariant checked by the
+   * OptionProcessor at compile time.
+   */
+  public static OptionValueDescription createOptionValueDescription(OptionDefinition option) {
+    if (option.allowsMultiple()) {
+      return new RepeatableOptionValueDescription(option);
+    } else if (option.isExpansionOption()) {
+      return new ExpansionOptionValueDescription(option);
+    } else if (option.getImplicitRequirements().length > 0) {
+      return new OptionWithImplicitRequirementsValueDescription(option);
+    } else if (option.isWrapperOption()) {
+      return new WrapperOptionValueDescription(option);
+    } else {
+      return new SingleOptionValueDescription(option);
+    }
   }
 
-  public String getOriginalValueString() {
-    return originalValueString;
+  /**
+   * For options that have not been set, this will return a correct OptionValueDescription for the
+   * default value.
+   */
+  public static OptionValueDescription getDefaultOptionValue(OptionDefinition option) {
+    return new DefaultOptionValueDescription(option);
   }
 
-  // Need to suppress unchecked warnings, because the "multiple occurrence"
-  // options use unchecked ListMultimaps due to limitations of Java generics.
-  @SuppressWarnings({"unchecked", "rawtypes"})
-  public Object getValue() {
-    if (isDefaultValue) {
-      // If no value was present, we want the default value for this option.
+  static class DefaultOptionValueDescription extends OptionValueDescription {
+
+    private DefaultOptionValueDescription(OptionDefinition optionDefinition) {
+      super(optionDefinition);
+    }
+
+    @Override
+    public Object getValue() {
       return optionDefinition.getDefaultValue();
     }
-    if (getAllowMultiple() && value != null) {
-      // Sort the results by option priority and return them in a new list.
-      // 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 = new ArrayList<>();
-      ListMultimap realValue = (ListMultimap) value;
+
+    @Override
+    public String getSourceString() {
+      return null;
+    }
+
+    @Override
+    void addOptionInstance(
+        ParsedOptionDescription parsedOption,
+        OptionDefinition implicitDependant,
+        OptionDefinition expandedFrom,
+        List<String> warnings) {
+      throw new IllegalStateException(
+          "Cannot add values to the default option value. Create a modifiable "
+              + "OptionValueDescription using createOptionValueDescription() instead.");
+    }
+  }
+
+  /**
+   * The form of a value for a default type of flag, one that does not accumulate multiple values
+   * and has no expansion.
+   */
+  static class SingleOptionValueDescription extends OptionValueDescription {
+    private ParsedOptionDescription effectiveOptionInstance;
+    private Object effectiveValue;
+    private OptionDefinition optionThatDependsOnEffectiveValue;
+    private OptionDefinition optionThatExpandedToEffectiveValue;
+
+    private SingleOptionValueDescription(OptionDefinition optionDefinition) {
+      super(optionDefinition);
+      if (optionDefinition.allowsMultiple()) {
+        throw new ConstructionException("Can't have a single value for an allowMultiple option.");
+      }
+      if (optionDefinition.isExpansionOption()) {
+        throw new ConstructionException("Can't have a single value for an expansion option.");
+      }
+      effectiveOptionInstance = null;
+      effectiveValue = null;
+      optionThatDependsOnEffectiveValue = null;
+      optionThatExpandedToEffectiveValue = null;
+    }
+
+    @Override
+    public Object getValue() {
+      return effectiveValue;
+    }
+
+    @Override
+    public String getSourceString() {
+      return effectiveOptionInstance.getSource();
+    }
+
+    // Warnings should not end with a '.' because the internal reporter adds one automatically.
+    @Override
+    void addOptionInstance(
+        ParsedOptionDescription parsedOption,
+        OptionDefinition implicitDependant,
+        OptionDefinition expandedFrom,
+        List<String> warnings)
+        throws OptionsParsingException {
+      // This might be the first value, in that case, just store it!
+      if (effectiveOptionInstance == null) {
+        effectiveOptionInstance = parsedOption;
+        optionThatDependsOnEffectiveValue = implicitDependant;
+        optionThatExpandedToEffectiveValue = expandedFrom;
+        effectiveValue = effectiveOptionInstance.getConvertedValue();
+        return;
+      }
+
+      // If there was another value, check whether the new one will override it, and if so,
+      // log warnings describing the change.
+      if (parsedOption.getPriority().compareTo(effectiveOptionInstance.getPriority()) >= 0) {
+        // Output warnings:
+        if ((implicitDependant != null) && (optionThatDependsOnEffectiveValue != null)) {
+          if (!implicitDependant.equals(optionThatDependsOnEffectiveValue)) {
+            warnings.add(
+                String.format(
+                    "Option '%s' is implicitly defined by both option '%s' and option '%s'",
+                    optionDefinition.getOptionName(),
+                    optionThatDependsOnEffectiveValue.getOptionName(),
+                    implicitDependant.getOptionName()));
+          }
+        } else if ((implicitDependant != null)
+            && parsedOption.getPriority().equals(effectiveOptionInstance.getPriority())) {
+          warnings.add(
+              String.format(
+                  "Option '%s' is implicitly defined by option '%s'; the implicitly set value "
+                      + "overrides the previous one",
+                  optionDefinition.getOptionName(), implicitDependant.getOptionName()));
+        } else if (optionThatDependsOnEffectiveValue != null) {
+          warnings.add(
+              String.format(
+                  "A new value for option '%s' overrides a previous implicit setting of that "
+                      + "option by option '%s'",
+                  optionDefinition.getOptionName(),
+                  optionThatDependsOnEffectiveValue.getOptionName()));
+        } else if ((parsedOption.getPriority() == effectiveOptionInstance.getPriority())
+            && ((optionThatExpandedToEffectiveValue == null) && (expandedFrom != null))) {
+          // Create a warning if an expansion option overrides an explicit option:
+          warnings.add(
+              String.format(
+                  "The option '%s' was expanded and now overrides a previous explicitly specified "
+                      + "option '%s'",
+                  expandedFrom.getOptionName(), optionDefinition.getOptionName()));
+        } else if ((optionThatExpandedToEffectiveValue != null) && (expandedFrom != null)) {
+          warnings.add(
+              String.format(
+                  "The option '%s' was expanded to from both options '%s' and '%s'",
+                  optionDefinition.getOptionName(),
+                  optionThatExpandedToEffectiveValue.getOptionName(),
+                  expandedFrom.getOptionName()));
+        }
+
+        // Record the new value:
+        effectiveOptionInstance = parsedOption;
+        optionThatDependsOnEffectiveValue = implicitDependant;
+        optionThatExpandedToEffectiveValue = expandedFrom;
+        effectiveValue = parsedOption.getConvertedValue();
+      } else {
+        // The new value does not override the old value, as it has lower priority.
+        warnings.add(
+            String.format(
+                "The lower priority option '%s' does not override the previous value '%s'",
+                parsedOption.getCommandLineForm(), effectiveOptionInstance.getCommandLineForm()));
+      }
+    }
+
+    @VisibleForTesting
+    ParsedOptionDescription getEffectiveOptionInstance() {
+      return effectiveOptionInstance;
+    }
+  }
+
+  /** The form of a value for an option that accumulates multiple values on the command line. */
+  static class RepeatableOptionValueDescription extends OptionValueDescription {
+    ListMultimap<OptionPriority, ParsedOptionDescription> parsedOptions;
+    ListMultimap<OptionPriority, Object> optionValues;
+
+    private RepeatableOptionValueDescription(OptionDefinition optionDefinition) {
+      super(optionDefinition);
+      if (!optionDefinition.allowsMultiple()) {
+        throw new ConstructionException(
+            "Can't have a repeated value for a non-allowMultiple option.");
+      }
+      parsedOptions = ArrayListMultimap.create();
+      optionValues = ArrayListMultimap.create();
+    }
+
+    @Override
+    public String getSourceString() {
+      return parsedOptions
+          .asMap()
+          .values()
+          .stream()
+          .flatMap(Collection::stream)
+          .map(ParsedOptionDescription::getSource)
+          .distinct()
+          .collect(Collectors.joining(", "));
+    }
+
+    @Override
+    public List<Object> getValue() {
+      // Sort the results by option priority and return them in a new list. 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<Object> result = new ArrayList<>();
       for (OptionPriority priority : OptionPriority.values()) {
         // If there is no mapping for this key, this check avoids object creation (because
         // ListMultimap has to return a new object on get) and also an unnecessary addAll call.
-        if (realValue.containsKey(priority)) {
-          result.addAll(realValue.get(priority));
+        if (optionValues.containsKey(priority)) {
+          result.addAll(optionValues.get(priority));
         }
       }
       return result;
     }
-    return value;
-  }
 
-  /**
-   * @return the priority of the thing that set this value for this flag
-   */
-  public OptionPriority getPriority() {
-    return priority;
+    @Override
+    void addOptionInstance(
+        ParsedOptionDescription parsedOption,
+        OptionDefinition implicitDependant,
+        OptionDefinition expandedFrom,
+        List<String> warnings)
+        throws OptionsParsingException {
+      // For repeatable options, we allow flags that take both single values and multiple values,
+      // potentially collapsing them down.
+      Object convertedValue = parsedOption.getConvertedValue();
+      if (convertedValue instanceof List<?>) {
+        optionValues.putAll(parsedOption.getPriority(), (List<?>) convertedValue);
+      } else {
+        optionValues.put(parsedOption.getPriority(), convertedValue);
+      }
+    }
   }
 
   /**
-   * @return the thing that set this value for this flag
+   * The form of a value for an expansion option, one that does not have its own value but expands
+   * in place to other options. This should be used for both flags with a static expansion defined
+   * in {@link Option#expansion()} and flags with an {@link Option#expansionFunction()}.
    */
-  public String getSource() {
-    return source;
-  }
+  static class ExpansionOptionValueDescription extends OptionValueDescription {
 
-  public OptionDefinition getImplicitDependant() {
-    return implicitDependant;
-  }
+    private ExpansionOptionValueDescription(OptionDefinition optionDefinition) {
+      super(optionDefinition);
+      if (!optionDefinition.isExpansionOption()) {
+        throw new ConstructionException(
+            "Options without expansions can't be tracked using ExpansionOptionValueDescription");
+      }
+    }
 
-  public boolean isImplicitDependency() {
-    return implicitDependant != null;
-  }
+    @Override
+    public Object getValue() {
+      return null;
+    }
 
-  public OptionDefinition getExpansionParent() {
-    return expandedFrom;
-  }
+    @Override
+    public String getSourceString() {
+      return null;
+    }
 
-  public boolean isExpansion() {
-    return expandedFrom != null;
+    @Override
+    void addOptionInstance(
+        ParsedOptionDescription parsedOption,
+        OptionDefinition implicitDependant,
+        OptionDefinition expandedFrom,
+        List<String> warnings) {
+      // TODO(b/65540004) Deal with expansion options here instead of in parse(), and track their
+      // link to the options they expanded to to.
+    }
   }
 
-  public boolean getAllowMultiple() {
-    return optionDefinition.allowsMultiple();
-  }
+  /** The form of a value for a flag with implicit requirements. */
+  static class OptionWithImplicitRequirementsValueDescription extends SingleOptionValueDescription {
 
-  @Override
-  public String toString() {
-    StringBuilder result = new StringBuilder();
-    result.append("option '").append(optionDefinition.getOptionName()).append("' ");
-    if (isDefaultValue) {
-      result
-          .append("set to its default value: '")
-          .append(optionDefinition.getUnparsedDefaultValue())
-          .append("'");
-      return result.toString();
-    } else {
-      result.append("set to '").append(value).append("' ");
-      result.append("with priority ").append(priority);
-      if (source != null) {
-        result.append(" and source '").append(source).append("'");
-      }
-      if (implicitDependant != null) {
-        result.append(" implicitly by ");
+    private OptionWithImplicitRequirementsValueDescription(OptionDefinition optionDefinition) {
+      super(optionDefinition);
+      if (optionDefinition.getImplicitRequirements().length == 0) {
+        throw new ConstructionException(
+            "Options without implicit requirements can't be tracked using "
+                + "OptionWithImplicitRequirementsValueDescription");
       }
-      return result.toString();
+    }
+
+    @Override
+    void addOptionInstance(
+        ParsedOptionDescription parsedOption,
+        OptionDefinition implicitDependant,
+        OptionDefinition expandedFrom,
+        List<String> warnings)
+        throws OptionsParsingException {
+      // This is a valued flag, its value is handled the same way as a normal
+      // SingleOptionValueDescription.
+      super.addOptionInstance(parsedOption, implicitDependant, expandedFrom, warnings);
+
+      // Now deal with the implicit requirements.
+      // TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(),
+      // and track their link to the options they implicitly expanded to to.
     }
   }
 
-  // Need to suppress unchecked warnings, because the "multiple occurrence"
-  // options use unchecked ListMultimaps due to limitations of Java generics.
-  @SuppressWarnings({"unchecked", "rawtypes"})
-  void addValue(OptionPriority addedPriority, Object addedValue) {
-    Preconditions.checkState(optionDefinition.allowsMultiple());
-    Preconditions.checkState(!isDefaultValue);
-    ListMultimap optionValueList = (ListMultimap) value;
-    if (addedValue instanceof List<?>) {
-      optionValueList.putAll(addedPriority, (List<?>) addedValue);
-    } else {
-      optionValueList.put(addedPriority, addedValue);
+  /** Form for options that contain other options in the value text to which they expand. */
+  static final class WrapperOptionValueDescription extends OptionValueDescription {
+
+    WrapperOptionValueDescription(OptionDefinition optionDefinition) {
+      super(optionDefinition);
+    }
+
+    @Override
+    public Object getValue() {
+      return null;
+    }
+
+    @Override
+    public String getSourceString() {
+      return null;
+    }
+
+    @Override
+    void addOptionInstance(
+        ParsedOptionDescription parsedOption,
+        OptionDefinition implicitDependant,
+        OptionDefinition expandedFrom,
+        List<String> warnings)
+        throws OptionsParsingException {
+      // TODO(b/65540004) Deal with options with implicit requirements here instead of in parse(),
+      // and track their link to the options they implicitly expanded to to.
     }
   }
 }
diff --git a/java/com/google/devtools/common/options/OptionsParserImpl.java b/java/com/google/devtools/common/options/OptionsParserImpl.java
index 3c41951..94568be 100644
--- a/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -19,7 +19,6 @@ import static java.util.stream.Collectors.toCollection;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterators;
 import com.google.common.collect.LinkedHashMultimap;
@@ -177,7 +176,7 @@ class OptionsParserImpl {
       OptionDefinition optionDefinition = mapEntry.getValue();
       OptionValueDescription optionValue = optionValues.get(optionDefinition);
       if (optionValue == null) {
-        result.add(OptionValueDescription.newDefaultValue(optionDefinition));
+        result.add(OptionValueDescription.getDefaultOptionValue(optionDefinition));
       } else {
         result.add(optionValue);
       }
@@ -198,119 +197,6 @@ class OptionsParserImpl {
         + (warning.isEmpty() ? "" : ": " + warning));
   }
 
-  // Warnings should not end with a '.' because the internal reporter adds one automatically.
-  private void setValue(
-      ParsedOptionDescription parsedOption,
-      OptionDefinition implicitDependant,
-      OptionDefinition expandedFrom)
-      throws OptionsParsingException {
-    OptionDefinition optionDefinition = parsedOption.getOptionDefinition();
-    Preconditions.checkArgument(!optionDefinition.allowsMultiple());
-    Object convertedValue = parsedOption.getConvertedValue();
-    OptionValueDescription optionValue = optionValues.get(parsedOption.getOptionDefinition());
-    if (optionValue != null) {
-      // Override existing option if the new value has higher or equal priority.
-      if (parsedOption.getPriority().compareTo(optionValue.getPriority()) >= 0) {
-        // Output warnings:
-        if ((implicitDependant != null) && (optionValue.getImplicitDependant() != null)) {
-          if (!implicitDependant.equals(optionValue.getImplicitDependant())) {
-            warnings.add(
-                "Option '"
-                    + optionDefinition.getOptionName()
-                    + "' is implicitly defined by both option '"
-                    + optionValue.getImplicitDependant().getOptionName()
-                    + "' and option '"
-                    + implicitDependant.getOptionName()
-                    + "'");
-          }
-        } else if ((implicitDependant != null)
-            && parsedOption.getPriority().equals(optionValue.getPriority())) {
-          warnings.add(
-              "Option '"
-                  + optionDefinition.getOptionName()
-                  + "' is implicitly defined by option '"
-                  + implicitDependant.getOptionName()
-                  + "'; the implicitly set value overrides the previous one");
-        } else if (optionValue.getImplicitDependant() != null) {
-          warnings.add(
-              "A new value for option '"
-                  + optionDefinition.getOptionName()
-                  + "' overrides a previous implicit setting of that option by option '"
-                  + optionValue.getImplicitDependant().getOptionName()
-                  + "'");
-        } else if ((parsedOption.getPriority() == optionValue.getPriority())
-            && ((optionValue.getExpansionParent() == null) && (expandedFrom != null))) {
-          // Create a warning if an expansion option overrides an explicit option:
-          warnings.add(
-              "The option '"
-                  + expandedFrom.getOptionName()
-                  + "' was expanded and now overrides a "
-                  + "previous explicitly specified option '"
-                  + optionDefinition.getOptionName()
-                  + "'");
-        } else if ((optionValue.getExpansionParent() != null) && (expandedFrom != null)) {
-          warnings.add(
-              "The option '"
-                  + optionDefinition.getOptionName()
-                  + "' was expanded to from both options '"
-                  + optionValue.getExpansionParent().getOptionName()
-                  + "' and '"
-                  + expandedFrom.getOptionName()
-                  + "'");
-        }
-
-        // Record the new value:
-        optionValues.put(
-            optionDefinition,
-            OptionValueDescription.newOptionValue(
-                optionDefinition,
-                null,
-                convertedValue,
-                parsedOption.getPriority(),
-                parsedOption.getSource(),
-                implicitDependant,
-                expandedFrom));
-      }
-    } else {
-      optionValues.put(
-          optionDefinition,
-          OptionValueDescription.newOptionValue(
-              optionDefinition,
-              null,
-              convertedValue,
-              parsedOption.getPriority(),
-              parsedOption.getSource(),
-              implicitDependant,
-              expandedFrom));
-      maybeAddDeprecationWarning(optionDefinition);
-    }
-  }
-
-  private void addListValue(
-      ParsedOptionDescription parsedOption,
-      OptionDefinition implicitDependant,
-      OptionDefinition expandedFrom)
-      throws OptionsParsingException {
-    OptionDefinition optionDefinition = parsedOption.getOptionDefinition();
-    Preconditions.checkArgument(optionDefinition.allowsMultiple());
-
-    OptionValueDescription optionValue = optionValues.get(optionDefinition);
-    if (optionValue == null) {
-      optionValue =
-          OptionValueDescription.newOptionValue(
-              optionDefinition,
-              /* originalValueString */ null,
-              ArrayListMultimap.create(),
-              parsedOption.getPriority(),
-              parsedOption.getSource(),
-              implicitDependant,
-              expandedFrom);
-      optionValues.put(optionDefinition, optionValue);
-      maybeAddDeprecationWarning(optionDefinition);
-    }
-    Object convertedValue = parsedOption.getConvertedValue();
-    optionValue.addValue(parsedOption.getPriority(), convertedValue);
-  }
 
   OptionValueDescription clearValue(OptionDefinition optionDefinition)
       throws OptionsParsingException {
@@ -453,6 +339,8 @@ class OptionsParserImpl {
           identifyOptionAndPossibleArgument(
               arg, argsIterator, priority, sourceFunction, isExplicit);
       OptionDefinition optionDefinition = parsedOption.getOptionDefinition();
+      // All options can be deprecated; check and warn before doing any option-type specific work.
+      maybeAddDeprecationWarning(optionDefinition);
       @Nullable String unconvertedValue = parsedOption.getUnconvertedValue();
 
       if (optionDefinition.isWrapperOption()) {
@@ -515,7 +403,6 @@ class OptionsParserImpl {
                 + " from "
                 + sourceFunction.apply(optionDefinition);
         Function<OptionDefinition, String> expansionSourceFunction = o -> sourceMessage;
-        maybeAddDeprecationWarning(optionDefinition);
         List<String> unparsed =
             parse(priority, expansionSourceFunction, null, optionDefinition, expansion);
         if (!unparsed.isEmpty()) {
@@ -528,15 +415,10 @@ class OptionsParserImpl {
                   + Joiner.on(' ').join(unparsed));
         }
       } else {
-        // ...but allow duplicates of single-use options across separate calls to
-        // parse(); latest wins:
-        if (!optionDefinition.allowsMultiple()) {
-          setValue(parsedOption, implicitDependent, expandedFrom);
-        } else {
-          // But if it's a multiple-use option, then accumulate the values, in the order in which
-          // they were seen.
-          addListValue(parsedOption, implicitDependent, expandedFrom);
-        }
+        OptionValueDescription entry =
+            optionValues.computeIfAbsent(
+                optionDefinition, OptionValueDescription::createOptionValueDescription);
+        entry.addOptionInstance(parsedOption, implicitDependent, expandedFrom, warnings);
       }
 
       // Collect any implicit requirements.
@@ -568,6 +450,13 @@ class OptionsParserImpl {
       }
     }
 
+    // Go through the final values and make sure they are valid values for their option. Unlike any
+    // checks that happened above, this also checks that flags that were not set have a valid
+    // default value. getValue() will throw if the value is invalid.
+    for (OptionValueDescription valueDescription : asListOfEffectiveOptions()) {
+      valueDescription.getValue();
+    }
+
     return unparsedArgs;
   }
 
@@ -690,8 +579,17 @@ class OptionsParserImpl {
       }
       try {
         optionDefinition.getField().set(optionsInstance, value);
+      } catch (IllegalArgumentException e) {
+        throw new IllegalStateException(
+            String.format(
+                "Unable to set option '%s' to value '%s'.",
+                optionDefinition.getOptionName(), value),
+            e);
       } catch (IllegalAccessException e) {
-        throw new IllegalStateException(e);
+        throw new IllegalStateException(
+            "Could not set the field due to access issues. This is impossible, as the "
+                + "OptionProcessor checks that all options are non-final public fields.",
+            e);
       }
     }
     return optionsInstance;