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
GitOrigin-RevId: 80399bc14ced39936ef19a20f3b8c2d1536aa6c2
Change-Id: Id2d7edfcace3f21b9ed4997fc9b94e4c39731a4a
diff --git a/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java b/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java
index ad4c975..0f4dc08 100644
--- a/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java
+++ b/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/java/com/google/devtools/common/options/IsolatedOptionsData.java b/java/com/google/devtools/common/options/IsolatedOptionsData.java
index d1e7ba3..57b4d23 100644
--- a/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -19,7 +19,6 @@
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 @@
*/
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 @@
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 @@
this(
other.optionsClasses,
other.nameToField,
+ other.oldNameToField,
other.abbrevToField,
other.usesOnlyCoreTypes);
}
@@ -134,16 +144,20 @@
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 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 @@
}
}
+ /**
+ * 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 @@
// 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 @@
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.
- if (optionDefinition.isBooleanField()) {
- checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, oldName);
- }
- }
- if (optionDefinition.getAbbreviation() != '\0') {
+ try {
+ String optionName = optionDefinition.getOptionName();
+ checkForCollisions(nameToFieldBuilder, optionName, "option name collision");
checkForCollisions(
- abbrevToFieldBuilder, optionDefinition.getAbbreviation(), "option abbreviation");
- abbrevToFieldBuilder.put(optionDefinition.getAbbreviation(), optionDefinition);
+ oldNameToFieldBuilder,
+ optionName,
+ "option name collision with another option's old name");
+ checkForBooleanAliasCollisions(booleanAliasMap, optionName, "option");
+ if (optionDefinition.isBooleanField()) {
+ checkAndUpdateBooleanAliases(
+ nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap, optionName);
+ }
+ 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 @@
return new IsolatedOptionsData(
constructorBuilder,
nameToFieldBuilder,
+ oldNameToFieldBuilder,
abbrevToFieldBuilder,
usesOnlyCoreTypesBuilder);
}
diff --git a/java/com/google/devtools/common/options/OptionsData.java b/java/com/google/devtools/common/options/OptionsData.java
index d29af3d..5b9a436 100644
--- a/java/com/google/devtools/common/options/OptionsData.java
+++ b/java/com/google/devtools/common/options/OptionsData.java
@@ -128,7 +128,7 @@
// 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/java/com/google/devtools/common/options/OptionsParser.java b/java/com/google/devtools/common/options/OptionsParser.java
index 618e0dd..6ad7bb2 100644
--- a/java/com/google/devtools/common/options/OptionsParser.java
+++ b/java/com/google/devtools/common/options/OptionsParser.java
@@ -16,6 +16,7 @@
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 @@
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 String getName() {
return name;
}
-
OptionDefinition getOptionDefinition() {
return optionDefinition;
}
diff --git a/java/com/google/devtools/common/options/OptionsParserImpl.java b/java/com/google/devtools/common/options/OptionsParserImpl.java
index 993fbbe..6ae8170 100644
--- a/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -168,7 +168,7 @@
*/
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 @@
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 @@
}
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 @@
}
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 @@
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 @@
}
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 @@
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.
