Options with oldNames will no longer get reported twice in the effective option lists.

Tracking the names together for option identification was useful, but then the same list was being used as the source of options for the parser, which lead to some options being listed twice.

Also complete a few tests that should have already been tested in different orders.

PiperOrigin-RevId: 168024719

10 files changed, 238 insertions, 76 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java b/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java
index 6c42c207c2..849e18cc92 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java
@@ -157,7 +157,8 @@ public class AllIncompatibleChangesExpansion implements ExpansionFunction {
     // Grab all registered options that are identified as incompatible changes by either name or
     // by category. Ensure they satisfy our requirements.
     ArrayList<String> incompatibleChanges = new ArrayList<>();
-    for (Map.Entry<String, OptionDefinition> entry : context.getOptionsData().getAllNamedFields()) {
+    for (Map.Entry<String, OptionDefinition> entry :
+        context.getOptionsData().getAllOptionDefinitions()) {
       OptionDefinition optionDefinition = entry.getValue();
       if (optionDefinition.getOptionName().startsWith(INCOMPATIBLE_NAME_PREFIX)
           || optionDefinition.getOptionCategory().equals(INCOMPATIBLE_CATEGORY)) {
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 ad4c975713..0f4dc080f5 100644
--- a/src/main/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java
+++ b/src/main/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java
@@ -15,7 +15,7 @@
 package com.google.devtools.common.options;
 
 /** Indicates that a flag is declared more than once. */
-public class DuplicateOptionDeclarationException extends RuntimeException {
+public class DuplicateOptionDeclarationException extends Exception {
 
   DuplicateOptionDeclarationException(String message) {
     super(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 d1e7ba30a9..57b4d232c1 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableMap;
 import com.google.devtools.common.options.OptionDefinition.NotAnOptionException;
 import com.google.devtools.common.options.OptionsParser.ConstructionException;
 import java.lang.reflect.Constructor;
-import java.lang.reflect.Field;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
@@ -92,6 +91,14 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
    */
   private final ImmutableMap<String, OptionDefinition> nameToField;
 
+  /**
+   * For options that have an "OldName", this is a mapping from old name to its corresponding {@code
+   * OptionDefinition}. Entries appear ordered first by their options class (the order in which they
+   * were passed to {@link #from(Collection)}, and then in alphabetic order within each options
+   * class.
+   */
+  private final ImmutableMap<String, OptionDefinition> oldNameToField;
+
   /** Mapping from option abbreviation to {@code OptionDefinition} (unordered). */
   private final ImmutableMap<Character, OptionDefinition> abbrevToField;
 
@@ -105,10 +112,12 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
   private IsolatedOptionsData(
       Map<Class<? extends OptionsBase>, Constructor<?>> optionsClasses,
       Map<String, OptionDefinition> nameToField,
+      Map<String, OptionDefinition> oldNameToField,
       Map<Character, OptionDefinition> abbrevToField,
       Map<Class<? extends OptionsBase>, Boolean> usesOnlyCoreTypes) {
     this.optionsClasses = ImmutableMap.copyOf(optionsClasses);
     this.nameToField = ImmutableMap.copyOf(nameToField);
+    this.oldNameToField = ImmutableMap.copyOf(oldNameToField);
     this.abbrevToField = ImmutableMap.copyOf(abbrevToField);
     this.usesOnlyCoreTypes = ImmutableMap.copyOf(usesOnlyCoreTypes);
   }
@@ -117,6 +126,7 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     this(
         other.optionsClasses,
         other.nameToField,
+        other.oldNameToField,
         other.abbrevToField,
         other.usesOnlyCoreTypes);
   }
@@ -134,16 +144,20 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     return (Constructor<T>) optionsClasses.get(clazz);
   }
 
-  public OptionDefinition getFieldFromName(String name) {
-    return nameToField.get(name);
+  /**
+   * Returns the option in this parser by the provided name, or {@code null} if none is found. This
+   * will match both the canonical name of an option, and any old name listed that we still accept.
+   */
+  public OptionDefinition getOptionDefinitionFromName(String name) {
+    return nameToField.getOrDefault(name, oldNameToField.get(name));
   }
 
   /**
-   * Returns all pairs of option names (not field names) and their corresponding {@link Field}
-   * objects. Entries appear ordered first by their options class (the order in which they were
-   * passed to {@link #from(Collection)}, and then in alphabetic order within each options class.
+   * Returns all {@link OptionDefinition} objects loaded, mapped by their canonical names. Entries
+   * appear ordered first by their options class (the order in which they were passed to {@link
+   * #from(Collection)}, and then in alphabetic order within each options class.
    */
-  public Iterable<Map.Entry<String, OptionDefinition>> getAllNamedFields() {
+  public Iterable<Map.Entry<String, OptionDefinition>> getAllOptionDefinitions() {
     return nameToField.entrySet();
   }
 
@@ -154,18 +168,27 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
   public boolean getUsesOnlyCoreTypes(Class<? extends OptionsBase> optionsClass) {
     return usesOnlyCoreTypes.get(optionsClass);
   }
+
+  /**
+   * Generic method to check for collisions between the names we give options. Useful for checking
+   * both single-character abbreviations and full names.
+   */
   private static <A> void checkForCollisions(
-      Map<A, OptionDefinition> aFieldMap, A optionName, String description) {
+      Map<A, OptionDefinition> aFieldMap, A optionName, String description)
+      throws DuplicateOptionDeclarationException {
     if (aFieldMap.containsKey(optionName)) {
       throw new DuplicateOptionDeclarationException(
           "Duplicate option name, due to " + description + ": --" + optionName);
     }
   }
 
+  /**
+   * All options, even non-boolean ones, should check that they do not conflict with previously
+   * loaded boolean options.
+   */
   private static void checkForBooleanAliasCollisions(
-      Map<String, String> booleanAliasMap,
-      String optionName,
-      String description) {
+      Map<String, String> booleanAliasMap, String optionName, String description)
+      throws DuplicateOptionDeclarationException {
     if (booleanAliasMap.containsKey(optionName)) {
       throw new DuplicateOptionDeclarationException(
           "Duplicate option name, due to "
@@ -177,12 +200,20 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     }
   }
 
+  /**
+   * For an {@code option} of boolean type, this checks that the boolean alias does not conflict
+   * with other names, and adds the boolean alias to a list so that future flags can find if they
+   * conflict with a boolean alias..
+   */
   private static void checkAndUpdateBooleanAliases(
       Map<String, OptionDefinition> nameToFieldMap,
+      Map<String, OptionDefinition> oldNameToFieldMap,
       Map<String, String> booleanAliasMap,
-      String optionName) {
+      String optionName)
+      throws DuplicateOptionDeclarationException {
     // Check that the negating alias does not conflict with existing flags.
     checkForCollisions(nameToFieldMap, "no" + optionName, "boolean option alias");
+    checkForCollisions(oldNameToFieldMap, "no" + optionName, "boolean option alias");
 
     // Record that the boolean option takes up additional namespace for its negating alias.
     booleanAliasMap.put("no" + optionName, optionName);
@@ -197,6 +228,7 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     // Mind which fields have to preserve order.
     Map<Class<? extends OptionsBase>, Constructor<?>> constructorBuilder = new LinkedHashMap<>();
     Map<String, OptionDefinition> nameToFieldBuilder = new LinkedHashMap<>();
+    Map<String, OptionDefinition> oldNameToFieldBuilder = new LinkedHashMap<>();
     Map<Character, OptionDefinition> abbrevToFieldBuilder = new HashMap<>();
 
     // Maps the negated boolean flag aliases to the original option name.
@@ -219,30 +251,46 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
           getAllOptionDefinitionsForClass(parsedOptionsClass);
 
       for (OptionDefinition optionDefinition : optionDefinitions) {
-        String optionName = optionDefinition.getOptionName();
-        checkForCollisions(nameToFieldBuilder, optionName, "option");
-
-        if (optionDefinition.isBooleanField()) {
-          checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, optionName);
-        }
-        checkForBooleanAliasCollisions(booleanAliasMap, optionName, "option");
-        nameToFieldBuilder.put(optionName, optionDefinition);
-
-        if (!optionDefinition.getOldOptionName().isEmpty()) {
-          String oldName = optionDefinition.getOldOptionName();
-          checkForCollisions(nameToFieldBuilder, oldName, "old option name");
-          checkForBooleanAliasCollisions(booleanAliasMap, oldName, "old option name");
-          nameToFieldBuilder.put(optionDefinition.getOldOptionName(), optionDefinition);
-
-          // If boolean, repeat the alias dance for the old name.
+        try {
+          String optionName = optionDefinition.getOptionName();
+          checkForCollisions(nameToFieldBuilder, optionName, "option name collision");
+          checkForCollisions(
+              oldNameToFieldBuilder,
+              optionName,
+              "option name collision with another option's old name");
+          checkForBooleanAliasCollisions(booleanAliasMap, optionName, "option");
           if (optionDefinition.isBooleanField()) {
-            checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, oldName);
+            checkAndUpdateBooleanAliases(
+                nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap, optionName);
           }
-        }
-        if (optionDefinition.getAbbreviation() != '\0') {
-          checkForCollisions(
-              abbrevToFieldBuilder, optionDefinition.getAbbreviation(), "option abbreviation");
-          abbrevToFieldBuilder.put(optionDefinition.getAbbreviation(), optionDefinition);
+          nameToFieldBuilder.put(optionName, optionDefinition);
+
+          if (!optionDefinition.getOldOptionName().isEmpty()) {
+            String oldName = optionDefinition.getOldOptionName();
+            checkForCollisions(
+                nameToFieldBuilder,
+                oldName,
+                "old option name collision with another option's canonical name");
+            checkForCollisions(
+                oldNameToFieldBuilder,
+                oldName,
+                "old option name collision with another old option name");
+            checkForBooleanAliasCollisions(booleanAliasMap, oldName, "old option name");
+            // If boolean, repeat the alias dance for the old name.
+            if (optionDefinition.isBooleanField()) {
+              checkAndUpdateBooleanAliases(
+                  nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap, oldName);
+            }
+            // Now that we've checked for conflicts, confidently store the old name.
+            oldNameToFieldBuilder.put(oldName, optionDefinition);
+          }
+          if (optionDefinition.getAbbreviation() != '\0') {
+            checkForCollisions(
+                abbrevToFieldBuilder, optionDefinition.getAbbreviation(), "option abbreviation");
+            abbrevToFieldBuilder.put(optionDefinition.getAbbreviation(), optionDefinition);
+          }
+        } catch (DuplicateOptionDeclarationException e) {
+          throw new ConstructionException(e);
         }
       }
 
@@ -271,6 +319,7 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     return new IsolatedOptionsData(
         constructorBuilder,
         nameToFieldBuilder,
+        oldNameToFieldBuilder,
         abbrevToFieldBuilder,
         usesOnlyCoreTypesBuilder);
   }
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 d29af3df39..5b9a436337 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsData.java
@@ -128,7 +128,7 @@ final class OptionsData extends IsolatedOptionsData {
     // All that's left is to compute expansions.
     ImmutableMap.Builder<OptionDefinition, ExpansionData> expansionDataBuilder =
         ImmutableMap.<OptionDefinition, ExpansionData>builder();
-    for (Map.Entry<String, OptionDefinition> entry : isolatedData.getAllNamedFields()) {
+    for (Map.Entry<String, OptionDefinition> entry : isolatedData.getAllOptionDefinitions()) {
       OptionDefinition optionDefinition = entry.getValue();
       // Determine either the hard-coded expansion, or the ExpansionFunction class. The
       // OptionProcessor checks at compile time that these aren't used together.
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 618e0ddafa..6ad7bb2185 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -16,6 +16,7 @@ package com.google.devtools.common.options;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ListMultimap;
@@ -126,6 +127,7 @@ public class OptionsParser implements OptionsProvider {
       try {
         result = OptionsData.from(immutableOptionsClasses);
       } catch (Exception e) {
+        Throwables.throwIfInstanceOf(e, ConstructionException.class);
         throw new ConstructionException(e.getMessage(), e);
       }
       optionsData.put(immutableOptionsClasses, result);
@@ -425,7 +427,6 @@ public class OptionsParser implements OptionsProvider {
     public String getName() {
       return name;
     }
-
     OptionDefinition getOptionDefinition() {
       return optionDefinition;
     }
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 993fbbe500..6ae8170bf3 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -168,7 +168,7 @@ class OptionsParserImpl {
    */
   List<OptionValueDescription> asListOfEffectiveOptions() {
     List<OptionValueDescription> result = new ArrayList<>();
-    for (Map.Entry<String, OptionDefinition> mapEntry : optionsData.getAllNamedFields()) {
+    for (Map.Entry<String, OptionDefinition> mapEntry : optionsData.getAllOptionDefinitions()) {
       String fieldName = mapEntry.getKey();
       OptionDefinition optionDefinition = mapEntry.getValue();
       OptionValueDescription entry = parsedValues.get(optionDefinition);
@@ -302,7 +302,7 @@ class OptionsParserImpl {
 
   OptionValueDescription clearValue(String optionName)
       throws OptionsParsingException {
-    OptionDefinition optionDefinition = optionsData.getFieldFromName(optionName);
+    OptionDefinition optionDefinition = optionsData.getOptionDefinitionFromName(optionName);
     if (optionDefinition == null) {
       throw new IllegalArgumentException("No such option '" + optionName + "'");
     }
@@ -313,7 +313,7 @@ class OptionsParserImpl {
   }
 
   OptionValueDescription getOptionValueDescription(String name) {
-    OptionDefinition optionDefinition = optionsData.getFieldFromName(name);
+    OptionDefinition optionDefinition = optionsData.getOptionDefinitionFromName(name);
     if (optionDefinition == null) {
       throw new IllegalArgumentException("No such option '" + name + "'");
     }
@@ -321,7 +321,7 @@ class OptionsParserImpl {
   }
 
   OptionDescription getOptionDescription(String name) throws OptionsParsingException {
-    OptionDefinition optionDefinition = optionsData.getFieldFromName(name);
+    OptionDefinition optionDefinition = optionsData.getOptionDefinitionFromName(name);
     if (optionDefinition == null) {
       return null;
     }
@@ -370,7 +370,7 @@ class OptionsParserImpl {
   ImmutableList<OptionValueDescription> getExpansionOptionValueDescriptions(
       String flagName, @Nullable String flagValue) throws OptionsParsingException {
     ImmutableList.Builder<OptionValueDescription> builder = ImmutableList.builder();
-    OptionDefinition optionDefinition = optionsData.getFieldFromName(flagName);
+    OptionDefinition optionDefinition = optionsData.getOptionDefinitionFromName(flagName);
 
     ImmutableList<String> options = optionsData.getEvaluatedExpansion(optionDefinition, flagValue);
     Iterator<String> optionsIterator = options.iterator();
@@ -393,7 +393,7 @@ class OptionsParserImpl {
   }
 
   boolean containsExplicitOption(String name) {
-    OptionDefinition optionDefinition = optionsData.getFieldFromName(name);
+    OptionDefinition optionDefinition = optionsData.getOptionDefinitionFromName(name);
     if (optionDefinition == null) {
       throw new IllegalArgumentException("No such option '" + name + "'");
     }
@@ -633,12 +633,12 @@ class OptionsParserImpl {
         throw new OptionsParsingException("Invalid options syntax: " + arg, arg);
       }
       value = equalsAt == -1 ? null : arg.substring(equalsAt + 1);
-      optionDefinition = optionsData.getFieldFromName(name);
+      optionDefinition = optionsData.getOptionDefinitionFromName(name);
 
       // Look for a "no"-prefixed option name: "no<optionName>".
       if (optionDefinition == null && name.startsWith("no")) {
         name = name.substring(2);
-        optionDefinition = optionsData.getFieldFromName(name);
+        optionDefinition = optionsData.getOptionDefinitionFromName(name);
         booleanValue = false;
         if (optionDefinition != null) {
           // TODO(bazel-team): Add tests for these cases.
diff --git a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
index 648b271a0f..4a8ab371dc 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
@@ -19,6 +19,7 @@ import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.truth.Correspondence;
+import com.google.devtools.common.options.OptionsParser.ConstructionException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -77,8 +78,11 @@ public class OptionsDataTest {
     try {
       construct(ExampleNameConflictOptions.class);
       fail("foo should conflict with the previous flag foo");
-    } catch (DuplicateOptionDeclarationException e) {
-      assertThat(e).hasMessageThat().contains("Duplicate option name, due to option: --foo");
+    } catch (ConstructionException e) {
+      assertThat(e).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
+      assertThat(e)
+          .hasMessageThat()
+          .contains("Duplicate option name, due to option name collision: --foo");
     }
   }
 
@@ -109,13 +113,16 @@ public class OptionsDataTest {
     try {
       construct(ExampleIntegerFooOptions.class, ExampleBooleanFooOptions.class);
       fail("foo should conflict with the previous flag foo");
-    } catch (DuplicateOptionDeclarationException e) {
-      assertThat(e).hasMessageThat().contains("Duplicate option name, due to option: --foo");
+    } catch (ConstructionException e) {
+      assertThat(e).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
+      assertThat(e)
+          .hasMessageThat()
+          .contains("Duplicate option name, due to option name collision: --foo");
     }
   }
 
   /** Dummy options class. */
-  public static class ExamplePrefixFooOptions extends OptionsBase {
+  public static class ExamplePrefixedFooOptions extends OptionsBase {
     @Option(
       name = "nofoo",
       documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
@@ -130,10 +137,11 @@ public class OptionsDataTest {
     // 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 {
-      construct(ExampleBooleanFooOptions.class, ExamplePrefixFooOptions.class);
+      construct(ExampleBooleanFooOptions.class, ExamplePrefixedFooOptions.class);
       fail("nofoo should conflict with the previous flag foo, "
          + "since foo, as a boolean flag, can be written as --nofoo");
-    } catch (DuplicateOptionDeclarationException e) {
+    } catch (ConstructionException e) {
+      assertThat(e).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
       assertThat(e)
           .hasMessageThat()
           .contains(
@@ -142,10 +150,12 @@ public class OptionsDataTest {
     }
 
     try {
-      construct(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) {
+      construct(ExamplePrefixedFooOptions.class, ExampleBooleanFooOptions.class);
+      fail(
+          "option nofoo should conflict with the previous flag foo, "
+              + "since foo, as a boolean flag, can be written as --nofoo");
+    } catch (ConstructionException e) {
+      assertThat(e).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
       assertThat(e)
           .hasMessageThat()
           .contains("Duplicate option name, due to boolean option alias: --nofoo");
@@ -169,14 +179,30 @@ public class OptionsDataTest {
     // 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 {
-      construct(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) {
+      construct(ExamplePrefixedFooOptions.class, ExampleBarWasNamedFooOption.class);
+      fail(
+          "bar has old name foo, which is a boolean flag and can be named as nofoo, so it "
+              + "should conflict with the previous option --nofoo");
+    } catch (ConstructionException e) {
+      assertThat(e).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
       assertThat(e)
           .hasMessageThat()
           .contains("Duplicate option name, due to boolean option alias: --nofoo");
     }
+
+    try {
+      construct(ExampleBarWasNamedFooOption.class, ExamplePrefixedFooOptions.class);
+      fail(
+          "nofoo should conflict with the previous flag bar that has old name foo, "
+              + "since foo, as a boolean flag, can be written as --nofoo");
+    } catch (ConstructionException e) {
+      assertThat(e).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
+      assertThat(e)
+          .hasMessageThat()
+          .contains(
+              "Duplicate option name, due to option --nofoo, it conflicts with a negating "
+                  + "alias for boolean flag --foo");
+    }
   }
 
   /** Dummy options class. */
@@ -197,19 +223,60 @@ public class OptionsDataTest {
     // before or after the boolean flag introduces the alias.
     try {
       construct(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) {
+      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 (ConstructionException e) {
+      assertThat(e).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
       assertThat(e)
           .hasMessageThat()
           .contains(
               "Duplicate option name, due to old option name --nofoo, it conflicts with a "
                   + "negating alias for boolean flag --foo");
     }
+
+    try {
+      construct(ExampleBarWasNamedNoFooOption.class, ExampleBooleanFooOptions.class);
+      fail(
+          "foo, as a boolean flag, can be written as --nofoo and should conflict with the "
+              + "previous option bar that has old name nofoo");
+    } catch (ConstructionException e) {
+      assertThat(e).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
+      assertThat(e)
+          .hasMessageThat()
+          .contains("Duplicate option name, due to boolean option alias: --nofoo");
+    }
   }
 
   /** Dummy options class. */
-  public static class OldNameConflictExample extends OptionsBase {
+  public static class ExampleFooBooleanConflictsWithOwnOldName extends OptionsBase {
+    @Option(
+      name = "nofoo",
+      oldName = "foo",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.NO_OP},
+      defaultValue = "false"
+    )
+    public boolean foo;
+  }
+
+  @Test
+  public void testSelfConflictBooleanAliases() {
+    // 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 {
+      construct(ExampleFooBooleanConflictsWithOwnOldName.class);
+      fail("foo, the old name for boolean option nofoo, should conflict with its own new name.");
+    } catch (ConstructionException e) {
+      assertThat(e).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
+      assertThat(e)
+          .hasMessageThat()
+          .contains("Duplicate option name, due to boolean option alias: --nofoo");
+    }
+  }
+
+  /** Dummy options class. */
+  public static class OldNameToCanonicalNameConflictExample extends OptionsBase {
     @Option(
       name = "new_name",
       oldName = "old_name",
@@ -229,11 +296,53 @@ public class OptionsDataTest {
   }
 
   @Test
-  public void testOldNameConflict() {
+  public void testOldNameToCanonicalNameConflict() {
+    try {
+      construct(OldNameToCanonicalNameConflictExample.class);
+      fail("old_name should conflict with the flag already named old_name");
+    } catch (ConstructionException expected) {
+      assertThat(expected).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
+      assertThat(expected)
+          .hasMessageThat()
+          .contains(
+              "Duplicate option name, due to option name collision with another option's old name:"
+                  + " --old_name");
+    }
+  }
+
+  /** Dummy options class. */
+  public static class OldNameConflictExample extends OptionsBase {
+    @Option(
+      name = "new_name",
+      oldName = "old_name",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.NO_OP},
+      defaultValue = "defaultValue"
+    )
+    public String flag1;
+
+    @Option(
+      name = "another_name",
+      oldName = "old_name",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.NO_OP},
+      defaultValue = "defaultValue"
+    )
+    public String flag2;
+  }
+
+  @Test
+  public void testOldNameToOldNameConflict() {
     try {
       construct(OldNameConflictExample.class);
       fail("old_name should conflict with the flag already named old_name");
-    } catch (DuplicateOptionDeclarationException expected) {
+    } catch (ConstructionException expected) {
+      assertThat(expected).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class);
+      assertThat(expected)
+          .hasMessageThat()
+          .contains(
+              "Duplicate option name, due to old option name collision with another "
+                  + "old option name: --old_name");
     }
   }
 
@@ -357,7 +466,7 @@ public class OptionsDataTest {
         EndOfAlphabetOptions.class,
         ReverseOrderedOptions.class);
     ArrayList<String> names = new ArrayList<>();
-    for (Map.Entry<String, OptionDefinition> entry : data.getAllNamedFields()) {
+    for (Map.Entry<String, OptionDefinition> entry : data.getAllOptionDefinitions()) {
       names.add(entry.getKey());
     }
     assertThat(names).containsExactly(
@@ -416,7 +525,8 @@ public class OptionsDataTest {
     IsolatedOptionsData data = construct(class1, class2, class3);
     ArrayList<OptionDefinition> optionDefinitionsFromData =
         new ArrayList<>(optionDefinitions.size());
-    data.getAllNamedFields().forEach(entry -> optionDefinitionsFromData.add(entry.getValue()));
+    data.getAllOptionDefinitions()
+        .forEach(entry -> optionDefinitionsFromData.add(entry.getValue()));
 
     ReferenceEqualityCorrespondence referenceEquality = new ReferenceEqualityCorrespondence();
     assertThat(optionDefinitionsFromData)
@@ -430,13 +540,13 @@ public class OptionsDataTest {
     ArrayList<OptionDefinition> optionDefinitionsFromGroupedData =
         new ArrayList<>(optionDefinitions.size());
     data1
-        .getAllNamedFields()
+        .getAllOptionDefinitions()
         .forEach(entry -> optionDefinitionsFromGroupedData.add(entry.getValue()));
     data2
-        .getAllNamedFields()
+        .getAllOptionDefinitions()
         .forEach(entry -> optionDefinitionsFromGroupedData.add(entry.getValue()));
     data3
-        .getAllNamedFields()
+        .getAllOptionDefinitions()
         .forEach(entry -> optionDefinitionsFromGroupedData.add(entry.getValue()));
 
     assertThat(optionDefinitionsFromGroupedData)
diff --git a/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java b/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java
index 30e115b80b..b96e57aa8b 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java
@@ -43,7 +43,7 @@ public class OptionsMapConversionTest {
     OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
     Map<Field, Object> result = new LinkedHashMap<>();
     for (Map.Entry<String, Object> entry : map.entrySet()) {
-      OptionDefinition optionDefinition = data.getFieldFromName(entry.getKey());
+      OptionDefinition optionDefinition = data.getOptionDefinitionFromName(entry.getKey());
       result.put(optionDefinition.getField(), entry.getValue());
     }
     return result;
diff --git a/src/test/java/com/google/devtools/common/options/OptionsUsageTest.java b/src/test/java/com/google/devtools/common/options/OptionsUsageTest.java
index c81af7d2dc..6e665b47b2 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsUsageTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsUsageTest.java
@@ -37,13 +37,14 @@ public final class OptionsUsageTest {
 
   private String getHtmlUsage(String fieldName) {
     StringBuilder builder = new StringBuilder();
-    OptionsUsage.getUsageHtml(data.getFieldFromName(fieldName), builder, HTML_ESCAPER, data);
+    OptionsUsage.getUsageHtml(
+        data.getOptionDefinitionFromName(fieldName), builder, HTML_ESCAPER, data);
     return builder.toString();
   }
 
   private String getTerminalUsage(String fieldName, HelpVerbosity verbosity) {
     StringBuilder builder = new StringBuilder();
-    OptionsUsage.getUsage(data.getFieldFromName(fieldName), builder, verbosity, data);
+    OptionsUsage.getUsage(data.getOptionDefinitionFromName(fieldName), builder, verbosity, data);
     return builder.toString();
   }
 
diff --git a/src/test/java/com/google/devtools/common/options/TestOptions.java b/src/test/java/com/google/devtools/common/options/TestOptions.java
index 4fa03096fa..04aeee9bc0 100644
--- a/src/test/java/com/google/devtools/common/options/TestOptions.java
+++ b/src/test/java/com/google/devtools/common/options/TestOptions.java
@@ -298,7 +298,7 @@ public class TestOptions extends OptionsBase {
     @Override
     public ImmutableList<String> getExpansion(ExpansionContext context) {
       TreeSet<String> flags = new TreeSet<>();
-      for (Map.Entry<String, ?> entry : context.getOptionsData().getAllNamedFields()) {
+      for (Map.Entry<String, ?> entry : context.getOptionsData().getAllOptionDefinitions()) {
         if (entry.getKey().startsWith("specialexp_")) {
           flags.add("--" + entry.getKey());
         }