Batch all DependencyResolver#getTarget calls. This leads to some duplicate iteration, but that should be cheap, while requesting packages sequentially can hurt...

PiperOrigin-RevId: 208126130

7 files changed, 114 insertions, 64 deletions

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 b6107f4915..3b6221ad82 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
@@ -46,6 +46,7 @@ import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.pkgcache.LoadingFailureEvent;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
+import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.testutil.Suite;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
@@ -1321,6 +1322,43 @@ public class BuildViewTest extends BuildViewTestBase {
   }
 
   @Test
+  public void errorInImplicitDeps() throws Exception {
+    setRulesAvailableInTests(
+        (MockRule)
+            () -> {
+              try {
+                return MockRule.define(
+                    "custom_rule",
+                    attr("$implicit1", BuildType.LABEL_LIST)
+                        .defaultValue(
+                            ImmutableList.of(
+                                Label.parseAbsoluteUnchecked("//bad2:label"),
+                                Label.parseAbsoluteUnchecked("//foo:dep"))),
+                    attr("$implicit2", BuildType.LABEL)
+                        .defaultValue(Label.parseAbsoluteUnchecked("//bad:label")));
+              } catch (Type.ConversionException e) {
+                throw new IllegalStateException(e);
+              }
+            });
+    scratch.file("foo/BUILD", "custom_rule(name = 'foo')", "sh_library(name = 'dep')");
+    scratch.file(
+        "bad/BUILD",
+        "sh_library(name = 'other_label', nonexistent_attribute = 'blah')",
+        "sh_library(name = 'label')");
+    // bad2/BUILD is completely missing.
+    reporter.removeHandler(failFastHandler);
+    update(defaultFlags().with(Flag.KEEP_GOING), "//foo:foo");
+    assertContainsEvent(
+        "every rule of type custom_rule implicitly depends upon the target '//bad2:label', but this"
+            + " target could not be found because of: no such package 'bad2': BUILD file not found "
+            + "on package path");
+    assertContainsEvent(
+        "every rule of type custom_rule implicitly depends upon the target '//bad:label', but this "
+            + "target could not be found because of: Target '//bad:label' contains an error and its"
+            + " package is in error");
+  }
+
+  @Test
   public void onlyAllowedRuleClassesWithWarning() throws Exception {
     setRulesAvailableInTests(
         (MockRule) () -> MockRule.define(
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/ConfiguredAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ConfiguredAttributeMapperTest.java
index 15a90a87a1..f3014f20d0 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/ConfiguredAttributeMapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/ConfiguredAttributeMapperTest.java
@@ -19,13 +19,13 @@ import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.config.CompilationMode;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
 import com.google.devtools.build.lib.syntax.Type;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -114,37 +114,36 @@ public class ConfiguredAttributeMapperTest extends BuildViewTestCase {
         "    name = 'defaultdep',",
         "    srcs = ['defaultdep.sh'])");
 
-    final List<Label> visitedLabels = new ArrayList<>();
-    AttributeMap.AcceptsLabelAttribute testVisitor =
-        new AttributeMap.AcceptsLabelAttribute() {
-          @Override
-          public void acceptLabelAttribute(Label label, Attribute attribute) {
-            if (label.toString().contains("//a:")) { // Ignore implicit common dependencies.
-              visitedLabels.add(label);
-            }
-          }
-        };
-
-    final Label binSrc = Label.parseAbsolute("//a:bin.sh", ImmutableMap.of());
+    List<Label> visitedLabels = new ArrayList<>();
+    Label binSrc = Label.parseAbsolute("//a:bin.sh", ImmutableMap.of());
 
     useConfiguration("--define", "mode=a");
-    getMapper("//a:bin").visitLabels(testVisitor);
+    addRelevantLabels(getMapper("//a:bin").visitLabels(), visitedLabels);
     assertThat(visitedLabels)
         .containsExactly(binSrc, Label.parseAbsolute("//a:adep", ImmutableMap.of()));
 
     visitedLabels.clear();
     useConfiguration("--define", "mode=b");
-    getMapper("//a:bin").visitLabels(testVisitor);
+    addRelevantLabels(getMapper("//a:bin").visitLabels(), visitedLabels);
     assertThat(visitedLabels)
         .containsExactly(binSrc, Label.parseAbsolute("//a:bdep", ImmutableMap.of()));
 
     visitedLabels.clear();
     useConfiguration("--define", "mode=c");
-    getMapper("//a:bin").visitLabels(testVisitor);
+    addRelevantLabels(getMapper("//a:bin").visitLabels(), visitedLabels);
     assertThat(visitedLabels)
         .containsExactly(binSrc, Label.parseAbsolute("//a:defaultdep", ImmutableMap.of()));
   }
 
+  private static void addRelevantLabels(
+      Collection<AttributeMap.DepEdge> depEdges, Collection<Label> visitedLabels) {
+    depEdges
+        .stream()
+        .map(AttributeMap.DepEdge::getLabel)
+        .filter((label) -> label.getPackageIdentifier().getPackageFragment().toString().equals("a"))
+        .forEach(visitedLabels::add);
+  }
+
   /**
    * Tests that for configurable attributes where the *values* are evaluated in different
    * configurations, the configuration checking still uses the original configuration.
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 26005cfc3d..4ee313c221 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
@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertWithMessage;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
@@ -40,6 +41,9 @@ 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.Map;
+import java.util.function.Function;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import org.junit.Before;
 import org.junit.Test;
@@ -80,14 +84,26 @@ public class DependencyResolverTest extends AnalysisTestCase {
             throw new IllegalStateException(e);
           }
 
-          @Nullable
           @Override
-          protected Target getTarget(Target from, Label label, NestedSetBuilder<Cause> rootCauses) {
-            try {
-              return packageManager.getTarget(reporter, label);
-            } catch (NoSuchPackageException | NoSuchTargetException | InterruptedException e) {
-              throw new IllegalStateException(e);
-            }
+          protected Map<Label, Target> getTargets(
+              Iterable<Label> labels,
+              Target fromTarget,
+              NestedSetBuilder<Cause> rootCauses,
+              int labelsSizeHint) {
+            return Streams.stream(labels)
+                .distinct()
+                .collect(
+                    Collectors.toMap(
+                        Function.identity(),
+                        label -> {
+                          try {
+                            return packageManager.getTarget(reporter, label);
+                          } catch (NoSuchPackageException
+                              | NoSuchTargetException
+                              | InterruptedException e) {
+                            throw new IllegalStateException(e);
+                          }
+                        }));
           }
 
           @Nullable
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/select/AbstractAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/select/AbstractAttributeMapperTest.java
index ac89f9f0eb..4901884590 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/select/AbstractAttributeMapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/select/AbstractAttributeMapperTest.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.analysis.select;
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.fail;
 
-import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.AbstractAttributeMapper;
@@ -29,6 +28,7 @@ import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.syntax.Type;
 import java.util.List;
+import java.util.stream.Collectors;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -116,27 +116,20 @@ public class AbstractAttributeMapperTest extends BuildViewTestCase {
     assertThat(mapper.isAttributeValueExplicitlySpecified("nonsense")).isFalse();
   }
 
-  protected static class VisitationRecorder implements AttributeMap.AcceptsLabelAttribute {
-    public List<String> labelsVisited = Lists.newArrayList();
-    private final String attrName;
-
-    public VisitationRecorder(String attrName) {
-      this.attrName = attrName;
-    }
-
-    @Override
-    public void acceptLabelAttribute(Label label, Attribute attribute) {
-      if (attribute.getName().equals(attrName)) {
-        labelsVisited.add(label.toString());
-      }
-    }
-  }
-
   @Test
   public void testVisitation() throws Exception {
-    VisitationRecorder recorder = new VisitationRecorder("srcs");
-    mapper.visitLabels(recorder);
-    assertThat(recorder.labelsVisited).containsExactly("//p:a", "//p:b", "//p:c");
+    assertThat(getLabelsForAttribute(mapper, "srcs")).containsExactly("//p:a", "//p:b", "//p:c");
+  }
+
+  protected static List<String> getLabelsForAttribute(
+      AttributeMap attributeMap, String attributeName) throws InterruptedException {
+    return attributeMap
+        .visitLabels()
+        .stream()
+        .filter((d) -> d.getAttribute().getName().equals(attributeName))
+        .map(AttributeMap.DepEdge::getLabel)
+        .map(Label::toString)
+        .collect(Collectors.toList());
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java
index a8f912602c..073c751766 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/select/AggregatingAttributeMapperTest.java
@@ -146,9 +146,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
         "              '" + BuildType.Selector.DEFAULT_CONDITION_KEY + "': ['default.sh'],",
         "          }))");
 
-    VisitationRecorder recorder = new VisitationRecorder("srcs");
-    AggregatingAttributeMapper.of(rule).visitLabels(recorder);
-    assertThat(recorder.labelsVisited)
+    assertThat(getLabelsForAttribute(AggregatingAttributeMapper.of(rule), "srcs"))
         .containsExactlyElementsIn(
             ImmutableList.of(
                 "//a:a.sh", "//a:b.sh", "//a:default.sh", "//conditions:a", "//conditions:b"));
@@ -163,9 +161,7 @@ public class AggregatingAttributeMapperTest extends AbstractAttributeMapperTest
         "        '//conditions:a': None,",
         "    }))");
 
-    VisitationRecorder recorder = new VisitationRecorder("malloc");
-    AggregatingAttributeMapper.of(rule).visitLabels(recorder);
-    assertThat(recorder.labelsVisited)
+    assertThat(getLabelsForAttribute(AggregatingAttributeMapper.of(rule), "malloc"))
         .containsExactly("//conditions:a", getDefaultMallocLabel(rule).toString());
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java
index e5f38cb650..2078904857 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/select/RawAttributeMapperTest.java
@@ -18,8 +18,6 @@ import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.packages.Attribute;
-import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.RawAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
@@ -97,12 +95,7 @@ public class RawAttributeMapperTest extends AbstractAttributeMapperTest {
   public void testVisitLabels() throws Exception {
     RawAttributeMapper rawMapper = RawAttributeMapper.of(setupGenRule());
     try {
-      rawMapper.visitLabels(new AttributeMap.AcceptsLabelAttribute() {
-        @Override
-        public void acceptLabelAttribute(Label label, Attribute attribute) {
-          // Nothing to do.
-        }
-      });
+      rawMapper.visitLabels();
       fail("Expected label visitation to fail since one attribute is configurable");
     } catch (IllegalArgumentException e) {
       assertThat(e)
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
index 083688d02b..1f92bea669 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Streams;
 import com.google.common.eventbus.EventBus;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.PackageRoots;
@@ -90,6 +91,8 @@ import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.Function;
+import java.util.stream.Collectors;
 
 /**
  * A util class that contains all the helper stuff previously in BuildView that only exists to give
@@ -310,13 +313,25 @@ public class BuildViewForTesting {
       }
 
       @Override
-      protected Target getTarget(Target from, Label label, NestedSetBuilder<Cause> rootCauses)
-          throws InterruptedException {
-        try {
-          return skyframeExecutor.getPackageManager().getTarget(eventHandler, label);
-        } catch (NoSuchThingException e) {
-          throw new IllegalStateException(e);
-        }
+      protected Map<Label, Target> getTargets(
+          Iterable<Label> labels,
+          Target fromTarget,
+          NestedSetBuilder<Cause> rootCauses,
+          int labelsSizeHint) {
+        return Streams.stream(labels)
+            .distinct()
+            .collect(
+                Collectors.toMap(
+                    Function.identity(),
+                    label -> {
+                      try {
+                        return skyframeExecutor.getPackageManager().getTarget(eventHandler, label);
+                      } catch (NoSuchPackageException
+                          | NoSuchTargetException
+                          | InterruptedException e) {
+                        throw new IllegalStateException(e);
+                      }
+                    }));
       }
 
       @Override