diff options
4 files changed, 337 insertions, 39 deletions
diff --git a/src/main/java/com/google/devtools/common/options/OptionsBase.java b/src/main/java/com/google/devtools/common/options/OptionsBase.java index 0ca9e44a65..48658f7f1a 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsBase.java +++ b/src/main/java/com/google/devtools/common/options/OptionsBase.java @@ -16,15 +16,15 @@ package com.google.devtools.common.options; import com.google.common.escape.CharEscaperBuilder; import com.google.common.escape.Escaper; - +import java.lang.reflect.Field; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; /** - * Base class for all options classes. Extend this class, adding public - * instance fields annotated with @Option. Then you can create instances - * either programmatically: + * Base class for all options classes. Extend this class, adding public instance fields annotated + * with {@link Option}. Then you can create instances either programmatically: * * <pre> * X x = Options.getDefaults(X.class); @@ -40,11 +40,10 @@ import java.util.Map.Entry; * X x = parser.getOptions(X.class); * </pre> * - * <p>Subclasses of OptionsBase <b>must</b> be constructed reflectively, - * i.e. using not {@code new MyOptions}, but one of the two methods above - * instead. (Direct construction creates an empty instance, not containing - * default values. This leads to surprising behavior and often - * NullPointerExceptions, etc.) + * <p>Subclasses of {@code OptionsBase} <i>must</i> be constructed reflectively, i.e. using not + * {@code new MyOptions()}, but one of the above methods instead. (Direct construction creates an + * empty instance, not containing default values. This leads to surprising behavior and often {@code + * NullPointerExceptions}, etc.) */ public abstract class OptionsBase { @@ -61,13 +60,24 @@ public abstract class OptionsBase { } /** - * Returns this options object in the form of a (new) mapping from option - * names, including inherited ones, to option values. If the public fields - * are mutated, this will be reflected in subsequent calls to {@code asMap}. - * Mutation of this map by the caller does not affect this options object. + * Returns a mapping from option names to values, for each option on this object, including + * inherited ones. The mapping is a copy, so subsequent mutations to it or to this object are + * independent. Entries are sorted alphabetically. */ - public final Map<String, Object> asMap() { - return OptionsParserImpl.optionsAsMap(this); + public final <O extends OptionsBase> Map<String, Object> asMap() { + // Generic O is needed to tell the type system that the toMap() call is safe. + // The casts are safe because this <= O <= OptionsBase. + @SuppressWarnings("unchecked") + O castThis = (O) this; + @SuppressWarnings("unchecked") + Class<O> castClass = (Class<O>) getClass(); + + Map<String, Object> map = new LinkedHashMap<>(); + for (Map.Entry<Field, Object> entry : OptionsParser.toMap(castClass, castThis).entrySet()) { + String name = entry.getKey().getAnnotation(Option.class).name(); + map.put(name, entry.getValue()); + } + return map; } @Override 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 a871fd18e2..355deeaad6 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java @@ -21,6 +21,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ListMultimap; import com.google.common.escape.Escaper; +import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.nio.file.FileSystem; import java.util.ArrayList; @@ -28,6 +29,8 @@ import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import javax.annotation.Nullable; @@ -781,4 +784,109 @@ public class OptionsParser implements OptionsProvider { public List<String> canonicalize() { return impl.asCanonicalizedList(); } + + /** + * Returns a mapping from each option {@link Field} in {@code optionsClass} (including inherited + * ones) to its value in {@code options}. + * + * <p>The map is a mutable copy; changing the map won't affect {@code options} and vice versa. + * The map entries appear sorted alphabetically by option name. + * + * If {@code options} is an instance of a subclass of {@code optionsClass}, any options defined + * by the subclass are not included in the map. + * + * @throws IllegalArgumentException if {@code options} is not an instance of {@code optionsClass} + */ + static <O extends OptionsBase> Map<Field, Object> toMap(Class<O> optionsClass, O options) { + OptionsData data = getOptionsDataInternal(optionsClass); + // Alphabetized due to getFieldsForClass()'s order. + Map<Field, Object> map = new LinkedHashMap<>(); + for (Field field : data.getFieldsForClass(optionsClass)) { + try { + map.put(field, field.get(options)); + } catch (IllegalAccessException e) { + // All options fields of options classes should be public. + throw new IllegalStateException(e); + } catch (IllegalArgumentException e) { + // This would indicate an inconsistency in the cached OptionsData. + throw new IllegalStateException(e); + } + } + return map; + } + + /** + * Given a mapping as returned by {@link #toMap}, and the options class it that its entries + * correspond to, this constructs the corresponding instance of the options class. + * + * @throws IllegalArgumentException if {@code map} does not contain exactly the fields of {@code + * optionsClass}, with values of the appropriate type + */ + static <O extends OptionsBase> O fromMap(Class<O> optionsClass, Map<Field, Object> map) { + // Instantiate the options class. + OptionsData data = getOptionsDataInternal(optionsClass); + O optionsInstance; + try { + Constructor<O> constructor = data.getConstructor(optionsClass); + Preconditions.checkNotNull(constructor, "No options class constructor available"); + optionsInstance = constructor.newInstance(); + } catch (ReflectiveOperationException e) { + throw new IllegalStateException("Error while instantiating options class", e); + } + + List<Field> fields = data.getFieldsForClass(optionsClass); + // Ensure all fields are covered, no extraneous fields. + validateFieldsSets(new LinkedHashSet<>(fields), new LinkedHashSet<>(map.keySet())); + // Populate the instance. + for (Field field : fields) { + // Non-null as per above check. + Object value = map.get(field); + try { + field.set(optionsInstance, value); + } catch (IllegalAccessException e) { + throw new IllegalStateException(e); + } + // May also throw IllegalArgumentException if map value is ill typed. + } + return optionsInstance; + } + + /** + * Raises a pretty {@link IllegalArgumentException} if the two sets of fields are not equal. + * + * <p>The entries in {@code fieldsFromMap} may be ill formed by being null or lacking an {@link + * Option} annotation. (This isn't done for {@code fieldsFromClass} because they come from an + * {@link OptionsData} object.) + */ + private static void validateFieldsSets( + LinkedHashSet<Field> fieldsFromClass, LinkedHashSet<Field> fieldsFromMap) { + if (!fieldsFromClass.equals(fieldsFromMap)) { + List<String> extraNamesFromClass = new ArrayList<>(); + List<String> extraNamesFromMap = new ArrayList<>(); + for (Field field : fieldsFromClass) { + if (!fieldsFromMap.contains(field)) { + extraNamesFromClass.add("'" + field.getAnnotation(Option.class).name() + "'"); + } + } + for (Field field : fieldsFromMap) { + // Extra validation on the map keys since they don't come from OptionsData. + if (!fieldsFromClass.contains(field)) { + if (field == null) { + extraNamesFromMap.add("<null field>"); + } else { + Option annotation = field.getAnnotation(Option.class); + if (annotation == null) { + extraNamesFromMap.add("<non-Option field>"); + } else { + extraNamesFromMap.add("'" + annotation.name() + "'"); + } + } + } + } + throw new IllegalArgumentException( + "Map keys do not match fields of options class; extra map keys: {" + + Joiner.on(", ").join(extraNamesFromMap) + "}; extra options class options: {" + + Joiner.on(", ").join(extraNamesFromClass) + "}"); + } + } } 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 2d442a14fa..c5f31c43e5 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -85,7 +85,7 @@ class OptionsParserImpl { private final List<String> warnings = new ArrayList<>(); private boolean allowSingleDashLongOptions = false; - + private ArgsPreProcessor argsPreProcessor = new ArgsPreProcessor() { @Override @@ -93,7 +93,7 @@ class OptionsParserImpl { return args; } }; - + /** * Create a new parser object */ @@ -112,32 +112,13 @@ class OptionsParserImpl { void setAllowSingleDashLongOptions(boolean allowSingleDashLongOptions) { this.allowSingleDashLongOptions = allowSingleDashLongOptions; } - + /** Sets the ArgsPreProcessor for manipulations of the options before parsing. */ void setArgsPreProcessor(ArgsPreProcessor preProcessor) { this.argsPreProcessor = Preconditions.checkNotNull(preProcessor); } /** - * The implementation of {@link OptionsBase#asMap}. - */ - static Map<String, Object> optionsAsMap(OptionsBase optionsInstance) { - Class<? extends OptionsBase> optionsClass = optionsInstance.getClass(); - OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass); - Map<String, Object> map = new HashMap<>(); - for (Field field : data.getFieldsForClass(optionsClass)) { - try { - String name = field.getAnnotation(Option.class).name(); - Object value = field.get(optionsInstance); - map.put(name, value); - } catch (IllegalAccessException e) { - throw new IllegalStateException(e); // unreachable - } - } - return map; - } - - /** * Implements {@link OptionsParser#asListOfUnparsedOptions()}. */ List<UnparsedOptionValueDescription> asListOfUnparsedOptions() { @@ -707,8 +688,8 @@ class OptionsParserImpl { return null; } optionsInstance = constructor.newInstance(); - } catch (Exception e) { - throw new IllegalStateException(e); + } catch (ReflectiveOperationException e) { + throw new IllegalStateException("Error while instantiating options class", e); } // Set the fields diff --git a/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java b/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java new file mode 100644 index 0000000000..ea60323abc --- /dev/null +++ b/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java @@ -0,0 +1,199 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.common.options; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.fail; + +import com.google.common.collect.ImmutableMap; +import java.lang.reflect.Field; +import java.util.LinkedHashMap; +import java.util.Map; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for converting {@link OptionsBase} subclass instances to and from maps. */ +@RunWith(JUnit4.class) +public class OptionsMapConversionTest { + + private static Map<String, Object> keysToStrings(Map<Field, Object> map) { + Map<String, Object> result = new LinkedHashMap<>(); + for (Map.Entry<Field, Object> entry : map.entrySet()) { + String name = entry.getKey().getAnnotation(Option.class).name(); + result.put(name, entry.getValue()); + } + return result; + } + + private static Map<Field, Object> keysToFields( + Class<? extends OptionsBase> optionsClass, Map<String, Object> map) { + OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass); + Map<Field, Object> result = new LinkedHashMap<>(); + for (Map.Entry<String, Object> entry : map.entrySet()) { + Field field = data.getFieldFromName(entry.getKey()); + result.put(field, entry.getValue()); + } + return result; + } + + /** Dummy options base class. */ + public static class FooOptions extends OptionsBase { + @Option(name = "foo", defaultValue = "false") + public boolean foo; + } + + /** Dummy options derived class. */ + public static class BazOptions extends FooOptions { + @Option(name = "bar", defaultValue = "true") + public boolean bar; + + @Option(name = "baz", defaultValue = "5") + public int baz; + } + + @Test + public void toMap_Basic() { + FooOptions foo = Options.getDefaults(FooOptions.class); + assertThat(keysToStrings(OptionsParser.toMap(FooOptions.class, foo))) + .containsExactly("foo", false); + } + + @Test + public void asMap_Basic() { + FooOptions foo = Options.getDefaults(FooOptions.class); + assertThat(foo.asMap()) + .containsExactly("foo", false); + } + + @Test + public void toMap_Inheritance() { + BazOptions baz = Options.getDefaults(BazOptions.class); + assertThat(keysToStrings(OptionsParser.toMap(BazOptions.class, baz))) + .containsExactly("foo", false, "bar", true, "baz", 5); + } + + @Test + public void asMap_Inheritance() { + // Static type is base class, dynamic type is derived. We still get the derived fields. + FooOptions foo = Options.getDefaults(BazOptions.class); + assertThat(foo.asMap()) + .containsExactly("foo", false, "bar", true, "baz", 5); + } + + @Test + public void toMap_InheritanceBaseFieldsOnly() { + BazOptions baz = Options.getDefaults(BazOptions.class); + assertThat(keysToStrings(OptionsParser.toMap(FooOptions.class, baz))) + .containsExactly("foo", false); + } + + /** + * Dummy options class for checking alphabetizing. + * + * <p>Note that field name order differs from option name order. + */ + public static class AlphaOptions extends OptionsBase { + + @Option(name = "c", defaultValue = "0") + public int v; + + @Option(name = "d", defaultValue = "0") + public int w; + + @Option(name = "a", defaultValue = "0") + public int x; + + @Option(name = "e", defaultValue = "0") + public int y; + + @Option(name = "b", defaultValue = "0") + public int z; + } + + @Test + public void toMap_AlphabeticalOrder() { + AlphaOptions alpha = Options.getDefaults(AlphaOptions.class); + assertThat(keysToStrings(OptionsParser.toMap(AlphaOptions.class, alpha))) + .containsExactly("a", 0, "b", 0, "c", 0, "d", 0, "e", 0).inOrder(); + } + + @Test + public void asMap_AlphabeticalOrder() { + AlphaOptions alpha = Options.getDefaults(AlphaOptions.class); + assertThat(alpha.asMap()) + .containsExactly("a", 0, "b", 0, "c", 0, "d", 0, "e", 0).inOrder(); + } + + @Test + public void fromMap_Basic() { + Map<String, Object> map = ImmutableMap.<String, Object>of("foo", true); + Map<Field, Object> fieldMap = keysToFields(FooOptions.class, map); + FooOptions foo = OptionsParser.fromMap(FooOptions.class, fieldMap); + assertThat(foo.foo).isTrue(); + } + + /** Dummy subclass of foo. */ + public static class SubFooAOptions extends FooOptions { + @Option(name = "a", defaultValue = "false") + public boolean a; + } + + /** Dummy subclass of foo. */ + public static class SubFooBOptions extends FooOptions { + @Option(name = "b1", defaultValue = "false") + public boolean b1; + + @Option(name = "b2", defaultValue = "false") + public boolean b2; + } + + @Test + public void fromMap_FailsOnWrongKeys() { + Map<String, Object> map = ImmutableMap.<String, Object>of("foo", true, "a", false); + Map<Field, Object> fieldMap = keysToFields(SubFooAOptions.class, map); + try { + OptionsParser.fromMap(SubFooBOptions.class, fieldMap); + fail("Should have failed due to the given map's fields not matching the ones on the class"); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage()).contains( + "Map keys do not match fields of options class; extra map keys: {'a'}; " + + "extra options class options: {'b1', 'b2'}"); + } + } + + @Test + public void fromMap_FailsOnWrongTypes() { + Map<String, Object> map = ImmutableMap.<String, Object>of("foo", 5); + Map<Field, Object> fieldMap = keysToFields(SubFooAOptions.class, map); + try { + OptionsParser.fromMap(FooOptions.class, fieldMap); + fail("Should have failed due to trying to assign a field value with the wrong type"); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage()).matches( + "Can not set boolean field .*\\.foo to java\\.lang\\.Integer"); + } + } + + @Test + public void fromMap_Inheritance() { + Map<String, Object> map = ImmutableMap.<String, Object>of("foo", true, "bar", true, "baz", 3); + Map<Field, Object> fieldMap = keysToFields(BazOptions.class, map); + BazOptions baz = OptionsParser.fromMap(BazOptions.class, fieldMap); + assertThat(baz.foo).isTrue(); + assertThat(baz.bar).isTrue(); + assertThat(baz.baz).isEqualTo(3); + } +} |