Split the PackageManager type hierarchy; no longer inherit LoadedPackageProv.

This limits the exposure of LoadedPackageProvider, such that there will be
no regressions in the use of getLoadedTarget. Unfortunately, fully removing
LoadedPackageProvider is more work than I'm willing to take on right now, and
this is the cleanest intermediate solution I could come up with.

This unblocks my other work (removing SkyframeExecutor.errorEventHandler).
Someone else will have to shave this yak.

--
MOS_MIGRATED_REVID=103943375

13 files changed, 85 insertions, 70 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 7fd00a0b35..d6c3d413a0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -261,7 +261,7 @@ public class BuildView {
       SkyframeExecutor skyframeExecutor,
       BinTools binTools, CoverageReportActionFactory coverageReportActionFactory) {
     this.directories = directories;
-    this.packageManager = skyframeExecutor.getPackageManager();
+    this.packageManager = skyframeExecutor.getLoadedPackageProvider();
     this.binTools = binTools;
     this.coverageReportActionFactory = coverageReportActionFactory;
     this.ruleClassProvider = ruleClassProvider;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java
index 44ef2d89ac..441e84a1d2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationEnvironment.java
@@ -17,18 +17,16 @@ package com.google.devtools.build.lib.analysis.config;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.concurrent.Uninterruptibles;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
 import com.google.devtools.build.lib.pkgcache.PackageProvider;
 import com.google.devtools.build.lib.pkgcache.TargetProvider;
 import com.google.devtools.build.lib.vfs.Path;
 
-import java.util.concurrent.Callable;
-
 import javax.annotation.Nullable;
 
 /**
@@ -61,14 +59,12 @@ public interface ConfigurationEnvironment {
    * An implementation backed by a {@link PackageProvider} instance.
    */
   public static final class TargetProviderEnvironment implements ConfigurationEnvironment {
-    private final PackageProvider packageProvider;
-    private final EventHandler eventHandler;
+    private final LoadedPackageProvider.Bridge packageProvider;
     private final BlazeDirectories blazeDirectories;
 
     public TargetProviderEnvironment(PackageProvider packageProvider,
         EventHandler eventHandler, BlazeDirectories blazeDirectories) {
-      this.packageProvider = packageProvider;
-      this.eventHandler = eventHandler;
+      this.packageProvider = new LoadedPackageProvider.Bridge(packageProvider, eventHandler);
       this.blazeDirectories = blazeDirectories;
     }
 
@@ -80,19 +76,7 @@ public interface ConfigurationEnvironment {
     @Override
     public Target getTarget(final Label label)
         throws NoSuchPackageException, NoSuchTargetException {
-      try {
-        return Uninterruptibles.callUninterruptibly(new Callable<Target>() {
-          @Override
-          public Target call()
-              throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
-            return packageProvider.getTarget(eventHandler, label);
-          }
-        });
-      } catch (NoSuchPackageException | NoSuchTargetException e) {
-        throw e;
-      } catch (Exception e) {
-        throw new IllegalStateException(e);
-      }
+      return packageProvider.getLoadedTarget(label);
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index ebb5efecaa..b7a74c695e 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -197,8 +197,9 @@ public final class BuildTool {
       result.setActualTargets(analysisResult.getTargetsToBuild());
       result.setTestTargets(analysisResult.getTargetsToTest());
 
-      checkTargetEnvironmentRestrictions(analysisResult.getTargetsToBuild(),
-          runtime.getPackageManager());
+      LoadedPackageProvider.Bridge bridge =
+          new LoadedPackageProvider.Bridge(env.getPackageManager(), env.getReporter());
+      checkTargetEnvironmentRestrictions(analysisResult.getTargetsToBuild(), bridge);
       reportTargets(analysisResult);
 
       // Execution phase.
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadedPackageProvider.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadedPackageProvider.java
index 552a3cc886..9194466efa 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadedPackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadedPackageProvider.java
@@ -14,10 +14,14 @@
 package com.google.devtools.build.lib.pkgcache;
 
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.concurrent.Uninterruptibles;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Target;
 
+import java.util.concurrent.Callable;
+
 /**
  * Read-only API for retrieving packages, i.e., calling this API should not result in packages being
  * loaded.
@@ -34,4 +38,46 @@ public interface LoadedPackageProvider {
    * target.
    */
   Target getLoadedTarget(Label label) throws NoSuchPackageException, NoSuchTargetException;
+
+  /**
+   * A bridge class that implements the legacy semantics of {@link #getLoadedTarget} using a
+   * normal {@link PackageProvider} instance.
+   */
+  public static final class Bridge implements LoadedPackageProvider {
+    private final PackageProvider packageProvider;
+    private final EventHandler eventHandler;
+
+    public Bridge(PackageProvider packageProvider, EventHandler eventHandler) {
+      this.packageProvider = packageProvider;
+      this.eventHandler = eventHandler;
+    }
+
+    @Override
+    public Target getLoadedTarget(final Label label)
+        throws NoSuchPackageException, NoSuchTargetException {
+      return getLoadedTarget(packageProvider, eventHandler, label);
+    }
+
+    /**
+     * Uninterruptible method to convert a label into a target using a given package provider and
+     * event handler.
+     */
+    public static Target getLoadedTarget(
+        final PackageProvider packageProvider, final EventHandler eventHandler, final Label label)
+            throws NoSuchPackageException, NoSuchTargetException {
+      try {
+        return Uninterruptibles.callUninterruptibly(new Callable<Target>() {
+          @Override
+          public Target call()
+              throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
+            return packageProvider.getTarget(eventHandler, label);
+          }
+        });
+      } catch (NoSuchPackageException | NoSuchTargetException e) {
+        throw e;
+      } catch (Exception e) {
+        throw new IllegalStateException(e);
+      }
+    }
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java
index ac6d86b200..b05a52336c 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunner.java
@@ -485,7 +485,7 @@ public class LoadingPhaseRunner {
       targetsToAnalyze = targetsToLoad;
     } else if (keepGoing) {
       // Keep going: filter out the error-free targets and only continue with those.
-      targetsToAnalyze = filterErrorFreeTargets(eventBus, targetsToLoad,
+      targetsToAnalyze = filterErrorFreeTargets(eventHandler, eventBus, targetsToLoad,
           labelsToLoadUnconditionally);
       reportAboutPartiallySuccesfulLoading(targetsToLoad, targetsToAnalyze, eventHandler);
     } else {
@@ -537,11 +537,12 @@ public class LoadingPhaseRunner {
     }
   }
 
-  private Set<Target> getTargetsForLabels(Collection<Label> labels) {
+  private Set<Target> getTargetsForLabels(
+      LoadedPackageProvider loadedPackageProvider, Collection<Label> labels) {
     Set<Target> result = new HashSet<>();
     for (Label label : labels) {
       try {
-        result.add(packageManager.getLoadedTarget(label));
+        result.add(loadedPackageProvider.getLoadedTarget(label));
       } catch (NoSuchThingException e) {
         throw new IllegalStateException(e);  // The target should have been loaded
       }
@@ -549,7 +550,7 @@ public class LoadingPhaseRunner {
     return result;
   }
 
-  private ImmutableSet<Target> filterErrorFreeTargets(
+  private ImmutableSet<Target> filterErrorFreeTargets(EventHandler eventHandler,
       EventBus eventBus, Collection<Target> targetsToLoad,
       ListMultimap<String, Label> labelsToLoadUnconditionally) throws LoadingFailedException {
     // Error out if any of the labels needed for the configuration could not be loaded.
@@ -570,8 +571,10 @@ public class LoadingPhaseRunner {
       eventBus.post(new LoadingFailureEvent(entry.getKey(), entry.getValue()));
     }
 
+    LoadedPackageProvider.Bridge bridge =
+        new LoadedPackageProvider.Bridge(packageManager, eventHandler);
     return ImmutableSet.copyOf(Sets.difference(ImmutableSet.copyOf(targetsToLoad),
-        getTargetsForLabels(rootCauses.keySet())));
+        getTargetsForLabels(bridge, rootCauses.keySet())));
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageManager.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageManager.java
index 55bfe39397..bf45f8a40d 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageManager.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageManager.java
@@ -22,8 +22,7 @@ import java.io.PrintStream;
  * A PackageManager keeps state about loaded packages around for quick lookup, and provides
  * related functionality: Recursive package finding, loaded package checking, etc.
  */
-public interface PackageManager extends PackageProvider, CachingPackageLocator,
-    LoadedPackageProvider {
+public interface PackageManager extends PackageProvider, CachingPackageLocator {
 
   /**
    * Returns the package cache statistics.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AbstractLabelCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/AbstractLabelCycleReporter.java
index 67aad20b62..d87e787dd4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AbstractLabelCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AbstractLabelCycleReporter.java
@@ -22,6 +22,7 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
 import com.google.devtools.build.skyframe.CycleInfo;
 import com.google.devtools.build.skyframe.CyclesReporter;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -29,9 +30,9 @@ import com.google.devtools.build.skyframe.SkyKey;
 /** Reports cycles between skyframe values whose keys contains {@link Label}s. */
 abstract class AbstractLabelCycleReporter implements CyclesReporter.SingleCycleReporter {
 
-  private final SkyframePackageManager loadedPackageProvider;
+  private final LoadedPackageProvider loadedPackageProvider;
 
-  AbstractLabelCycleReporter(SkyframePackageManager loadedPackageProvider) {
+  AbstractLabelCycleReporter(LoadedPackageProvider loadedPackageProvider) {
     this.loadedPackageProvider = loadedPackageProvider;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
index b341a7b85e..07d776c3bc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
@@ -20,6 +20,7 @@ import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.analysis.LabelAndConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
 import com.google.devtools.build.lib.skyframe.ArtifactValue.OwnedArtifact;
 import com.google.devtools.build.skyframe.CycleInfo;
 import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -35,7 +36,7 @@ public class ActionArtifactCycleReporter extends AbstractLabelCycleReporter {
       SkyFunctions.isSkyFunction(SkyFunctions.ACTION_EXECUTION),
       SkyFunctions.isSkyFunction(SkyFunctions.TARGET_COMPLETION));
 
-  ActionArtifactCycleReporter(SkyframePackageManager loadedPackageProvider) {
+  ActionArtifactCycleReporter(LoadedPackageProvider loadedPackageProvider) {
     super(loadedPackageProvider);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
index 980ef2f111..9a77801348 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
@@ -17,6 +17,7 @@ import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
 import com.google.devtools.build.skyframe.CycleInfo;
 import com.google.devtools.build.skyframe.SkyKey;
 
@@ -33,7 +34,7 @@ class ConfiguredTargetCycleReporter extends AbstractLabelCycleReporter {
   private static final Predicate<SkyKey> IS_CONFIGURED_TARGET_SKY_KEY =
       SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET);
 
-  ConfiguredTargetCycleReporter(SkyframePackageManager loadedPackageProvider) {
+  ConfiguredTargetCycleReporter(LoadedPackageProvider loadedPackageProvider) {
     super(loadedPackageProvider);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 8addeecaca..0ff269d2bd 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -86,6 +86,7 @@ import com.google.devtools.build.lib.packages.Preprocessor.Result;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.RuleVisibility;
 import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
 import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
 import com.google.devtools.build.lib.pkgcache.PackageManager;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
@@ -1366,7 +1367,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     /**
      * Loads the specified {@link TransitiveTargetValue}s.
      */
-    EvaluationResult<TransitiveTargetValue> loadTransitiveTargets(
+    EvaluationResult<TransitiveTargetValue> loadTransitiveTargets(EventHandler eventHandler,
         Iterable<Target> targetsToVisit, Iterable<Label> labelsToVisit, boolean keepGoing)
         throws InterruptedException {
       List<SkyKey> valueNames = new ArrayList<>();
@@ -1378,7 +1379,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       }
 
       return buildDriver.evaluate(valueNames, keepGoing, DEFAULT_THREAD_COUNT,
-          errorEventListener);
+          eventHandler);
     }
 
     public Set<Package> retrievePackages(Set<PackageIdentifier> packageIds) {
@@ -1494,6 +1495,10 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     return packageManager;
   }
 
+  public LoadedPackageProvider getLoadedPackageProvider() {
+    return new LoadedPackageProvider.Bridge(packageManager, errorEventListener);
+  }
+
   class SkyframePackageLoader {
     /**
      * Looks up a particular package (mostly used after the loading phase, so packages should
@@ -1535,25 +1540,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       }
     }
 
-    Package getLoadedPackage(final PackageIdentifier pkgName) throws NoSuchPackageException {
-      // Note that in Skyframe there is no way to tell if the package has been loaded before or not,
-      // so this will never throw for packages that are not loaded. However, no code currently
-      // relies on having the exception thrown.
-      try {
-        return callUninterruptibly(
-            new Callable<Package>() {
-              @Override
-              public Package call() throws InterruptedException, NoSuchPackageException {
-                return getPackage(errorEventListener, pkgName);
-              }
-            });
-      } catch (NoSuchPackageException e) {
-        throw e;
-      } catch (Exception e) {
-        throw new IllegalStateException(e);  // Should never happen.
-      }
-    }
-
     /**
      * Returns whether the given package should be consider deleted and thus should be ignored.
      */
@@ -1606,11 +1592,12 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   }
 
   private CyclesReporter createCyclesReporter() {
+    LoadedPackageProvider loadedPackageProvider = getLoadedPackageProvider();
     return new CyclesReporter(
-        new TransitiveTargetCycleReporter(packageManager),
-        new ActionArtifactCycleReporter(packageManager),
+        new TransitiveTargetCycleReporter(loadedPackageProvider),
+        new ActionArtifactCycleReporter(loadedPackageProvider),
         new SkylarkModuleCycleReporter(),
-        new ConfiguredTargetCycleReporter(packageManager));
+        new ConfiguredTargetCycleReporter(loadedPackageProvider));
   }
 
   CyclesReporter getCyclesReporter() {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java
index 321c6f043b..f325dc3ba3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeLabelVisitor.java
@@ -71,8 +71,8 @@ final class SkyframeLabelVisitor implements TransitivePackageLoader {
       throws InterruptedException {
     rootCauses.clear();
     lastBuildKeepGoing = false;
-    EvaluationResult<TransitiveTargetValue> result =
-        transitivePackageLoader.loadTransitiveTargets(targetsToVisit, labelsToVisit, keepGoing);
+    EvaluationResult<TransitiveTargetValue> result = transitivePackageLoader.loadTransitiveTargets(
+        eventHandler, targetsToVisit, labelsToVisit, keepGoing);
     updateVisitedValues(result.values());
     lastBuildKeepGoing = keepGoing;
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
index 43f24393d6..5a389e481a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
@@ -69,10 +69,6 @@ class SkyframePackageManager implements PackageManager {
     this.skyframeExecutor = skyframeExecutor;
   }
 
-  private Package getLoadedPackage(PackageIdentifier pkgIdentifier) throws NoSuchPackageException {
-    return packageLoader.getLoadedPackage(pkgIdentifier);
-  }
-
   @ThreadSafe
   @Override
   public Package getPackage(EventHandler eventHandler, PackageIdentifier packageIdentifier)
@@ -81,11 +77,6 @@ class SkyframePackageManager implements PackageManager {
   }
 
   @Override
-  public Target getLoadedTarget(Label label) throws NoSuchPackageException, NoSuchTargetException {
-    return getLoadedPackage(label.getPackageIdentifier()).getTarget(label.getName());
-  }
-
-  @Override
   public Target getTarget(EventHandler eventHandler, Label label)
       throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
     return getPackage(eventHandler, label.getPackageIdentifier()).getTarget(label.getName());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetCycleReporter.java
index dc7312ade1..e3aba5768c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetCycleReporter.java
@@ -20,6 +20,7 @@ import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.PackageGroup;
 import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
 import com.google.devtools.build.skyframe.CycleInfo;
 import com.google.devtools.build.skyframe.SkyKey;
 
@@ -34,7 +35,7 @@ class TransitiveTargetCycleReporter extends AbstractLabelCycleReporter {
   private static final Predicate<SkyKey> IS_TRANSITIVE_TARGET_SKY_KEY =
       SkyFunctions.isSkyFunction(SkyFunctions.TRANSITIVE_TARGET);
 
-  TransitiveTargetCycleReporter(SkyframePackageManager loadedPackageProvider) {
+  TransitiveTargetCycleReporter(LoadedPackageProvider loadedPackageProvider) {
     super(loadedPackageProvider);
   }