Optimize how null configurations get created and add test infrastructure for Bazel's dep configuration creation logic.

This essentially implements the following TODOs:

https://github.com/bazelbuild/bazel/blob/bc6045dcc8fa33d4241d231138020ac4bdecc14f/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java#L599
https://github.com/bazelbuild/bazel/blob/bc6045dcc8fa33d4241d231138020ac4bdecc14f/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java#L42

--
MOS_MIGRATED_REVID=134607049

6 files changed, 257 insertions, 14 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index 67efd8385e..3f126df7cd 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -328,6 +328,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:io",
         "//src/main/java/com/google/devtools/build/lib:java-rules",
         "//src/main/java/com/google/devtools/build/lib:packages",
+        "//src/main/java/com/google/devtools/build/lib:proto-rules",
         "//src/main/java/com/google/devtools/build/lib:python-rules",
         "//src/main/java/com/google/devtools/build/lib:runtime",
         "//src/main/java/com/google/devtools/build/lib:util",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index 73642c5fe4..25d833c2c2 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -345,20 +345,15 @@ public class BuildViewTest extends BuildViewTestBase {
               Label.parseAbsolute("//package:inner"),
               Attribute.ConfigurationTransition.NONE,
               ImmutableSet.<AspectDescriptor>of());
-      fileDependency =
-          Dependency.withTransitionAndAspects(
-              Label.parseAbsolute("//package:file"),
-              Attribute.ConfigurationTransition.NULL,
-              ImmutableSet.<AspectDescriptor>of());
     } else {
       innerDependency =
           Dependency.withConfiguration(
               Label.parseAbsolute("//package:inner"),
               getTargetConfiguration());
-      fileDependency =
-          Dependency.withNullConfiguration(
-              Label.parseAbsolute("//package:file"));
     }
+    fileDependency =
+        Dependency.withNullConfiguration(
+            Label.parseAbsolute("//package:file"));
 
     assertThat(targets).containsExactly(innerDependency, fileDependency);
   }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java b/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
index d498709148..2d50ef7e77 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
@@ -32,6 +32,8 @@ 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.Target;
+import com.google.devtools.build.lib.testutil.Suite;
+import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.util.OrderedSetMultimap;
 import java.util.List;
 import java.util.Set;
@@ -109,7 +111,7 @@ public class DependencyResolverTest extends AnalysisTestCase {
   }
 
   @SafeVarargs
-  private final void assertDep(
+  private final Dependency assertDep(
       OrderedSetMultimap<Attribute, Dependency> dependentNodeMap,
       String attrName,
       String dep,
@@ -133,6 +135,7 @@ public class DependencyResolverTest extends AnalysisTestCase {
 
     assertNotNull("Dependency '" + dep + "' on attribute '" + attrName + "' not found", dependency);
     assertThat(dependency.getAspects()).containsExactly((Object[]) aspects);
+    return dependency;
   }
 
   @Test
@@ -182,4 +185,39 @@ public class DependencyResolverTest extends AnalysisTestCase {
         dependentNodeMap("//a:a", TestAspects.EXTRA_ATTRIBUTE_ASPECT);
     assertDep(map, "$dep", "//extra:extra");
   }
+
+  /**
+   * Null configurations should be static whether we're building with static or dynamic
+   * configurations. This is because the dynamic config logic that translates transitions into
+   * final configurations can be trivially skipped in those cases.
+   */
+  @Test
+  public void nullConfigurationsAlwaysStatic() throws Exception {
+    pkg("a",
+        "genrule(name = 'gen', srcs = ['gen.in'], cmd = '', outs = ['gen.out'])");
+    update();
+    Dependency dep = assertDep(dependentNodeMap("//a:gen", null), "srcs", "//a:gen.in");
+    assertThat(dep.hasStaticConfiguration()).isTrue();
+    assertThat(dep.getConfiguration()).isNull();
+  }
+
+  /** Runs the same test with trimmed dynamic configurations. */
+  @TestSpec(size = Suite.SMALL_TESTS)
+  @RunWith(JUnit4.class)
+  public static class WithDynamicConfigurations extends DependencyResolverTest {
+    @Override
+    protected FlagBuilder defaultFlags() {
+      return super.defaultFlags().with(Flag.DYNAMIC_CONFIGURATIONS);
+    }
+  }
+
+  /** Runs the same test with untrimmed dynamic configurations. */
+  @TestSpec(size = Suite.SMALL_TESTS)
+  @RunWith(JUnit4.class)
+  public static class WithDynamicConfigurationsNoTrim extends DependencyResolverTest {
+    @Override
+    protected FlagBuilder defaultFlags() {
+      return super.defaultFlags().with(Flag.DYNAMIC_CONFIGURATIONS_NOTRIM);
+    }
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
index 210c8b140c..5c79699d6a 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
@@ -36,6 +36,7 @@ import com.google.devtools.build.lib.rules.java.JavaConfigurationLoader;
 import com.google.devtools.build.lib.rules.java.JvmConfigurationLoader;
 import com.google.devtools.build.lib.rules.objc.J2ObjcConfiguration;
 import com.google.devtools.build.lib.rules.objc.ObjcConfigurationLoader;
+import com.google.devtools.build.lib.rules.proto.ProtoConfiguration;
 import com.google.devtools.build.lib.rules.python.PythonConfigurationLoader;
 import com.google.devtools.build.lib.testutil.BuildRuleBuilder;
 import com.google.devtools.build.lib.testutil.BuildRuleWithDefaultsBuilder;
@@ -216,6 +217,7 @@ public final class BazelAnalysisMock extends AnalysisMock {
         new ObjcConfigurationLoader(),
         new AppleConfiguration.Loader(),
         new J2ObjcConfiguration.Loader(),
+        new ProtoConfiguration.Loader(),
         new AndroidConfiguration.Loader());
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index 8aea62c609..6fdec64bbf 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -91,7 +91,10 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
   public enum Flag {
     KEEP_GOING,
     SKYFRAME_LOADING_PHASE,
+    // Dynamic configurations that only include the fragments a target needs to properly analyze.
     DYNAMIC_CONFIGURATIONS,
+    // Dynamic configurations that always include all fragments even for targets don't need them.
+    DYNAMIC_CONFIGURATIONS_NOTRIM
   }
 
   /** Helper class to make it easy to enable and disable flags. */
@@ -220,6 +223,8 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
     optionsParser.parse(args);
     if (defaultFlags().contains(Flag.DYNAMIC_CONFIGURATIONS)) {
       optionsParser.parse("--experimental_dynamic_configs=on");
+    } else if (defaultFlags().contains(Flag.DYNAMIC_CONFIGURATIONS_NOTRIM)) {
+      optionsParser.parse("--experimental_dynamic_configs=notrim");
     }
 
     InvocationPolicyEnforcer optionsPolicyEnforcer = analysisMock.getInvocationPolicyEnforcer();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java
index b68a1190ef..a0829826ab 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java
@@ -20,13 +20,34 @@ import static org.junit.Assert.fail;
 import com.google.common.base.VerifyException;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.HashMultimap;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.SetMultimap;
+import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.DependencyResolver;
+import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
-import com.google.devtools.build.lib.testutil.FoundationTestCase;
+import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
+import com.google.devtools.build.lib.analysis.util.AnalysisMock;
+import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.RuleClassProvider;
+import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
 import com.google.devtools.build.lib.testutil.Suite;
 import com.google.devtools.build.lib.testutil.TestSpec;
+import com.google.devtools.build.lib.util.OrderedSetMultimap;
+import com.google.devtools.build.skyframe.EvaluationResult;
+import com.google.devtools.build.skyframe.SkyFunction;
+import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.build.skyframe.SkyFunctionName;
+import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyValue;
+import java.util.Collection;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -34,15 +55,167 @@ import org.junit.runners.JUnit4;
 /**
  * Tests {@link ConfiguredTargetFunction}'s logic for determining each target's
  * {@link BuildConfiguration}.
+ *
+ * <p>This is essentially an integration test for
+ * {@link ConfiguredTargetFunction#computeDependencies} and {@link DependencyResolver}. These
+ * methods form the core logic that figures out what a target's deps are, how their configurations
+ * should differ from their parent, and how to instantiate those configurations as tangible
+ * {@link BuildConfiguration} objects.
+ *
+ * <p>{@link ConfiguredTargetFunction} is a complicated class that does a lot of things. This test
+ * focuses purely on the task of determining configurations for deps. So instead of evaluating
+ * full {@link ConfiguredTargetFunction} instances, it evaluates a mock {@link SkyFunction} that
+ * just wraps the {@link ConfiguredTargetFunction#computeDependencies} part. This keeps focus tight
+ * and integration dependencies narrow.
+ *
+ * <p>We can't just call {@link ConfiguredTargetFunction#computeDependencies} directly because that
+ * method needs a {@link SkyFunction.Environment} and Blaze's test infrastructure doesn't support
+ * direct access to environments.
  */
 @TestSpec(size = Suite.SMALL_TESTS)
 @RunWith(JUnit4.class)
-public class ConfigurationsForTargetsTest extends FoundationTestCase {
+public class ConfigurationsForTargetsTest extends AnalysisTestCase {
 
-  // TODO(gregce): add thorough unit testing for getDynamicConfigurations.
+  /**
+   * A mock {@link SkyFunction} that just calls {@link ConfiguredTargetFunction#computeDependencies}
+   * and returns its results.
+   */
+  private static class ComputeDependenciesFunction implements SkyFunction {
+    static final SkyFunctionName SKYFUNCTION_NAME =
+         SkyFunctionName.create("CONFIGURED_TARGET_FUNCTION_COMPUTE_DEPENDENCIES");
+
+    private final LateBoundStateProvider stateProvider;
+
+    ComputeDependenciesFunction(LateBoundStateProvider lateBoundStateProvider) {
+      this.stateProvider = lateBoundStateProvider;
+    }
+
+    /**
+     * Returns a {@link SkyKey} for a given <Target, BuildConfiguration> pair.
+     */
+    static SkyKey key(Target target, BuildConfiguration config) {
+      return SkyKey.create(SKYFUNCTION_NAME, new TargetAndConfiguration(target, config));
+    }
+
+    /**
+     * Returns a {@link OrderedSetMultimap<Attribute, ConfiguredTarget>} map representing the
+     * deps of given target.
+     */
+    static class Value implements SkyValue {
+      OrderedSetMultimap<Attribute, ConfiguredTarget> depMap;
+      Value(OrderedSetMultimap<Attribute, ConfiguredTarget> depMap) {
+        this.depMap = depMap;
+      }
+    }
+
+    @Override
+    public SkyValue compute(SkyKey skyKey, Environment env)
+        throws EvalException, InterruptedException {
+      try {
+        OrderedSetMultimap<Attribute, ConfiguredTarget> depMap =
+            ConfiguredTargetFunction.computeDependencies(
+                env, new SkyframeDependencyResolver(env),
+                (TargetAndConfiguration) skyKey.argument(), null,
+                ImmutableMap.<Label, ConfigMatchingProvider>of(),
+                stateProvider.lateBoundRuleClassProvider(), stateProvider.lateBoundHostConfig(),
+                NestedSetBuilder.stableOrder(), NestedSetBuilder.stableOrder());
+        return env.valuesMissing() ? null : new Value(depMap);
+      } catch (Exception e) {
+        throw new EvalException(e);
+      }
+    }
+
+    private static class EvalException extends SkyFunctionException {
+      public EvalException(Exception cause) {
+        super(cause, Transience.PERSISTENT); // We can generalize the transience if/when needed.
+      }
+    }
+
+    @Override
+    public String extractTag(SkyKey skyKey) {
+      return ((TargetAndConfiguration) skyKey.argument()).getLabel().getName();
+    }
+  }
+
+  /**
+   * Provides build state to {@link ComputeDependenciesFunction}. This needs to be late-bound (i.e.
+   * we can't just pass the contents directly) because of the way {@link AnalysisTestCase} works:
+   * the {@link AnalysisMock} instance that instantiates the function gets created before the rest
+   * of the build state. See {@link AnalysisTestCase#createMocks} for details.
+   */
+  private class LateBoundStateProvider {
+    RuleClassProvider lateBoundRuleClassProvider() {
+      return ruleClassProvider;
+    }
+    BuildConfiguration lateBoundHostConfig() {
+      return getHostConfiguration();
+    }
+  }
+
+  /**
+   * An {@link AnalysisMock} that injects {@link ComputeDependenciesFunction} into the Skyframe
+   * executor.
+   */
+  private static final class AnalysisMockWithComputeDepsFunction extends AnalysisMock.Delegate {
+    private final LateBoundStateProvider stateProvider;
+    AnalysisMockWithComputeDepsFunction(LateBoundStateProvider stateProvider) {
+      super(AnalysisMock.get());
+      this.stateProvider = stateProvider;
+    }
+
+    @Override
+    public ImmutableMap<SkyFunctionName, SkyFunction> getSkyFunctions() {
+      return ImmutableMap.<SkyFunctionName, SkyFunction>builder()
+          .putAll(super.getSkyFunctions())
+          .put(
+              ComputeDependenciesFunction.SKYFUNCTION_NAME,
+              new ComputeDependenciesFunction(stateProvider))
+          .build();
+    }
+  };
+
+  @Override
+  protected AnalysisMock getAnalysisMock() {
+    return new AnalysisMockWithComputeDepsFunction(new LateBoundStateProvider());
+  }
+
+  /**
+   * Returns the configured deps for a given target, assuming the target uses the target
+   * configuration.
+   */
+  private Multimap<Attribute, ConfiguredTarget> getConfiguredDeps(String targetLabel)
+      throws Exception {
+    update(targetLabel);
+    SkyKey key = ComputeDependenciesFunction.key(getTarget(targetLabel), getTargetConfiguration());
+    // Must re-enable analysis for Skyframe functions that create configured targets.
+    skyframeExecutor.getSkyframeBuildView().enableAnalysis(true);
+    Object evalResult = SkyframeExecutorTestUtils.evaluate(
+        skyframeExecutor, key, /*keepGoing=*/false, reporter);
+    skyframeExecutor.getSkyframeBuildView().enableAnalysis(false);
+    SkyValue value = ((EvaluationResult<ComputeDependenciesFunction.Value>) evalResult).get(key);
+    return ((ComputeDependenciesFunction.Value) value).depMap;
+  }
+
+  /**
+   * Returns the configured deps for a given target under the given attribute. Assumes the target
+   * uses the target configuration.
+   *
+   * <p>Throws an exception if the attribute can't be found.
+   */
+  private Collection<ConfiguredTarget> getConfiguredDeps(String targetLabel, String attrName)
+      throws Exception {
+    Multimap<Attribute, ConfiguredTarget> allDeps = getConfiguredDeps(targetLabel);
+    for (Attribute attribute : allDeps.keySet()) {
+      if (attribute.getName().equals(attrName)) {
+        return allDeps.get(attribute);
+      }
+    }
+    throw new AssertionError(
+        String.format("Couldn't find attribute %s for label %s", attrName, targetLabel));
+  }
 
   @Test
-  public void testPutOnlyEntryWithSetMultimap() throws Exception {
+  public void putOnlyEntryCorrectWithSetMultimap() throws Exception {
     internalTestPutOnlyEntry(HashMultimap.create());
   }
 
@@ -51,7 +224,7 @@ public class ConfigurationsForTargetsTest extends FoundationTestCase {
    * sure that doesn't fool {@link ConfiguredTargetFunction#putOnlyEntry}.
    */
   @Test
-  public void testPutOnlyEntryWithListMultimap() throws Exception {
+  public void putOnlyEntryCorrectWithListMultimap() throws Exception {
     internalTestPutOnlyEntry(ArrayListMultimap.create());
   }
 
@@ -71,4 +244,33 @@ public class ConfigurationsForTargetsTest extends FoundationTestCase {
       assertThat(e).hasMessage("couldn't insert bar: map already has key foo");
     }
   }
+
+  @Test
+  public void nullConfiguredDepsHaveExpectedConfigs() throws Exception {
+    scratch.file(
+        "a/BUILD",
+        "genrule(name = 'gen', srcs = ['gen.in'], cmd = '', outs = ['gen.out'])");
+    ConfiguredTarget genIn = Iterables.getOnlyElement(getConfiguredDeps("//a:gen", "srcs"));
+    assertThat(genIn.getConfiguration()).isNull();
+  }
+
+  /** Runs the same test with trimmed dynamic configurations. */
+  @TestSpec(size = Suite.SMALL_TESTS)
+  @RunWith(JUnit4.class)
+  public static class WithDynamicConfigurations extends ConfigurationsForTargetsTest {
+    @Override
+    protected FlagBuilder defaultFlags() {
+      return super.defaultFlags().with(Flag.DYNAMIC_CONFIGURATIONS);
+    }
+  }
+
+  /** Runs the same test with untrimmed dynamic configurations. */
+  @TestSpec(size = Suite.SMALL_TESTS)
+  @RunWith(JUnit4.class)
+  public static class WithDynamicConfigurationsNoTrim extends ConfigurationsForTargetsTest {
+    @Override
+    protected FlagBuilder defaultFlags() {
+      return super.defaultFlags().with(Flag.DYNAMIC_CONFIGURATIONS_NOTRIM);
+    }
+  }
 }