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

4 files changed, 195 insertions, 18 deletions

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 5c636b8a7d..ad4c975713 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 27f42f48b1..60a32a907f 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 @@ class IsolatedOptionsData extends OpaqueOptionsData {
     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 @@ class IsolatedOptionsData extends OpaqueOptionsData {
     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 @@ class IsolatedOptionsData extends OpaqueOptionsData {
 
       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 @@ class IsolatedOptionsData extends OpaqueOptionsData {
             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 @@ class IsolatedOptionsData extends OpaqueOptionsData {
           }
         }
 
-        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 @@ class IsolatedOptionsData extends OpaqueOptionsData {
         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 a818ec477f..aeb0da08f9 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 @@ class OptionsParserImpl {
       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 b8f669e1d5..69b4c9647e 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 @@ public class OptionsParserTest {
         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",