Fix OptionsParserImpl.clearValue (and invocation policy by extension) to work

correctly with flags that have expansion flags or implicit requirements.

--
MOS_MIGRATED_REVID=121862297

3 files changed, 122 insertions, 81 deletions

diff --git a/src/main/java/com/google/devtools/common/options/Option.java b/src/main/java/com/google/devtools/common/options/Option.java
index 784ba640d4..e269d33910 100644
--- a/src/main/java/com/google/devtools/common/options/Option.java
+++ b/src/main/java/com/google/devtools/common/options/Option.java
@@ -147,6 +147,11 @@ public @interface Option {
    * {@link Option#implicitRequirements()}, {@link Option#expansion()}, {@link Option#converter()}
    * attributes will not be processed. Wrapper options are implicitly repeatable (i.e., as though
    * {@link Option#allowMultiple()} is true regardless of its value in the annotation).
+   *
+   * <p>Wrapper options are provided only for transitioning flags which appear as values to other
+   * flags, to top-level flags. Wrapper options should not be used in Invocation Policy, as
+   * expansion flags to other flags, or as implicit requirements to other flags. Use the inner
+   * flags instead.  
    */
   boolean wrapperOption() default false;
 }
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 fd6d91507f..83a7b08b3a 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -593,7 +593,8 @@ 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 Map<String, OptionValueDescription> clearValue(String optionName)
+      throws OptionsParsingException {
     Map<String, OptionValueDescription> clearedValues = Maps.newHashMap();
     impl.clearValue(optionName, clearedValues);
     return clearedValues;
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 96fb4963fc..6d91dec600 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -22,6 +22,7 @@ import com.google.common.base.Predicate;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Iterators;
 import com.google.common.collect.LinkedHashMultimap;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
@@ -39,6 +40,7 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -444,27 +446,36 @@ class OptionsParserImpl {
     entry.addValue(priority, value);
   }
 
-  void clearValue(String optionName, Map<String, OptionValueDescription> clearedValues) {
+  void clearValue(String optionName, Map<String, OptionValueDescription> clearedValues)
+      throws OptionsParsingException {
     Field field = optionsData.getFieldFromName(optionName);
     if (field == null) {
       throw new IllegalArgumentException("No such option '" + optionName + "'");
     }
+    Option option = field.getAnnotation(Option.class);
+    clearValue(field, option, clearedValues);
+  }
+
+  private void clearValue(
+      Field field, Option option, Map<String, OptionValueDescription> clearedValues)
+      throws OptionsParsingException {
 
     ParsedOptionEntry removed = parsedValues.remove(field);
     if (removed != null) {
-      clearedValues.put(optionName, removed.asOptionValueDescription(optionName));
+      clearedValues.put(option.name(), removed.asOptionValueDescription(option.name()));
     }
 
     canonicalizeValues.removeAll(field);
 
     // Recurse to remove any implicit or expansion flags that this flag may have added when
     // originally parsed.
-    Option option = field.getAnnotation(Option.class);
-    for (String implicitRequirement : option.implicitRequirements()) {
-      clearValue(implicitRequirement, clearedValues);
-    }
-    for (String expansion : option.expansion()) {
-      clearValue(expansion, clearedValues);
+    for (String[] args : new String[][] {option.implicitRequirements(), option.expansion()}) {
+      Iterator<String> argsIterator = Iterators.forArray(args);
+      while (argsIterator.hasNext()) {
+        String arg = argsIterator.next();
+        ParseOptionResult parseOptionResult = parseOption(arg, argsIterator);
+        clearValue(parseOptionResult.field, parseOptionResult.option, clearedValues);
+      }
     }
   }
 
@@ -532,86 +543,24 @@ class OptionsParserImpl {
     List<String> unparsedArgs = Lists.newArrayList();
     LinkedHashMap<String,List<String>> implicitRequirements = Maps.newLinkedHashMap();
 
-    for (int pos = 0; pos < args.size(); pos++) {
-      String arg = args.get(pos);
+    Iterator<String> argsIterator = args.iterator();
+    while (argsIterator.hasNext()) {
+      String arg = argsIterator.next();
+
       if (!arg.startsWith("-")) {
         unparsedArgs.add(arg);
         continue;  // not an option arg
       }
+
       if (arg.equals("--")) {  // "--" means all remaining args aren't options
-        while (++pos < args.size()) {
-          unparsedArgs.add(args.get(pos));
-        }
+        Iterators.addAll(unparsedArgs, argsIterator);
         break;
       }
 
-      String value = null;
-      Field field;
-      boolean booleanValue = true;
-
-      if (arg.length() == 2) { // -l  (may be nullary or unary)
-        field = optionsData.getFieldForAbbrev(arg.charAt(1));
-        booleanValue = true;
-
-      } else if (arg.length() == 3 && arg.charAt(2) == '-') { // -l-  (boolean)
-        field = optionsData.getFieldForAbbrev(arg.charAt(1));
-        booleanValue = false;
-
-      } else if (allowSingleDashLongOptions // -long_option
-          || arg.startsWith("--")) { // or --long_option
-
-        int equalsAt = arg.indexOf('=');
-        int nameStartsAt = arg.startsWith("--") ? 2 : 1;
-        String name =
-            equalsAt == -1 ? arg.substring(nameStartsAt) : arg.substring(nameStartsAt, equalsAt);
-        if (name.trim().isEmpty()) {
-          throw new OptionsParsingException("Invalid options syntax: " + arg, arg);
-        }
-        value = equalsAt == -1 ? null : arg.substring(equalsAt + 1);
-        field = optionsData.getFieldFromName(name);
-
-        // look for a "no"-prefixed option name: "no<optionname>";
-        // (Undocumented: we also allow --no_foo.  We're generous like that.)
-        if (field == null && name.startsWith("no")) {
-          name = name.substring(name.startsWith("no_") ? 3 : 2);
-          field = optionsData.getFieldFromName(name);
-          booleanValue = false;
-          if (field != null) {
-            // TODO(bazel-team): Add tests for these cases.
-            if (!OptionsParserImpl.isBooleanField(field)) {
-              throw new OptionsParsingException(
-                  "Illegal use of 'no' prefix on non-boolean option: " + arg, arg);
-            }
-            if (value != null) {
-              throw new OptionsParsingException(
-                  "Unexpected value after boolean option: " + arg, arg);
-            }
-            // "no<optionname>" signifies a boolean option w/ false value
-            value = "0";
-          }
-        }
-      } else {
-        throw new OptionsParsingException("Invalid options syntax: " + arg, arg);
-      }
-
-      if (field == null) {
-        throw new OptionsParsingException("Unrecognized option: " + arg, arg);
-      }
-
-      Option option = field.getAnnotation(Option.class);
-
-      if (value == null) {
-        // Special-case boolean to supply value based on presence of "no" prefix.
-        if (OptionsParserImpl.isBooleanField(field)) {
-          value = booleanValue ? "1" : "0";
-        } else if (field.getType().equals(Void.class) && !option.wrapperOption()) {
-          // This is expected, Void type options have no args (unless they're wrapper options).
-        } else if (pos != args.size() - 1) {
-          value = args.get(++pos);  // "--flag value" form
-        } else {
-          throw new OptionsParsingException("Expected value after " + arg);
-        }
-      }
+      ParseOptionResult optionParseResult = parseOption(arg, argsIterator);
+      Field field = optionParseResult.field;
+      Option option = optionParseResult.option;
+      String value = optionParseResult.value;
 
       final String originalName = option.name();
 
@@ -731,6 +680,92 @@ class OptionsParserImpl {
     return unparsedArgs;
   }
 
+  private static final class ParseOptionResult {
+    final Field field;
+    final Option option;
+    final String value;
+
+    ParseOptionResult(Field field, Option option, String value) {
+      this.field = field;
+      this.option = option;
+      this.value = value;
+    }
+  }
+
+  private ParseOptionResult parseOption(String arg, Iterator<String> nextArgs)
+      throws OptionsParsingException {
+
+    String value = null;
+    Field field;
+    boolean booleanValue = true;
+
+    if (arg.length() == 2) { // -l  (may be nullary or unary)
+      field = optionsData.getFieldForAbbrev(arg.charAt(1));
+      booleanValue = true;
+
+    } else if (arg.length() == 3 && arg.charAt(2) == '-') { // -l-  (boolean)
+      field = optionsData.getFieldForAbbrev(arg.charAt(1));
+      booleanValue = false;
+
+    } else if (allowSingleDashLongOptions // -long_option
+        || arg.startsWith("--")) { // or --long_option
+
+      int equalsAt = arg.indexOf('=');
+      int nameStartsAt = arg.startsWith("--") ? 2 : 1;
+      String name =
+          equalsAt == -1 ? arg.substring(nameStartsAt) : arg.substring(nameStartsAt, equalsAt);
+      if (name.trim().isEmpty()) {
+        throw new OptionsParsingException("Invalid options syntax: " + arg, arg);
+      }
+      value = equalsAt == -1 ? null : arg.substring(equalsAt + 1);
+      field = optionsData.getFieldFromName(name);
+
+      // look for a "no"-prefixed option name: "no<optionname>";
+      // (Undocumented: we also allow --no_foo.  We're generous like that.)
+      if (field == null && name.startsWith("no")) {
+        name = name.substring(name.startsWith("no_") ? 3 : 2);
+        field = optionsData.getFieldFromName(name);
+        booleanValue = false;
+        if (field != null) {
+          // TODO(bazel-team): Add tests for these cases.
+          if (!OptionsParserImpl.isBooleanField(field)) {
+            throw new OptionsParsingException(
+                "Illegal use of 'no' prefix on non-boolean option: " + arg, arg);
+          }
+          if (value != null) {
+            throw new OptionsParsingException(
+                "Unexpected value after boolean option: " + arg, arg);
+          }
+          // "no<optionname>" signifies a boolean option w/ false value
+          value = "0";
+        }
+      }
+    } else {
+      throw new OptionsParsingException("Invalid options syntax: " + arg, arg);
+    }
+
+    if (field == null) {
+      throw new OptionsParsingException("Unrecognized option: " + arg, arg);
+    }
+
+    Option option = field.getAnnotation(Option.class);
+
+    if (value == null) {
+      // Special-case boolean to supply value based on presence of "no" prefix.
+      if (OptionsParserImpl.isBooleanField(field)) {
+        value = booleanValue ? "1" : "0";
+      } else if (field.getType().equals(Void.class) && !option.wrapperOption()) {
+        // This is expected, Void type options have no args (unless they're wrapper options).
+      } else if (nextArgs.hasNext()) {
+        value = nextArgs.next();  // "--flag value" form
+      } else {
+        throw new OptionsParsingException("Expected value after " + arg);
+      }
+    }
+
+    return new ParseOptionResult(field, option, value);
+  }
+
   /**
    * Gets the result of parsing the options.
    */