Pass the UnparsedOptionValues when setting or adding option values.

Stop breaking the value apart to be recombined later. Also stop using OptionValueDescriptions as though we have a final option when expanding flags for invocation policy. These values are explicitly the output from parsing the expansion strings, not yet converted or combined with other values of the same flag.

After this change, only UnparsedOptionValueDescription should be used when strings of flags are parsed, and OptionValueDescription should be used when the final version of a flag is created or used.

PiperOrigin-RevId: 168688063
GitOrigin-RevId: a8c0c8dfad38437bc617b4b66d271bc0beb1b7c1
Change-Id: I159b1a1a076704bc3b90760b8c8ad628a135830a

4 files changed, 89 insertions, 90 deletions

diff --git a/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java b/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
index 8b5ba08..59d1a41 100644
--- a/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
+++ b/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
@@ -232,7 +232,7 @@ public final class InvocationPolicyEnforcer {
         String.format("Disallow_Values on expansion flags like %s is not allowed.", flagName));
   }
 
-  private static ImmutableList<OptionValueDescription> getExpansionsFromFlagPolicy(
+  private static ImmutableList<UnparsedOptionValueDescription> getExpansionsFromFlagPolicy(
       FlagPolicy expansionPolicy, OptionDescription optionDescription, OptionsParser parser)
       throws OptionsParsingException {
     if (!optionDescription.isExpansion()) {
@@ -248,7 +248,7 @@ public final class InvocationPolicyEnforcer {
             optionDescription.getOptionDefinition().getOptionName(),
             expansionPolicy.getFlagName()));
 
-    ImmutableList.Builder<OptionValueDescription> resultsBuilder = ImmutableList.builder();
+    ImmutableList.Builder<UnparsedOptionValueDescription> resultsBuilder = ImmutableList.builder();
     switch (expansionPolicy.getOperationCase()) {
       case SET_VALUE:
         {
@@ -327,10 +327,10 @@ public final class InvocationPolicyEnforcer {
       return expandedPolicies;
     }
 
-    ImmutableList<OptionValueDescription> expansions =
+    ImmutableList<UnparsedOptionValueDescription> expansions =
         getExpansionsFromFlagPolicy(originalPolicy, originalOptionDescription, parser);
-    ImmutableList.Builder<OptionValueDescription> subflagBuilder = ImmutableList.builder();
-    ImmutableList<OptionValueDescription> subflags =
+    ImmutableList.Builder<UnparsedOptionValueDescription> subflagBuilder = ImmutableList.builder();
+    ImmutableList<UnparsedOptionValueDescription> subflags =
         subflagBuilder
             .addAll(originalOptionDescription.getImplicitRequirements())
             .addAll(expansions)
@@ -342,7 +342,7 @@ public final class InvocationPolicyEnforcer {
       // only really useful for understanding the invocation policy itself. Most of the time,
       // invocation policy does not change, so this can be a log level fine.
       List<String> subflagNames = new ArrayList<>(subflags.size());
-      for (OptionValueDescription subflag : subflags) {
+      for (UnparsedOptionValueDescription subflag : subflags) {
         subflagNames.add("--" + subflag.getOptionDefinition().getOptionName());
       }
 
@@ -360,13 +360,13 @@ public final class InvocationPolicyEnforcer {
 
     // Repeated flags are special, and could set multiple times in an expansion, with the user
     // expecting both values to be valid. Collect these separately.
-    Multimap<OptionDefinition, OptionValueDescription> repeatableSubflagsInSetValues =
+    Multimap<OptionDefinition, UnparsedOptionValueDescription> repeatableSubflagsInSetValues =
         ArrayListMultimap.create();
 
     // Create a flag policy for the child that looks like the parent's policy "transferred" to its
     // child. Note that this only makes sense for SetValue, when setting an expansion flag, or
     // UseDefault, when preventing it from being set.
-    for (OptionValueDescription currentSubflag : subflags) {
+    for (UnparsedOptionValueDescription currentSubflag : subflags) {
       if (currentSubflag.getOptionDefinition().allowsMultiple()
           && originalPolicy.getOperationCase().equals(OperationCase.SET_VALUE)) {
         repeatableSubflagsInSetValues.put(currentSubflag.getOptionDefinition(), currentSubflag);
@@ -384,8 +384,9 @@ public final class InvocationPolicyEnforcer {
     for (OptionDefinition repeatableFlag : repeatableSubflagsInSetValues.keySet()) {
       int numValues = repeatableSubflagsInSetValues.get(repeatableFlag).size();
       ArrayList<String> newValues = new ArrayList<>(numValues);
-      for (OptionValueDescription setValue : repeatableSubflagsInSetValues.get(repeatableFlag)) {
-        newValues.add(setValue.getOriginalValueString());
+      for (UnparsedOptionValueDescription setValue :
+          repeatableSubflagsInSetValues.get(repeatableFlag)) {
+        newValues.add(setValue.getUnconvertedValue());
       }
       expandedPolicies.add(getSetValueSubflagAsPolicy(repeatableFlag, newValues, originalPolicy));
     }
@@ -443,7 +444,7 @@ public final class InvocationPolicyEnforcer {
    * corresponding policy.
    */
   private static FlagPolicy getSingleValueSubflagAsPolicy(
-      OptionValueDescription currentSubflag, FlagPolicy originalPolicy, boolean isExpansion)
+      UnparsedOptionValueDescription currentSubflag, FlagPolicy originalPolicy, boolean isExpansion)
       throws OptionsParsingException {
     FlagPolicy subflagAsPolicy = null;
     switch (originalPolicy.getOperationCase()) {
@@ -456,10 +457,10 @@ public final class InvocationPolicyEnforcer {
         // Accept null originalValueStrings, they are expected when the subflag is also an expansion
         // flag.
         List<String> subflagValue;
-        if (currentSubflag.getOriginalValueString() == null) {
+        if (currentSubflag.getUnconvertedValue() == null) {
           subflagValue = ImmutableList.of();
         } else {
-          subflagValue = ImmutableList.of(currentSubflag.getOriginalValueString());
+          subflagValue = ImmutableList.of(currentSubflag.getUnconvertedValue());
         }
         subflagAsPolicy =
             getSetValueSubflagAsPolicy(
diff --git a/java/com/google/devtools/common/options/OptionsParser.java b/java/com/google/devtools/common/options/OptionsParser.java
index 68a9f02..881fb38 100644
--- a/java/com/google/devtools/common/options/OptionsParser.java
+++ b/java/com/google/devtools/common/options/OptionsParser.java
@@ -235,12 +235,12 @@ public class OptionsParser implements OptionsProvider {
 
     private final OptionDefinition optionDefinition;
     private final OptionsData.ExpansionData expansionData;
-    private final ImmutableList<OptionValueDescription> implicitRequirements;
+    private final ImmutableList<UnparsedOptionValueDescription> implicitRequirements;
 
     OptionDescription(
         OptionDefinition definition,
         OptionsData.ExpansionData expansionData,
-        ImmutableList<OptionValueDescription> implicitRequirements) {
+        ImmutableList<UnparsedOptionValueDescription> implicitRequirements) {
       this.optionDefinition = definition;
       this.expansionData = expansionData;
       this.implicitRequirements = implicitRequirements;
@@ -250,7 +250,7 @@ public class OptionsParser implements OptionsProvider {
       return optionDefinition;
     }
 
-    public ImmutableList<OptionValueDescription> getImplicitRequirements() {
+    public ImmutableList<UnparsedOptionValueDescription> getImplicitRequirements() {
       return implicitRequirements;
     }
 
@@ -417,7 +417,7 @@ public class OptionsParser implements OptionsProvider {
    * @return The {@link com.google.devtools.common.options.OptionValueDescription>} for the option,
    *     or null if there is no option by the given name.
    */
-  ImmutableList<OptionValueDescription> getExpansionOptionValueDescriptions(
+  ImmutableList<UnparsedOptionValueDescription> getExpansionOptionValueDescriptions(
       OptionDefinition option, @Nullable String optionValue, OptionPriority priority, String source)
       throws OptionsParsingException {
     return impl.getExpansionOptionValueDescriptions(option, optionValue, priority, source);
diff --git a/java/com/google/devtools/common/options/OptionsParserImpl.java b/java/com/google/devtools/common/options/OptionsParserImpl.java
index 5e58435..b79e72d 100644
--- a/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -200,16 +200,17 @@ class OptionsParserImpl {
 
   // Warnings should not end with a '.' because the internal reporter adds one automatically.
   private void setValue(
-      OptionDefinition optionDefinition,
-      Object value,
-      OptionPriority priority,
-      String source,
+      UnparsedOptionValueDescription optionValue,
       OptionDefinition implicitDependant,
-      OptionDefinition expandedFrom) {
-    OptionValueDescription entry = parsedValues.get(optionDefinition);
+      OptionDefinition expandedFrom)
+      throws OptionsParsingException {
+    OptionDefinition optionDefinition = optionValue.getOptionDefinition();
+    Preconditions.checkArgument(!optionDefinition.allowsMultiple());
+    Object convertedValue = optionValue.getConvertedValue();
+    OptionValueDescription entry = parsedValues.get(optionValue.getOptionDefinition());
     if (entry != null) {
       // Override existing option if the new value has higher or equal priority.
-      if (priority.compareTo(entry.getPriority()) >= 0) {
+      if (optionValue.getPriority().compareTo(entry.getPriority()) >= 0) {
         // Output warnings:
         if ((implicitDependant != null) && (entry.getImplicitDependant() != null)) {
           if (!implicitDependant.equals(entry.getImplicitDependant())) {
@@ -222,7 +223,8 @@ class OptionsParserImpl {
                     + implicitDependant.getOptionName()
                     + "'");
           }
-        } else if ((implicitDependant != null) && priority.equals(entry.getPriority())) {
+        } else if ((implicitDependant != null)
+            && optionValue.getPriority().equals(entry.getPriority())) {
           warnings.add(
               "Option '"
                   + optionDefinition.getOptionName()
@@ -236,7 +238,7 @@ class OptionsParserImpl {
                   + "' overrides a previous implicit setting of that option by option '"
                   + entry.getImplicitDependant().getOptionName()
                   + "'");
-        } else if ((priority == entry.getPriority())
+        } else if ((optionValue.getPriority() == entry.getPriority())
             && ((entry.getExpansionParent() == null) && (expandedFrom != null))) {
           // Create a warning if an expansion option overrides an explicit option:
           warnings.add(
@@ -261,24 +263,37 @@ class OptionsParserImpl {
         parsedValues.put(
             optionDefinition,
             OptionValueDescription.newOptionValue(
-                optionDefinition, null, value, priority, source, implicitDependant, expandedFrom));
+                optionDefinition,
+                null,
+                convertedValue,
+                optionValue.getPriority(),
+                optionValue.getSource(),
+                implicitDependant,
+                expandedFrom));
       }
     } else {
       parsedValues.put(
           optionDefinition,
           OptionValueDescription.newOptionValue(
-              optionDefinition, null, value, priority, source, implicitDependant, expandedFrom));
+              optionDefinition,
+              null,
+              convertedValue,
+              optionValue.getPriority(),
+              optionValue.getSource(),
+              implicitDependant,
+              expandedFrom));
       maybeAddDeprecationWarning(optionDefinition);
     }
   }
 
   private void addListValue(
-      OptionDefinition optionDefinition,
-      Object value,
-      OptionPriority priority,
-      String source,
+      UnparsedOptionValueDescription optionValue,
       OptionDefinition implicitDependant,
-      OptionDefinition expandedFrom) {
+      OptionDefinition expandedFrom)
+      throws OptionsParsingException {
+    OptionDefinition optionDefinition = optionValue.getOptionDefinition();
+    Preconditions.checkArgument(optionDefinition.allowsMultiple());
+
     OptionValueDescription entry = parsedValues.get(optionDefinition);
     if (entry == null) {
       entry =
@@ -286,14 +301,15 @@ class OptionsParserImpl {
               optionDefinition,
               /* originalValueString */ null,
               ArrayListMultimap.create(),
-              priority,
-              source,
+              optionValue.getPriority(),
+              optionValue.getSource(),
               implicitDependant,
               expandedFrom);
       parsedValues.put(optionDefinition, entry);
       maybeAddDeprecationWarning(optionDefinition);
     }
-    entry.addValue(priority, value);
+    Object convertedValue = optionValue.getConvertedValue();
+    entry.addValue(optionValue.getPriority(), convertedValue);
   }
 
   OptionValueDescription clearValue(OptionDefinition optionDefinition)
@@ -329,13 +345,13 @@ class OptionsParserImpl {
   }
 
   /** @return A list of the descriptions corresponding to the implicit dependant flags passed in. */
-  private ImmutableList<OptionValueDescription> getImplicitDependantDescriptions(
+  private ImmutableList<UnparsedOptionValueDescription> getImplicitDependantDescriptions(
       ImmutableList<String> options,
       OptionDefinition implicitDependant,
       OptionPriority priority,
       String source)
       throws OptionsParsingException {
-    ImmutableList.Builder<OptionValueDescription> builder = ImmutableList.builder();
+    ImmutableList.Builder<UnparsedOptionValueDescription> builder = ImmutableList.builder();
     Iterator<String> optionsIterator = options.iterator();
 
     Function<OptionDefinition, String> sourceFunction =
@@ -348,15 +364,7 @@ class OptionsParserImpl {
       UnparsedOptionValueDescription unparsedOption =
           identifyOptionAndPossibleArgument(
               unparsedFlagExpression, optionsIterator, priority, sourceFunction, false);
-      builder.add(
-          OptionValueDescription.newOptionValue(
-              unparsedOption.getOptionDefinition(),
-              unparsedOption.getUnconvertedValue(),
-              /* value */ null,
-              unparsedOption.getPriority(),
-              unparsedOption.getSource(),
-              implicitDependant,
-              /* expendedFrom */ null));
+      builder.add(unparsedOption);
     }
     return builder.build();
   }
@@ -367,13 +375,13 @@ class OptionsParserImpl {
    *     correct priority or source for these, as they are not actually set values. The value itself
    *     is also a string, no conversion has taken place.
    */
-  ImmutableList<OptionValueDescription> getExpansionOptionValueDescriptions(
+  ImmutableList<UnparsedOptionValueDescription> getExpansionOptionValueDescriptions(
       OptionDefinition expansionFlag,
       @Nullable String flagValue,
       OptionPriority priority,
       String source)
       throws OptionsParsingException {
-    ImmutableList.Builder<OptionValueDescription> builder = ImmutableList.builder();
+    ImmutableList.Builder<UnparsedOptionValueDescription> builder = ImmutableList.builder();
 
     ImmutableList<String> options = optionsData.getEvaluatedExpansion(expansionFlag, flagValue);
     Iterator<String> optionsIterator = options.iterator();
@@ -384,15 +392,7 @@ class OptionsParserImpl {
       UnparsedOptionValueDescription unparsedOption =
           identifyOptionAndPossibleArgument(
               unparsedFlagExpression, optionsIterator, priority, sourceFunction, false);
-      builder.add(
-          OptionValueDescription.newOptionValue(
-              unparsedOption.getOptionDefinition(),
-              unparsedOption.getUnconvertedValue(),
-              /* value */ null,
-              unparsedOption.getPriority(),
-              unparsedOption.getSource(),
-              /* implicitDependant */ null,
-              expansionFlag));
+      builder.add(unparsedOption);
     }
     return builder.build();
   }
@@ -528,40 +528,14 @@ class OptionsParserImpl {
                   + Joiner.on(' ').join(unparsed));
         }
       } else {
-        Converter<?> converter = optionDefinition.getConverter();
-        Object convertedValue;
-        try {
-          convertedValue = converter.convert(unconvertedValue);
-        } catch (OptionsParsingException e) {
-          // The converter doesn't know the option name, so we supply it here by
-          // re-throwing:
-          throw new OptionsParsingException("While parsing option " + arg
-                                            + ": " + e.getMessage(), e);
-        }
-
         // ...but allow duplicates of single-use options across separate calls to
         // parse(); latest wins:
         if (!optionDefinition.allowsMultiple()) {
-          setValue(
-              optionDefinition,
-              convertedValue,
-              priority,
-              sourceFunction.apply(optionDefinition),
-              implicitDependent,
-              expandedFrom);
+          setValue(unparsedOption, implicitDependent, expandedFrom);
         } else {
-          // But if it's a multiple-use option, then just accumulate the
-          // values, in the order in which they were seen.
-          // Note: The type of the list member is not known; Java introspection
-          // only makes it available in String form via the signature string
-          // for the field declaration.
-          addListValue(
-              optionDefinition,
-              convertedValue,
-              priority,
-              sourceFunction.apply(optionDefinition),
-              implicitDependent,
-              expandedFrom);
+          // But if it's a multiple-use option, then accumulate the values, in the order in which
+          // they were seen.
+          addListValue(unparsedOption, implicitDependent, expandedFrom);
         }
       }
 
@@ -605,6 +579,9 @@ class OptionsParserImpl {
       boolean explicit)
       throws OptionsParsingException {
 
+    // Store the way this option was parsed on the command line.
+    StringBuilder commandLineForm = new StringBuilder();
+    commandLineForm.append(arg);
     String unparsedValue = null;
     OptionDefinition optionDefinition;
     boolean booleanValue = true;
@@ -668,7 +645,9 @@ class OptionsParserImpl {
           && !optionDefinition.isWrapperOption()) {
         // This is expected, Void type options have no args (unless they're wrapper options).
       } else if (nextArgs.hasNext()) {
-        unparsedValue = nextArgs.next(); // "--flag value" form
+        // "--flag value" form
+        unparsedValue = nextArgs.next();
+        commandLineForm.append(" ").append(unparsedValue);
       } else {
         throw new OptionsParsingException("Expected value after " + arg);
       }
@@ -676,6 +655,7 @@ class OptionsParserImpl {
 
     return new UnparsedOptionValueDescription(
         optionDefinition,
+        commandLineForm.toString(),
         unparsedValue,
         priority,
         sourceFunction.apply(optionDefinition),
diff --git a/java/com/google/devtools/common/options/UnparsedOptionValueDescription.java b/java/com/google/devtools/common/options/UnparsedOptionValueDescription.java
index 8cd858a..c6fbbf7 100644
--- a/java/com/google/devtools/common/options/UnparsedOptionValueDescription.java
+++ b/java/com/google/devtools/common/options/UnparsedOptionValueDescription.java
@@ -23,11 +23,12 @@ import javax.annotation.Nullable;
  * <p>This class represents an option as the parser received it, which is distinct from the final
  * value of an option, as these values may be overridden or combined in some way.
  *
- * <p>The origin includes the value it was set to, its priority, a message about where it came
+ * <p>The origin includes the form it had when parsed, its priority, a message about where it came
  * from, and whether it was set explicitly or expanded/implied by other flags.
  */
 public final class UnparsedOptionValueDescription {
   private final OptionDefinition optionDefinition;
+  private final String commandLineForm;
   @Nullable private final String unconvertedValue;
   private final OptionPriority priority;
   @Nullable private final String source;
@@ -39,11 +40,13 @@ public final class UnparsedOptionValueDescription {
 
   public UnparsedOptionValueDescription(
       OptionDefinition optionDefinition,
+      String commandLineForm,
       @Nullable String unconvertedValue,
       OptionPriority priority,
       @Nullable String source,
       boolean explicit) {
     this.optionDefinition = optionDefinition;
+    this.commandLineForm = commandLineForm;
     this.unconvertedValue = unconvertedValue;
     this.priority = priority;
     this.source = source;
@@ -54,6 +57,10 @@ public final class UnparsedOptionValueDescription {
     return optionDefinition;
   }
 
+  public String getCommandLineForm() {
+    return commandLineForm;
+  }
+
   public boolean isBooleanOption() {
     return optionDefinition.getType().equals(boolean.class);
   }
@@ -99,6 +106,17 @@ public final class UnparsedOptionValueDescription {
     return explicit;
   }
 
+  public Object getConvertedValue() throws OptionsParsingException {
+    Converter<?> converter = optionDefinition.getConverter();
+    try {
+      return converter.convert(unconvertedValue);
+    } catch (OptionsParsingException e) {
+      // The converter doesn't know the option name, so we supply it here by re-throwing:
+      throw new OptionsParsingException(
+          String.format("While parsing option %s: %s", commandLineForm, e.getMessage()), e);
+    }
+  }
+
   @Override
   public String toString() {
     StringBuilder result = new StringBuilder();