Reimplement the configuration sanity check as a per-fragment check.

This is conceptually better (because each fragment should be
self-contained), and gives us better performance, as we no longer need to
load all explicit labels.

--
MOS_MIGRATED_REVID=118674470

5 files changed, 104 insertions, 115 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java
index 3e3b35f7b3..77c51d9e44 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java
@@ -39,8 +39,6 @@ public interface ConfigurationCollectionFactory {
    * @param loadedPackageProvider the package provider
    * @param buildOptions top-level build options representing the command-line
    * @param errorEventListener the event listener for errors
-   * @param performSanityCheck flag to signal about performing sanity check. Can be false only for
-   * tests in skyframe. Legacy mode uses loadedPackageProvider == null condition for this.
    * @return the top-level configuration
    * @throws InvalidConfigurationException
    */
@@ -50,8 +48,7 @@ public interface ConfigurationCollectionFactory {
       Cache<String, BuildConfiguration> cache,
       PackageProviderForConfigurations loadedPackageProvider,
       BuildOptions buildOptions,
-      EventHandler errorEventListener,
-      boolean performSanityCheck) throws InvalidConfigurationException;
+      EventHandler errorEventListener) throws InvalidConfigurationException;
 
   /**
    * Returns the module the given configuration should use for choosing dynamic transitions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 96a79209d8..9935a305b9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -148,9 +148,26 @@ public final class BuildConfiguration {
      * The resulting set only contains labels that were derived from command-line options; the
      * intention is that it can be used to sanity-check that the command-line options actually
      * contain these in their transitive closure.
+     *
+     * <p>This functionality only exists for legacy configuration fragments that compute labels from
+     * command-line option values. Don't do that! Instead, use a rule that specifies the mapping
+     * explicitly.
      */
     @SuppressWarnings("unused")
-    public void addImplicitLabels(Multimap<String, Label> implicitLabels) {
+    protected void addImplicitLabels(Multimap<String, Label> implicitLabels) {
+    }
+
+    /**
+     * Returns a multimap of all labels that should be implicitly loaded from labels that were
+     * specified as options, keyed by the name to be displayed to the user if something goes wrong.
+     * The returned set only contains labels that were derived from command-line options; the
+     * intention is that it can be used to sanity-check that the command-line options actually
+     * contain these in their transitive closure.
+     */
+    public final ListMultimap<String, Label> getImplicitLabels() {
+      ListMultimap<String, Label> implicitLabels = ArrayListMultimap.create();
+      addImplicitLabels(implicitLabels);
+      return implicitLabels;
     }
 
     /**
@@ -1849,21 +1866,6 @@ public final class BuildConfiguration {
   }
 
   /**
-   * Returns a multimap of all labels that should be implicitly loaded from labels that were
-   * specified as options, keyed by the name to be displayed to the user if something goes wrong.
-   * The returned set only contains labels that were derived from command-line options; the
-   * intention is that it can be used to sanity-check that the command-line options actually contain
-   * these in their transitive closure.
-   */
-  public ListMultimap<String, Label> getImplicitLabels() {
-    ListMultimap<String, Label> implicitLabels = ArrayListMultimap.create();
-    for (Fragment fragment : fragments.values()) {
-      fragment.addImplicitLabels(implicitLabels);
-    }
-    return implicitLabels;
-  }
-
-  /**
    * For an given environment, returns a subset containing all
    * variables in the given list if they are defined in the given
    * environment.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java
index cd8e10cc93..31eb150cab 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.analysis.config;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.cache.Cache;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -47,7 +46,6 @@ import javax.annotation.Nullable;
 public final class ConfigurationFactory {
   private final List<ConfigurationFragmentFactory> configurationFragmentFactories;
   private final ConfigurationCollectionFactory configurationCollectionFactory;
-  private boolean performSanityCheck = true;
 
   public ConfigurationFactory(
       ConfigurationCollectionFactory configurationCollectionFactory,
@@ -63,12 +61,8 @@ public final class ConfigurationFactory {
     this.configurationFragmentFactories = ImmutableList.copyOf(fragmentFactories);
   }
 
-  @VisibleForTesting
-  public void setSanityCheck(boolean performSanityCheck) {
-    this.performSanityCheck = performSanityCheck;
-  }
-
-  /** Creates a set of build configurations with top-level configuration having the given options.
+  /**
+   * Creates a set of build configurations with top-level configuration having the given options.
    *
    * <p>The rest of the configurations are created based on the set of transitions available.
    */
@@ -79,7 +73,7 @@ public final class ConfigurationFactory {
       EventHandler errorEventListener)
           throws InvalidConfigurationException {
     return configurationCollectionFactory.createConfigurations(this, cache,
-        loadedPackageProvider, buildOptions, errorEventListener, performSanityCheck);
+        loadedPackageProvider, buildOptions, errorEventListener);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java
index 4452f3da2d..3564564dfa 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java
@@ -31,20 +31,11 @@ import com.google.devtools.build.lib.analysis.config.ConfigurationFactory;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.config.PackageProviderForConfigurations;
 import com.google.devtools.build.lib.bazel.rules.cpp.BazelCppRuleClasses.CppTransition;
-import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.Attribute.SplitTransition;
 import com.google.devtools.build.lib.packages.Attribute.Transition;
-import com.google.devtools.build.lib.packages.BuildType;
-import com.google.devtools.build.lib.packages.NoSuchThingException;
-import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.packages.Target;
 
-import java.util.Collection;
-import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -63,8 +54,7 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
       Cache<String, BuildConfiguration> cache,
       PackageProviderForConfigurations packageProvider,
       BuildOptions buildOptions,
-      EventHandler eventHandler,
-      boolean performSanityCheck) throws InvalidConfigurationException {
+      EventHandler eventHandler) throws InvalidConfigurationException {
     // Target configuration
     BuildConfiguration targetConfiguration = configurationFactory.getConfiguration(
         packageProvider, buildOptions, false, cache);
@@ -107,29 +97,6 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
     if (packageProvider.valuesMissing()) {
       return null;
     }
-
-    // Sanity check that the implicit labels are all in the transitive closure of explicit ones.
-    // This also registers all targets in the cache entry and validates them on subsequent requests.
-    Set<Label> reachableLabels = new HashSet<>();
-    if (performSanityCheck) {
-      // We allow the package provider to be null for testing.
-      for (Map.Entry<String, Label> entry : buildOptions.getAllLabels().entries()) {
-        Label label = entry.getValue();
-        try {
-          collectTransitiveClosure(packageProvider, reachableLabels, label);
-        } catch (NoSuchThingException e) {
-          eventHandler.handle(Event.error(e.getMessage()));
-          throw new InvalidConfigurationException(
-              String.format("Failed to load required %s target: '%s'", entry.getKey(), label));
-        }
-      }
-      if (packageProvider.valuesMissing()) {
-        return null;
-      }
-      sanityCheckImplicitLabels(reachableLabels, targetConfiguration);
-      sanityCheckImplicitLabels(reachableLabels, hostConfiguration);
-    }
-
     BuildConfiguration result = setupTransitions(
         targetConfiguration, dataConfiguration, hostConfiguration, splitTransitionsTable);
     result.reportInvalidOptions(eventHandler);
@@ -252,52 +219,4 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
 
     return targetConfiguration;
   }
-
-  /**
-   * Checks that the implicit labels are reachable from the loaded labels. The loaded labels are
-   * those returned from {@link BuildOptions#getAllLabels()}, and the implicit ones are those that
-   * need to be available for late-bound attributes.
-   */
-  private void sanityCheckImplicitLabels(Collection<Label> reachableLabels,
-      BuildConfiguration config) throws InvalidConfigurationException {
-    for (Map.Entry<String, Label> entry : config.getImplicitLabels().entries()) {
-      if (!reachableLabels.contains(entry.getValue())) {
-        throw new InvalidConfigurationException("The required " + entry.getKey()
-            + " target is not transitively reachable from a command-line option: '"
-            + entry.getValue() + "'");
-      }
-    }
-  }
-
-  private void collectTransitiveClosure(PackageProviderForConfigurations packageProvider,
-      Set<Label> reachableLabels, Label from) throws NoSuchThingException {
-    if (!reachableLabels.add(from)) {
-      return;
-    }
-    Target fromTarget = packageProvider.getTarget(from);
-    if (fromTarget instanceof Rule) {
-      Rule rule = (Rule) fromTarget;
-      if (rule.getRuleClassObject().hasAttr("srcs", BuildType.LABEL_LIST)) {
-        // TODO(bazel-team): refine this. This visits "srcs" reachable under *any* configuration,
-        // not necessarily the configuration actually applied to the rule. We should correlate the
-        // two. However, doing so requires faithfully reflecting the configuration transitions that
-        // might happen as we traverse the dependency chain.
-        // TODO(bazel-team): Why don't we use AbstractAttributeMapper#visitLabels() here?
-        for (List<Label> labelsForConfiguration :
-            AggregatingAttributeMapper.of(rule).visitAttribute("srcs", BuildType.LABEL_LIST)) {
-          for (Label label : labelsForConfiguration) {
-            collectTransitiveClosure(packageProvider, reachableLabels,
-                from.resolveRepositoryRelative(label));
-          }
-        }
-      }
-
-      if (rule.getRuleClass().equals("bind")) {
-        Label actual = AggregatingAttributeMapper.of(rule).get("actual", BuildType.LABEL);
-        if (actual != null) {
-          collectTransitiveClosure(packageProvider, reachableLabels, actual);
-        }
-      }
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java
index 0242f3a79c..f2a12e7b34 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ListMultimap;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
@@ -24,10 +25,16 @@ import com.google.devtools.build.lib.analysis.config.InvalidConfigurationExcepti
 import com.google.devtools.build.lib.analysis.config.PackageProviderForConfigurations;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
+import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.AttributeMap.AcceptsLabelAttribute;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
+import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.skyframe.ConfigurationFragmentValue.ConfigurationFragmentKey;
@@ -38,12 +45,15 @@ import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
 import java.io.IOException;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.Map;
+import java.util.Set;
 
 /**
  * A builder for {@link ConfigurationFragmentValue}s.
  */
-public class ConfigurationFragmentFunction implements SkyFunction {
-
+public final class ConfigurationFragmentFunction implements SkyFunction {
   private final Supplier<ImmutableList<ConfigurationFragmentFactory>> configurationFragments;
   private final RuleClassProvider ruleClassProvider;
 
@@ -66,7 +76,11 @@ public class ConfigurationFragmentFunction implements SkyFunction {
           new SkyframePackageLoaderWithValueEnvironment(env, ruleClassProvider);
       ConfigurationEnvironment confEnv = new ConfigurationBuilderEnvironment(packageProvider);
       Fragment fragment = factory.create(confEnv, buildOptions);
-      
+
+      if (env.valuesMissing()) {
+        return null;
+      }
+      sanityCheck(fragment, buildOptions, packageProvider);
       if (env.valuesMissing()) {
         return null;
       }
@@ -80,7 +94,70 @@ public class ConfigurationFragmentFunction implements SkyFunction {
       throw new ConfigurationFragmentFunctionException(e);
     }
   }
-  
+
+  /**
+   * Checks that the implicit labels are reachable from the loaded labels. The loaded labels are
+   * those returned from {@link BuildOptions#getAllLabels()}, and the implicit ones are those that
+   * are returned from {@link Fragment#getImplicitLabels}.
+   */
+  private void sanityCheck(Fragment fragment, BuildOptions buildOptions,
+      PackageProviderForConfigurations packageProvider) throws InvalidConfigurationException {
+    if (fragment == null) {
+      return;
+    }
+    ListMultimap<String, Label> implicitLabels = fragment.getImplicitLabels();
+    if (implicitLabels.isEmpty()) {
+      return;
+    }
+    // Sanity check that the implicit labels are all in the transitive closure of explicit ones.
+    // This also registers all targets in the cache entry and validates them on subsequent requests.
+    Set<Label> reachableLabels = new HashSet<>();
+    for (Map.Entry<String, Label> entry : buildOptions.getAllLabels().entries()) {
+      Label label = entry.getValue();
+      try {
+        collectAllTransitiveLabels(packageProvider, reachableLabels, label);
+      } catch (NoSuchThingException e) {
+        packageProvider.getEventHandler().handle(Event.error(e.getMessage()));
+        throw new InvalidConfigurationException(
+            String.format("Failed to load required %s target: '%s'", entry.getKey(), label));
+      }
+    }
+    if (packageProvider.valuesMissing()) {
+      return;
+    }
+    for (Map.Entry<String, Label> entry : implicitLabels.entries()) {
+      if (!reachableLabels.contains(entry.getValue())) {
+        throw new InvalidConfigurationException(
+            String.format("The required %s target is not transitively reachable from a "
+            + "command-line option: '%s'", entry.getKey(), entry.getValue()));
+      }
+    }
+  }
+
+  private void collectAllTransitiveLabels(PackageProviderForConfigurations packageProvider,
+      Set<Label> reachableLabels, Label from) throws NoSuchThingException {
+    if (!reachableLabels.add(from)) {
+      return;
+    }
+    Target fromTarget = packageProvider.getTarget(from);
+    if (fromTarget == null) {
+      return;
+    }
+    if (fromTarget instanceof Rule) {
+      Rule rule = (Rule) fromTarget;
+      final Set<Label> allLabels = new LinkedHashSet<>();
+      AggregatingAttributeMapper.of(rule).visitLabels(new AcceptsLabelAttribute() {
+        @Override
+        public void acceptLabelAttribute(Label label, Attribute attribute) {
+          allLabels.add(label);
+        }
+      });
+      for (Label label : allLabels) {
+        collectAllTransitiveLabels(packageProvider, reachableLabels, label);
+      }
+    }
+  }
+
   private ConfigurationFragmentFactory getFactory(Class<? extends Fragment> fragmentType) {
     for (ConfigurationFragmentFactory factory : configurationFragments.get()) {
       if (factory.creates().equals(fragmentType)) {