Expand implicitRequirements in the location of the option that required it.

Removes the special casing of implicit requirements. Accumulating them and parsing them at the end of the parse() function was never enough to actually guarantee that the value not be replaced. I've gone through all options with implicit requirements to make sure that the expectation is checked after options parsing, so this change should be relatively safe.

Implicit requirements is still a broken concept - they don't actually expand based on the value given, so a user that is explicitly NOT setting a flag might unwittingly be setting all the requirements for that unset flag. Removing it fully requires redesigning or removing the flags that set it, though, so for now we are standardizing the behavior so that it behaves like any other expansion options, just one with a value.

Also consolidate the deprecated wrapper option behavior into the expansion work. It will soon be removed entirely, but for now it can get grouped in with the expansion logic, so that its differences are more explicit.

RELNOTES: None.
PiperOrigin-RevId: 171957502

2 files changed, 9 insertions, 8 deletions

diff --git a/src/test/java/com/google/devtools/common/options/InvocationPolicyUseDefaultTest.java b/src/test/java/com/google/devtools/common/options/InvocationPolicyUseDefaultTest.java
index 975befb159..3a41915a03 100644
--- a/src/test/java/com/google/devtools/common/options/InvocationPolicyUseDefaultTest.java
+++ b/src/test/java/com/google/devtools/common/options/InvocationPolicyUseDefaultTest.java
@@ -273,16 +273,17 @@ public class InvocationPolicyUseDefaultTest extends InvocationPolicyEnforcerTest
     InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
     parser.parse(
         "--test_implicit_requirement=" + TEST_STRING_USER_VALUE,
-        "--implicit_requirement_a=thrownaway value");
+        "--implicit_requirement_a=" + TEST_STRING_USER_VALUE);
 
-    // test_implicit_requirement sets implicit_requirement_a to "foo", which ignores the user's
-    // value because the parser processes implicit values last.
+    // test_implicit_requirement sets implicit_requirement_a to "foo", but it gets overwritten
+    // by the user value.
     TestOptions testOptions = getTestOptions();
     assertThat(testOptions.testImplicitRequirement).isEqualTo(TEST_STRING_USER_VALUE);
-    assertThat(testOptions.implicitRequirementA)
-        .isEqualTo(TestOptions.IMPLICIT_REQUIREMENT_A_REQUIRED);
+    assertThat(testOptions.implicitRequirementA).isEqualTo(TEST_STRING_USER_VALUE);
 
-    // Then policy puts implicit_requirement_a back to its default.
+    // Then policy puts implicit_requirement_a back to its default. This is "broken" since it wipes
+    // the user value, but this is the behavior that was agreed on and is documented for expansion
+    // flags as well.
     enforcer.enforce(parser, BUILD_COMMAND);
 
     testOptions = getTestOptions();
diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
index 491facccfc..1d2b6940f9 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
@@ -1280,8 +1280,8 @@ public class OptionsParserTest {
     parser.parse("--first=first", "--second=second");
     assertThat(parser.getWarnings())
         .containsExactly(
-            "Option 'second' is implicitly defined by "
-                + "option 'first'; the implicitly set value overrides the previous one");
+            "A new value for option 'second' overrides a previous implicit setting of that "
+                + "option by option 'first'");
   }
 
   @Test