Clean up clearValue and parsed option storage.

Now that policy expands itself before being applied, clearValues never has to
clear more than a single value. This makes that more clear.
OptionValueDescription had not been consistently storing the snake_case name
for an option, leading to some weird behavior when removing the map of return
values from clearValues.

PiperOrigin-RevId: 152156746

3 files changed, 20 insertions, 28 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java b/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
index db220d94be..7e512c1b9c 100644
--- a/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
+++ b/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
@@ -33,8 +33,6 @@ import com.google.devtools.common.options.OptionsParsingException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
-import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -394,18 +392,18 @@ public final class InvocationPolicyEnforcer {
 
   private static void applyUseDefaultOperation(
       OptionsParser parser, String policyType, String flagName) throws OptionsParsingException {
-
-    Map<String, OptionValueDescription> clearedValues = parser.clearValue(flagName);
-    for (Entry<String, OptionValueDescription> clearedValue : clearedValues.entrySet()) {
-
-      OptionValueDescription clearedValueDescription = clearedValue.getValue();
-      String clearedFlagName = clearedValue.getKey();
+    OptionValueDescription clearedValueDescription = parser.clearValue(flagName);
+    if (clearedValueDescription != null) {
+      // Log the removed value.
+      String clearedFlagName = clearedValueDescription.getName();
       String originalValue = clearedValueDescription.getValue().toString();
       String source = clearedValueDescription.getSource();
 
-      Object clearedFlagDefaultValue =
-          parser.getOptionDescription(clearedFlagName).getDefaultValue();
-
+      OptionDescription desc = parser.getOptionDescription(clearedFlagName);
+      Object clearedFlagDefaultValue = null;
+      if (desc != null) {
+        clearedFlagDefaultValue = desc.getDefaultValue();
+      }
       log.info(
           String.format(
               "Using default value '%s' for flag '%s' as "
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index 62f9ee778c..eaa1e81065 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -725,8 +725,7 @@ public class OptionsParser implements OptionsProvider {
   }
 
   /**
-   * Clears the given option. Also clears expansion arguments and implicit requirements for that
-   * option.
+   * Clears the given option.
    *
    * <p>This will not affect options objects that have already been retrieved from this parser
    * through {@link #getOptions(Class)}.
@@ -735,11 +734,10 @@ public class OptionsParser implements OptionsProvider {
    * @return A map of an option name to the old value of the options that were cleared.
    * @throws IllegalArgumentException If the flag does not exist.
    */
-  public Map<String, OptionValueDescription> clearValue(String optionName)
+  public OptionValueDescription clearValue(String optionName)
       throws OptionsParsingException {
-    Map<String, OptionValueDescription> clearedValues = Maps.newHashMap();
-    impl.clearValue(optionName, clearedValues);
-    return clearedValues;
+    OptionValueDescription clearedValue = impl.clearValue(optionName);
+    return clearedValue;
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
index aeb0da08f9..94702c93bb 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -313,13 +313,13 @@ class OptionsParserImpl {
     }
   }
 
-  private void addListValue(Field field, Object value, OptionPriority priority, String source,
-      String implicitDependant, String expandedFrom) {
+  private void addListValue(Field field, String originalName, Object value, OptionPriority priority,
+      String source, String implicitDependant, String expandedFrom) {
     OptionValueDescription entry = parsedValues.get(field);
     if (entry == null) {
       entry =
           new OptionValueDescription(
-              field.getName(),
+              originalName,
               /* originalValueString */ null,
               ArrayListMultimap.create(),
               priority,
@@ -333,20 +333,16 @@ class OptionsParserImpl {
     entry.addValue(priority, value);
   }
 
-  void clearValue(String optionName, Map<String, OptionValueDescription> clearedValues)
+  OptionValueDescription clearValue(String optionName)
       throws OptionsParsingException {
     Field field = optionsData.getFieldFromName(optionName);
     if (field == null) {
       throw new IllegalArgumentException("No such option '" + optionName + "'");
     }
-    Option option = field.getAnnotation(Option.class);
 
     // Actually remove the value from various lists tracking effective options.
     canonicalizeValues.removeAll(field);
-    OptionValueDescription removed = parsedValues.remove(field);
-    if (removed != null) {
-      clearedValues.put(option.name(), removed);
-    }
+    return parsedValues.remove(field);
   }
 
   OptionValueDescription getOptionValueDescription(String name) {
@@ -560,8 +556,8 @@ class OptionsParserImpl {
           // 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(field, convertedValue, priority, sourceFunction.apply(originalName),
-              implicitDependent, expandedFrom);
+          addListValue(field, originalName, convertedValue, priority,
+              sourceFunction.apply(originalName), implicitDependent, expandedFrom);
         }
       }