Enables AllowValues and DisallowValues invocation policy operations to take

a value to use instead of the given flag value if it is disallowed by the
policy. This is a generalization of the existing new_default_value behavior to
cover any value of the flag, not just the default value if the flag is unset.

--
MOS_MIGRATED_REVID=137104944

1 files changed, 97 insertions, 38 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java b/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
index b9307c4976..1b46473210 100644
--- a/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
+++ b/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
@@ -18,7 +18,6 @@ import com.google.common.base.Function;
 import com.google.common.base.Functions;
 import com.google.common.base.Strings;
 import com.google.common.base.Verify;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import com.google.common.io.BaseEncoding;
@@ -179,7 +178,7 @@ public final class InvocationPolicyEnforcer {
           break;
 
         case USE_DEFAULT:
-          applyUseDefaultOperation(parser, flagName);
+          applyUseDefaultOperation(parser, "UseDefault", flagName);
           break;
 
         case ALLOW_VALUES:
@@ -187,7 +186,8 @@ public final class InvocationPolicyEnforcer {
           FilterValueOperation.ALLOW_VALUE_OPERATION.apply(
               parser,
               allowValues.getAllowedValuesList(),
-              allowValues.hasNewDefaultValue() ? allowValues.getNewDefaultValue() : null,
+              allowValues.hasNewValue() ? allowValues.getNewValue() : null,
+              allowValues.hasUseDefault(),
               flagName,
               valueDescription,
               optionDescription);
@@ -198,7 +198,8 @@ public final class InvocationPolicyEnforcer {
           FilterValueOperation.DISALLOW_VALUE_OPERATION.apply(
               parser,
               disallowValues.getDisallowedValuesList(),
-              disallowValues.hasNewDefaultValue() ? disallowValues.getNewDefaultValue() : null,
+              disallowValues.hasNewValue() ? disallowValues.getNewValue() : null,
+              disallowValues.hasUseDefault(),
               flagName,
               valueDescription,
               optionDescription);
@@ -289,8 +290,8 @@ public final class InvocationPolicyEnforcer {
     }
   }
 
-  private static void applyUseDefaultOperation(OptionsParser parser, String flagName)
-      throws OptionsParsingException {
+  private static void applyUseDefaultOperation(
+      OptionsParser parser, String policyType, String flagName) throws OptionsParsingException {
 
     Map<String, OptionValueDescription> clearedValues = parser.clearValue(flagName);
     for (Entry<String, OptionValueDescription> clearedValue : clearedValues.entrySet()) {
@@ -306,9 +307,10 @@ public final class InvocationPolicyEnforcer {
       log.info(
           String.format(
               "Using default value '%s' for flag '%s' as "
-                  + "specified by invocation policy, overriding original value '%s' from '%s'",
+                  + "specified by %s invocation policy, overriding original value '%s' from '%s'",
               clearedFlagDefaultValue,
               clearedFlagName,
+              policyType,
               originalValue,
               source));
     }
@@ -322,7 +324,7 @@ public final class InvocationPolicyEnforcer {
     private static final FilterValueOperation ALLOW_VALUE_OPERATION =
         new FilterValueOperation("Allow") {
           @Override
-          boolean filter(Set<Object> convertedPolicyValues, Object value) {
+          boolean isFlagValueAllowed(Set<Object> convertedPolicyValues, Object value) {
             return convertedPolicyValues.contains(value);
           }
         };
@@ -330,7 +332,7 @@ public final class InvocationPolicyEnforcer {
     private static final FilterValueOperation DISALLOW_VALUE_OPERATION =
         new FilterValueOperation("Disallow") {
           @Override
-          boolean filter(Set<Object> convertedPolicyValues, Object value) {
+          boolean isFlagValueAllowed(Set<Object> convertedPolicyValues, Object value) {
             // In a disallow operation, the values that the flag policy specifies are not allowed,
             // so the value is allowed if the set of policy values does not contain the current
             // flag value.
@@ -351,12 +353,13 @@ public final class InvocationPolicyEnforcer {
      * @param value The user value of the flag.
      * @return True if the value should be allowed, false if it should not.
      */
-    abstract boolean filter(Set<Object> convertedPolicyValues, Object value);
+    abstract boolean isFlagValueAllowed(Set<Object> convertedPolicyValues, Object value);
 
     void apply(
         OptionsParser parser,
         List<String> policyValues,
-        String newDefaultValue,
+        String newValue,
+        boolean useDefault,
         String flagName,
         OptionValueDescription valueDescription,
         OptionDescription optionDescription)
@@ -372,6 +375,25 @@ public final class InvocationPolicyEnforcer {
         convertedPolicyValues.add(optionDescription.getConverter().convert(value));
       }
 
+      // Check that if the default value of the flag is disallowed by the policy, that the policy
+      // does not also set use_default. Otherwise the default value would will still be set if the
+      // user uses a disallowed value. This doesn't apply to repeatable flags since the default
+      // value for repeatable flags is always the empty list.
+      if (!optionDescription.getAllowMultiple()) {
+
+        boolean defaultValueAllowed =
+            isFlagValueAllowed(convertedPolicyValues, optionDescription.getDefaultValue());
+        if (!defaultValueAllowed && useDefault) {
+          throw new OptionsParsingException(
+              String.format(
+                  "%sValues policy disallows the default value '%s' for flag '%s' but also "
+                      + "specifies to use the default value",
+                  policyType,
+                  optionDescription.getDefaultValue(),
+                  flagName));
+        }
+      }
+
       if (valueDescription == null) {
         // Nothing has set the value yet, so check that the default value from the flag's
         // definition is allowed. The else case below (i.e. valueDescription is not null) checks for
@@ -381,45 +403,52 @@ public final class InvocationPolicyEnforcer {
         checkDefaultValue(
             parser,
             policyValues,
-            newDefaultValue,
+            newValue,
             flagName,
             optionDescription,
             convertedPolicyValues);
       } else {
         checkUserValue(
-            policyValues, flagName, valueDescription, optionDescription, convertedPolicyValues);
+            parser,
+            policyValues,
+            newValue,
+            useDefault,
+            flagName,
+            valueDescription,
+            optionDescription,
+            convertedPolicyValues);
       }
     }
 
     void checkDefaultValue(
         OptionsParser parser,
         List<String> policyValues,
-        String newDefaultValue,
+        String newValue,
         String flagName,
         OptionDescription optionDescription,
         Set<Object> convertedPolicyValues)
         throws OptionsParsingException {
 
-      if (!filter(convertedPolicyValues, optionDescription.getDefaultValue())) {
-        if (newDefaultValue != null) {
+      if (!isFlagValueAllowed(convertedPolicyValues, optionDescription.getDefaultValue())) {
+        if (newValue != null) {
           // Use the default value from the policy.
           log.info(
               String.format(
-                  "Overriding default value '%s' for flag '%s' with new default value '%s' "
+                  "Overriding default value '%s' for flag '%s' with value '%s' "
                       + "specified by invocation policy. %sed values are: %s",
                   optionDescription.getDefaultValue(),
                   flagName,
-                  newDefaultValue,
+                  newValue,
                   policyType,
                   policyValues));
           parser.clearValue(flagName);
-          setFlagValue(parser, flagName, newDefaultValue);
+          setFlagValue(parser, flagName, newValue);
         } else {
-          // The operation disallows the default value, but doesn't supply its own default.
+          // The operation disallows the default value, but doesn't supply a new value.
           throw new OptionsParsingException(
               String.format(
                   "Default flag value '%s' for flag '%s' is not allowed by invocation policy, but "
-                      + "the policy does not provide a new default value. "
+                      + "the policy does not provide a new value. "
                       + "%sed values are: %s",
                   optionDescription.getDefaultValue(),
                   flagName,
@@ -430,34 +459,64 @@ public final class InvocationPolicyEnforcer {
     }
 
     void checkUserValue(
+        OptionsParser parser,
         List<String> policyValues,
+        String newValue,
+        boolean useDefault,
         String flagName,
         OptionValueDescription valueDescription,
         OptionDescription optionDescription,
         Set<Object> convertedPolicyValues)
         throws OptionsParsingException {
-
-      // Get the option values: there might be one of them or a list of them, so convert everything
-      // to a list (possibly of just the one value).
-      List<?> optionValues;
+      
       if (optionDescription.getAllowMultiple()) {
         // allowMultiple requires that the type of the option be List<T>, so cast from Object
         // to List<?>.
-        optionValues = (List<?>) valueDescription.getValue();
+        List<?> optionValues = (List<?>) valueDescription.getValue();
+        for (Object value : optionValues) {
+          if (!isFlagValueAllowed(convertedPolicyValues, value)) {
+            if (useDefault) {
+              applyUseDefaultOperation(parser, policyType + "Values", flagName);
+            } 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));
+            }
+          }
+        }
+
       } else {
-        optionValues = ImmutableList.of(valueDescription.getValue());
-      }
 
-      for (Object value : optionValues) {
-        if (!filter(convertedPolicyValues, value)) {
-          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));
+        if (!isFlagValueAllowed(convertedPolicyValues, valueDescription.getValue())) {
+          if (newValue != null) {
+            log.info(
+                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);
+          } else if (useDefault) {
+            applyUseDefaultOperation(parser, policyType + "Values", flagName);
+          } 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));
+          }
         }
       }
     }