Defer raising error in ConfigFeatureFlag until values actually consumed.

After ceddfb1, select statements and thus their associated config_condition are tentatively evaluated before the rule implementation. As discovered in b/299313984, this can lead to issues where those select statements are potentially not resolvable b/c they rely on flag changes done by the transition.

To mitigate that issue, need to make those select resolutions and consequent config_setting resolution 'best effort' rather than mandatory. This CL adds machinery so that config_setting (and it's dependencies) can register deferred errors. Thus, the boolean ConfigMatchingProvider.matches is now a tri-state MatchResult called onfigMatchingProvider.result.

In particular, configuration-specific errors registered by determining the value of a config_feature_flag are now deferred until they are actually consumed (via config_setting) by a select statement or (via FeatureFlagInfo.value) by Starlark code.

Future work will make the evaluation of select statements before rule transitions 'best effort'.

PiperOrigin-RevId: 565169008
Change-Id: I1efcf8b7f4f4538297f82db2f342bdc3b8c2f3ea

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -1730,6 +1730,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//third_party:auto_value",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )
 

src/main/java/com/google/devtools/build/lib/analysis/config/ConfigMatchingProvider.java

@@ -22,6 +22,7 @@
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import java.util.Map;
+import javax.annotation.Nullable;
 
 /**
  * A "configuration target" that asserts whether or not it matches the configuration it's bound to.
@@ -33,26 +34,78 @@
 @Immutable
 @AutoValue
 public abstract class ConfigMatchingProvider implements TransitiveInfoProvider {
+  /**
+   * Potential values for result field.
+   *
+   * <p>Note that while it is possible to be more aggressive in interpreting and merging
+   * MatchResult, currently taking a more cautious approach and focusing on propagating errors.
+   *
+   * <p>e.g. If merging where one is InError and the other is No, then currently will propagate the
+   * errors, versus a more aggressive future approach could just propagate No.)
+   */
+  // TODO(twigg): This is more cleanly implemented when Java record is available.
+  public static interface MatchResult {
+    // Only InError should return non-null.
+    public abstract @Nullable String getError();
+
+    public static final MatchResult MATCH = HasResult.MATCH;
+    public static final MatchResult NOMATCH = HasResult.NOMATCH;
+
+    /** Some specified error makes the match question irresolvable. */
+    @AutoValue
+    public abstract class InError implements MatchResult {
+      public static InError create(String error) {
+        return new AutoValue_ConfigMatchingProvider_MatchResult_InError(error);
+      }
+    }
+
+    public static MatchResult create(boolean matches) {
+      return matches ? MATCH : NOMATCH;
+    }
+
+    /** If previously InError or No, keep previous else convert newData. */
+    public static MatchResult merge(MatchResult previousMatch, boolean andWith) {
+      if (!previousMatch.equals(MATCH)) {
+        return previousMatch;
+      }
+      return andWith ? MATCH : NOMATCH;
+    }
+  }
+
+  /**
+   * The result was resolved.
+   *
+   * <p>Using an enum to get convenient toString, equals, and hashCode implementations. Interfaces
+   * can't have private members so this is defined here privately and then exported into the
+   * MatchResult interface to allow for ergonomic MatchResult.MATCH checks.
+   */
+  private static enum HasResult implements MatchResult {
+    MATCH,
+    NOMATCH;
+
+    @Override
+    public @Nullable String getError() {
+      return null;
+    }
+  }
 
   /**
+   * Create a ConfigMatchingProvider.
+   *
    * @param label the build label corresponding to this matcher
    * @param settingsMap the condition settings that trigger this matcher
    * @param flagSettingsMap the label-keyed settings that trigger this matcher
-   * @param matches whether or not this matcher matches the configuration associated with its
-   *     configured target
+   * @param result whether the current associated configuration matches, doesn't match, or is
+   *     irresolvable due to specified issue
    */
   public static ConfigMatchingProvider create(
       Label label,
       ImmutableMultimap<String, String> settingsMap,
       ImmutableMap<Label, String> flagSettingsMap,
       ImmutableSet<Label> constraintValueSettings,
-      boolean matches) {
+      MatchResult result) {
     return new AutoValue_ConfigMatchingProvider(
-        label,
-        settingsMap,
-        flagSettingsMap,
-        constraintValueSettings,
-        matches);
+        label, settingsMap, flagSettingsMap, constraintValueSettings, result);
   }
 
   /** The target's label. */
@@ -68,7 +121,7 @@ public static ConfigMatchingProvider create(
    * Whether or not the configuration criteria defined by this target match its actual
    * configuration.
    */
-  public abstract boolean matches();
+  public abstract MatchResult result();
 
   /**
    * Returns true if this matcher's conditions are a proper superset of another matcher's

src/main/java/com/google/devtools/build/lib/analysis/platform/ConstraintValueInfo.java

@@ -78,7 +78,8 @@ public ConfigMatchingProvider configMatchingProvider(PlatformInfo platformInfo)
         ImmutableMultimap.of(),
         ImmutableMap.of(),
         ImmutableSet.of(),
-        platformInfo.constraints().hasConstraintValue(this));
+        ConfigMatchingProvider.MatchResult.create(
+            platformInfo.constraints().hasConstraintValue(this)));
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/packages/ConfiguredAttributeMapper.java

@@ -182,6 +182,7 @@ private <T> ConfigKeyAndValue<T> resolveSelector(String attributeName, Selector<
     // closely match how users see select() structures in BUILD files.
     LinkedHashSet<Label> conditionLabels = new LinkedHashSet<>();
 
+    ArrayList<String> errors = new ArrayList<>();
     // Find the matching condition and record its value (checking for duplicates).
     selector.forEach(
         (selectorKey, value) -> {
@@ -196,7 +197,14 @@ private <T> ConfigKeyAndValue<T> resolveSelector(String attributeName, Selector<
           }
           conditionLabels.add(selectorKey);
 
-          if (curCondition.matches()) {
+          ConfigMatchingProvider.MatchResult matchResult = curCondition.result();
+
+          if (matchResult.getError() != null) {
+            // Resolving selects so last chance to actually surface these errors.
+            String message = matchResult.getError();
+            errors.add("config_setting " + selectorKey + " is unresolvable because: " + message);
+            // Defer the throw in order to collect all possible config_setting that are in error.
+          } else if (matchResult.equals(ConfigMatchingProvider.MatchResult.MATCH)) {
             // We keep track of all matches which are more precise than any we have found so
             // far. Therefore, we remove any previous matches which are strictly less precise
             // than this one, and only add this one if none of the previous matches are more
@@ -219,6 +227,15 @@ private <T> ConfigKeyAndValue<T> resolveSelector(String attributeName, Selector<
             }
           }
         });
+    if (!errors.isEmpty()) {
+      throw new ValidationException(
+          "Unresolvable config_settings for configurable attribute \""
+              + attributeName
+              + "\" in "
+              + getLabel()
+              + ":\n"
+              + Joiner.on("\n").join(errors));
+    }
 
     if (matchingConditions.values().stream().map(s -> s.value).distinct().count() > 1) {
       throw new ValidationException(

src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlag.java

@@ -116,7 +116,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
       throws InterruptedException, RuleErrorException, ActionConflictException {
     List<String> specifiedValues = ruleContext.attributes().get("allowed_values", STRING_LIST);
     ImmutableSet<String> values = ImmutableSet.copyOf(specifiedValues);
-    Predicate<String> isValidValue = Predicates.in(values);
     if (values.size() != specifiedValues.size()) {
       ImmutableMultiset<String> groupedValues = ImmutableMultiset.copyOf(specifiedValues);
       ImmutableList.Builder<String> duplicates = new ImmutableList.Builder<>();
@@ -125,6 +124,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
           duplicates.add(value.getElement());
         }
       }
+      // This is a problem with attributes of config_feature_flag itself so throw error here.
       ruleContext.attributeError(
           "allowed_values",
           "cannot contain duplicates, but contained multiple of "
@@ -135,7 +135,8 @@ public ConfiguredTarget create(RuleContext ruleContext)
         ruleContext.attributes().isAttributeValueExplicitlySpecified("default_value")
             ? Optional.of(ruleContext.attributes().get("default_value", STRING))
             : Optional.empty();
-    if (defaultValue.isPresent() && !isValidValue.apply(defaultValue.get())) {
+    if (defaultValue.isPresent() && !values.contains(defaultValue.get())) {
+      // This is a problem with attributes of config_feature_flag itself so throw error here.
       ruleContext.attributeError(
           "default_value",
           "must be one of "
@@ -150,35 +151,91 @@ public ConfiguredTarget create(RuleContext ruleContext)
       return null;
     }
 
-    Optional<String> configuredValue =
+    Object rawStarlarkValue =
         ruleContext
-            .getFragment(ConfigFeatureFlagConfiguration.class)
-            .getFeatureFlagValue(ruleContext.getOwner());
-
-    if (configuredValue.isPresent() && !isValidValue.apply(configuredValue.get())) {
-      // TODO(b/140635901): When configurationError is available, use that instead.
-      ruleContext.ruleError(
-          "value must be one of "
-              + Starlark.repr(values.asList())
-              + ", but was "
-              + Starlark.repr(configuredValue.get()));
-      return null;
+            .getConfiguration()
+            .getOptions()
+            .getStarlarkOptions()
+            .get(ruleContext.getLabel());
+    if (!(rawStarlarkValue instanceof FeatureFlagValue)) {
+      // Retain legacy behavior of treating feature flags that somehow are not FeatureFlagValue
+      // as set to default
+      rawStarlarkValue = FeatureFlagValue.DefaultValue.INSTANCE;
     }
 
-    if (!configuredValue.isPresent() && !defaultValue.isPresent()) {
-      // TODO(b/140635901): When configurationError is available, use that instead.
-      ruleContext.ruleError("flag has no default and must be set, but was not set");
+    @Nullable
+    ConfigFeatureFlagProvider provider =
+        constructProvider((FeatureFlagValue) rawStarlarkValue, defaultValue, values, ruleContext);
+    if (provider == null) {
       return null;
     }
 
-    String value = configuredValue.orElseGet(defaultValue::get);
-
-    ConfigFeatureFlagProvider provider = ConfigFeatureFlagProvider.create(value, isValidValue);
-
     return new RuleConfiguredTargetBuilder(ruleContext)
         .setFilesToBuild(NestedSetBuilder.<Artifact>emptySet(STABLE_ORDER))
         .addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY)
         .addNativeDeclaredProvider(provider)
         .build();
   }
+
+  /**
+   * Calculate and return a ConfigFeatureFlagProvider.
+   *
+   * <p>At this point any errors here are due to something being 'wrong' with the configuration. In
+   * particular, this provider may be constructed BEFORE a rule transition sets the expected
+   * configuration and thus want to defer errors until later in analysis when this value is actually
+   * consumed as the errors would otherwise be unfixable.
+   *
+   * <p>An exception is made for if the value is explicitly set to value not in the allowed values
+   * list (either on the commandline or as part of a previous transition). In that case, that is an
+   * immediate error as can be fixed by ensuring those places set to allowed values. This is
+   * consistent with the behavior of build_setting.
+   */
+  @Nullable
+  private static ConfigFeatureFlagProvider constructProvider(
+      FeatureFlagValue featureFlagValue,
+      Optional<String> defaultValue,
+      ImmutableSet<String> values,
+      RuleContext ruleContext) {
+    Predicate<String> isValidValue = Predicates.in(values);
+    if (featureFlagValue instanceof FeatureFlagValue.SetValue) {
+      String setValue = ((FeatureFlagValue.SetValue) featureFlagValue).value();
+      if (!isValidValue.apply(setValue)) {
+        // This is consistent with build_setting, which also immediate checks that
+        // explicitly set values are valid values.
+        ruleContext.ruleError(
+            "value must be one of "
+                + Starlark.repr(values.asList())
+                + ", but was "
+                + Starlark.repr(setValue));
+        return null;
+      }
+      return ConfigFeatureFlagProvider.create(setValue, null, isValidValue);
+    } else if (featureFlagValue.equals(FeatureFlagValue.DefaultValue.INSTANCE)) {
+      if (!defaultValue.isPresent()) {
+        // Should defer error in case value is set by upcoming rule transition.
+        // (Although, rule authors could just add a default.)
+        // build_setting always has a default so this can't happen for them.
+        return ConfigFeatureFlagProvider.create(
+            null,
+            String.format(
+                "Feature flag %s has no default but no value was explicitly specified.",
+                ruleContext.getLabel()),
+            isValidValue);
+      }
+      return ConfigFeatureFlagProvider.create(defaultValue.get(), null, isValidValue);
+    } else if (featureFlagValue.equals(FeatureFlagValue.UnknownValue.INSTANCE)) {
+      // Must defer error in case value is set by upcoming rule transition.
+      // build_setting doesn't have trimming logic so this can't happen for them.
+      return ConfigFeatureFlagProvider.create(
+          null,
+          String.format(
+              "Feature flag %1$s was accessed in a configuration it is not present in. All "
+                  + "targets which depend on %1$s directly or indirectly must name it in their "
+                  + "transitive_configs attribute.",
+              ruleContext.getLabel()),
+          isValidValue);
+    } else {
+      throw new IllegalStateException("Impossible state for FeatureFlagValue: " + featureFlagValue);
+    }
+  }
 }

src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfiguration.java

@@ -15,57 +15,19 @@
 package com.google.devtools.build.lib.rules.config;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.collect.ImmutableSortedMap;
-import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.Fragment;
-import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.config.RequiresOptions;
-import com.google.devtools.build.lib.cmdline.Label;
-import java.util.Optional;
 
 /**
  * Configuration fragment for Android's config_feature_flag, flags which can be defined in BUILD
- * files.
+ * files. This exists only so that ConfigFeatureFlagOptions.class is retained.
  */
 @RequiresOptions(options = ConfigFeatureFlagOptions.class, starlark = true)
 public final class ConfigFeatureFlagConfiguration extends Fragment {
-  private final ImmutableSortedMap<Label, String> flagValues;
-
   /** Creates a new configuration fragment from the given {@link ConfigFeatureFlagOptions}. */
-  public ConfigFeatureFlagConfiguration(BuildOptions buildOptions)
-      throws InvalidConfigurationException {
-    this(FeatureFlagValue.getFlagValues(buildOptions));
-  }
+  public ConfigFeatureFlagConfiguration(BuildOptions buildOptions) {}
 
   @VisibleForTesting
-  ConfigFeatureFlagConfiguration(ImmutableSortedMap<Label, String> flagValues) {
-    this.flagValues = flagValues;
-  }
-
-  /**
-   * Retrieves the value of a configuration flag.
-   *
-   * <p>If the flag is not set in the current configuration, then the returned value will be empty.
-   *
-   * <p>Because the configuration should fail to construct if a required flag is missing, and
-   * because config_feature_flag (the only intended user of this method) automatically requires its
-   * own label, this method should never incorrectly return the default value for a flag which was
-   * set to a non-default value. If the flag value is available and non-default, it will be in
-   * flagValues; if the flag value is available and default, it will not be in flagValues; if the
-   * flag value is unavailable, this fragment's loader will fail and this method will never be
-   * called.
-   *
-   * <p>This method should only be used by the rule whose label is passed here. Other rules should
-   * depend on that rule and read a provider exported by it. To encourage callers of this method to
-   * do the right thing, this class takes {@link ArtifactOwner} instead of {@link Label}; to get the
-   * ArtifactOwner for a rule, call {@code ruleContext.getOwner()}.
-   */
-  public Optional<String> getFeatureFlagValue(ArtifactOwner owner) {
-    if (flagValues.containsKey(owner.getLabel())) {
-      return Optional.of(flagValues.get(owner.getLabel()));
-    } else {
-      return Optional.empty();
-    }
-  }
+  ConfigFeatureFlagConfiguration() {}
 }