Make config_feature_flag's default_value optional.

With this change, if the default_value is not set, but the flag
has been given a value by a configuration transition, that value
is used and no error is produced.

If the default_value is not set and the flag has not been given
a value by a configuration transition, the rule produces an
error (as it did before, but now the error is different).

config_feature_flags with default_values behave as they did before.
(use the default value if no configured value is set, otherwise use
the configured value)

Also transition the Optional used in feature flags from Guava to
Java 8.

RELNOTES: config_feature_flag's default_value is optional. It is
only an error to have a config_feature_flag with no default_value
if that config_feature_flag has not been set in the configuration
it is being evaluated in.
PiperOrigin-RevId: 178418907

6 files changed, 75 insertions, 16 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlag.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlag.java
index 68bf177ce5..175588cc6d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlag.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlag.java
@@ -40,6 +40,7 @@ import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
 import com.google.devtools.build.lib.syntax.Printer;
 import java.util.List;
+import java.util.Optional;
 
 /**
  * The implementation of the config_feature_flag rule for defining custom flags for Android rules.
@@ -115,14 +116,17 @@ public class ConfigFeatureFlag implements RuleConfiguredTargetFactory {
               + Printer.repr(duplicates.build()));
     }
 
-    String defaultValue = ruleContext.attributes().get("default_value", STRING);
-    if (!isValidValue.apply(defaultValue)) {
+    Optional<String> defaultValue =
+        ruleContext.attributes().isAttributeValueExplicitlySpecified("default_value")
+            ? Optional.of(ruleContext.attributes().get("default_value", STRING))
+            : Optional.empty();
+    if (defaultValue.isPresent() && !isValidValue.apply(defaultValue.get())) {
       ruleContext.attributeError(
           "default_value",
           "must be one of "
               + Printer.repr(values.asList())
               + ", but was "
-              + Printer.repr(defaultValue));
+              + Printer.repr(defaultValue.get()));
     }
 
     if (ruleContext.hasErrors()) {
@@ -131,22 +135,29 @@ public class ConfigFeatureFlag implements RuleConfiguredTargetFactory {
       return null;
     }
 
-    String value =
+    Optional<String> configuredValue =
         ruleContext
             .getFragment(ConfigFeatureFlagConfiguration.class)
-            .getFeatureFlagValue(ruleContext.getOwner())
-            .or(defaultValue);
+            .getFeatureFlagValue(ruleContext.getOwner());
 
-    if (!isValidValue.apply(value)) {
+    if (configuredValue.isPresent() && !isValidValue.apply(configuredValue.get())) {
       // TODO(mstaib): When configurationError is available, use that instead.
       ruleContext.ruleError(
           "value must be one of "
               + Printer.repr(values.asList())
               + ", but was "
-              + Printer.repr(value));
+              + Printer.repr(configuredValue.get()));
       return null;
     }
 
+    if (!configuredValue.isPresent() && !defaultValue.isPresent()) {
+      // TODO(mstaib): When configurationError is available, use that instead.
+      ruleContext.ruleError("flag has no default and must be set, but was not set");
+      return null;
+    }
+
+    String value = configuredValue.orElseGet(defaultValue::get);
+
     ConfigFeatureFlagProvider provider = ConfigFeatureFlagProvider.create(value, isValidValue);
 
     return new RuleConfiguredTargetBuilder(ruleContext)
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfiguration.java
index 9f34de2937..be93cb9353 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfiguration.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.rules.config;
 
-import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.hash.Hasher;
@@ -32,6 +31,7 @@ import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
 import com.google.devtools.common.options.OptionMetadataTag;
 import java.util.Map;
+import java.util.Optional;
 import java.util.SortedMap;
 import javax.annotation.Nullable;
 
@@ -141,7 +141,7 @@ public final class ConfigFeatureFlagConfiguration extends BuildConfiguration.Fra
    * ArtifactOwner for a rule, call {@code ruleContext.getOwner()}.
    */
   public Optional<String> getFeatureFlagValue(ArtifactOwner owner) {
-    return Optional.fromNullable(flagValues.get(owner.getLabel()));
+    return Optional.ofNullable(flagValues.get(owner.getLabel()));
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java
index 7ba3611dad..fb8c973973 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java
@@ -411,7 +411,6 @@ platform(
                   .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON))
           .add(
               attr("default_value", STRING)
-                  .mandatory()
                   .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON))
           .add(ConfigFeatureFlag.getWhitelistAttribute(env))
           .build();
diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/BUILD b/src/test/java/com/google/devtools/build/lib/rules/config/BUILD
index 4e115e79e0..feedac96ae 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/config/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/rules/config/BUILD
@@ -33,6 +33,7 @@ java_library(
         "//third_party:jsr305",
         "//third_party:junit4",
         "//third_party:truth",
+        "//third_party:truth8",
     ],
 )
 
diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfigurationTest.java
index 9f1e0c2d9f..09bf495e2b 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfigurationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfigurationTest.java
@@ -15,9 +15,9 @@
 package com.google.devtools.build.lib.rules.config;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
 import static org.junit.Assert.fail;
 
-import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.testing.EqualsTester;
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.util.Map;
+import java.util.Optional;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -152,11 +153,11 @@ public final class ConfigFeatureFlagConfigurationTest {
   }
 
   @Test
-  public void getFeatureFlagValue_returnsAbsentOptionalWhenRequestingUnsetFlag() throws Exception {
+  public void getFeatureFlagValue_returnsEmptyOptionalWhenRequestingUnsetFlag() throws Exception {
     Optional<String> flagValue =
         getConfigurationWithFlags(ImmutableMap.of(Label.parseAbsoluteUnchecked("//a:a"), "valued"))
             .getFeatureFlagValue(new LabelArtifactOwner(Label.parseAbsoluteUnchecked("//b:b")));
-    assertThat(flagValue).isAbsent();
+    assertThat(flagValue).isEmpty();
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTest.java
index 9629bbd54e..dcd80382f7 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTest.java
@@ -76,6 +76,25 @@ public final class ConfigFeatureFlagTest extends SkylarkTestCase {
         "config_feature_flag(",
         "    name = 'flag',",
         "    allowed_values = ['default', 'configured', 'other'],",
+        ")");
+    assertThat(ConfigFeatureFlagProvider.fromTarget(getConfiguredTarget("//test:top")).getValue())
+        .isEqualTo("configured");
+  }
+
+  @Test
+  public void configFeatureFlagProvider_usesConfiguredValueOverDefault() throws Exception {
+    scratch.file(
+        "test/BUILD",
+        "feature_flag_setter(",
+        "    name = 'top',",
+        "    exports_flag = ':flag',",
+        "    flag_values = {",
+        "        ':flag': 'configured',",
+        "    },",
+        ")",
+        "config_feature_flag(",
+        "    name = 'flag',",
+        "    allowed_values = ['default', 'configured', 'other'],",
         "    default_value = 'default',",
         ")");
     assertThat(ConfigFeatureFlagProvider.fromTarget(getConfiguredTarget("//test:top")).getValue())
@@ -250,6 +269,34 @@ public final class ConfigFeatureFlagTest extends SkylarkTestCase {
   }
 
   @Test
+  public void configFeatureFlagProvider_throwsErrorIfNeitherDefaultNorConfiguredValueSet()
+      throws Exception {
+    reporter.removeHandler(failFastHandler); // expecting an error
+    scratch.file(
+        "test/BUILD",
+        "feature_flag_setter(",
+        "    name = 'top',",
+        "    exports_flag = ':flag',",
+        "    flag_values = {",
+        "        ':other': 'configured',",
+        "    },",
+        ")",
+        "config_feature_flag(",
+        "    name = 'flag',",
+        "    allowed_values = ['other', 'configured'],",
+        ")",
+        "config_feature_flag(",
+        "    name = 'other',",
+        "    allowed_values = ['default', 'configured', 'other'],",
+        "    default_value = 'default',",
+        ")");
+    assertThat(getConfiguredTarget("//test:flag")).isNull();
+    assertContainsEvent(
+        "in config_feature_flag rule //test:flag: "
+            + "flag has no default and must be set, but was not set");
+  }
+
+  @Test
   public void allowedValuesAttribute_cannotBeEmpty() throws Exception {
     reporter.removeHandler(failFastHandler); // expecting an error
     scratch.file(
@@ -282,7 +329,7 @@ public final class ConfigFeatureFlagTest extends SkylarkTestCase {
   }
 
   @Test
-  public void defaultValueAttribute_mustBeMemberOfAllowedValues() throws Exception {
+  public void defaultValueAttribute_mustBeMemberOfAllowedValuesIfPresent() throws Exception {
     reporter.removeHandler(failFastHandler); // expecting an error
     scratch.file(
         "test/BUILD",
@@ -305,7 +352,7 @@ public final class ConfigFeatureFlagTest extends SkylarkTestCase {
   }
 
   @Test
-  public void configurationValue_mustBeMemberOfAllowedValues() throws Exception {
+  public void configurationValue_mustBeMemberOfAllowedValuesIfPresent() throws Exception {
     reporter.removeHandler(failFastHandler); // expecting an error
     scratch.file(
         "test/BUILD",