Fix invocation policy's handling of the null default when filtering values.

For a filter on option values (either by whitelist, allow_values, or blacklist, disallow_values), one of the options for what to do when encountering a disallowed value is to replace it with the default. This default must be itself an allowed value for this to make sense, so this is checked. This check, however, shouldn't apply to flags that are null by default, since these flags' default value is not parsed by the converter, so there is no guarantee that there exists an accepted user-input value that would also set the value to NULL. In these cases, we assume that "unset" is a distinct value that is always allowed.

RELNOTES: None.
PiperOrigin-RevId: 187475696

3 files changed, 96 insertions, 0 deletions

diff --git a/src/test/java/com/google/devtools/common/options/InvocationPolicyAllowValuesTest.java b/src/test/java/com/google/devtools/common/options/InvocationPolicyAllowValuesTest.java
index b8625883fa..575aad1fbd 100644
--- a/src/test/java/com/google/devtools/common/options/InvocationPolicyAllowValuesTest.java
+++ b/src/test/java/com/google/devtools/common/options/InvocationPolicyAllowValuesTest.java
@@ -16,7 +16,9 @@ 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.AllowValues;
 import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
+import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.UseDefault;
 import java.util.Arrays;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -270,4 +272,65 @@ public class InvocationPolicyAllowValuesTest extends InvocationPolicyEnforcerTes
                   + "policy");
     }
   }
+
+  @Test
+  public void testAllowValuesWithNullDefault_AcceptedValue() throws Exception {
+    InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
+    invocationPolicyBuilder
+        .addFlagPoliciesBuilder()
+        .setFlagName("test_string_null_by_default")
+        .setAllowValues(
+            AllowValues.newBuilder().addAllowedValues("a").setUseDefault(UseDefault.newBuilder()));
+    InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+    // Check the value before invocation policy enforcement.
+    parser.parse("--test_string_null_by_default=a");
+    TestOptions testOptions = getTestOptions();
+    assertThat(testOptions.testStringNullByDefault).isEqualTo("a");
+
+    // Check the value afterwards.
+    enforcer.enforce(parser, BUILD_COMMAND);
+    testOptions = getTestOptions();
+    assertThat(testOptions.testStringNullByDefault).isEqualTo("a");
+  }
+
+  @Test
+  public void testAllowValuesWithNullDefault_UsesNullDefaultToOverrideUnacceptedValue()
+      throws Exception {
+    InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
+    invocationPolicyBuilder
+        .addFlagPoliciesBuilder()
+        .setFlagName("test_string_null_by_default")
+        .setAllowValues(
+            AllowValues.newBuilder().addAllowedValues("a").setUseDefault(UseDefault.newBuilder()));
+    InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+    // Check the value before invocation policy enforcement.
+    parser.parse("--test_string_null_by_default=b");
+    TestOptions testOptions = getTestOptions();
+    assertThat(testOptions.testStringNullByDefault).isEqualTo("b");
+
+    // Check the value afterwards.
+    enforcer.enforce(parser, BUILD_COMMAND);
+    testOptions = getTestOptions();
+    assertThat(testOptions.testStringNullByDefault).isNull();
+  }
+
+  @Test
+  public void testAllowValuesWithNullDefault_AllowsUnsetValue() throws Exception {
+    InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
+    invocationPolicyBuilder
+        .addFlagPoliciesBuilder()
+        .setFlagName("test_string_null_by_default")
+        .setAllowValues(
+            AllowValues.newBuilder().addAllowedValues("a").setUseDefault(UseDefault.newBuilder()));
+    InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+    // Check the value before invocation policy enforcement.
+    parser.parse();
+    TestOptions testOptions = getTestOptions();
+    assertThat(testOptions.testStringNullByDefault).isNull();
+
+    // Check the value afterwards.
+    enforcer.enforce(parser, BUILD_COMMAND);
+    testOptions = getTestOptions();
+    assertThat(testOptions.testStringNullByDefault).isNull();
+  }
 }
diff --git a/src/test/java/com/google/devtools/common/options/InvocationPolicyDisallowValuesTest.java b/src/test/java/com/google/devtools/common/options/InvocationPolicyDisallowValuesTest.java
index 2cfaccd41d..68fa7fe587 100644
--- a/src/test/java/com/google/devtools/common/options/InvocationPolicyDisallowValuesTest.java
+++ b/src/test/java/com/google/devtools/common/options/InvocationPolicyDisallowValuesTest.java
@@ -16,7 +16,9 @@ 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.DisallowValues;
 import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
+import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.UseDefault;
 import java.util.Arrays;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -272,4 +274,26 @@ public class InvocationPolicyDisallowValuesTest extends InvocationPolicyEnforcer
                   + "policy");
     }
   }
+
+  @Test
+  public void testAllowValuesWithNullDefault_DoesNotConfuseNullForDefault() throws Exception {
+    InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
+    invocationPolicyBuilder
+        .addFlagPoliciesBuilder()
+        .setFlagName("test_string_null_by_default")
+        .setDisallowValues(
+            DisallowValues.newBuilder()
+                .addDisallowedValues("null")
+                .setUseDefault(UseDefault.newBuilder()));
+    InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+    // Check the value before invocation policy enforcement.
+    parser.parse("--test_string_null_by_default=null");
+    TestOptions testOptions = getTestOptions();
+    assertThat(testOptions.testStringNullByDefault).isEqualTo("null");
+
+    // Check the value afterwards.
+    enforcer.enforce(parser, BUILD_COMMAND);
+    testOptions = getTestOptions();
+    assertThat(testOptions.testStringNullByDefault).isNull();
+  }
 }
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 2067b28672..c38d975a84 100644
--- a/src/test/java/com/google/devtools/common/options/TestOptions.java
+++ b/src/test/java/com/google/devtools/common/options/TestOptions.java
@@ -37,6 +37,15 @@ public class TestOptions extends OptionsBase {
   )
   public String testString;
 
+  @Option(
+    name = "test_string_null_by_default",
+    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+    effectTags = {OptionEffectTag.NO_OP},
+    defaultValue = "null",
+    help = "a string-valued option that has the special string 'null' as its default."
+  )
+  public String testStringNullByDefault;
+
   /*
    * Repeated flags
    */