Adds an oldName attribute and a wrapperOption attribute to the @Option annotation for the options parser. oldName indicates the old name for the option, and the option will be parsed under both name and oldName. wrapperOption indicates that the option is a wrapper for other options. For example, in "--foo=--bar=baz", --foo wraps --bar=baz. With wrapperOption set to true for --foo, the options parser will "unwrap" --bar=baz and parse them as top-level flags.

--
MOS_MIGRATED_REVID=105924412

4 files changed, 187 insertions, 19 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 ca3add9bec..a62558c4e7 100644
--- a/src/main/java/com/google/devtools/common/options/Option.java
+++ b/src/main/java/com/google/devtools/common/options/Option.java
@@ -130,4 +130,23 @@ public @interface Option {
    * is used.
    */
   String deprecationWarning() default "";
+
+  /**
+   * The old name for this option. If an option has a name "foo" and an old name "bar",
+   * --foo=baz and --bar=baz will be equivalent. If the old name is used, a warning will be printed
+   * indicating that the old name is deprecated and the new name should be used.
+   */
+  String oldName() default "";
+
+  /**
+   * Indicates that this option is a wrapper for other options, and will be unwrapped
+   * when parsed. For example, if foo is a wrapper option, then "--foo=--bar=baz"
+   * will be parsed as the flag "--bar=baz" (rather than --foo taking the value
+   * "--bar=baz"). A wrapper option should have the type {@link Void} (if it is something other
+   * than Void, the parser will not assign a value to it). The 
+   * {@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).
+   */
+  boolean wrapperOption() default false;
 }
diff --git a/src/main/java/com/google/devtools/common/options/OptionsData.java b/src/main/java/com/google/devtools/common/options/OptionsData.java
index 61d798a837..c5fc91e0c8 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsData.java
@@ -260,12 +260,19 @@ final class OptionsData {
           throw new DuplicateOptionDeclarationException(
               "Duplicate option name: --" + annotation.name());
         }
+        if (!annotation.oldName().isEmpty()) {
+          if (nameToFieldBuilder.put(annotation.oldName(), field) != null) {
+            throw new DuplicateOptionDeclarationException(
+                "Old option name duplicates option name: --" + annotation.oldName());
+          }
+        }
         if (annotation.abbrev() != '\0') {
           if (abbrevToFieldBuilder.put(annotation.abbrev(), field) != null) {
             throw new DuplicateOptionDeclarationException(
                   "Duplicate option abbrev: -" + annotation.abbrev());
           }
         }
+
         optionDefaultsBuilder.put(field, retrieveDefaultFromAnnotation(field));
 
         convertersBuilder.put(field, OptionsParserImpl.findConverter(field));
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 59d4a0c524..e2cc6b8ce2 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -379,12 +379,16 @@ class OptionsParserImpl {
     Option option = field.getAnnotation(Option.class);
     // Continue to support the old behavior for @Deprecated options.
     String warning = option.deprecationWarning();
-    if (!warning.equals("") || (field.getAnnotation(Deprecated.class) != null)) {
-      warnings.add("Option '" + option.name() + "' is deprecated"
-          + (warning.equals("") ? "" : ": " + warning));
+    if (!warning.isEmpty() || (field.getAnnotation(Deprecated.class) != null)) {
+      addDeprecationWarning(option.name(), warning);
     }
   }
 
+  private void addDeprecationWarning(String optionName, String warning) {
+    warnings.add("Option '" + optionName + "' is deprecated"
+        + (warning.isEmpty() ? "" : ": " + warning));
+  }
+
   // Warnings should not end with a '.' because the internal reporter adds one automatically.
   private void setValue(Field field, String name, Object value,
       OptionPriority priority, String source, String implicitDependant, String expandedFrom) {
@@ -515,11 +519,16 @@ class OptionsParserImpl {
    * dependent nor an expanded from value, then it must have been explicitly
    * set.
    */
-  private List<String> parse(OptionPriority priority,
-      final Function<? super String, String> sourceFunction, String implicitDependant,
-      String expandedFrom, List<String> args) throws OptionsParsingException {
+  private List<String> parse(
+      OptionPriority priority,
+      Function<? super String, String> sourceFunction,
+      String implicitDependent,
+      String expandedFrom,
+      List<String> args) throws OptionsParsingException {
+
     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);
       if (!arg.startsWith("-")) {
@@ -547,11 +556,12 @@ class OptionsParserImpl {
 
       } 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().equals("")) {
+        if (name.trim().isEmpty()) {
           throw new OptionsParsingException("Invalid options syntax: " + arg, arg);
         }
         value = equalsAt == -1 ? null : arg.substring(equalsAt + 1);
@@ -560,8 +570,8 @@ class OptionsParserImpl {
         // 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")) {
-          String realname = name.substring(name.startsWith("no_") ? 3 : 2);
-          field = optionsData.getFieldFromName(realname);
+          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.
@@ -578,6 +588,15 @@ class OptionsParserImpl {
           }
         }
 
+        // Add a deprecation warning if the old name for the flag is used.
+        if (field != null) {
+          Option option = field.getAnnotation(Option.class);
+          String oldName = option.oldName();
+          if (name.equals(oldName)) {
+            addDeprecationWarning(oldName, "Use --" + option.name() + " instead");
+          }
+        }
+
       } else {
         throw new OptionsParsingException("Invalid options syntax: " + arg, arg);
       }
@@ -585,13 +604,15 @@ class OptionsParserImpl {
       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
+        // 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)) {
-          // this is expected, Void type options have no args
+        } 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 {
@@ -599,9 +620,35 @@ class OptionsParserImpl {
         }
       }
 
-      Option option = field.getAnnotation(Option.class);
       final String originalName = option.name();
-      if (implicitDependant == null) {
+
+      if (option.wrapperOption()) {
+        if (value.startsWith("-")) {
+
+          List<String> unparsed = parse(
+              priority,
+              Functions.constant("Unwrapped from wrapper option --" + originalName),
+              null, // implicitDependent
+              null, // expandedFrom
+              ImmutableList.of(value));
+
+          if (!unparsed.isEmpty()) {
+            throw new OptionsParsingException("Unparsed options remain after unwrapping " +
+              arg + ": " + Joiner.on(' ').join(unparsed));
+          }
+
+          // Don't process implicitRequirements or expansions for wrapper options. In particular,
+          // don't record this option in unparsedValues, so that only the wrapped option shows
+          // up in canonicalized options.
+          continue;
+
+        } else {
+          throw new OptionsParsingException("Invalid --" + originalName + " value format. "
+              + "You may have meant --" + originalName + "=--" + value);
+        }
+      }
+
+      if (implicitDependent == null) {
         // Log explicit options and expanded options in the order they are parsed (can be sorted
         // later). Also remember whether they were expanded or not. This information is needed to
         // correctly canonicalize flags.
@@ -621,7 +668,7 @@ class OptionsParserImpl {
           // Throw an assertion, because this indicates an error in the code that specified the
           // expansion for the current option.
           throw new AssertionError("Unparsed options remain after parsing expansion of " +
-            arg + ":" + Joiner.on(' ').join(unparsed));
+            arg + ": " + Joiner.on(' ').join(unparsed));
         }
       } else {
         Converter<?> converter = optionsData.getConverter(field);
@@ -639,7 +686,7 @@ class OptionsParserImpl {
         // parse(); latest wins:
         if (!option.allowMultiple()) {
           setValue(field, originalName, convertedValue,
-              priority, sourceFunction.apply(originalName), implicitDependant, expandedFrom);
+              priority, sourceFunction.apply(originalName), implicitDependent, expandedFrom);
         } else {
           // But if it's a multiple-use option, then just accumulate the
           // values, in the order in which they were seen.
@@ -647,7 +694,7 @@ class OptionsParserImpl {
           // only makes it available in String form via the signature string
           // for the field declaration.
           addListValue(field, convertedValue, priority, sourceFunction.apply(originalName),
-              implicitDependant, expandedFrom);
+              implicitDependent, expandedFrom);
         }
       }
 
@@ -670,7 +717,7 @@ class OptionsParserImpl {
         if (!unparsed.isEmpty()) {
           // Throw an assertion, because this indicates an error in the code that specified in the
           // implicit requirements for the option(s).
-          throw new AssertionError("Unparsed options remain after parsing implicit options:"
+          throw new AssertionError("Unparsed options remain after parsing implicit options: "
               + Joiner.on(' ').join(unparsed));
         }
       }
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 0bb8a15758..910e59e15f 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
@@ -1024,4 +1024,99 @@ public class OptionsParserTest {
     result = parser.getOptions(LongValueExample.class);
     assertEquals(100, result.longval);
   }
+
+  public static class OldNameExample extends OptionsBase { 
+    @Option(name = "new_name",
+            oldName = "old_name",
+            defaultValue = "defaultValue")
+    public String flag; 
+  }
+
+  @Test
+  public void testOldName() throws OptionsParsingException {
+    OptionsParser parser = newOptionsParser(OldNameExample.class);
+    parser.parse("--old_name=foo");
+    OldNameExample result = parser.getOptions(OldNameExample.class);
+    assertEquals("foo", result.flag);
+    assertThat(parser.getWarnings()).contains(
+        "Option 'old_name' is deprecated: Use --new_name instead");
+
+    // Should also work by its new name.
+    parser = newOptionsParser(OldNameExample.class);
+    parser.parse("--new_name=foo");
+    result = parser.getOptions(OldNameExample.class);
+    assertEquals("foo", result.flag);
+  }
+
+  @Test
+  public void testOldNameCanonicalization() throws Exception {
+    assertEquals(
+        Arrays.asList("--new_name=foo"), canonicalize(OldNameExample.class, "--old_name=foo"));
+  }
+  
+  public static class OldNameConflictExample extends OptionsBase { 
+    @Option(name = "new_name",
+            oldName = "old_name",
+            defaultValue = "defaultValue")
+    public String flag1; 
+    
+    @Option(name = "old_name",
+            defaultValue = "defaultValue")
+    public String flag2;
+  }
+
+  @Test
+  public void testOldNameConflict() {
+    try {
+      newOptionsParser(OldNameConflictExample.class);
+      fail("old_name should conflict with the flag already named old_name");
+    } catch (DuplicateOptionDeclarationException e) {
+      // expected
+    }
+  }
+  
+  public static class WrapperOptionExample extends OptionsBase { 
+    @Option(name = "wrapper",
+            defaultValue = "null",
+            wrapperOption = true)
+    public Void wrapperOption;
+
+    @Option(name = "flag1", defaultValue = "false")
+    public boolean flag1;
+
+    @Option(name = "flag2", defaultValue = "42")
+    public int flag2;
+
+    @Option(name = "flag3", defaultValue = "foo")
+    public String flag3;
+  }
+  
+  @Test
+  public void testWrapperOption() throws OptionsParsingException {
+    OptionsParser parser = newOptionsParser(WrapperOptionExample.class);
+    parser.parse("--wrapper=--flag1=true", "--wrapper=--flag2=87", "--wrapper=--flag3=bar");
+    WrapperOptionExample result = parser.getOptions(WrapperOptionExample.class);
+    assertEquals(true, result.flag1);
+    assertEquals(87, result.flag2);
+    assertEquals("bar", result.flag3);
+  }
+
+  @Test
+  public void testInvalidWrapperOptionFormat() {
+    OptionsParser parser = newOptionsParser(WrapperOptionExample.class);
+    try {
+      parser.parse("--wrapper=foo");
+      fail();
+    } catch (OptionsParsingException e) {
+      // Check that the message looks like it's suggesting the correct format.
+      assertThat(e.getMessage()).contains("--foo");
+    }
+  }
+
+  @Test
+  public void testWrapperCanonicalization() throws OptionsParsingException {
+    List<String> canonicalized = canonicalize(WrapperOptionExample.class,
+        "--wrapper=--flag1=true", "--wrapper=--flag2=87", "--wrapper=--flag3=bar");
+    assertEquals(Arrays.asList("--flag1=true", "--flag2=87", "--flag3=bar"), canonicalized);
+  }
 }