diff options
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); + } } |