Clean up InvocationPolicy's use of OptionDescription.
OptionDescription is basically a hack to get the expansion data for options from outside the options parser, but it was being used at various points of invocation policy enforcement. In order to correctly track option origin, we only want to get this information once. Do it during the invocation policy expansion stage, not at enforcement, so that we track the information of the option's origin in the original invocation policy passed to the enforcer, not the expanded one.
PiperOrigin-RevId: 171661669
GitOrigin-RevId: ca74482825e0c0ca5d119eceab74ba4292428557
Change-Id: Ib57ed5c67054c051a420507506807a8eb6809f47
diff --git a/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java b/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
index 87fb577..ee0e487 100644
--- a/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
+++ b/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
@@ -79,6 +79,16 @@
this.loglevel = loglevel;
}
+ private static final class FlagPolicyWithContext {
+ private final FlagPolicy policy;
+ private final OptionDescription description;
+
+ public FlagPolicyWithContext(FlagPolicy policy, OptionDescription description) {
+ this.policy = policy;
+ this.description = description;
+ }
+ }
+
public InvocationPolicy getInvocationPolicy() {
return invocationPolicy;
}
@@ -110,11 +120,11 @@
// The effective policy returned is expanded, filtered for applicable commands, and cleaned of
// redundancies and conflicts.
- List<FlagPolicy> effectivePolicies =
+ List<FlagPolicyWithContext> effectivePolicies =
getEffectivePolicies(invocationPolicy, parser, command, loglevel);
- for (FlagPolicy flagPolicy : effectivePolicies) {
- String flagName = flagPolicy.getFlagName();
+ for (FlagPolicyWithContext flagPolicy : effectivePolicies) {
+ String flagName = flagPolicy.policy.getFlagName();
OptionValueDescription valueDescription;
try {
@@ -129,26 +139,23 @@
continue;
}
- OptionDescription optionDescription =
- parser.getOptionDescription(
- flagName, OptionPriority.INVOCATION_POLICY, INVOCATION_POLICY_SOURCE);
// getOptionDescription() will return null if the option does not exist, however
// getOptionValueDescription() above would have thrown an IllegalArgumentException if that
// were the case.
- Verify.verifyNotNull(optionDescription);
+ Verify.verifyNotNull(flagPolicy.description);
- switch (flagPolicy.getOperationCase()) {
+ switch (flagPolicy.policy.getOperationCase()) {
case SET_VALUE:
- applySetValueOperation(parser, flagPolicy, valueDescription, optionDescription, loglevel);
+ applySetValueOperation(parser, flagPolicy, valueDescription, loglevel);
break;
case USE_DEFAULT:
applyUseDefaultOperation(
- parser, "UseDefault", optionDescription.getOptionDefinition(), loglevel);
+ parser, "UseDefault", flagPolicy.description.getOptionDefinition(), loglevel);
break;
case ALLOW_VALUES:
- AllowValues allowValues = flagPolicy.getAllowValues();
+ AllowValues allowValues = flagPolicy.policy.getAllowValues();
FilterValueOperation.AllowValueOperation allowValueOperation =
new FilterValueOperation.AllowValueOperation(loglevel);
allowValueOperation.apply(
@@ -157,11 +164,11 @@
allowValues.hasNewValue() ? allowValues.getNewValue() : null,
allowValues.hasUseDefault(),
valueDescription,
- optionDescription);
+ flagPolicy.description);
break;
case DISALLOW_VALUES:
- DisallowValues disallowValues = flagPolicy.getDisallowValues();
+ DisallowValues disallowValues = flagPolicy.policy.getDisallowValues();
FilterValueOperation.DisallowValueOperation disallowValueOperation =
new FilterValueOperation.DisallowValueOperation(loglevel);
disallowValueOperation.apply(
@@ -170,7 +177,7 @@
disallowValues.hasNewValue() ? disallowValues.getNewValue() : null,
disallowValues.hasUseDefault(),
valueDescription,
- optionDescription);
+ flagPolicy.description);
break;
case OPERATION_NOT_SET:
@@ -180,7 +187,7 @@
logger.warning(
String.format(
"Unknown operation '%s' from invocation policy for flag '%s'",
- flagPolicy.getOperationCase(), flagName));
+ flagPolicy.policy.getOperationCase(), flagName));
break;
}
}
@@ -202,12 +209,26 @@
return !Collections.disjoint(policy.getCommandsList(), applicableCommands);
}
+ /** Returns the expanded and filtered policy that would be enforced for the given command. */
+ public static InvocationPolicy getEffectiveInvocationPolicy(
+ InvocationPolicy invocationPolicy, OptionsParser parser, String command, Level loglevel)
+ throws OptionsParsingException {
+ ImmutableList<FlagPolicyWithContext> effectivePolicies =
+ getEffectivePolicies(invocationPolicy, parser, command, loglevel);
+
+ InvocationPolicy.Builder builder = InvocationPolicy.newBuilder();
+ for (FlagPolicyWithContext policyWithContext : effectivePolicies) {
+ builder.addFlagPolicies(policyWithContext.policy);
+ }
+ return builder.build();
+ }
+
/**
* Takes the provided policy and processes it to the form that can be used on the user options.
*
* <p>Expands any policies on expansion flags.
*/
- public static ImmutableList<FlagPolicy> getEffectivePolicies(
+ private static ImmutableList<FlagPolicyWithContext> getEffectivePolicies(
InvocationPolicy invocationPolicy, OptionsParser parser, String command, Level loglevel)
throws OptionsParsingException {
if (invocationPolicy == null) {
@@ -220,20 +241,35 @@
: CommandNameCache.CommandNameCacheInstance.INSTANCE.get(command);
// Expand all policies to transfer policies on expansion flags to policies on the child flags.
- List<FlagPolicy> expandedPolicies = new ArrayList<>();
+ List<FlagPolicyWithContext> expandedPolicies = new ArrayList<>();
for (FlagPolicy policy : invocationPolicy.getFlagPoliciesList()) {
if (!policyApplies(policy, commandAndParentCommands)) {
// Only keep and expand policies that are applicable to the current command.
continue;
}
- List<FlagPolicy> policies = expandPolicy(policy, parser, loglevel);
+ OptionDescription optionDescription =
+ parser.getOptionDescription(
+ policy.getFlagName(), OptionPriority.INVOCATION_POLICY, INVOCATION_POLICY_SOURCE);
+ if (optionDescription == null) {
+ // InvocationPolicy ignores policy on non-existing flags by design, for version
+ // compatibility.
+ logger.log(
+ loglevel,
+ String.format(
+ "Flag '%s' specified by invocation policy does not exist, and will be ignored",
+ policy.getFlagName()));
+ continue;
+ }
+ FlagPolicyWithContext policyWithContext =
+ new FlagPolicyWithContext(policy, optionDescription);
+ List<FlagPolicyWithContext> policies = expandPolicy(policyWithContext, parser, loglevel);
expandedPolicies.addAll(policies);
}
// Only keep that last policy for each flag.
- Map<String, FlagPolicy> effectivePolicy = new HashMap<>();
- for (FlagPolicy expandedPolicy : expandedPolicies) {
- String flagName = expandedPolicy.getFlagName();
+ Map<String, FlagPolicyWithContext> effectivePolicy = new HashMap<>();
+ for (FlagPolicyWithContext expandedPolicy : expandedPolicies) {
+ String flagName = expandedPolicy.policy.getFlagName();
effectivePolicy.put(flagName, expandedPolicy);
}
@@ -253,31 +289,27 @@
}
private static ImmutableList<ParsedOptionDescription> getExpansionsFromFlagPolicy(
- FlagPolicy expansionPolicy, OptionDescription optionDescription, OptionsParser parser)
- throws OptionsParsingException {
- if (!optionDescription.isExpansion()) {
+ FlagPolicyWithContext expansionPolicy, OptionsParser parser) throws OptionsParsingException {
+ if (!expansionPolicy.description.isExpansion()) {
return ImmutableList.of();
}
-
+ String policyFlagName = expansionPolicy.policy.getFlagName();
+ String optionName = expansionPolicy.description.getOptionDefinition().getOptionName();
Preconditions.checkArgument(
- expansionPolicy
- .getFlagName()
- .equals(optionDescription.getOptionDefinition().getOptionName()),
- String.format(
+ policyFlagName.equals(optionName),
"The optionDescription provided (for flag %s) does not match the policy for flag %s.",
- optionDescription.getOptionDefinition().getOptionName(),
- expansionPolicy.getFlagName()));
+ optionName, policyFlagName);
ImmutableList.Builder<ParsedOptionDescription> resultsBuilder = ImmutableList.builder();
- switch (expansionPolicy.getOperationCase()) {
+ switch (expansionPolicy.policy.getOperationCase()) {
case SET_VALUE:
{
- SetValue setValue = expansionPolicy.getSetValue();
+ SetValue setValue = expansionPolicy.policy.getSetValue();
if (setValue.getFlagValueCount() > 0) {
for (String value : setValue.getFlagValueList()) {
resultsBuilder.addAll(
parser.getExpansionOptionValueDescriptions(
- optionDescription.getOptionDefinition(),
+ expansionPolicy.description.getOptionDefinition(),
value,
OptionPriority.INVOCATION_POLICY,
INVOCATION_POLICY_SOURCE));
@@ -285,7 +317,7 @@
} else {
resultsBuilder.addAll(
parser.getExpansionOptionValueDescriptions(
- optionDescription.getOptionDefinition(),
+ expansionPolicy.description.getOptionDefinition(),
null,
OptionPriority.INVOCATION_POLICY,
INVOCATION_POLICY_SOURCE));
@@ -295,7 +327,7 @@
case USE_DEFAULT:
resultsBuilder.addAll(
parser.getExpansionOptionValueDescriptions(
- optionDescription.getOptionDefinition(),
+ expansionPolicy.description.getOptionDefinition(),
null,
OptionPriority.INVOCATION_POLICY,
INVOCATION_POLICY_SOURCE));
@@ -305,19 +337,19 @@
// 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());
+ throwAllowValuesOnExpansionFlagException(optionName);
break;
case DISALLOW_VALUES:
- throwDisallowValuesOnExpansionFlagException(expansionPolicy.getFlagName());
+ throwDisallowValuesOnExpansionFlagException(optionName);
break;
case OPERATION_NOT_SET:
- throw new PolicyOperationNotSetException(expansionPolicy.getFlagName());
+ throw new PolicyOperationNotSetException(optionName);
default:
logger.warning(
String.format(
"Unknown operation '%s' from invocation policy for flag '%s'",
- expansionPolicy.getOperationCase(),
- optionDescription.getOptionDefinition().getOptionName()));
+ expansionPolicy.policy.getOperationCase(),
+ optionName));
break;
}
@@ -331,30 +363,20 @@
*
* <p>None of the flagPolicies returned should be on expansion flags.
*/
- private static List<FlagPolicy> expandPolicy(
- FlagPolicy originalPolicy, OptionsParser parser, Level loglevel)
+ private static List<FlagPolicyWithContext> expandPolicy(
+ FlagPolicyWithContext originalPolicy, OptionsParser parser, Level loglevel)
throws OptionsParsingException {
- List<FlagPolicy> expandedPolicies = new ArrayList<>();
-
- OptionDescription originalOptionDescription =
- parser.getOptionDescription(
- originalPolicy.getFlagName(),
- OptionPriority.INVOCATION_POLICY,
- INVOCATION_POLICY_SOURCE);
- if (originalOptionDescription == null) {
- // InvocationPolicy ignores policy on non-existing flags by design, for version compatibility.
- return expandedPolicies;
- }
+ List<FlagPolicyWithContext> expandedPolicies = new ArrayList<>();
ImmutableList<ParsedOptionDescription> expansions =
- getExpansionsFromFlagPolicy(originalPolicy, originalOptionDescription, parser);
+ getExpansionsFromFlagPolicy(originalPolicy, parser);
ImmutableList.Builder<ParsedOptionDescription> subflagBuilder = ImmutableList.builder();
ImmutableList<ParsedOptionDescription> subflags =
subflagBuilder
- .addAll(originalOptionDescription.getImplicitRequirements())
+ .addAll(originalPolicy.description.getImplicitRequirements())
.addAll(expansions)
.build();
- boolean isExpansion = originalOptionDescription.isExpansion();
+ boolean isExpansion = originalPolicy.description.isExpansion();
if (!subflags.isEmpty() && logger.isLoggable(loglevel)) {
// Log the expansion. This is only really useful for understanding the invocation policy
@@ -370,27 +392,34 @@
"expandPolicy",
String.format(
"Expanding %s on option %s to its %s: %s.",
- originalPolicy.getOperationCase(),
- originalPolicy.getFlagName(),
+ originalPolicy.policy.getOperationCase(),
+ originalPolicy.policy.getFlagName(),
isExpansion ? "expansions" : "implied flags",
Joiner.on("; ").join(subflagNames)));
}
// 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<OptionDefinition, ParsedOptionDescription> repeatableSubflagsInSetValues =
+ Multimap<OptionDescription, ParsedOptionDescription> 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 (ParsedOptionDescription currentSubflag : subflags) {
+ OptionDescription subflagOptionDescription =
+ parser.getOptionDescription(
+ currentSubflag.getOptionDefinition().getOptionName(),
+ OptionPriority.INVOCATION_POLICY,
+ INVOCATION_POLICY_SOURCE);
+
if (currentSubflag.getOptionDefinition().allowsMultiple()
- && originalPolicy.getOperationCase().equals(OperationCase.SET_VALUE)) {
- repeatableSubflagsInSetValues.put(currentSubflag.getOptionDefinition(), currentSubflag);
+ && originalPolicy.policy.getOperationCase().equals(OperationCase.SET_VALUE)) {
+ repeatableSubflagsInSetValues.put(subflagOptionDescription, currentSubflag);
} else {
- FlagPolicy subflagAsPolicy =
- getSingleValueSubflagAsPolicy(currentSubflag, originalPolicy, isExpansion);
+ FlagPolicyWithContext subflagAsPolicy =
+ getSingleValueSubflagAsPolicy(
+ subflagOptionDescription, currentSubflag, originalPolicy, isExpansion);
// In case any of the expanded flags are themselves expansions, recurse.
expandedPolicies.addAll(expandPolicy(subflagAsPolicy, parser, loglevel));
}
@@ -399,7 +428,7 @@
// 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 (OptionDefinition repeatableFlag : repeatableSubflagsInSetValues.keySet()) {
+ for (OptionDescription repeatableFlag : repeatableSubflagsInSetValues.keySet()) {
int numValues = repeatableSubflagsInSetValues.get(repeatableFlag).size();
ArrayList<String> newValues = new ArrayList<>(numValues);
for (ParsedOptionDescription setValue : repeatableSubflagsInSetValues.get(repeatableFlag)) {
@@ -422,16 +451,19 @@
* policies that set the flag, and so interact with repeatable flags, flags that can be set
* multiple times, in subtle ways.
*
- * @param subflag, the definition of the flag the SetValue'd expansion flag expands to.
+ * @param subflagDesc, the description 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 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.
*/
- private static FlagPolicy getSetValueSubflagAsPolicy(
- OptionDefinition subflag, List<String> subflagValue, FlagPolicy originalPolicy) {
+ private static FlagPolicyWithContext getSetValueSubflagAsPolicy(
+ OptionDescription subflagDesc,
+ List<String> subflagValue,
+ FlagPolicyWithContext originalPolicy) {
// Some sanity checks.
- Verify.verify(originalPolicy.getOperationCase().equals(OperationCase.SET_VALUE));
+ OptionDefinition subflag = subflagDesc.getOptionDefinition();
+ Verify.verify(originalPolicy.policy.getOperationCase().equals(OperationCase.SET_VALUE));
if (!subflag.allowsMultiple()) {
Verify.verify(subflagValue.size() <= 1);
}
@@ -443,28 +475,33 @@
setValueExpansion.addFlagValue(value);
}
if (subflag.allowsMultiple()) {
- setValueExpansion.setAppend(originalPolicy.getSetValue().getOverridable());
+ setValueExpansion.setAppend(originalPolicy.policy.getSetValue().getOverridable());
} else {
- setValueExpansion.setOverridable(originalPolicy.getSetValue().getOverridable());
+ setValueExpansion.setOverridable(originalPolicy.policy.getSetValue().getOverridable());
}
// Commands from the original policy, flag name of the expansion
- return FlagPolicy.newBuilder()
- .addAllCommands(originalPolicy.getCommandsList())
- .setFlagName(subflag.getOptionName())
- .setSetValue(setValueExpansion)
- .build();
+ return new FlagPolicyWithContext(
+ FlagPolicy.newBuilder()
+ .addAllCommands(originalPolicy.policy.getCommandsList())
+ .setFlagName(subflag.getOptionName())
+ .setSetValue(setValueExpansion)
+ .build(),
+ subflagDesc);
}
/**
* For an expansion flag in an invocation policy, each flag it expands to must be given a
* corresponding policy.
*/
- private static FlagPolicy getSingleValueSubflagAsPolicy(
- ParsedOptionDescription currentSubflag, FlagPolicy originalPolicy, boolean isExpansion)
+ private static FlagPolicyWithContext getSingleValueSubflagAsPolicy(
+ OptionDescription subflagContext,
+ ParsedOptionDescription currentSubflag,
+ FlagPolicyWithContext originalPolicy,
+ boolean isExpansion)
throws OptionsParsingException {
- FlagPolicy subflagAsPolicy = null;
- switch (originalPolicy.getOperationCase()) {
+ FlagPolicyWithContext subflagAsPolicy = null;
+ switch (originalPolicy.policy.getOperationCase()) {
case SET_VALUE:
if (currentSubflag.getOptionDefinition().allowsMultiple()) {
throw new AssertionError(
@@ -479,24 +516,24 @@
} else {
subflagValue = ImmutableList.of(currentSubflag.getUnconvertedValue());
}
- subflagAsPolicy =
- getSetValueSubflagAsPolicy(
- currentSubflag.getOptionDefinition(), subflagValue, originalPolicy);
+ subflagAsPolicy = getSetValueSubflagAsPolicy(subflagContext, subflagValue, originalPolicy);
break;
case USE_DEFAULT:
// Commands from the original policy, flag name of the expansion
subflagAsPolicy =
- FlagPolicy.newBuilder()
- .addAllCommands(originalPolicy.getCommandsList())
- .setFlagName(currentSubflag.getOptionDefinition().getOptionName())
- .setUseDefault(UseDefault.getDefaultInstance())
- .build();
+ new FlagPolicyWithContext(
+ FlagPolicy.newBuilder()
+ .addAllCommands(originalPolicy.policy.getCommandsList())
+ .setFlagName(currentSubflag.getOptionDefinition().getOptionName())
+ .setUseDefault(UseDefault.getDefaultInstance())
+ .build(),
+ subflagContext);
break;
case ALLOW_VALUES:
if (isExpansion) {
- throwAllowValuesOnExpansionFlagException(originalPolicy.getFlagName());
+ throwAllowValuesOnExpansionFlagException(originalPolicy.policy.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
@@ -505,7 +542,7 @@
case DISALLOW_VALUES:
if (isExpansion) {
- throwDisallowValuesOnExpansionFlagException(originalPolicy.getFlagName());
+ throwDisallowValuesOnExpansionFlagException(originalPolicy.policy.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
@@ -513,7 +550,7 @@
break;
case OPERATION_NOT_SET:
- throw new PolicyOperationNotSetException(originalPolicy.getFlagName());
+ throw new PolicyOperationNotSetException(originalPolicy.policy.getFlagName());
default:
return null;
@@ -534,13 +571,12 @@
private static void applySetValueOperation(
OptionsParser parser,
- FlagPolicy flagPolicy,
+ FlagPolicyWithContext flagPolicy,
OptionValueDescription valueDescription,
- OptionDescription optionDescription,
Level loglevel)
throws OptionsParsingException {
- SetValue setValue = flagPolicy.getSetValue();
- OptionDefinition optionDefinition = optionDescription.getOptionDefinition();
+ SetValue setValue = flagPolicy.policy.getSetValue();
+ OptionDefinition optionDefinition = flagPolicy.description.getOptionDefinition();
// SetValue.flag_value must have at least 1 value.
if (setValue.getFlagValueCount() == 0) {
@@ -552,7 +588,7 @@
// Flag must allow multiple values if multiple values are specified by the policy.
if (setValue.getFlagValueCount() > 1
- && !optionDescription.getOptionDefinition().allowsMultiple()) {
+ && !flagPolicy.description.getOptionDefinition().allowsMultiple()) {
throw new OptionsParsingException(
String.format(
"SetValue operation from invocation policy sets multiple values for flag '%s' which "
@@ -575,7 +611,7 @@
if (!setValue.getAppend()) {
// Clear the value in case the flag is a repeated flag so that values don't accumulate.
- parser.clearValue(optionDescription.getOptionDefinition());
+ parser.clearValue(flagPolicy.description.getOptionDefinition());
}
// Set all the flag values from the policy.
@@ -585,7 +621,9 @@
loglevel,
"Setting value for flag '%s' from invocation policy to '%s', overriding the "
+ "default value '%s'",
- optionDefinition.getOptionName(), flagValue, optionDefinition.getDefaultValue());
+ optionDefinition.getOptionName(),
+ flagValue,
+ optionDefinition.getDefaultValue());
} else {
logInApplySetValueOperation(
loglevel,
@@ -608,14 +646,8 @@
if (clearedValueDescription != null) {
// Log the removed value.
String clearedFlagName = clearedValueDescription.getOptionDefinition().getOptionName();
-
- OptionDescription desc =
- parser.getOptionDescription(
- clearedFlagName, OptionPriority.INVOCATION_POLICY, INVOCATION_POLICY_SOURCE);
- Object clearedFlagDefaultValue = null;
- if (desc != null) {
- clearedFlagDefaultValue = desc.getOptionDefinition().getDefaultValue();
- }
+ Object clearedFlagDefaultValue =
+ clearedValueDescription.getOptionDefinition().getDefaultValue();
logger.log(
loglevel,
String.format(
diff --git a/java/com/google/devtools/common/options/OptionsParser.java b/java/com/google/devtools/common/options/OptionsParser.java
index 2b4bd2a..25733c7 100644
--- a/java/com/google/devtools/common/options/OptionsParser.java
+++ b/java/com/google/devtools/common/options/OptionsParser.java
@@ -275,6 +275,22 @@
throws OptionsParsingException {
return expansionData.getExpansion(context);
}
+
+ @Override
+ public boolean equals(Object obj) {
+ if (obj instanceof OptionDescription) {
+ OptionDescription other = (OptionDescription) obj;
+ // Check that the option is the same and that it is in the same context (expansionData)
+ return other.optionDefinition.equals(optionDefinition)
+ && other.expansionData.equals(expansionData);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return optionDefinition.hashCode() + expansionData.hashCode();
+ }
}
/**