Watch for --no and --no_ flag name conflicts.
Prevent OptionsBases with conflicting names due to --no boolean flag aliases to
successfully combine into parser.
Also remove the comment about --no_ not being documented, since it has been documented since Bazel was open-sourced.
PiperOrigin-RevId: 151738453
diff --git a/src/main/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java b/src/main/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java
index 5c636b8..ad4c975 100644
--- a/src/main/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java
+++ b/src/main/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java
@@ -14,9 +14,7 @@
package com.google.devtools.common.options;
-/**
- * Indicates that an option is declared in more than one class.
- */
+/** Indicates that a flag is declared more than once. */
public class DuplicateOptionDeclarationException extends RuntimeException {
DuplicateOptionDeclarationException(String message) {
diff --git a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
index 27f42f4..60a32a9 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -260,6 +260,45 @@
return convertedValue;
}
+ private static <A> void checkForCollisions(
+ Map<A, Field> aFieldMap,
+ A optionName,
+ String description) {
+ if (aFieldMap.containsKey(optionName)) {
+ throw new DuplicateOptionDeclarationException(
+ "Duplicate option name, due to " + description + ": --" + optionName);
+ }
+ }
+
+ private static void checkForBooleanAliasCollisions(
+ Map<String, String> booleanAliasMap,
+ String optionName,
+ String description) {
+ if (booleanAliasMap.containsKey(optionName)) {
+ throw new DuplicateOptionDeclarationException(
+ "Duplicate option name, due to "
+ + description
+ + " --"
+ + optionName
+ + ", it conflicts with a negating alias for boolean flag --"
+ + booleanAliasMap.get(optionName));
+ }
+ }
+
+ private static void checkAndUpdateBooleanAliases(
+ Map<String, Field> nameToFieldMap,
+ Map<String, String> booleanAliasMap,
+ String optionName) {
+ // Check that the two aliases do not conflict with existing flags.
+ checkForCollisions(nameToFieldMap, "no_" + optionName, "boolean option alias");
+ checkForCollisions(nameToFieldMap, "no" + optionName, "boolean option alias");
+
+ // Record that the boolean option takes up additional namespace for its two negating
+ // aliases.
+ booleanAliasMap.put("no_" + optionName, optionName);
+ booleanAliasMap.put("no" + optionName, optionName);
+ }
+
/**
* Constructs an {@link IsolatedOptionsData} object for a parser that knows about the given
* {@link OptionsBase} classes. No inter-option analysis is done. Performs basic sanity checking
@@ -274,6 +313,9 @@
Map<Field, Converter<?>> convertersBuilder = Maps.newHashMap();
Map<Field, Boolean> allowMultipleBuilder = Maps.newHashMap();
+ // Maps the negated boolean flag aliases to the original option name.
+ Map<String, String> booleanAliasMap = Maps.newHashMap();
+
// Read all Option annotations:
for (Class<? extends OptionsBase> parsedOptionsClass : classes) {
try {
@@ -289,8 +331,8 @@
for (Field field : fields) {
Option annotation = field.getAnnotation(Option.class);
-
- if (annotation.name() == null) {
+ String optionName = annotation.name();
+ if (optionName == null) {
throw new AssertionError("Option cannot have a null name");
}
@@ -326,7 +368,7 @@
Type elementType =
((ParameterizedType) converterResultType).getActualTypeArguments()[0];
if (!GenericTypeHelper.isAssignableFrom(fieldType, elementType)) {
- throw new AssertionError("If the converter return type of a multiple occurance " +
+ throw new AssertionError("If the converter return type of a multiple occurrence " +
"option is a list, then the type of list elements (" + fieldType + ") must be " +
"assignable from the converter list element type (" + elementType + ")");
}
@@ -345,21 +387,28 @@
}
}
- if (nameToFieldBuilder.put(annotation.name(), field) != null) {
- throw new DuplicateOptionDeclarationException(
- "Duplicate option name: --" + annotation.name());
+ if (isBooleanField(field)) {
+ checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, optionName);
}
+
+ checkForCollisions(nameToFieldBuilder, optionName, "option");
+ checkForBooleanAliasCollisions(booleanAliasMap, optionName, "option");
+ nameToFieldBuilder.put(optionName, field);
+
if (!annotation.oldName().isEmpty()) {
- if (nameToFieldBuilder.put(annotation.oldName(), field) != null) {
- throw new DuplicateOptionDeclarationException(
- "Old option name duplicates option name: --" + annotation.oldName());
+ String oldName = annotation.oldName();
+ checkForCollisions(nameToFieldBuilder, oldName, "old option name");
+ checkForBooleanAliasCollisions(booleanAliasMap, oldName, "old option name");
+ nameToFieldBuilder.put(annotation.oldName(), field);
+
+ // If boolean, repeat the alias dance for the old name.
+ if (isBooleanField(field)) {
+ checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, oldName);
}
}
if (annotation.abbrev() != '\0') {
- if (abbrevToFieldBuilder.put(annotation.abbrev(), field) != null) {
- throw new DuplicateOptionDeclarationException(
- "Duplicate option abbrev: -" + annotation.abbrev());
- }
+ checkForCollisions(abbrevToFieldBuilder, annotation.abbrev(), "option abbreviation");
+ abbrevToFieldBuilder.put(annotation.abbrev(), field);
}
optionDefaultsBuilder.put(field, retrieveDefaultFromAnnotation(field));
@@ -379,4 +428,5 @@
convertersBuilder,
allowMultipleBuilder);
}
+
}
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 a818ec4..aeb0da0 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -636,8 +636,7 @@
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.)
+ // Look for a "no"-prefixed option name: "no<optionName>" or "no_<optionName>".
if (field == null && name.startsWith("no")) {
name = name.substring(name.startsWith("no_") ? 3 : 2);
field = optionsData.getFieldFromName(name);
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 b8f669e..69b4c96 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
@@ -1564,6 +1564,136 @@
Arrays.asList("--new_name=foo"), canonicalize(OldNameExample.class, "--old_name=foo"));
}
+ public static class ExampleNameConflictOptions extends OptionsBase {
+ @Option(name = "foo", defaultValue = "1")
+ public int foo;
+
+ @Option(name = "foo", defaultValue = "I should conflict with foo")
+ public String anotherFoo;
+ }
+
+ @Test
+ public void testNameConflictInSingleClass() {
+ try {
+ newOptionsParser(ExampleNameConflictOptions.class);
+ fail("foo should conflict with the previous flag foo");
+ } catch (DuplicateOptionDeclarationException e) {
+ // Expected, check that the error message gives useful information.
+ assertThat(e.getMessage()).contains("--foo");
+ }
+ }
+
+ public static class ExampleBooleanFooOptions extends OptionsBase {
+ @Option(name = "foo", defaultValue = "false")
+ public boolean foo;
+ }
+
+ @Test
+ public void testNameConflictInTwoClasses() {
+ try {
+ newOptionsParser(ExampleFoo.class, ExampleBooleanFooOptions.class);
+ fail("foo should conflict with the previous flag foo");
+ } catch (DuplicateOptionDeclarationException e) {
+ // Expected, check that the error message gives useful information.
+ assertThat(e.getMessage()).contains("--foo");
+ }
+ }
+
+ public static class ExamplePrefixFooOptions extends OptionsBase {
+ @Option(name = "nofoo", defaultValue = "false")
+ public boolean noFoo;
+ }
+
+ @Test
+ public void testBooleanPrefixNameConflict() {
+ // Try the same test in both orders, the parser should fail if the overlapping flag is defined
+ // before or after the boolean flag introduces the alias.
+ try {
+ newOptionsParser(ExampleBooleanFooOptions.class, ExamplePrefixFooOptions.class);
+ fail("nofoo should conflict with the previous flag foo, since foo, as a boolean flag, "
+ + "can be written as --nofoo");
+ } catch (DuplicateOptionDeclarationException e) {
+ // Expected, check that the error message gives useful information.
+ assertThat(e.getMessage()).contains("--nofoo");
+ }
+
+ try {
+ newOptionsParser(ExamplePrefixFooOptions.class, ExampleBooleanFooOptions.class);
+ fail("nofoo should conflict with the previous flag foo, since foo, as a boolean flag, "
+ + "can be written as --nofoo");
+ } catch (DuplicateOptionDeclarationException e) {
+ // Expected, check that the error message gives useful information.
+ assertThat(e.getMessage()).contains("--nofoo");
+ }
+ }
+
+ public static class ExampleUnderscorePrefixFooOptions extends OptionsBase {
+ @Option(name = "no_foo", defaultValue = "false")
+ public boolean noFoo;
+ }
+
+ @Test
+ public void testBooleanUnderscorePrefixNameConflict() {
+ // Try the same test in both orders, the parser should fail if the overlapping flag is defined
+ // before or after the boolean flag introduces the alias.
+ try {
+ newOptionsParser(ExampleBooleanFooOptions.class, ExampleUnderscorePrefixFooOptions.class);
+ fail("no_foo should conflict with the previous flag foo, since foo, as a boolean flag, "
+ + "can be written as --no_foo");
+ } catch (DuplicateOptionDeclarationException e) {
+ // Expected, check that the error message gives useful information.
+ assertThat(e.getMessage()).contains("--no_foo");
+ }
+
+ try {
+ newOptionsParser(ExampleUnderscorePrefixFooOptions.class, ExampleBooleanFooOptions.class);
+ fail("no_foo should conflict with the previous flag foo, since foo, as a boolean flag, "
+ + "can be written as --no_foo");
+ } catch (DuplicateOptionDeclarationException e) {
+ // Expected, check that the error message gives useful information.
+ assertThat(e.getMessage()).contains("--no_foo");
+ }
+ }
+
+ public static class ExampleBarWasNamedFooOption extends OptionsBase {
+ @Option(name = "bar", oldName = "foo", defaultValue = "false")
+ public boolean bar;
+ }
+
+ @Test
+ public void testBooleanAliasWithOldNameConflict() {
+ // Try the same test in both orders, the parser should fail if the overlapping flag is defined
+ // before or after the boolean flag introduces the alias.
+ try {
+ newOptionsParser(ExamplePrefixFooOptions.class, ExampleBarWasNamedFooOption.class);
+ fail("nofoo should conflict with the previous flag foo, since foo, as a boolean flag, "
+ + "can be written as --nofoo");
+ } catch (DuplicateOptionDeclarationException e) {
+ // Expected, check that the error message gives useful information.
+ assertThat(e.getMessage()).contains("--nofoo");
+ }
+ }
+
+
+ public static class ExampleBarWasNamedNoFooOption extends OptionsBase {
+ @Option(name = "bar", oldName = "nofoo", defaultValue = "false")
+ public boolean bar;
+ }
+
+ @Test
+ public void testBooleanWithOldNameAsAliasOfBooleanConflict() {
+ // Try the same test in both orders, the parser should fail if the overlapping flag is defined
+ // before or after the boolean flag introduces the alias.
+ try {
+ newOptionsParser(ExampleBooleanFooOptions.class, ExampleBarWasNamedNoFooOption.class);
+ fail("nofoo, the old name for bar, should conflict with the previous flag foo, since foo, "
+ + "as a boolean flag, can be written as --nofoo");
+ } catch (DuplicateOptionDeclarationException e) {
+ // Expected, check that the error message gives useful information.
+ assertThat(e.getMessage()).contains("--nofoo");
+ }
+ }
+
public static class OldNameConflictExample extends OptionsBase {
@Option(name = "new_name",
oldName = "old_name",
