Replace referrals to options by their name to option definitions.
Now that we have a standard way of referring to an option, remove all of the places that we were referring to them by their name. Since options can have multiple names, this is more clear and provides the additional information needed to understand the option. It also stops the habit of requesting unqualified strings, which was hard to read.
RELNOTES: None.
PiperOrigin-RevId: 168254584
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 37753a9..73c6c5e 100644
--- a/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
+++ b/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
@@ -14,6 +14,7 @@
package com.google.devtools.common.options;
import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
import com.google.common.base.Verify;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
@@ -51,7 +52,8 @@
private static final Logger logger = Logger.getLogger(InvocationPolicyEnforcer.class.getName());
- private static final Function<Object, String> INVOCATION_POLICY_SOURCE = o -> "Invocation policy";
+ private static final Function<OptionDefinition, String> INVOCATION_POLICY_SOURCE =
+ o -> "Invocation policy";
@Nullable private final InvocationPolicy invocationPolicy;
@@ -121,11 +123,11 @@
switch (flagPolicy.getOperationCase()) {
case SET_VALUE:
- applySetValueOperation(parser, flagPolicy, flagName, valueDescription, optionDescription);
+ applySetValueOperation(parser, flagPolicy, valueDescription, optionDescription);
break;
case USE_DEFAULT:
- applyUseDefaultOperation(parser, "UseDefault", flagName);
+ applyUseDefaultOperation(parser, "UseDefault", optionDescription.getOptionDefinition());
break;
case ALLOW_VALUES:
@@ -135,7 +137,6 @@
allowValues.getAllowedValuesList(),
allowValues.hasNewValue() ? allowValues.getNewValue() : null,
allowValues.hasUseDefault(),
- flagName,
valueDescription,
optionDescription);
break;
@@ -147,7 +148,6 @@
disallowValues.getDisallowedValuesList(),
disallowValues.hasNewValue() ? disallowValues.getNewValue() : null,
disallowValues.hasUseDefault(),
- flagName,
valueDescription,
optionDescription);
break;
@@ -238,7 +238,14 @@
return ImmutableList.of();
}
- String expansionFlagName = expansionPolicy.getFlagName();
+ Preconditions.checkArgument(
+ expansionPolicy
+ .getFlagName()
+ .equals(optionDescription.getOptionDefinition().getOptionName()),
+ String.format(
+ "The optionDescription provided (for flag %s) does not match the policy for flag %s.",
+ optionDescription.getOptionDefinition().getOptionName(),
+ expansionPolicy.getFlagName()));
ImmutableList.Builder<OptionValueDescription> resultsBuilder = ImmutableList.builder();
switch (expansionPolicy.getOperationCase()) {
@@ -248,16 +255,20 @@
if (setValue.getFlagValueCount() > 0) {
for (String value : setValue.getFlagValueList()) {
resultsBuilder.addAll(
- parser.getExpansionOptionValueDescriptions(expansionFlagName, value));
+ parser.getExpansionOptionValueDescriptions(
+ optionDescription.getOptionDefinition(), value));
}
} else {
resultsBuilder.addAll(
- parser.getExpansionOptionValueDescriptions(expansionFlagName, null));
+ parser.getExpansionOptionValueDescriptions(
+ optionDescription.getOptionDefinition(), null));
}
}
break;
case USE_DEFAULT:
- resultsBuilder.addAll(parser.getExpansionOptionValueDescriptions(expansionFlagName, null));
+ resultsBuilder.addAll(
+ parser.getExpansionOptionValueDescriptions(
+ optionDescription.getOptionDefinition(), null));
break;
case ALLOW_VALUES:
// All expansions originally given to the parser have been expanded by now, so these two
@@ -275,7 +286,8 @@
logger.warning(
String.format(
"Unknown operation '%s' from invocation policy for flag '%s'",
- expansionPolicy.getOperationCase(), expansionFlagName));
+ expansionPolicy.getOperationCase(),
+ optionDescription.getOptionDefinition().getOptionName()));
break;
}
@@ -318,7 +330,7 @@
// invocation policy does not change, so this can be a log level fine.
List<String> subflagNames = new ArrayList<>(subflags.size());
for (OptionValueDescription subflag : subflags) {
- subflagNames.add("--" + subflag.getName());
+ subflagNames.add("--" + subflag.getOptionDefinition().getOptionName());
}
logger.logp(
@@ -335,16 +347,16 @@
// Repeated flags are special, and could set multiple times in an expansion, with the user
// expecting both values to be valid. Collect these separately.
- Multimap<String, OptionValueDescription> repeatableSubflagsInSetValues =
+ Multimap<OptionDefinition, OptionValueDescription> repeatableSubflagsInSetValues =
ArrayListMultimap.create();
// Create a flag policy for the child that looks like the parent's policy "transferred" to its
// child. Note that this only makes sense for SetValue, when setting an expansion flag, or
// UseDefault, when preventing it from being set.
for (OptionValueDescription currentSubflag : subflags) {
- if (currentSubflag.getAllowMultiple()
+ if (currentSubflag.getOptionDefinition().allowsMultiple()
&& originalPolicy.getOperationCase().equals(OperationCase.SET_VALUE)) {
- repeatableSubflagsInSetValues.put(currentSubflag.getName(), currentSubflag);
+ repeatableSubflagsInSetValues.put(currentSubflag.getOptionDefinition(), currentSubflag);
} else {
FlagPolicy subflagAsPolicy =
getSingleValueSubflagAsPolicy(currentSubflag, originalPolicy, isExpansion);
@@ -356,15 +368,13 @@
// If there are any repeatable flag SetValues, deal with them together now.
// Note that expansion flags have no value, and so cannot have multiple values either.
// Skipping the recursion above is fine.
- for (String repeatableFlag : repeatableSubflagsInSetValues.keySet()) {
+ for (OptionDefinition repeatableFlag : repeatableSubflagsInSetValues.keySet()) {
int numValues = repeatableSubflagsInSetValues.get(repeatableFlag).size();
ArrayList<String> newValues = new ArrayList<>(numValues);
for (OptionValueDescription setValue : repeatableSubflagsInSetValues.get(repeatableFlag)) {
newValues.add(setValue.getOriginalValueString());
}
- expandedPolicies.add(
- getSetValueSubflagAsPolicy(
- repeatableFlag, newValues, /* allowMultiple */ true, originalPolicy));
+ expandedPolicies.add(getSetValueSubflagAsPolicy(repeatableFlag, newValues, originalPolicy));
}
// Don't add the original policy if it was an expansion flag, which have no value, but do add
@@ -381,21 +391,17 @@
* policies that set the flag, and so interact with repeatable flags, flags that can be set
* multiple times, in subtle ways.
*
- * @param subflagName, the flag the SetValue'd expansion flag expands to.
+ * @param subflag, the definition of the flag the SetValue'd expansion flag expands to.
* @param subflagValue, the values that the SetValue'd expansion flag expands to for this flag.
- * @param allowMultiple, whether the flag is multivalued.
* @param originalPolicy, the original policy on the expansion flag.
* @return the flag policy for the subflag given, this will be part of the expanded form of the
- * SetValue policy on the original flag.
+ * SetValue policy on the original flag.
*/
private static FlagPolicy getSetValueSubflagAsPolicy(
- String subflagName,
- List<String> subflagValue,
- boolean allowMultiple,
- FlagPolicy originalPolicy) {
+ OptionDefinition subflag, List<String> subflagValue, FlagPolicy originalPolicy) {
// Some sanity checks.
Verify.verify(originalPolicy.getOperationCase().equals(OperationCase.SET_VALUE));
- if (!allowMultiple) {
+ if (!subflag.allowsMultiple()) {
Verify.verify(subflagValue.size() <= 1);
}
@@ -405,7 +411,7 @@
for (String value : subflagValue) {
setValueExpansion.addFlagValue(value);
}
- if (allowMultiple) {
+ if (subflag.allowsMultiple()) {
setValueExpansion.setAppend(originalPolicy.getSetValue().getOverridable());
} else {
setValueExpansion.setOverridable(originalPolicy.getSetValue().getOverridable());
@@ -413,10 +419,10 @@
// Commands from the original policy, flag name of the expansion
return FlagPolicy.newBuilder()
- .addAllCommands(originalPolicy.getCommandsList())
- .setFlagName(subflagName)
- .setSetValue(setValueExpansion)
- .build();
+ .addAllCommands(originalPolicy.getCommandsList())
+ .setFlagName(subflag.getOptionName())
+ .setSetValue(setValueExpansion)
+ .build();
}
/**
@@ -429,7 +435,7 @@
FlagPolicy subflagAsPolicy = null;
switch (originalPolicy.getOperationCase()) {
case SET_VALUE:
- if (currentSubflag.getAllowMultiple()) {
+ if (currentSubflag.getOptionDefinition().allowsMultiple()) {
throw new AssertionError(
"SetValue subflags with allowMultiple should have been dealt with separately and "
+ "accumulated into a single FlagPolicy.");
@@ -444,7 +450,7 @@
}
subflagAsPolicy =
getSetValueSubflagAsPolicy(
- currentSubflag.getName(), subflagValue, /*allowMultiple=*/ false, originalPolicy);
+ currentSubflag.getOptionDefinition(), subflagValue, originalPolicy);
break;
case USE_DEFAULT:
@@ -452,10 +458,8 @@
subflagAsPolicy =
FlagPolicy.newBuilder()
.addAllCommands(originalPolicy.getCommandsList())
- .setFlagName(currentSubflag.getName())
- .setUseDefault(
- UseDefault
- .getDefaultInstance())
+ .setFlagName(currentSubflag.getOptionDefinition().getOptionName())
+ .setUseDefault(UseDefault.getDefaultInstance())
.build();
break;
@@ -499,19 +503,18 @@
private static void applySetValueOperation(
OptionsParser parser,
FlagPolicy flagPolicy,
- String flagName,
OptionValueDescription valueDescription,
OptionDescription optionDescription)
throws OptionsParsingException {
-
SetValue setValue = flagPolicy.getSetValue();
+ OptionDefinition optionDefinition = optionDescription.getOptionDefinition();
// SetValue.flag_value must have at least 1 value.
if (setValue.getFlagValueCount() == 0) {
throw new OptionsParsingException(
String.format(
"SetValue operation from invocation policy for flag '%s' does not have a value",
- flagName));
+ optionDefinition.getOptionName()));
}
// Flag must allow multiple values if multiple values are specified by the policy.
@@ -521,7 +524,7 @@
String.format(
"SetValue operation from invocation policy sets multiple values for flag '%s' which "
+ "does not allow multiple values",
- flagName));
+ optionDefinition.getOptionName()));
}
if (setValue.getOverridable() && valueDescription != null) {
@@ -532,13 +535,13 @@
+ "because the invocation policy specifying the value(s) '%s' is overridable",
valueDescription.getValue(),
valueDescription.getSource(),
- flagName,
+ optionDefinition.getOptionName(),
setValue.getFlagValueList());
} else {
if (!setValue.getAppend()) {
// Clear the value in case the flag is a repeated flag so that values don't accumulate.
- parser.clearValue(flagName);
+ parser.clearValue(optionDescription.getOptionDefinition());
}
// Set all the flag values from the policy.
@@ -547,24 +550,28 @@
logInApplySetValueOperation(
"Setting value for flag '%s' from invocation "
+ "policy to '%s', overriding the default value '%s'",
- flagName, flagValue, optionDescription.getOptionDefinition().getDefaultValue());
+ optionDefinition.getOptionName(), flagValue, optionDefinition.getDefaultValue());
} else {
logInApplySetValueOperation(
"Setting value for flag '%s' from invocation "
+ "policy to '%s', overriding value '%s' from '%s'",
- flagName, flagValue, valueDescription.getValue(), valueDescription.getSource());
+ optionDefinition.getOptionName(),
+ flagValue,
+ valueDescription.getValue(),
+ valueDescription.getSource());
}
- setFlagValue(parser, flagName, flagValue);
+ setFlagValue(parser, optionDefinition, flagValue);
}
}
}
private static void applyUseDefaultOperation(
- OptionsParser parser, String policyType, String flagName) throws OptionsParsingException {
- OptionValueDescription clearedValueDescription = parser.clearValue(flagName);
+ OptionsParser parser, String policyType, OptionDefinition option)
+ throws OptionsParsingException {
+ OptionValueDescription clearedValueDescription = parser.clearValue(option);
if (clearedValueDescription != null) {
// Log the removed value.
- String clearedFlagName = clearedValueDescription.getName();
+ String clearedFlagName = clearedValueDescription.getOptionDefinition().getOptionName();
String originalValue = clearedValueDescription.getValue().toString();
String source = clearedValueDescription.getSource();
@@ -625,11 +632,10 @@
List<String> policyValues,
String newValue,
boolean useDefault,
- String flagName,
OptionValueDescription valueDescription,
OptionDescription optionDescription)
throws OptionsParsingException {
-
+ OptionDefinition optionDefinition = optionDescription.getOptionDefinition();
// Convert all the allowed values from strings to real objects using the options'
// converters so that they can be checked for equality using real .equals() instead
// of string comparison. For example, "--foo=0", "--foo=false", "--nofoo", and "-f-"
@@ -637,18 +643,15 @@
// can be arbitrarily complex.
Set<Object> convertedPolicyValues = new HashSet<>();
for (String value : policyValues) {
- Object convertedValue =
- optionDescription.getOptionDefinition().getConverter().convert(value);
+ Object convertedValue = optionDefinition.getConverter().convert(value);
// Some converters return lists, and if the flag is a repeatable flag, the items in the
// list from the converter should be added, and not the list itself. Otherwise the items
// from invocation policy will be compared to lists, which will never work.
// See OptionsParserImpl.ParsedOptionEntry.addValue.
- if (optionDescription.getOptionDefinition().allowsMultiple()
- && convertedValue instanceof List<?>) {
+ if (optionDefinition.allowsMultiple() && convertedValue instanceof List<?>) {
convertedPolicyValues.addAll((List<?>) convertedValue);
} else {
- convertedPolicyValues.add(
- optionDescription.getOptionDefinition().getConverter().convert(value));
+ convertedPolicyValues.add(optionDefinition.getConverter().convert(value));
}
}
@@ -666,7 +669,9 @@
String.format(
"%sValues policy disallows the default value '%s' for flag '%s' but also "
+ "specifies to use the default value",
- policyType, optionDescription.getOptionDefinition().getDefaultValue(), flagName));
+ policyType,
+ optionDefinition.getDefaultValue(),
+ optionDefinition.getOptionName()));
}
}
@@ -676,35 +681,28 @@
// the flag allowing multiple values, however, flags that allow multiple values cannot have
// default values, and their value is always the empty list if they haven't been specified,
// which is why new_default_value is not a repeated field.
- checkDefaultValue(
- parser,
- policyValues,
- newValue,
- flagName,
- optionDescription,
- convertedPolicyValues);
+ checkDefaultValue(parser, optionDescription, policyValues, newValue, convertedPolicyValues);
} else {
checkUserValue(
parser,
+ optionDescription,
+ valueDescription,
policyValues,
newValue,
useDefault,
- flagName,
- valueDescription,
- optionDescription,
convertedPolicyValues);
}
}
void checkDefaultValue(
OptionsParser parser,
+ OptionDescription optionDescription,
List<String> policyValues,
String newValue,
- String flagName,
- OptionDescription optionDescription,
Set<Object> convertedPolicyValues)
throws OptionsParsingException {
+ OptionDefinition optionDefinition = optionDescription.getOptionDefinition();
if (!isFlagValueAllowed(
convertedPolicyValues, optionDescription.getOptionDefinition().getDefaultValue())) {
if (newValue != null) {
@@ -713,13 +711,13 @@
String.format(
"Overriding default value '%s' for flag '%s' with value '%s' "
+ "specified by invocation policy. %sed values are: %s",
- optionDescription.getOptionDefinition().getDefaultValue(),
- flagName,
+ optionDefinition.getDefaultValue(),
+ optionDefinition.getOptionName(),
newValue,
policyType,
policyValues));
- parser.clearValue(flagName);
- setFlagValue(parser, flagName, newValue);
+ parser.clearValue(optionDefinition);
+ setFlagValue(parser, optionDefinition, newValue);
} else {
// The operation disallows the default value, but doesn't supply a new value.
throw new OptionsParsingException(
@@ -728,7 +726,7 @@
+ "the policy does not provide a new value. "
+ "%sed values are: %s",
optionDescription.getOptionDefinition().getDefaultValue(),
- flagName,
+ optionDefinition.getOptionName(),
policyType,
policyValues));
}
@@ -737,15 +735,14 @@
void checkUserValue(
OptionsParser parser,
+ OptionDescription optionDescription,
+ OptionValueDescription valueDescription,
List<String> policyValues,
String newValue,
boolean useDefault,
- String flagName,
- OptionValueDescription valueDescription,
- OptionDescription optionDescription,
Set<Object> convertedPolicyValues)
throws OptionsParsingException {
-
+ OptionDefinition option = optionDescription.getOptionDefinition();
if (optionDescription.getOptionDefinition().allowsMultiple()) {
// allowMultiple requires that the type of the option be List<T>, so cast from Object
// to List<?>.
@@ -753,16 +750,13 @@
for (Object value : optionValues) {
if (!isFlagValueAllowed(convertedPolicyValues, value)) {
if (useDefault) {
- applyUseDefaultOperation(parser, policyType + "Values", flagName);
+ applyUseDefaultOperation(parser, policyType + "Values", option);
} else {
throw new OptionsParsingException(
String.format(
"Flag value '%s' for flag '%s' is not allowed by invocation policy. "
+ "%sed values are: %s",
- value,
- flagName,
- policyType,
- policyValues));
+ value, option.getOptionName(), policyType, policyValues));
}
}
}
@@ -775,33 +769,34 @@
String.format(
"Overriding disallowed value '%s' for flag '%s' with value '%s' "
+ "specified by invocation policy. %sed values are: %s",
- valueDescription.getValue(), flagName, newValue, policyType, policyValues));
- parser.clearValue(flagName);
- setFlagValue(parser, flagName, newValue);
+ valueDescription.getValue(),
+ option.getOptionName(),
+ newValue,
+ policyType,
+ policyValues));
+ parser.clearValue(option);
+ setFlagValue(parser, option, newValue);
} else if (useDefault) {
- applyUseDefaultOperation(parser, policyType + "Values", flagName);
+ applyUseDefaultOperation(parser, policyType + "Values", option);
} else {
throw new OptionsParsingException(
String.format(
"Flag value '%s' for flag '%s' is not allowed by invocation policy and the "
+ "policy does not specify a new value. %sed values are: %s",
- valueDescription.getValue(),
- flagName,
- policyType,
- policyValues));
+ valueDescription.getValue(), option.getOptionName(), policyType, policyValues));
}
}
}
}
}
- private static void setFlagValue(OptionsParser parser, String flagName, String flagValue)
+ private static void setFlagValue(OptionsParser parser, OptionDefinition flag, String flagValue)
throws OptionsParsingException {
parser.parseWithSourceFunction(
OptionPriority.INVOCATION_POLICY,
INVOCATION_POLICY_SOURCE,
- ImmutableList.of(String.format("--%s=%s", flagName, flagValue)));
+ ImmutableList.of(String.format("--%s=%s", flag.getOptionName(), flagValue)));
}
}
