From 7c7255ec8d6da20526c2c4078c57aadaf3dd3612 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 27 Jun 2017 20:05:20 +0200 Subject: [PATCH] Allow expansion flags to have values. This lets us change what it expands based on the argument passed to the flag. RELNOTES: Allows flags that expand to take values. PiperOrigin-RevId: 160298412 --- .../AllIncompatibleChangesExpansion.java | 5 +- .../runtime/commands/CanonicalizeCommand.java | 4 +- .../common/options/ExpansionContext.java | 54 +++++++ .../common/options/ExpansionFunction.java | 4 +- .../options/ExpansionNeedsValueException.java | 25 +++ .../options/InvocationPolicyEnforcer.java | 138 ++++++++++++----- .../devtools/common/options/OptionsData.java | 109 +++++++++++-- .../common/options/OptionsParser.java | 42 +++-- .../common/options/OptionsParserImpl.java | 83 ++++++---- .../devtools/common/options/OptionsUsage.java | 17 +- .../AllIncompatibleChangesExpansionTest.java | 4 +- .../options/InvocationPolicySetValueTest.java | 146 ++++++++++++++++-- .../InvocationPolicyUseDefaultTest.java | 45 ++++++ .../common/options/OptionsParserTest.java | 73 ++++++++- .../devtools/common/options/OptionsTest.java | 43 +++++- .../devtools/common/options/TestOptions.java | 44 ++++++ 16 files changed, 709 insertions(+), 127 deletions(-) create mode 100644 src/main/java/com/google/devtools/common/options/ExpansionContext.java create mode 100644 src/main/java/com/google/devtools/common/options/ExpansionNeedsValueException.java 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 3f7a193cd3e6af..9d283314a2b416 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 @@ -16,6 +16,7 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.common.options.Converter; +import com.google.devtools.common.options.ExpansionContext; import com.google.devtools.common.options.ExpansionFunction; import com.google.devtools.common.options.IsolatedOptionsData; import com.google.devtools.common.options.Option; @@ -153,11 +154,11 @@ private static void validateIncompatibleChange(Field field, Option annotation) { } @Override - public ImmutableList getExpansion(IsolatedOptionsData optionsData) { + public ImmutableList getExpansion(ExpansionContext context) { // Grab all registered options that are identified as incompatible changes by either name or // by category. Ensure they satisfy our requirements. ArrayList incompatibleChanges = new ArrayList<>(); - for (Map.Entry entry : optionsData.getAllNamedFields()) { + for (Map.Entry entry : context.getOptionsData().getAllNamedFields()) { Field field = entry.getValue(); Option annotation = field.getAnnotation(Option.class); if (annotation.name().startsWith(INCOMPATIBLE_NAME_PREFIX) diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java index 18455b17e53aab..53540e6f187f3e 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java @@ -165,8 +165,8 @@ public ExitCode exec(CommandEnvironment env, OptionsProvider options) { // Print out the canonical invocation policy if requested. if (canonicalizeOptions.canonicalizePolicy) { - List effectiveFlagPolicies = - InvocationPolicyEnforcer.getEffectivePolicy(policy, parser, commandName); + ImmutableList effectiveFlagPolicies = + InvocationPolicyEnforcer.getEffectivePolicies(policy, parser, commandName); InvocationPolicy effectivePolicy = InvocationPolicy.newBuilder().addAllFlagPolicies(effectiveFlagPolicies).build(); env.getReporter().getOutErr().printOutLn(effectivePolicy.toString()); diff --git a/src/main/java/com/google/devtools/common/options/ExpansionContext.java b/src/main/java/com/google/devtools/common/options/ExpansionContext.java new file mode 100644 index 00000000000000..74dac97b573374 --- /dev/null +++ b/src/main/java/com/google/devtools/common/options/ExpansionContext.java @@ -0,0 +1,54 @@ +// 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 java.lang.reflect.Field; +import javax.annotation.Nullable; +import javax.annotation.concurrent.ThreadSafe; + +/** + * Encapsulates the data given to {@link ExpansionFunction} objects. This lets {@link + * ExpansionFunction} objects change how it expands flags based on the arguments given to the {@link + * OptionsParser}. + */ +@ThreadSafe +public final class ExpansionContext { + private final IsolatedOptionsData optionsData; + private final Field field; + @Nullable private final String unparsedValue; + + public ExpansionContext( + IsolatedOptionsData optionsData, Field field, @Nullable String unparsedValue) { + this.optionsData = optionsData; + this.field = field; + this.unparsedValue = unparsedValue; + } + + /** Metadata for the option that is being expanded. */ + public IsolatedOptionsData getOptionsData() { + return optionsData; + } + + /** {@link Field} object for option that is being expanded. */ + public Field getField() { + return field; + } + + /** Argument given to this flag during options parsing. Will be null if no argument was given. */ + @Nullable + public String getUnparsedValue() { + return unparsedValue; + } +} diff --git a/src/main/java/com/google/devtools/common/options/ExpansionFunction.java b/src/main/java/com/google/devtools/common/options/ExpansionFunction.java index be4773ec3942a5..10311251a9bc19 100644 --- a/src/main/java/com/google/devtools/common/options/ExpansionFunction.java +++ b/src/main/java/com/google/devtools/common/options/ExpansionFunction.java @@ -27,7 +27,7 @@ public interface ExpansionFunction { * * @param optionsData the parser's indexed information about its own options, before expansion * information is computed - * @return An expansion to use for all occurrences of this option in this parser + * @return An expansion to use on an empty list */ - ImmutableList getExpansion(IsolatedOptionsData optionsData); + ImmutableList getExpansion(ExpansionContext context) throws OptionsParsingException; } diff --git a/src/main/java/com/google/devtools/common/options/ExpansionNeedsValueException.java b/src/main/java/com/google/devtools/common/options/ExpansionNeedsValueException.java new file mode 100644 index 00000000000000..d63b9885707844 --- /dev/null +++ b/src/main/java/com/google/devtools/common/options/ExpansionNeedsValueException.java @@ -0,0 +1,25 @@ +// 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; + +/** + * Exception specific to evaluating {@link ExpansionFunction} objects. Used when expansion isn't + * possible because of a missing input. + */ +public final class ExpansionNeedsValueException extends OptionsParsingException { + + public ExpansionNeedsValueException(String message) { + super(message); + } +} diff --git a/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java b/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java index 651cd7858bcbf6..97be190d40f068 100644 --- a/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java +++ b/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java @@ -99,9 +99,9 @@ public void enforce(OptionsParser parser, @Nullable String command) // The effective policy returned is expanded, filtered for applicable commands, and cleaned of // redundancies and conflicts. - List effectivePolicy = getEffectivePolicy(invocationPolicy, parser, command); + List effectivePolicies = getEffectivePolicies(invocationPolicy, parser, command); - for (FlagPolicy flagPolicy : effectivePolicy) { + for (FlagPolicy flagPolicy : effectivePolicies) { String flagName = flagPolicy.getFlagName(); OptionValueDescription valueDescription; @@ -190,7 +190,7 @@ private static boolean policyApplies(FlagPolicy policy, ImmutableSet app * *

Expands any policies on expansion flags. */ - public static List getEffectivePolicy( + public static ImmutableList getEffectivePolicies( InvocationPolicy invocationPolicy, OptionsParser parser, String command) throws OptionsParsingException { if (invocationPolicy == null) { @@ -220,7 +220,71 @@ public static List getEffectivePolicy( effectivePolicy.put(flagName, expandedPolicy); } - return new ArrayList<>(effectivePolicy.values()); + return ImmutableList.copyOf(effectivePolicy.values()); + } + + private static void throwAllowValuesOnExpansionFlagException(String flagName) + throws OptionsParsingException { + throw new OptionsParsingException( + String.format("Allow_Values on expansion flags like %s is not allowed.", flagName)); + } + + private static void throwDisallowValuesOnExpansionFlagException(String flagName) + throws OptionsParsingException { + throw new OptionsParsingException( + String.format("Disallow_Values on expansion flags like %s is not allowed.", flagName)); + } + + private static ImmutableList getExpansionsFromFlagPolicy( + FlagPolicy expansionPolicy, OptionDescription optionDescription, OptionsParser parser) + throws OptionsParsingException { + if (!optionDescription.isExpansion()) { + return ImmutableList.of(); + } + + String expansionFlagName = expansionPolicy.getFlagName(); + + ImmutableList.Builder resultsBuilder = + ImmutableList.builder(); + switch (expansionPolicy.getOperationCase()) { + case SET_VALUE: + { + SetValue setValue = expansionPolicy.getSetValue(); + if (setValue.getFlagValueCount() > 0) { + for (String value : setValue.getFlagValueList()) { + resultsBuilder.addAll( + parser.getExpansionOptionValueDescriptions(expansionFlagName, value)); + } + } else { + resultsBuilder.addAll( + parser.getExpansionOptionValueDescriptions(expansionFlagName, null)); + } + } + break; + case USE_DEFAULT: + resultsBuilder.addAll(parser.getExpansionOptionValueDescriptions(expansionFlagName, null)); + break; + case ALLOW_VALUES: + // All expansions originally given to the parser have been expanded by now, so these two + // cases aren't necessary (the values given in the flag policy shouldn't need to be + // checked). If you care about blocking specific flag values you should block the behavior + // on the specific ones, not the expansion that contains them. + throwAllowValuesOnExpansionFlagException(expansionPolicy.getFlagName()); + break; + case DISALLOW_VALUES: + throwDisallowValuesOnExpansionFlagException(expansionPolicy.getFlagName()); + break; + case OPERATION_NOT_SET: + throw new PolicyOperationNotSetException(expansionPolicy.getFlagName()); + default: + log.warning( + String.format( + "Unknown operation '%s' from invocation policy for flag '%s'", + expansionPolicy.getOperationCase(), expansionFlagName)); + break; + } + + return resultsBuilder.build(); } /** @@ -234,21 +298,24 @@ private static List expandPolicy( FlagPolicy originalPolicy, OptionsParser parser) throws OptionsParsingException { - List expandedPolicy = new ArrayList<>(); + List expandedPolicies = new ArrayList<>(); - OptionDescription originalDesc = parser.getOptionDescription(originalPolicy.getFlagName()); - if (originalDesc == null) { + OptionDescription originalOptionDescription = + parser.getOptionDescription(originalPolicy.getFlagName()); + if (originalOptionDescription == null) { // InvocationPolicy ignores policy on non-existing flags by design, for version compatibility. - return expandedPolicy; + return expandedPolicies; } + ImmutableList expansions = + getExpansionsFromFlagPolicy(originalPolicy, originalOptionDescription, parser); ImmutableList.Builder subflagBuilder = new ImmutableList.Builder<>(); ImmutableList subflags = subflagBuilder - .addAll(originalDesc.getImplicitRequirements()) - .addAll(originalDesc.getExpansions()) + .addAll(originalOptionDescription.getImplicitRequirements()) + .addAll(expansions) .build(); - boolean isExpansion = !originalDesc.getExpansions().isEmpty(); + boolean isExpansion = originalOptionDescription.isExpansion(); if (!subflags.isEmpty() && log.isLoggable(Level.FINE)) { // Log the expansion. Since this is logged regardless of user provided command line, it is @@ -283,10 +350,10 @@ private static List expandPolicy( && originalPolicy.getOperationCase().equals(OperationCase.SET_VALUE)) { repeatableSubflagsInSetValues.put(currentSubflag.getName(), currentSubflag); } else { - FlagPolicy subflagAsPolicy = getSubflagAsPolicy( - currentSubflag, originalPolicy, originalDesc); + FlagPolicy subflagAsPolicy = + getSubflagAsPolicy(currentSubflag, originalPolicy, isExpansion); // In case any of the expanded flags are themselves expansions, recurse. - expandedPolicy.addAll(expandPolicy(subflagAsPolicy, parser)); + expandedPolicies.addAll(expandPolicy(subflagAsPolicy, parser)); } } @@ -299,22 +366,18 @@ private static List expandPolicy( for (OptionValueDescription setValue : repeatableSubflagsInSetValues.get(repeatableFlag)) { newValues.add(setValue.getOriginalValueString()); } - expandedPolicy.add( + expandedPolicies.add( getSetValueSubflagAsPolicy( - repeatableFlag, - newValues, - /* allowMultiple */ true, - originalPolicy)); - + repeatableFlag, newValues, /* allowMultiple */ true, originalPolicy)); } // Don't add the original policy if it was an expansion flag, which have no value, but do add // it if there was either no expansion or if it was a valued flag with implicit requirements. if (!isExpansion) { - expandedPolicy.add(originalPolicy); + expandedPolicies.add(originalPolicy); } - return expandedPolicy; + return expandedPolicies; } /** @@ -365,19 +428,18 @@ private static FlagPolicy getSetValueSubflagAsPolicy( * corresponding policy. */ private static FlagPolicy getSubflagAsPolicy( - OptionValueDescription currentSubflag, - FlagPolicy originalPolicy, - OptionDescription originalDesc) throws OptionsParsingException { - boolean isExpansion = !originalDesc.getExpansions().isEmpty(); + OptionValueDescription currentSubflag, FlagPolicy originalPolicy, boolean isExpansion) + throws OptionsParsingException { FlagPolicy subflagAsPolicy = null; switch (originalPolicy.getOperationCase()) { case SET_VALUE: - assert(!currentSubflag.getAllowMultiple()); - subflagAsPolicy = getSetValueSubflagAsPolicy( - currentSubflag.getName(), - ImmutableList.of(currentSubflag.getOriginalValueString()), - /* allowMultiple */ false, - originalPolicy); + assert (!currentSubflag.getAllowMultiple()); + subflagAsPolicy = + getSetValueSubflagAsPolicy( + currentSubflag.getName(), + ImmutableList.of(currentSubflag.getOriginalValueString()), + /*allowMultiple=*/ false, + originalPolicy); break; case USE_DEFAULT: @@ -394,10 +456,7 @@ private static FlagPolicy getSubflagAsPolicy( case ALLOW_VALUES: if (isExpansion) { - throw new OptionsParsingException( - String.format( - "Allow_Values on expansion flags like %s is not allowed.", - originalPolicy.getFlagName())); + throwAllowValuesOnExpansionFlagException(originalPolicy.getFlagName()); } // If this flag is an implicitRequirement, and some values for the parent flag are // allowed, nothing needs to happen on the implicitRequirement that is set for all @@ -406,10 +465,7 @@ private static FlagPolicy getSubflagAsPolicy( case DISALLOW_VALUES: if (isExpansion) { - throw new OptionsParsingException( - String.format( - "Disallow_Values on expansion flags like %s is not allowed.", - originalPolicy.getFlagName())); + throwDisallowValuesOnExpansionFlagException(originalPolicy.getFlagName()); } // If this flag is an implicitRequirement, and some values for the parent flag are // disallowed, that implies that all others are allowed, so nothing needs to happen @@ -684,7 +740,7 @@ void checkUserValue( OptionDescription optionDescription, Set convertedPolicyValues) throws OptionsParsingException { - + if (optionDescription.getAllowMultiple()) { // allowMultiple requires that the type of the option be List, so cast from Object // to List. 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 ba5317221164f7..48f47ff5045790 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsData.java +++ b/src/main/java/com/google/devtools/common/options/OptionsData.java @@ -14,14 +14,15 @@ package com.google.devtools.common.options; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.util.Collection; import java.util.Map; +import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; /** @@ -32,38 +33,100 @@ @Immutable final class OptionsData extends IsolatedOptionsData { - /** Mapping from each Option-annotated field to expansion strings, if it has any. */ - private final ImmutableMap> evaluatedExpansions; + /** + * Keeps track of all the information needed to calculate expansion flags, whether they come from + * a static list or a @{link ExpansionFunction} object. + */ + static class ExpansionData { + private final ImmutableList staticExpansion; + @Nullable private final ExpansionFunction dynamicExpansions; + + ExpansionData(ImmutableList staticExpansion) { + Preconditions.checkArgument(staticExpansion != null); + this.staticExpansion = staticExpansion; + this.dynamicExpansions = null; + } + + ExpansionData(ExpansionFunction dynamicExpansions) { + Preconditions.checkArgument(dynamicExpansions != null); + this.staticExpansion = EMPTY_EXPANSION; + this.dynamicExpansions = dynamicExpansions; + } + + ImmutableList getExpansion(ExpansionContext context) throws OptionsParsingException { + Preconditions.checkArgument(context != null); + if (dynamicExpansions != null) { + ImmutableList result = dynamicExpansions.getExpansion(context); + if (result == null) { + String valueString = + context.getUnparsedValue() != null ? context.getUnparsedValue() : "(null)"; + String name = context.getField().getAnnotation(Option.class).name(); + throw new OptionsParsingException( + "Error expanding option '" + + name + + "': no expansions defined for value: " + + valueString, + name); + } + return result; + } else { + return staticExpansion; + } + } + + boolean isEmpty() { + return staticExpansion.isEmpty() && (dynamicExpansions == null); + } + } + + /** + * Mapping from each Option-annotated field with expansion information to the {@link + * ExpansionData} needed to caclulate it. + */ + private final ImmutableMap expansionDataForFields; /** Construct {@link OptionsData} by extending an {@link IsolatedOptionsData} with new info. */ - private OptionsData( - IsolatedOptionsData base, Map> evaluatedExpansions) { + private OptionsData(IsolatedOptionsData base, Map expansionDataForFields) { super(base); - this.evaluatedExpansions = ImmutableMap.copyOf(evaluatedExpansions); + this.expansionDataForFields = ImmutableMap.copyOf(expansionDataForFields); } private static final ImmutableList EMPTY_EXPANSION = ImmutableList.of(); + private static final ExpansionData EMPTY_EXPANSION_DATA = new ExpansionData(EMPTY_EXPANSION); /** * Returns the expansion of an options field, regardless of whether it was defined using {@link * Option#expansion} or {@link Option#expansionFunction}. If the field is not an expansion option, * returns an empty array. */ - public ImmutableList getEvaluatedExpansion(Field field) { - ImmutableList result = evaluatedExpansions.get(field); - return result != null ? result : EMPTY_EXPANSION; + public ImmutableList getEvaluatedExpansion(Field field, @Nullable String unparsedValue) + throws OptionsParsingException { + ExpansionData expansionData = expansionDataForFields.get(field); + if (expansionData == null) { + return EMPTY_EXPANSION; + } + + return expansionData.getExpansion(new ExpansionContext(this, field, unparsedValue)); + } + + ExpansionData getExpansionDataForField(Field field) { + ExpansionData result = expansionDataForFields.get(field); + return result != null ? result : EMPTY_EXPANSION_DATA; } /** * Constructs an {@link OptionsData} object for a parser that knows about the given {@link * OptionsBase} classes. In addition to the work done to construct the {@link - * IsolatedOptionsData}, this also computes expansion information. + * IsolatedOptionsData}, this also computes expansion information. If an option has static + * expansions or uses an expansion function that takes a Void object, try to precalculate the + * expansion here. */ - public static OptionsData from(Collection> classes) { + static OptionsData from(Collection> classes) { IsolatedOptionsData isolatedData = IsolatedOptionsData.from(classes); // All that's left is to compute expansions. - Map> evaluatedExpansionsBuilder = Maps.newHashMap(); + ImmutableMap.Builder expansionDataBuilder = + ImmutableMap.builder(); for (Map.Entry entry : isolatedData.getAllNamedFields()) { Field field = entry.getValue(); Option annotation = field.getAnnotation(Option.class); @@ -74,7 +137,7 @@ public static OptionsData from(Collection> classes) throw new AssertionError( "Cannot set both expansion and expansionFunction for option --" + annotation.name()); } else if (constExpansion.length > 0) { - evaluatedExpansionsBuilder.put(field, ImmutableList.copyOf(constExpansion)); + expansionDataBuilder.put(field, new ExpansionData(ImmutableList.copyOf(constExpansion))); } else if (usesExpansionFunction(annotation)) { if (Modifier.isAbstract(expansionFunctionClass.getModifiers())) { throw new AssertionError( @@ -90,11 +153,25 @@ public static OptionsData from(Collection> classes) // time it is used. throw new AssertionError(e); } - ImmutableList expansion = instance.getExpansion(isolatedData); - evaluatedExpansionsBuilder.put(field, expansion); + + ImmutableList staticExpansion = null; + try { + staticExpansion = instance.getExpansion(new ExpansionContext(isolatedData, field, null)); + Preconditions.checkState( + staticExpansion != null, + "Error calling expansion function for option: %s", + annotation.name()); + expansionDataBuilder.put(field, new ExpansionData(staticExpansion)); + } catch (ExpansionNeedsValueException e) { + // This expansion function needs data that isn't available yet. Save the instance and call + // it later. + expansionDataBuilder.put(field, new ExpansionData(instance)); + } catch (OptionsParsingException e) { + throw new IllegalStateException("Error expanding void expansion function: ", e); + } } } - return new OptionsData(isolatedData, evaluatedExpansionsBuilder); + return new OptionsData(isolatedData, expansionDataBuilder.build()); } } 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 728c49070eef70..c007db87a10a35 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java @@ -239,7 +239,7 @@ public static final class OptionDescription { private final Converter converter; private final boolean allowMultiple; - private final ImmutableList expansions; + private final OptionsData.ExpansionData expansionData; private final ImmutableList implicitRequirements; OptionDescription( @@ -247,13 +247,13 @@ public static final class OptionDescription { Object defaultValue, Converter converter, boolean allowMultiple, - ImmutableList expansions, + OptionsData.ExpansionData expansionData, ImmutableList implicitRequirements) { this.name = name; this.defaultValue = defaultValue; this.converter = converter; this.allowMultiple = allowMultiple; - this.expansions = expansions; + this.expansionData = expansionData; this.implicitRequirements = implicitRequirements; } @@ -277,8 +277,14 @@ public ImmutableList getImplicitRequirements() { return implicitRequirements; } - public ImmutableList getExpansions() { - return expansions; + public boolean isExpansion() { + return !expansionData.isEmpty(); + } + + /** Return a list of flags that this option expands to. */ + public ImmutableList getExpansion(ExpansionContext context) + throws OptionsParsingException { + return expansionData.getExpansion(context); } } @@ -658,21 +664,33 @@ public int compare(Field f1, Field f2) { * @return The {@link OptionDescription} for the option, or null if there is no option by the * given name. */ - public OptionDescription getOptionDescription(String name) throws OptionsParsingException { + OptionDescription getOptionDescription(String name) throws OptionsParsingException { return impl.getOptionDescription(name); } /** - * Returns a description of the option value set by the last previous call to - * {@link #parse(OptionPriority, String, List)} that successfully set the given - * option. If the option is of type {@link List}, the description will - * correspond to any one of the calls, but not necessarily the last. + * Returns a description of the options values that get expanded from this flag with the given + * flag value. + * + * @return The {@link ImmutableList} for the option, or null if there is + * no option by the given name. + */ + ImmutableList getExpansionOptionValueDescriptions( + String flagName, @Nullable String flagValue) throws OptionsParsingException { + return impl.getExpansionOptionValueDescriptions(flagName, flagValue); + } + + /** + * Returns a description of the option value set by the last previous call to {@link + * #parse(OptionPriority, String, List)} that successfully set the given option. If the option is + * of type {@link List}, the description will correspond to any one of the calls, but not + * necessarily the last. * * @return The {@link OptionValueDescription} for the option, or null if the value has not been - * set. + * set. * @throws IllegalArgumentException if there is no option by the given name. */ - public OptionValueDescription getOptionValueDescription(String name) { + OptionValueDescription getOptionValueDescription(String name) { return impl.getOptionValueDescription(name); } 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 e0fc06249e7375..f599cea36bf99f 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -41,6 +41,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import javax.annotation.Nullable; /** * The implementation of the options parser. This is intentionally package @@ -350,41 +351,66 @@ OptionDescription getOptionDescription(String name) throws OptionsParsingExcepti optionsData.getDefaultValue(field), optionsData.getConverter(field), optionsData.getAllowMultiple(field), - getExpansionDescriptions( - optionsData.getEvaluatedExpansion(field), - /* expandedFrom */ name, - /* implicitDependant */ null), - getExpansionDescriptions( - ImmutableList.copyOf(optionAnnotation.implicitRequirements()), - /* expandedFrom */ null, - /* implicitDependant */ name)); + optionsData.getExpansionDataForField(field), + getImplicitDependantDescriptions( + ImmutableList.copyOf(optionAnnotation.implicitRequirements()), name)); } /** - * @return A list of the descriptions corresponding to the list of unparsed flags passed in. These - * descriptions are are divorced from the command line - there is no correct priority or + * @return A list of the descriptions corresponding to the implicit dependant flags passed in. + * These descriptions are are divorced from the command line - there is no correct priority or * source for these, as they are not actually set values. The value itself is also a string, * no conversion has taken place. */ - private ImmutableList getExpansionDescriptions( - ImmutableList optionStrings, String expandedFrom, String implicitDependant) - throws OptionsParsingException { + private ImmutableList getImplicitDependantDescriptions( + ImmutableList options, String implicitDependant) throws OptionsParsingException { + ImmutableList.Builder builder = ImmutableList.builder(); + Iterator optionsIterator = options.iterator(); + + while (optionsIterator.hasNext()) { + String unparsedFlagExpression = optionsIterator.next(); + ParseOptionResult parseResult = parseOption(unparsedFlagExpression, optionsIterator); + builder.add( + new OptionValueDescription( + parseResult.option.name(), + parseResult.value, + /* value */ null, + /* priority */ null, + /* source */ null, + implicitDependant, + /* expendedFrom */ null, + optionsData.getAllowMultiple(parseResult.field))); + } + return builder.build(); + } + + /** + * @return A list of the descriptions corresponding to options expanded from the flag for the + * given value. These descriptions are are divorced from the command line - there is no + * correct priority or source for these, as they are not actually set values. The value itself + * is also a string, no conversion has taken place. + */ + ImmutableList getExpansionOptionValueDescriptions( + String flagName, @Nullable String flagValue) throws OptionsParsingException { ImmutableList.Builder builder = ImmutableList.builder(); - ImmutableList options = ImmutableList.copyOf(optionStrings); + Field field = optionsData.getFieldFromName(flagName); + + ImmutableList options = optionsData.getEvaluatedExpansion(field, flagValue); Iterator optionsIterator = options.iterator(); while (optionsIterator.hasNext()) { String unparsedFlagExpression = optionsIterator.next(); ParseOptionResult parseResult = parseOption(unparsedFlagExpression, optionsIterator); - builder.add(new OptionValueDescription( - parseResult.option.name(), - parseResult.value, - /* value */ null, - /* priority */ null, - /* source */null, - implicitDependant, - expandedFrom, - optionsData.getAllowMultiple(parseResult.field))); + builder.add( + new OptionValueDescription( + parseResult.option.name(), + parseResult.value, + /* value */ null, + /* priority */ null, + /* source */ null, + /* implicitDependant */ null, + flagName, + optionsData.getAllowMultiple(parseResult.field))); } return builder.build(); } @@ -444,7 +470,7 @@ private List parse( ParseOptionResult parseOptionResult = parseOption(arg, argsIterator); Field field = parseOptionResult.field; Option option = parseOptionResult.option; - String value = parseOptionResult.value; + @Nullable String value = parseOptionResult.value; final String originalName = option.name(); @@ -498,8 +524,9 @@ private List parse( } // Handle expansion options. - ImmutableList expansion = optionsData.getEvaluatedExpansion(field); - if (!expansion.isEmpty()) { + if (OptionsData.isExpansionOption(field.getAnnotation(Option.class))) { + ImmutableList expansion = optionsData.getEvaluatedExpansion(field, value); + Function expansionSourceFunction = Functions.constant( "expanded from option --" @@ -580,9 +607,9 @@ private List parse( private static final class ParseOptionResult { final Field field; final Option option; - final String value; + @Nullable final String value; - ParseOptionResult(Field field, Option option, String value) { + ParseOptionResult(Field field, Option option, @Nullable String value) { this.field = field; this.option = option; this.value = value; 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 742e9f98f84a41..5f7c48aad15ed3 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsUsage.java +++ b/src/main/java/com/google/devtools/common/options/OptionsUsage.java @@ -81,15 +81,22 @@ static String paragraphFill(String in, int indent, int width) { /** * Returns the expansion for an option, to the extent known. Precisely, if an {@link OptionsData} - * object is supplied, the expansion is read from that. Otherwise, the annotation is inspected: If - * the annotation uses {@link Option#expansion} it is returned, and if it uses {@link - * Option#expansionFunction} null is returned, indicating a lack of definite information. In all - * cases, when the option is not an expansion option, an empty list is returned. + * object is supplied, the expansion is read from that if the expansion function doesn't take an + * argument. Otherwise, the annotation is inspected: If the annotation uses {@link + * Option#expansion} it is returned, and if it uses {@link Option#expansionFunction} null is + * returned, indicating a lack of definite information. In all cases, when the option is not an + * expansion option, an empty list is returned. */ private static @Nullable ImmutableList getExpansionIfKnown( Field optionField, Option annotation, @Nullable OptionsData optionsData) { if (optionsData != null) { - return optionsData.getEvaluatedExpansion(optionField); + try { + return optionsData.getEvaluatedExpansion(optionField, null); + } catch (ExpansionNeedsValueException e) { + return null; + } catch (OptionsParsingException e) { + throw new IllegalStateException("Error expanding void expansion function: ", e); + } } else { if (OptionsData.usesExpansionFunction(annotation)) { return null; diff --git a/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java b/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java index 7f91d173e0512f..a5ff1503dcb840 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java @@ -21,9 +21,9 @@ import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy; import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.UseDefault; import com.google.devtools.common.options.Converters; +import com.google.devtools.common.options.ExpansionContext; import com.google.devtools.common.options.ExpansionFunction; import com.google.devtools.common.options.InvocationPolicyEnforcer; -import com.google.devtools.common.options.IsolatedOptionsData; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionsBase; @@ -110,7 +110,7 @@ public static class ExampleExpansionOptions extends OptionsBase { /** Dummy comment (linter suppression) */ public static class YExpansion implements ExpansionFunction { @Override - public ImmutableList getExpansion(IsolatedOptionsData optionsData) { + public ImmutableList getExpansion(ExpansionContext context) { return ImmutableList.of("--noY"); } } diff --git a/src/test/java/com/google/devtools/common/options/InvocationPolicySetValueTest.java b/src/test/java/com/google/devtools/common/options/InvocationPolicySetValueTest.java index beb0005a123264..430366579a86ad 100644 --- a/src/test/java/com/google/devtools/common/options/InvocationPolicySetValueTest.java +++ b/src/test/java/com/google/devtools/common/options/InvocationPolicySetValueTest.java @@ -201,14 +201,14 @@ public void testSetValueAppendsToMultipleValuedFlag() throws Exception { TEST_STRING_POLICY_VALUE_2) .inOrder(); } - + @Test public void testSetValueWithExpansionFlags() throws Exception { InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); - invocationPolicyBuilder.addFlagPoliciesBuilder() + invocationPolicyBuilder + .addFlagPoliciesBuilder() .setFlagName("test_expansion") - .getSetValueBuilder() - .addFlagValue("true"); // this value is arbitrary, the value for a Void flag is ignored + .getSetValueBuilder(); // This value must be empty for a Void flag. InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); // Unrelated flag, but --test_expansion is not set @@ -231,6 +231,80 @@ public void testSetValueWithExpansionFlags() throws Exception { assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_TEST_EXPANSION); } + @Test + public void testSetValueWithExpansionFunctionFlags() throws Exception { + InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); + invocationPolicyBuilder + .addFlagPoliciesBuilder() + .setFlagName("test_expansion_function") + .getSetValueBuilder() + .addFlagValue(TestOptions.TEST_EXPANSION_FUNCTION_ACCEPTED_VALUE); + + InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); + // Unrelated flag, but --test_expansion_function is not set + parser.parse("--test_string=throwaway value"); + + // The flags that --test_expansion_function expands into should still be their default values + TestOptions testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_DEFAULT); + + enforcer.enforce(parser, BUILD_COMMAND); + + // After policy enforcement, the flags should be the values from + // --test_expansion_function=valueA + testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_EXPANSION_FUNCTION_VALUE); + } + + @Test + public void testSetValueWithExpansionFunctionFlagsDefault() throws Exception { + InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); + invocationPolicyBuilder + .addFlagPoliciesBuilder() + .setFlagName("test_expansion_function") + .getSetValueBuilder(); + + InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); + // Unrelated flag, but --test_expansion_function is not set + parser.parse("--test_string=throwaway value"); + + // The flags that --test_expansion_function expands into should still be their default values + TestOptions testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_DEFAULT); + + try { + enforcer.enforce(parser, BUILD_COMMAND); + fail(); + } catch (OptionsParsingException e) { + assertThat(e).hasMessage("Expansion value not set."); + } + } + + @Test + public void testSetValueWithExpansionFunctionFlagsWrongValue() throws Exception { + InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); + invocationPolicyBuilder + .addFlagPoliciesBuilder() + .setFlagName("test_expansion_function") + .getSetValueBuilder() + .addFlagValue("unknown_value"); + + InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); + // Unrelated flag, but --test_expansion_function is not set + parser.parse("--test_string=throwaway value"); + + // The flags that --test_expansion_function expands into should still be their default values + TestOptions testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_DEFAULT); + + try { + enforcer.enforce(parser, BUILD_COMMAND); + fail(); + } catch (OptionsParsingException e) { + assertThat(e).hasMessage("Unrecognized expansion value: unknown_value"); + } + } + @Test public void testOverridableSetValueWithExpansionFlags() throws Exception { InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); @@ -264,14 +338,40 @@ public void testOverridableSetValueWithExpansionFlags() throws Exception { assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_TEST_EXPANSION); } + @Test + public void testOverridableSetValueWithExpansionFunction() throws Exception { + InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); + invocationPolicyBuilder + .addFlagPoliciesBuilder() + .setFlagName("test_expansion_function") + .getSetValueBuilder() + .addFlagValue(TestOptions.TEST_EXPANSION_FUNCTION_ACCEPTED_VALUE) + .setOverridable(true); + + InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); + // Unrelated flag, but --test_expansion_function is not set + parser.parse("--expanded_d=value that overrides"); + + // The flags that --test_expansion_function expands into should still be their default values + // except for the explicitly marked flag. + TestOptions testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo("value that overrides"); + + enforcer.enforce(parser, "build"); + + // After policy enforcement, the flags should be the values from --test_expansion_function, + // except for the user-set value, since the expansion flag was set to overridable. + testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo("value that overrides"); + } + @Test public void testOverridableSetValueWithExpansionToRepeatingFlag() throws Exception { InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); invocationPolicyBuilder .addFlagPoliciesBuilder() .setFlagName("test_expansion_to_repeatable") - .getSetValueBuilder() - .addFlagValue("") // this value is arbitrary, the value for a Void flag is ignored + .getSetValueBuilder() // This value must be empty for a Void flag. .setOverridable(true); InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); @@ -298,8 +398,7 @@ public void testNonOverridableSetValueWithExpansionFlags() throws Exception { invocationPolicyBuilder .addFlagPoliciesBuilder() .setFlagName("test_expansion") - .getSetValueBuilder() - .addFlagValue("") // this value is arbitrary, the value for a Void flag is ignored + .getSetValueBuilder() // This value must be empty for a Void flag. .setOverridable(false); InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); @@ -326,14 +425,41 @@ public void testNonOverridableSetValueWithExpansionFlags() throws Exception { assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_TEST_EXPANSION); } + @Test + public void testNonoverridableSetValueWithExpansionFlags() throws Exception { + InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); + invocationPolicyBuilder + .addFlagPoliciesBuilder() + .setFlagName("test_expansion_function") + .getSetValueBuilder() + .addFlagValue(TestOptions.TEST_EXPANSION_FUNCTION_ACCEPTED_VALUE) + .setOverridable(false); + + InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); + // Unrelated flag, but --test_expansion_function is not set + parser.parse("--expanded_d=value to override"); + + // The flags that --test_expansion_function expands into should still be their default values + // except for the explicitly marked flag. + TestOptions testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo("value to override"); + + enforcer.enforce(parser, "build"); + + // After policy enforcement, the flags should be the values from --test_expansion_function, + // including the value that the user tried to set, since the expansion flag was set + // non-overridably. + testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_EXPANSION_FUNCTION_VALUE); + } + @Test public void testNonOverridableSetValueWithExpansionToRepeatingFlag() throws Exception { InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); invocationPolicyBuilder .addFlagPoliciesBuilder() .setFlagName("test_expansion_to_repeatable") - .getSetValueBuilder() - .addFlagValue("") // this value is arbitrary, the value for a Void flag is ignored + .getSetValueBuilder() // This value must be empty for a Void flag. .setOverridable(false); InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); diff --git a/src/test/java/com/google/devtools/common/options/InvocationPolicyUseDefaultTest.java b/src/test/java/com/google/devtools/common/options/InvocationPolicyUseDefaultTest.java index 0a046e50947648..975befb1593d02 100644 --- a/src/test/java/com/google/devtools/common/options/InvocationPolicyUseDefaultTest.java +++ b/src/test/java/com/google/devtools/common/options/InvocationPolicyUseDefaultTest.java @@ -14,6 +14,7 @@ package com.google.devtools.common.options; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.fail; import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy; import org.junit.Test; @@ -100,6 +101,50 @@ public void testUseDefaultWithExpansionFlags() throws Exception { assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_DEFAULT); } + @Test + public void testUseDefaultWithVoidExpansionFunction() throws Exception { + InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); + invocationPolicyBuilder + .addFlagPoliciesBuilder() + .setFlagName("test_void_expansion_function") + .getUseDefaultBuilder(); + + InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); + parser.parse("--expanded_d=value to override"); + + TestOptions testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo("value to override"); + + enforcer.enforce(parser, BUILD_COMMAND); + + // After policy enforcement, all the flags that --test_void_expansion_function expanded into + // should be back to their default values. + testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_DEFAULT); + } + + @Test + public void testUseDefaultWithExpansionFunction() throws Exception { + InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); + invocationPolicyBuilder + .addFlagPoliciesBuilder() + .setFlagName("test_expansion_function") + .getUseDefaultBuilder(); + + InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); + parser.parse("--expanded_d=value to override"); + + TestOptions testOptions = getTestOptions(); + assertThat(testOptions.expandedD).isEqualTo("value to override"); + + try { + enforcer.enforce(parser, BUILD_COMMAND); + fail(); + } catch (OptionsParsingException e) { + assertThat(e).hasMessage("Expansion value not set."); + } + } + @Test public void testUseDefaultWithExpansionFlagAndLaterOverride() throws Exception { InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); 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 d2e2faec1eaae2..18b6da90656225 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java @@ -868,7 +868,7 @@ public static class ConflictingExpansionsOptions extends OptionsBase { /** ExpFunc */ public static class ExpFunc implements ExpansionFunction { @Override - public ImmutableList getExpansion(IsolatedOptionsData optionsData) { + public ImmutableList getExpansion(ExpansionContext context) { return ImmutableList.of("--yyy"); } } @@ -900,7 +900,7 @@ public static class NullExpansionsOptions extends OptionsBase { /** ExpFunc */ public static class ExpFunc implements ExpansionFunction { @Override - public ImmutableList getExpansion(IsolatedOptionsData optionsData) { + public ImmutableList getExpansion(ExpansionContext context) { return null; } } @@ -917,8 +917,35 @@ public void nullExpansions() throws Exception { newOptionsParser(NullExpansionsOptions.class); fail("Should have failed due to null expansion function result"); } catch (OptionsParser.ConstructionException e) { - assertThat(e).hasCauseThat().isInstanceOf(NullPointerException.class); - assertThat(e).hasCauseThat().hasMessageThat().contains("null value in entry"); + assertThat(e).hasCauseThat().isInstanceOf(IllegalStateException.class); + } + } + + /** NullExpansionOptions */ + public static class NullExpansionsWithArgumentOptions extends OptionsBase { + + /** ExpFunc */ + public static class ExpFunc implements ExpansionFunction { + @Override + public ImmutableList getExpansion(ExpansionContext context) { + return null; + } + } + + @Option(name = "badness", expansionFunction = ExpFunc.class, defaultValue = "null") + public String badness; + } + + @Test + public void nullExpansionsWithArgument() throws Exception { + try { + // When an expansion takes a value, this exception should still happen at parse time. + newOptionsParser(NullExpansionsWithArgumentOptions.class); + fail("Should have failed due to null expansion function result"); + } catch (OptionsParser.ConstructionException e) { + assertThat(e) + .hasMessageThat() + .isEqualTo("Error calling expansion function for option: badness"); } } @@ -937,7 +964,7 @@ public static class ExpansionOptions extends OptionsBase { /** ExpFunc */ public static class ExpFunc implements ExpansionFunction { @Override - public ImmutableList getExpansion(IsolatedOptionsData optionsData) { + public ImmutableList getExpansion(ExpansionContext context) { return ImmutableList.of("--expands"); } } @@ -946,6 +973,29 @@ public ImmutableList getExpansion(IsolatedOptionsData optionsData) { public Void expandsByFunction; } + /** ExpansionMultipleOptions */ + public static class ExpansionMultipleOptions extends OptionsBase { + @Option(name = "underlying", defaultValue = "null", allowMultiple = true) + public List underlying; + + /** ExpFunc */ + public static class ExpFunc implements ExpansionFunction { + @Override + public ImmutableList getExpansion(ExpansionContext context) + throws OptionsParsingException { + String value = context.getUnparsedValue(); + if (value == null) { + throw new ExpansionNeedsValueException("No value given to 'expands_by_function'"); + } + + return ImmutableList.of("--underlying=pre_" + value, "--underlying=post_" + value); + } + } + + @Option(name = "expands_by_function", defaultValue = "null", expansionFunction = ExpFunc.class) + public Void expandsByFunction; + } + @Test public void describeOptionsWithExpansion() throws Exception { // We have to test this here rather than in OptionsTest because expansion functions require @@ -976,6 +1026,19 @@ public void overrideExplicitWithExpansion() throws Exception { assertThat(options.underlying).isEqualTo("from_expansion"); } + // Makes sure the expansion options are expanded in the right order if they affect flags that + // allow multiples. + @Test + public void multipleExpansionOptionsWithValue() throws Exception { + OptionsParser parser = OptionsParser.newOptionsParser(ExpansionMultipleOptions.class); + parser.parse( + OptionPriority.COMMAND_LINE, + null, + Arrays.asList("--expands_by_function=a", "--expands_by_function=b")); + ExpansionMultipleOptions options = parser.getOptions(ExpansionMultipleOptions.class); + assertThat(options.underlying).containsExactly("pre_a", "post_a", "pre_b", "post_b").inOrder(); + } + @Test public void overrideWithHigherPriority() throws Exception { OptionsParser parser = OptionsParser.newOptionsParser(NullTestOptions.class); diff --git a/src/test/java/com/google/devtools/common/options/OptionsTest.java b/src/test/java/com/google/devtools/common/options/OptionsTest.java index dbe680da5a89cd..bb0a22ef4e7747 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsTest.java @@ -71,9 +71,9 @@ public static class HttpOptions extends OptionsBase { /** SpecialExpansion */ public static class SpecialExpansion implements ExpansionFunction { @Override - public ImmutableList getExpansion(IsolatedOptionsData optionsData) { + public ImmutableList getExpansion(ExpansionContext context) { TreeSet flags = new TreeSet<>(); - for (Map.Entry entry : optionsData.getAllNamedFields()) { + for (Map.Entry entry : context.getOptionsData().getAllNamedFields()) { if (entry.getKey().startsWith("specialexp_")) { flags.add("--" + entry.getKey()); } @@ -82,6 +82,23 @@ public ImmutableList getExpansion(IsolatedOptionsData optionsData) { } } + /** VariableExpansion */ + public static class VariableExpansion implements ExpansionFunction { + @Override + public ImmutableList getExpansion(ExpansionContext context) + throws OptionsParsingException { + String value = context.getUnparsedValue(); + if (value == null) { + throw new ExpansionNeedsValueException("Expansion value not set."); + } + if (value.equals("foo_bar")) { + return ImmutableList.of("--specialexp_foo", "--specialexp_bar"); + } + + throw new OptionsParsingException("Unexpected expansion argument: " + value); + } + } + @Option(name = "specialexp_foo", defaultValue = "false") public boolean specialExpFoo; @@ -90,6 +107,9 @@ public ImmutableList getExpansion(IsolatedOptionsData optionsData) { @Option(name = "specialexp", defaultValue = "null", expansionFunction = SpecialExpansion.class) public Void specialExp; + + @Option(name = "dynamicexp", defaultValue = "null", expansionFunction = VariableExpansion.class) + public Void variableExpansion; } @Test @@ -539,4 +559,23 @@ public void expansionFunction() throws Exception { Options.parse(HttpOptions.class, new String[] {"--specialexp_foo", "--specialexp_bar"}); assertThat(options1.getOptions()).isEqualTo(options2.getOptions()); } + + @Test + public void dynamicExpansionFunctionWorks() throws Exception { + Options options1 = + Options.parse(HttpOptions.class, new String[] {"--dynamicexp=foo_bar"}); + Options options2 = + Options.parse(HttpOptions.class, new String[] {"--specialexp_foo", "--specialexp_bar"}); + assertThat(options1.getOptions()).isEqualTo(options2.getOptions()); + } + + @Test + public void dynamicExpansionFunctionUnknowValue() throws Exception { + try { + Options.parse(HttpOptions.class, new String[] {"--dynamicexp=foo"}); + fail("Unknown expansion argument should cause a failure."); + } catch (OptionsParsingException e) { + assertThat(e).hasMessage("Unexpected expansion argument: foo"); + } + } } 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 ce532bbafb6253..c9d606cf0c37fa 100644 --- a/src/test/java/com/google/devtools/common/options/TestOptions.java +++ b/src/test/java/com/google/devtools/common/options/TestOptions.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.common.options; +import com.google.common.collect.ImmutableList; import com.google.devtools.common.options.InvocationPolicyEnforcerTestBase.ToListConverter; import java.util.List; @@ -174,4 +175,47 @@ public class TestOptions extends OptionsBase { implicitRequirements = {"--test_implicit_requirement=" + TEST_IMPLICIT_REQUIREMENT_REQUIRED} ) public String testRecursiveImplicitRequirement; + + public static final String TEST_EXPANSION_FUNCTION_ACCEPTED_VALUE = "valueA"; + public static final String EXPANDED_D_EXPANSION_FUNCTION_VALUE = "expanded valueA"; + + /** Used for testing an expansion flag that requires a value. */ + public static class TestExpansionFunction implements ExpansionFunction { + @Override + public ImmutableList getExpansion(ExpansionContext expansionContext) + throws OptionsParsingException { + String value = expansionContext.getUnparsedValue(); + if (value == null) { + throw new ExpansionNeedsValueException("Expansion value not set."); + } else if (value.equals(TEST_EXPANSION_FUNCTION_ACCEPTED_VALUE)) { + return ImmutableList.of("--expanded_d", EXPANDED_D_EXPANSION_FUNCTION_VALUE); + } else { + throw new OptionsParsingException("Unrecognized expansion value: " + value); + } + } + } + + @Option( + name = "test_expansion_function", + defaultValue = "null", + expansionFunction = TestExpansionFunction.class + ) + public Void testExpansionFunction; + + public static final String EXPANDED_D_VOID_EXPANSION_FUNCTION_VALUE = "void expanded"; + + /** Used for testing an expansion flag that doesn't requires a value. */ + public static class TestVoidExpansionFunction implements ExpansionFunction { + @Override + public ImmutableList getExpansion(ExpansionContext expansionContext) { + return ImmutableList.of("--expanded_d", EXPANDED_D_VOID_EXPANSION_FUNCTION_VALUE); + } + } + + @Option( + name = "test_void_expansion_function", + defaultValue = "null", + expansionFunction = TestVoidExpansionFunction.class + ) + public Void testVoidExpansionFunction; }