Add ordering semantics for cached options data
This lets us easily get all options classes of a parser, or all fields of those options classes, in a deterministic way.
RELNOTES: None
PiperOrigin-RevId: 153376699
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 084daad..0dc787c 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -16,73 +16,89 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
+import com.google.common.collect.Ordering;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
+import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import javax.annotation.concurrent.Immutable;
/**
- * An immutable selection of options data corresponding to a set of options classes. The data is
- * collected using reflection, which can be expensive. Therefore this class can be used internally
- * to cache the results.
+ * A selection of options data corresponding to a set of {@link OptionsBase} subclasses (options
+ * classes). The data is collected using reflection, which can be expensive. Therefore this class
+ * can be used internally to cache the results.
*
- * <p>The data is isolated in the sense that it has not yet been processed to add inter-option-
- * dependent information -- namely, the results of evaluating expansion functions. The {@link
- * OptionsData} subclass stores this added information. The reason for the split is so that we can
- * avoid exposing to expansion functions the effects of evaluating other expansion functions, to
- * ensure that the order in which they run is not significant.
+ * <p>The data is isolated in the sense that it has not yet been processed to add
+ * inter-option-dependent information -- namely, the results of evaluating expansion functions. The
+ * {@link OptionsData} subclass stores this added information. The reason for the split is so that
+ * we can avoid exposing to expansion functions the effects of evaluating other expansion functions,
+ * to ensure that the order in which they run is not significant.
+ *
+ * <p>This class is immutable so long as the converters and default values associated with the
+ * options are immutable.
*/
-// TODO(brandjon): This class is technically not necessarily immutable due to optionsDefault
-// accepting Object values, and the List in allOptionsField should be ImmutableList. Either fix
-// this or remove @Immutable.
@Immutable
public class IsolatedOptionsData extends OpaqueOptionsData {
/**
- * These are the options-declaring classes which are annotated with {@link Option} annotations.
+ * Mapping from each options class to its no-arg constructor. Entries appear in the same order
+ * that they were passed to {@link #from(Collection)}.
*/
private final ImmutableMap<Class<? extends OptionsBase>, Constructor<?>> optionsClasses;
- /** Maps option name to Option-annotated Field. */
+ /**
+ * Mapping from option name to {@code @Option}-annotated field. 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, Field> nameToField;
- /** Maps option abbreviation to Option-annotated Field. */
+ /** Mapping from option abbreviation to {@code Option}-annotated field (unordered). */
private final ImmutableMap<Character, Field> abbrevToField;
- /** For each options class, contains a list of all Option-annotated fields in that class. */
- private final ImmutableMap<Class<? extends OptionsBase>, List<Field>> allOptionsFields;
+ /**
+ * Mapping from options class to a list of all {@code Option}-annotated fields in that class. The
+ * map entries are unordered, but the fields in the lists are ordered alphabetically.
+ */
+ private final ImmutableMap<Class<? extends OptionsBase>, ImmutableList<Field>> allOptionsFields;
- /** Mapping from each Option-annotated field to the default value for that field. */
- // Immutable like the others, but uses Collections.unmodifiableMap because of null values.
+ /**
+ * Mapping from each {@code Option}-annotated field to the default value for that field
+ * (unordered).
+ *
+ * <p>(This is immutable like the others, but uses {@code Collections.unmodifiableMap} to support
+ * null values.)
+ */
private final Map<Field, Object> optionDefaults;
/**
- * Mapping from each Option-annotated field to the proper converter.
+ * Mapping from each {@code Option}-annotated field to the proper converter (unordered).
*
* @see #findConverter
*/
private final ImmutableMap<Field, Converter<?>> converters;
/**
- * Mapping from each Option-annotated field to a boolean for whether that field allows multiple
- * values.
+ * Mapping from each {@code Option}-annotated field to a boolean for whether that field allows
+ * multiple values (unordered).
*/
private final ImmutableMap<Field, Boolean> allowMultiple;
private IsolatedOptionsData(
- Map<Class<? extends OptionsBase>, Constructor<?>> optionsClasses,
+ Map<Class<? extends OptionsBase>,
+ Constructor<?>> optionsClasses,
Map<String, Field> nameToField,
Map<Character, Field> abbrevToField,
- Map<Class<? extends OptionsBase>, List<Field>> allOptionsFields,
+ Map<Class<? extends OptionsBase>, ImmutableList<Field>> allOptionsFields,
Map<Field, Object> optionDefaults,
Map<Field, Converter<?>> converters,
Map<Field, Boolean> allowMultiple) {
@@ -107,6 +123,10 @@
other.allowMultiple);
}
+ /**
+ * Returns all options classes indexed by this options data object, in the order they were passed
+ * to {@link #from(Collection)}.
+ */
public Collection<Class<? extends OptionsBase>> getOptionsClasses() {
return optionsClasses.keySet();
}
@@ -120,6 +140,11 @@
return nameToField.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.
+ */
public Iterable<Map.Entry<String, Field>> getAllNamedFields() {
return nameToField.entrySet();
}
@@ -128,7 +153,11 @@
return abbrevToField.get(abbrev);
}
- public List<Field> getFieldsForClass(Class<? extends OptionsBase> optionsClass) {
+ /**
+ * Returns a list of all {@link Field} objects for options in the given options class, ordered
+ * alphabetically by option name.
+ */
+ public ImmutableList<Field> getFieldsForClass(Class<? extends OptionsBase> optionsClass) {
return allOptionsFields.get(optionsClass);
}
@@ -226,17 +255,29 @@
}
}
- private static List<Field> getAllAnnotatedFields(Class<? extends OptionsBase> optionsClass) {
- List<Field> allFields = Lists.newArrayList();
+ private static final Ordering<Field> fieldOrdering =
+ new Ordering<Field>() {
+ @Override
+ public int compare(Field f1, Field f2) {
+ String n1 = f1.getAnnotation(Option.class).name();
+ String n2 = f2.getAnnotation(Option.class).name();
+ return n1.compareTo(n2);
+ }
+ };
+
+ /**
+ * Return all {@code @Option}-annotated fields, alphabetically ordered by their option name (not
+ * their field name).
+ */
+ private static ImmutableList<Field> getAllAnnotatedFieldsSorted(
+ Class<? extends OptionsBase> optionsClass) {
+ List<Field> unsortedFields = new ArrayList<>();
for (Field field : optionsClass.getFields()) {
if (field.isAnnotationPresent(Option.class)) {
- allFields.add(field);
+ unsortedFields.add(field);
}
}
- if (allFields.isEmpty()) {
- throw new IllegalStateException(optionsClass + " has no public @Option-annotated fields");
- }
- return ImmutableList.copyOf(allFields);
+ return fieldOrdering.immutableSortedCopy(unsortedFields);
}
private static Object retrieveDefaultFromAnnotation(Field optionField) {
@@ -300,23 +341,25 @@
// Record that the boolean option takes up additional namespace for its negating alias.
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
* on each option in isolation.
*/
static IsolatedOptionsData from(Collection<Class<? extends OptionsBase>> classes) {
- Map<Class<? extends OptionsBase>, Constructor<?>> constructorBuilder = Maps.newHashMap();
- Map<Class<? extends OptionsBase>, List<Field>> allOptionsFieldsBuilder = Maps.newHashMap();
- Map<String, Field> nameToFieldBuilder = Maps.newHashMap();
- Map<Character, Field> abbrevToFieldBuilder = Maps.newHashMap();
- Map<Field, Object> optionDefaultsBuilder = Maps.newHashMap();
- Map<Field, Converter<?>> convertersBuilder = Maps.newHashMap();
- Map<Field, Boolean> allowMultipleBuilder = Maps.newHashMap();
+ // Mind which fields have to preserve order.
+ Map<Class<? extends OptionsBase>, Constructor<?>> constructorBuilder = new LinkedHashMap<>();
+ Map<Class<? extends OptionsBase>, ImmutableList<Field>> allOptionsFieldsBuilder =
+ new HashMap<>();
+ Map<String, Field> nameToFieldBuilder = new LinkedHashMap<>();
+ Map<Character, Field> abbrevToFieldBuilder = new HashMap<>();
+ Map<Field, Object> optionDefaultsBuilder = new HashMap<>();
+ Map<Field, Converter<?>> convertersBuilder = new HashMap<>();
+ Map<Field, Boolean> allowMultipleBuilder = new HashMap<>();
// Maps the negated boolean flag aliases to the original option name.
- Map<String, String> booleanAliasMap = Maps.newHashMap();
+ Map<String, String> booleanAliasMap = new HashMap<>();
// Read all Option annotations:
for (Class<? extends OptionsBase> parsedOptionsClass : classes) {
@@ -328,7 +371,7 @@
throw new IllegalArgumentException(parsedOptionsClass
+ " lacks an accessible default constructor");
}
- List<Field> fields = getAllAnnotatedFields(parsedOptionsClass);
+ ImmutableList<Field> fields = getAllAnnotatedFieldsSorted(parsedOptionsClass);
allOptionsFieldsBuilder.put(parsedOptionsClass, fields);
for (Field field : fields) {
diff --git a/src/main/java/com/google/devtools/common/options/OptionsUsage.java b/src/main/java/com/google/devtools/common/options/OptionsUsage.java
index aa48cb7..b898ae6 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsUsage.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsUsage.java
@@ -270,6 +270,7 @@
}
}
+ // TODO(brandjon): Should this use sorting by option name instead of field name?
private static final Comparator<Field> BY_NAME = new Comparator<Field>() {
@Override
public int compare(Field left, Field right) {
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 136c45b..5fc531a 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
@@ -18,7 +18,10 @@
import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableList;
+import java.lang.reflect.Field;
+import java.util.ArrayList;
import java.util.List;
+import java.util.Map;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -33,13 +36,24 @@
}
private static IsolatedOptionsData construct(
- Class<? extends OptionsBase> optionsClass1, Class<? extends OptionsBase> optionsClass2)
+ Class<? extends OptionsBase> optionsClass1,
+ Class<? extends OptionsBase> optionsClass2)
throws OptionsParser.ConstructionException {
return IsolatedOptionsData.from(
ImmutableList.<Class<? extends OptionsBase>>of(optionsClass1, optionsClass2));
}
- /** Dummy comment (linter suppression) */
+ private static IsolatedOptionsData construct(
+ Class<? extends OptionsBase> optionsClass1,
+ Class<? extends OptionsBase> optionsClass2,
+ Class<? extends OptionsBase> optionsClass3)
+ throws OptionsParser.ConstructionException {
+ return IsolatedOptionsData.from(
+ ImmutableList.<Class<? extends OptionsBase>>of(
+ optionsClass1, optionsClass2, optionsClass3));
+ }
+
+ /** Dummy options class. */
public static class ExampleNameConflictOptions extends OptionsBase {
@Option(
name = "foo",
@@ -65,7 +79,7 @@
}
}
- /** Dummy comment (linter suppression) */
+ /** Dummy options class. */
public static class ExampleIntegerFooOptions extends OptionsBase {
@Option(
name = "foo",
@@ -74,7 +88,7 @@
public int foo;
}
- /** Dummy comment (linter suppression) */
+ /** Dummy options class. */
public static class ExampleBooleanFooOptions extends OptionsBase {
@Option(
name = "foo",
@@ -94,7 +108,7 @@
}
}
- /** Dummy comment (linter suppression) */
+ /** Dummy options class. */
public static class ExamplePrefixFooOptions extends OptionsBase {
@Option(
name = "nofoo",
@@ -127,7 +141,7 @@
}
}
- /** Dummy comment (linter suppression) */
+ /** Dummy options class. */
public static class ExampleBarWasNamedFooOption extends OptionsBase {
@Option(
name = "bar",
@@ -151,7 +165,7 @@
}
}
- /** Dummy comment (linter suppression) */
+ /** Dummy options class. */
public static class ExampleBarWasNamedNoFooOption extends OptionsBase {
@Option(
name = "bar",
@@ -176,7 +190,7 @@
}
}
- /** Dummy comment (linter suppression) */
+ /** Dummy options class. */
public static class OldNameConflictExample extends OptionsBase {
@Option(
name = "new_name",
@@ -201,7 +215,7 @@
}
}
- /** Dummy comment (linter suppression) */
+ /** Dummy options class. */
public static class StringConverter implements Converter<String> {
@Override
public String convert(String input) {
@@ -214,7 +228,7 @@
}
}
- /** Dummy comment (linter suppression) */
+ /** Dummy options class. */
public static class InvalidOptionConverter extends OptionsBase {
@Option(
name = "foo",
@@ -235,7 +249,7 @@
fail();
}
- /** Dummy comment (linter suppression) */
+ /** Dummy options class. */
public static class InvalidListOptionConverter extends OptionsBase {
@Option(
name = "foo",
@@ -256,4 +270,122 @@
}
fail();
}
+
+ /**
+ * Dummy options class.
+ *
+ * <p>Option name order is different from field name order.
+ *
+ * <p>There are four fields to increase the likelihood of a non-deterministic order being noticed.
+ */
+ public static class FieldNamesDifferOptions extends OptionsBase {
+
+ @Option(
+ name = "foo",
+ defaultValue = "0"
+ )
+ public int aFoo;
+
+ @Option(
+ name = "bar",
+ defaultValue = "0"
+ )
+ public int bBar;
+
+ @Option(
+ name = "baz",
+ defaultValue = "0"
+ )
+ public int cBaz;
+
+ @Option(
+ name = "qux",
+ defaultValue = "0"
+ )
+ public int dQux;
+ }
+
+ /** Dummy options class. */
+ public static class EndOfAlphabetOptions extends OptionsBase {
+ @Option(
+ name = "X",
+ defaultValue = "0"
+ )
+ public int x;
+
+ @Option(
+ name = "Y",
+ defaultValue = "0"
+ )
+ public int y;
+ }
+
+ /** Dummy options class. */
+ public static class ReverseOrderedOptions extends OptionsBase {
+ @Option(
+ name = "C",
+ defaultValue = "0"
+ )
+ public int c;
+
+ @Option(
+ name = "B",
+ defaultValue = "0"
+ )
+ public int b;
+
+ @Option(
+ name = "A",
+ defaultValue = "0"
+ )
+ public int a;
+ }
+
+ @Test
+ public void optionsClassesIsOrdered() throws Exception {
+ IsolatedOptionsData data = construct(
+ FieldNamesDifferOptions.class,
+ EndOfAlphabetOptions.class,
+ ReverseOrderedOptions.class);
+ assertThat(data.getOptionsClasses()).containsExactly(
+ FieldNamesDifferOptions.class,
+ EndOfAlphabetOptions.class,
+ ReverseOrderedOptions.class).inOrder();
+ }
+
+ @Test
+ public void getAllNamedFieldsIsOrdered() throws Exception {
+ IsolatedOptionsData data = construct(
+ FieldNamesDifferOptions.class,
+ EndOfAlphabetOptions.class,
+ ReverseOrderedOptions.class);
+ ArrayList<String> names = new ArrayList<>();
+ for (Map.Entry<String, Field> entry : data.getAllNamedFields()) {
+ names.add(entry.getKey());
+ }
+ assertThat(names).containsExactly(
+ "bar", "baz", "foo", "qux", "X", "Y", "A", "B", "C").inOrder();
+ }
+
+ private List<String> getOptionNames(Iterable<Field> fields) {
+ ArrayList<String> result = new ArrayList<>();
+ for (Field field : fields) {
+ result.add(field.getAnnotation(Option.class).name());
+ }
+ return result;
+ }
+
+ @Test
+ public void getFieldsForClassIsOrdered() throws Exception {
+ IsolatedOptionsData data = construct(
+ FieldNamesDifferOptions.class,
+ EndOfAlphabetOptions.class,
+ ReverseOrderedOptions.class);
+ assertThat(getOptionNames(data.getFieldsForClass(FieldNamesDifferOptions.class)))
+ .containsExactly("bar", "baz", "foo", "qux").inOrder();
+ assertThat(getOptionNames(data.getFieldsForClass(EndOfAlphabetOptions.class)))
+ .containsExactly("X", "Y").inOrder();
+ assertThat(getOptionNames(data.getFieldsForClass(ReverseOrderedOptions.class)))
+ .containsExactly("A", "B", "C").inOrder();
+ }
}
