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

3 files changed, 52 insertions, 15 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 5a6e324bcb..886b5d5d2e 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
@@ -36,7 +36,9 @@ import com.google.devtools.common.options.OptionsParser.OptionValueDescription;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -209,7 +211,14 @@ public final class InvocationPolicyEnforcer {
       expandedPolicies.addAll(policies);
     }
 
-    return expandedPolicies;
+    // Only keep that last policy for each flag.
+    Map<String, FlagPolicy> effectivePolicy = new HashMap<>();
+    for (FlagPolicy expandedPolicy : expandedPolicies) {
+      String flagName = expandedPolicy.getFlagName();
+      effectivePolicy.put(flagName, expandedPolicy);
+    }
+
+    return new ArrayList<>(effectivePolicy.values());
   }
 
   /**
diff --git a/src/main/protobuf/invocation_policy.proto b/src/main/protobuf/invocation_policy.proto
index 218b00e0ed..f14db3e6ca 100644
--- a/src/main/protobuf/invocation_policy.proto
+++ b/src/main/protobuf/invocation_policy.proto
@@ -20,18 +20,10 @@ option java_package = "com.google.devtools.build.lib.runtime.proto";
 // The --invocation_policy flag takes a base64-encoded binary-serialized or text
 // formatted InvocationPolicy message.
 message InvocationPolicy {
-  // Policies will be applied in order. Later policies will override
-  // previous policies if they conflict, which is important for flags
-  // that interact with each other. For example, if there is a flag "--foo"
-  // which is an expansion flag that expands into "--bar=x --baz=y", and the
-  // policy list first has a SetValue policy for "set --bar to z", and then has
-  // a SetDefault policy to set "--foo" to its default value, both --bar and
-  // --baz will get reset to their default values, overriding the SetValue
-  // operation. The UseDefault should come before the SetValue.
-  //
-  // Note that currently, if user value is lost, either cleared by UseDefault
-  // or by being written over by a SetValue, a later white-listing of the user's
-  // inputted value will not restore it.
+  // Order matters.
+  // After expanding policies on expansion flags or flags with implicit
+  // requirements, only the final policy on a specific flag will be enforced
+  // onto the user's command line.
   repeated FlagPolicy flag_policies = 1;
 }
 
@@ -127,8 +119,10 @@ message UseDefault {
   // so that when the value is requested and no flag is found, the flag parser
   // returns the default. This is mostly relevant for expansion flags: it will
   // erase user values in *all* flags that the expansion flag expands to. Only
-  // use this on expansion flags if this is acceptable behavior. Otherwise, make
-  // an appropriate policy for the expanded flags that you care about.
+  // use this on expansion flags if this is acceptable behavior. Since the last
+  // policy wins, later policies on this same flag will still remove the
+  // expanded UseDefault, so there is a way around, but it's really best not to
+  // use this on expansion flags at all.
 }
 
 message DisallowValues {
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()