Filter out conflicting flag policies before invocation policy enforcement.

This is to minimize the likelihood of obscure policy conflict. Now, the last policy on
a flag (after policy expansion) will be the only one in the "canonical" invocation
policy. There should be no reason for explicitly setting multiple policies on a single
flag, but if an expansion flag is policy'd and one of its children has a more specific
policy on it, make sure that the policy on the child flag is after the policy on the
expansion flag.

Note that this restriction (only the last policy gets applied) also applies for repeatable flags. Make sure all values being set to a repeatable flag are set in a single SetValue operation, with multiple flagValues set.

PiperOrigin-RevId: 153584034

1 files changed, 34 insertions, 0 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyUseDefaultTest.java b/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyUseDefaultTest.java
index a37b3e45d1..dda2d319ca 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyUseDefaultTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyUseDefaultTest.java
@@ -102,6 +102,40 @@ public class InvocationPolicyUseDefaultTest extends InvocationPolicyEnforcerTest
   }
 
   @Test
+  public void testUseDefaultWithExpansionFlagAndLaterOverride() throws Exception {
+    InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
+    invocationPolicyBuilder
+        .addFlagPoliciesBuilder()
+        .setFlagName("test_expansion")
+        .getUseDefaultBuilder();
+    invocationPolicyBuilder
+        .addFlagPoliciesBuilder()
+        .setFlagName("expanded_b")
+        .getAllowValuesBuilder()
+        .addAllowedValues("false");
+
+    InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+    parser.parse("--test_expansion");
+
+    TestOptions testOptions = getTestOptions();
+    assertThat(testOptions.expandedA).isEqualTo(TestOptions.EXPANDED_A_TEST_EXPANSION);
+    assertThat(testOptions.expandedB).isEqualTo(TestOptions.EXPANDED_B_TEST_EXPANSION);
+    assertThat(testOptions.expandedC).isEqualTo(TestOptions.EXPANDED_C_TEST_EXPANSION);
+    assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_TEST_EXPANSION);
+
+    // If the UseDefault is run, then the value of --expanded_b is back to it's default true, which
+    // isn't allowed. However, the allowValues in the later policy should wipe the expansion's
+    // policy on --expanded_b, so that the enforcement does not fail.
+    enforcer.enforce(parser, BUILD_COMMAND);
+
+    testOptions = getTestOptions();
+    assertThat(testOptions.expandedA).isEqualTo(TestOptions.EXPANDED_A_DEFAULT);
+    assertThat(testOptions.expandedB).isEqualTo(TestOptions.EXPANDED_B_TEST_EXPANSION);
+    assertThat(testOptions.expandedC).isEqualTo(TestOptions.EXPANDED_C_DEFAULT);
+    assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_DEFAULT);
+  }
+
+  @Test
   public void testUseDefaultWithRecursiveExpansionFlags() throws Exception {
     InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
     invocationPolicyBuilder.addFlagPoliciesBuilder()