Split TargetPatternEvaluator into two interfaces

- the TargetPatternPreloader is still used for query in all its forms
- the remaining TargetPatternEvaluator part is no longer used except in tests
- also make both implementations stateless and pass the offset to the methods
  instead; note that they both modify the underlying skyframe graph, so there
  are side effects to the calls even if there's no direct state anymore

The intent is to migrate the relevant tests to LoadingPhaseRunnerTest (which
could also now be renamed since it's not doing a loading phase), and then
delete the TargetPatternEvaluator interface.

This depends on the previous commit that removed the last direct use of TPE
from an internal command.

PiperOrigin-RevId: 200198067

15 files changed, 191 insertions, 181 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/CqueryBuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/CqueryBuildTool.java
index 2bdd2b7750..1b9628e952 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/CqueryBuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/CqueryBuildTool.java
@@ -47,7 +47,7 @@ public class CqueryBuildTool extends PostAnalysisQueryBuildTool {
         extraFunctions,
         targetConfig,
         hostConfiguration,
-        env.newTargetPatternEvaluator().getOffset(),
+        env.getRelativeWorkingDirectory().getPathString(),
         env.getPackageManager().getPackagePath(),
         () -> walkableGraph,
         cqueryOptions);
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 0bbfae7ced..13f38cd1e5 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
@@ -59,7 +59,7 @@ public interface PackageManager extends PackageProvider, CachingPackageLocator {
   /**
    * Retrieve a target pattern parser that works with this package manager.
    */
-  TargetPatternEvaluator newTargetPatternEvaluator();
+  TargetPatternPreloader newTargetPatternPreloader();
 
   /**
    * Construct a new {@link TransitivePackageLoader}.
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluator.java
index a9e69bd5b4..ea2651d6ab 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluator.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluator.java
@@ -14,17 +14,13 @@
 
 package com.google.devtools.build.lib.pkgcache;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadHostile;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.util.Collection;
 import java.util.List;
-import java.util.Map;
 
 /**
  * A parser for target patterns.  Target patterns are a generalisation of
@@ -50,49 +46,10 @@ public interface TargetPatternEvaluator {
    * computed. Implements the specification described in the class-level comment.
    */
   ResolvedTargets<Target> parseTargetPatternList(
+      PathFragment relativeWorkingDirectory,
       ExtendedEventHandler eventHandler,
       List<String> targetPatterns,
       FilteringPolicy policy,
       boolean keepGoing)
       throws TargetParsingException, InterruptedException;
-
-  /**
-   * Attempts to parse a single target pattern while consulting the package cache to check for the
-   * existence of packages and directories and the build targets in them. Implements the
-   * specification described in the class-level comment. Returns a {@link ResolvedTargets} object.
-   *
-   * <p>If an error is encountered, a {@link TargetParsingException} is thrown, unless {@code
-   * keepGoing} is set to true. In that case, the returned object will have its error bit set.
-   */
-  ResolvedTargets<Target> parseTargetPattern(
-      ExtendedEventHandler eventHandler, String pattern, boolean keepGoing)
-      throws TargetParsingException, InterruptedException;
-
-  /**
-   * Attempts to parse and load the given collection of patterns; the returned map contains the
-   * results for each pattern successfully parsed.
-   *
-   * <p>If an error is encountered, a {@link TargetParsingException} is thrown, unless {@code
-   * keepGoing} is set to true. In that case, the patterns that failed to load have the error flag
-   * set.
-   */
-  Map<String, ResolvedTargets<Target>> preloadTargetPatterns(
-      ExtendedEventHandler eventHandler, Collection<String> patterns, boolean keepGoing)
-      throws TargetParsingException, InterruptedException;
-
-  /**
-   * Update the parser's offset, given the workspace and working directory.
-   *
-   * @param relativeWorkingDirectory the working directory relative to the workspace
-   */
-  @ThreadHostile
-  void updateOffset(PathFragment relativeWorkingDirectory);
-
-  /**
-   * @return the offset of this parser from the root of the workspace.
-   *         Non-absolute package-names will be resolved relative
-   *         to this offset.
-   */
-  @VisibleForTesting
-  String getOffset();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternPreloader.java b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternPreloader.java
new file mode 100644
index 0000000000..3b89098be2
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternPreloader.java
@@ -0,0 +1,46 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.pkgcache;
+
+import com.google.devtools.build.lib.cmdline.ResolvedTargets;
+import com.google.devtools.build.lib.cmdline.TargetParsingException;
+import com.google.devtools.build.lib.concurrent.ThreadSafety;
+import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import java.util.Collection;
+import java.util.Map;
+
+/**
+ * A preloader for target patterns. See {@link TargetPatternEvaluator} for more details.
+ */
+@ThreadSafety.ThreadSafe
+public interface TargetPatternPreloader {
+  /**
+   * Attempts to parse and load the given collection of patterns; the returned map contains the
+   * results for each pattern successfully parsed. As a side effect, calling this method populates
+   * the Skyframe graph, so subsequent calls are faster.
+   *
+   * <p>If an error is encountered, a {@link TargetParsingException} is thrown, unless {@code
+   * keepGoing} is set to true. In that case, the patterns that failed to load have the error flag
+   * set.
+   */
+  Map<String, ResolvedTargets<Target>> preloadTargetPatterns(
+      ExtendedEventHandler eventHandler,
+      PathFragment relativeWorkingDirectory,
+      Collection<String> patterns,
+      boolean keepGoing)
+          throws TargetParsingException, InterruptedException;
+}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryEnvironment.java
index 68ab117f37..62cc674d31 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryEnvironment.java
@@ -35,7 +35,7 @@ import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.TargetEdgeObserver;
-import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
+import com.google.devtools.build.lib.pkgcache.TargetPatternPreloader;
 import com.google.devtools.build.lib.pkgcache.TargetProvider;
 import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader;
 import com.google.devtools.build.lib.query2.engine.Callback;
@@ -71,7 +71,8 @@ import java.util.Set;
 public class BlazeQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
   private static final int MAX_DEPTH_FULL_SCAN_LIMIT = 20;
   private final Map<String, Set<Target>> resolvedTargetPatterns = new HashMap<>();
-  private final TargetPatternEvaluator targetPatternEvaluator;
+  private final TargetPatternPreloader targetPatternPreloader;
+  private final PathFragment relativeWorkingDirectory;
   private final TransitivePackageLoader transitivePackageLoader;
   private final TargetProvider targetProvider;
   private final CachingPackageLocator cachingPackageLocator;
@@ -98,7 +99,8 @@ public class BlazeQueryEnvironment extends AbstractBlazeQueryEnvironment<Target>
       TransitivePackageLoader transitivePackageLoader,
       TargetProvider targetProvider,
       CachingPackageLocator cachingPackageLocator,
-      TargetPatternEvaluator targetPatternEvaluator,
+      TargetPatternPreloader targetPatternPreloader,
+      PathFragment relativeWorkingDirectory,
       boolean keepGoing,
       boolean strictScope,
       int loadingPhaseThreads,
@@ -107,7 +109,8 @@ public class BlazeQueryEnvironment extends AbstractBlazeQueryEnvironment<Target>
       Set<Setting> settings,
       Iterable<QueryFunction> extraFunctions) {
     super(keepGoing, strictScope, labelFilter, eventHandler, settings, extraFunctions);
-    this.targetPatternEvaluator = targetPatternEvaluator;
+    this.targetPatternPreloader = targetPatternPreloader;
+    this.relativeWorkingDirectory = relativeWorkingDirectory;
     this.transitivePackageLoader = transitivePackageLoader;
     this.targetProvider = targetProvider;
     this.cachingPackageLocator = cachingPackageLocator;
@@ -441,7 +444,8 @@ public class BlazeQueryEnvironment extends AbstractBlazeQueryEnvironment<Target>
       // being called from within a SkyFunction.
       resolvedTargetPatterns.putAll(
           Maps.transformValues(
-              targetPatternEvaluator.preloadTargetPatterns(eventHandler, patterns, keepGoing),
+              targetPatternPreloader.preloadTargetPatterns(
+                  eventHandler, relativeWorkingDirectory, patterns, keepGoing),
               ResolvedTargets::getTargets));
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/query2/QueryEnvironmentFactory.java b/src/main/java/com/google/devtools/build/lib/query2/QueryEnvironmentFactory.java
index 3c02da6fe8..e7cc8820d4 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/QueryEnvironmentFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/QueryEnvironmentFactory.java
@@ -21,11 +21,12 @@ import com.google.devtools.build.lib.packages.CachingPackageLocator;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
-import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
+import com.google.devtools.build.lib.pkgcache.TargetPatternPreloader;
 import com.google.devtools.build.lib.pkgcache.TargetProvider;
 import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader;
 import com.google.devtools.build.lib.query2.engine.QueryEnvironment.QueryFunction;
 import com.google.devtools.build.lib.query2.engine.QueryEnvironment.Setting;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.WalkableGraph.WalkableGraphFactory;
 import java.util.List;
 import java.util.Set;
@@ -40,7 +41,8 @@ public class QueryEnvironmentFactory {
       WalkableGraphFactory graphFactory,
       TargetProvider targetProvider,
       CachingPackageLocator cachingPackageLocator,
-      TargetPatternEvaluator targetPatternEvaluator,
+      TargetPatternPreloader targetPatternPreloader,
+      PathFragment relativeWorkingDirectory,
       boolean keepGoing,
       boolean strictScope,
       boolean orderedResults,
@@ -60,7 +62,7 @@ public class QueryEnvironmentFactory {
           eventHandler,
           settings,
           functions,
-          targetPatternEvaluator.getOffset(),
+          relativeWorkingDirectory.getPathString(),
           graphFactory,
           universeScope,
           packagePath,
@@ -70,7 +72,8 @@ public class QueryEnvironmentFactory {
           transitivePackageLoader,
           targetProvider,
           cachingPackageLocator,
-          targetPatternEvaluator,
+          targetPatternPreloader,
+          relativeWorkingDirectory,
           keepGoing,
           strictScope,
           loadingPhaseThreads,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
index 00a51524df..4ae0c1a0b8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
@@ -52,9 +52,8 @@ 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.FilteringPolicies;
-import com.google.devtools.build.lib.pkgcache.FilteringPolicy;
 import com.google.devtools.build.lib.pkgcache.PackageProvider;
-import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
+import com.google.devtools.build.lib.pkgcache.TargetPatternPreloader;
 import com.google.devtools.build.lib.query2.BlazeQueryEnvironment;
 import com.google.devtools.build.lib.query2.QueryEnvironmentFactory;
 import com.google.devtools.build.lib.query2.engine.DigraphQueryEvalResult;
@@ -279,10 +278,10 @@ public class GenQuery implements RuleConfiguredTargetFactory {
     ImmutableMap<Label, Target> validTargetsMap = closureInfo.second;
     PreloadedMapPackageProvider packageProvider =
         new PreloadedMapPackageProvider(packageMap, validTargetsMap);
-    TargetPatternEvaluator evaluator = new SkyframeEnvTargetPatternEvaluator(env);
+    TargetPatternPreloader preloader = new SkyframeEnvTargetPatternEvaluator(env);
     Predicate<Label> labelFilter = Predicates.in(validTargetsMap.keySet());
 
-    return doQuery(queryOptions, packageProvider, labelFilter, evaluator, query, ruleContext);
+    return doQuery(queryOptions, packageProvider, labelFilter, preloader, query, ruleContext);
   }
 
   @SuppressWarnings("unchecked")
@@ -291,7 +290,7 @@ public class GenQuery implements RuleConfiguredTargetFactory {
       QueryOptions queryOptions,
       PreloadedMapPackageProvider packageProvider,
       Predicate<Label> labelFilter,
-      TargetPatternEvaluator evaluator,
+      TargetPatternPreloader preloader,
       String query,
       RuleContext ruleContext)
       throws InterruptedException {
@@ -330,7 +329,8 @@ public class GenQuery implements RuleConfiguredTargetFactory {
                   /* graphFactory= */ null,
                   packageProvider,
                   packageProvider,
-                  evaluator,
+                  preloader,
+                  PathFragment.EMPTY_FRAGMENT,
                   /*keepGoing=*/ false,
                   ruleContext.attributes().get("strict", Type.BOOLEAN),
                   /*orderedResults=*/ !QueryOutputUtils.shouldStreamResults(
@@ -396,7 +396,7 @@ public class GenQuery implements RuleConfiguredTargetFactory {
    * Provide target pattern evaluation to the query operations using Skyframe dep lookup. For thread
    * safety, we must synchronize access to the SkyFunction.Environment.
    */
-  private static final class SkyframeEnvTargetPatternEvaluator implements TargetPatternEvaluator {
+  private static final class SkyframeEnvTargetPatternEvaluator implements TargetPatternPreloader {
     private final SkyFunction.Environment env;
 
     public SkyframeEnvTargetPatternEvaluator(SkyFunction.Environment env) {
@@ -415,9 +415,13 @@ public class GenQuery implements RuleConfiguredTargetFactory {
 
     @Override
     public Map<String, ResolvedTargets<Target>> preloadTargetPatterns(
-        ExtendedEventHandler eventHandler, Collection<String> patterns, boolean keepGoing)
-        throws TargetParsingException, InterruptedException {
+        ExtendedEventHandler eventHandler,
+        PathFragment relativeWorkingDirectory,
+        Collection<String> patterns,
+        boolean keepGoing)
+            throws TargetParsingException, InterruptedException {
       Preconditions.checkArgument(!keepGoing);
+      Preconditions.checkArgument(relativeWorkingDirectory.isEmpty());
       boolean ok = true;
       Map<String, ResolvedTargets<Target>> preloadedPatterns =
           Maps.newHashMapWithExpectedSize(patterns.size());
@@ -498,33 +502,6 @@ public class GenQuery implements RuleConfiguredTargetFactory {
             String.format("recursive target patterns are not permitted: '%s''", pattern));
       }
     }
-
-    @Override
-    public ResolvedTargets<Target> parseTargetPatternList(
-        ExtendedEventHandler eventHandler,
-        List<String> targetPatterns,
-        FilteringPolicy policy,
-        boolean keepGoing)
-        throws TargetParsingException {
-      throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public ResolvedTargets<Target> parseTargetPattern(
-        ExtendedEventHandler eventHandler, String pattern, boolean keepGoing)
-        throws TargetParsingException {
-      throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public void updateOffset(PathFragment relativeWorkingDirectory) {
-      throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public String getOffset() {
-      throw new UnsupportedOperationException();
-    }
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
index 2ad145d1a3..32d31a2979 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
@@ -31,7 +31,7 @@ import com.google.devtools.build.lib.packages.SkylarkSemanticsOptions;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
 import com.google.devtools.build.lib.pkgcache.PackageManager;
-import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
+import com.google.devtools.build.lib.pkgcache.TargetPatternPreloader;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.profiler.SilentCloseable;
@@ -316,12 +316,10 @@ public final class CommandEnvironment {
   }
 
   /**
-   * Creates and returns a new target pattern parser.
+   * Creates and returns a new target pattern preloader.
    */
-  public TargetPatternEvaluator newTargetPatternEvaluator() {
-    TargetPatternEvaluator result = getPackageManager().newTargetPatternEvaluator();
-    result.updateOffset(relativeWorkingDirectory);
-    return result;
+  public TargetPatternPreloader newTargetPatternPreloader() {
+    return getPackageManager().newTargetPatternPreloader();
   }
 
   public PackageRootResolver getPackageRootResolver() {
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
index b9e071ce6a..8b06f1cbaf 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
@@ -300,7 +300,8 @@ public final class QueryCommand implements BlazeCommand {
             env.getSkyframeExecutor(),
             targetProviderForQueryEnvironment,
             env.getPackageManager(),
-            env.newTargetPatternEvaluator(),
+            env.newTargetPatternPreloader(),
+            env.getRelativeWorkingDirectory(),
             keepGoing,
             /*strictScope=*/ true,
             orderedResults,
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 633b8b873b..69a9b436ed 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
@@ -117,6 +117,7 @@ import com.google.devtools.build.lib.pkgcache.PackageManager;
 import com.google.devtools.build.lib.pkgcache.PackageManager.PackageManagerStatistics;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
 import com.google.devtools.build.lib.pkgcache.TargetParsingPhaseTimeEvent;
+import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
 import com.google.devtools.build.lib.pkgcache.TestFilter;
 import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader;
 import com.google.devtools.build.lib.profiler.AutoProfiler;
@@ -2026,6 +2027,11 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     return packageManager;
   }
 
+  @VisibleForTesting
+  public TargetPatternEvaluator newTargetPatternEvaluator() {
+    return new SkyframeTargetPatternEvaluator(this);
+  }
+
   public ActionKeyContext getActionKeyContext() {
     return actionKeyContext;
   }
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 b0e36037ba..d89593ca07 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
@@ -24,7 +24,7 @@ import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.PackageManager;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
-import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
+import com.google.devtools.build.lib.pkgcache.TargetPatternPreloader;
 import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor.SkyframePackageLoader;
 import com.google.devtools.build.lib.vfs.Path;
@@ -125,7 +125,7 @@ class SkyframePackageManager implements PackageManager, CachingPackageLocator {
   }
 
   @Override
-  public TargetPatternEvaluator newTargetPatternEvaluator() {
+  public TargetPatternPreloader newTargetPatternPreloader() {
     return new SkyframeTargetPatternEvaluator(skyframeExecutor);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
index 4c8cf80a27..01d68f4cc7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.pkgcache.FilteringPolicies;
 import com.google.devtools.build.lib.pkgcache.FilteringPolicy;
 import com.google.devtools.build.lib.pkgcache.ParsingFailedEvent;
 import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
+import com.google.devtools.build.lib.pkgcache.TargetPatternPreloader;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternSkyKeyOrException;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -40,47 +41,38 @@ import java.util.Map;
 /**
  * Skyframe-based target pattern parsing.
  */
-final class SkyframeTargetPatternEvaluator implements TargetPatternEvaluator {
+final class SkyframeTargetPatternEvaluator
+    implements TargetPatternEvaluator, TargetPatternPreloader {
   private final SkyframeExecutor skyframeExecutor;
-  private String offset = "";
 
   SkyframeTargetPatternEvaluator(SkyframeExecutor skyframeExecutor) {
     this.skyframeExecutor = skyframeExecutor;
   }
 
+  // Only used by AnalyzeCommand at this point. All build commands use SkyframeLoadingPhaseRunner.
   @Override
   public ResolvedTargets<Target> parseTargetPatternList(
+      PathFragment relativeWorkingDirectory,
       ExtendedEventHandler eventHandler,
       List<String> targetPatterns,
       FilteringPolicy policy,
       boolean keepGoing)
       throws TargetParsingException, InterruptedException {
     return parseTargetPatternList(
-        offset, eventHandler, ImmutableList.copyOf(targetPatterns), policy, keepGoing);
-  }
-
-  @Override
-  public ResolvedTargets<Target> parseTargetPattern(
-      ExtendedEventHandler eventHandler, String pattern, boolean keepGoing)
-      throws TargetParsingException, InterruptedException {
-    return parseTargetPatternList(eventHandler, ImmutableList.of(pattern),
-        DEFAULT_FILTERING_POLICY, keepGoing);
-  }
-
-  @Override
-  public void updateOffset(PathFragment relativeWorkingDirectory) {
-    offset = relativeWorkingDirectory.getPathString();
-  }
-
-  @Override
-  public String getOffset() {
-    return offset;
+        relativeWorkingDirectory.getPathString(),
+        eventHandler,
+        ImmutableList.copyOf(targetPatterns),
+        policy,
+        keepGoing);
   }
 
   @Override
   public Map<String, ResolvedTargets<Target>> preloadTargetPatterns(
-      ExtendedEventHandler eventHandler, Collection<String> patterns, boolean keepGoing)
-      throws TargetParsingException, InterruptedException {
+      ExtendedEventHandler eventHandler,
+      PathFragment relativeWorkingDirectory,
+      Collection<String> patterns,
+      boolean keepGoing)
+          throws TargetParsingException, InterruptedException {
     // TODO(bazel-team): This is used only in "blaze query". There are plans to dramatically change
     // how query works on Skyframe, in which case this method is likely to go away.
     ImmutableList.Builder<TargetPatternsAndKeysAndResultBuilder>
@@ -91,7 +83,11 @@ final class SkyframeTargetPatternEvaluator implements TargetPatternEvaluator {
       targetPatternsAndKeysAndResultListBuilder.add(new TargetPatternsAndKeysAndResultBuilder(
           singletonPatternList,
           getTargetPatternKeys(
-              offset, eventHandler, singletonPatternList, policy, keepGoing),
+              relativeWorkingDirectory.getPathString(),
+              eventHandler,
+              singletonPatternList,
+              policy,
+              keepGoing),
           createTargetPatternEvaluatorUtil(policy, eventHandler, keepGoing)));
 
     }
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/AbstractTargetPatternEvaluatorTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/AbstractTargetPatternEvaluatorTest.java
index 10f8779909..85424470f7 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/AbstractTargetPatternEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/AbstractTargetPatternEvaluatorTest.java
@@ -21,10 +21,12 @@ import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.events.DelegatingEventHandler;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.packages.ConstantRuleVisibility;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
 import com.google.devtools.build.lib.util.Pair;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashSet;
@@ -49,17 +51,40 @@ public abstract class AbstractTargetPatternEvaluatorTest extends PackageLoadingT
       boolean keepGoing)
       throws TargetParsingException, InterruptedException {
     return parseTargetPatternList(
-        parser, eventHandler, targetPatterns, FilteringPolicies.NO_FILTER, keepGoing);
+        PathFragment.EMPTY_FRAGMENT,
+        parser,
+        eventHandler,
+        targetPatterns,
+        FilteringPolicies.NO_FILTER,
+        keepGoing);
   }
 
   protected static ResolvedTargets<Target> parseTargetPatternList(
+      PathFragment relativeWorkingDirectory,
+      TargetPatternEvaluator parser,
+      ExtendedEventHandler eventHandler,
+      List<String> targetPatterns,
+      boolean keepGoing)
+      throws TargetParsingException, InterruptedException {
+    return parseTargetPatternList(
+        relativeWorkingDirectory,
+        parser,
+        eventHandler,
+        targetPatterns,
+        FilteringPolicies.NO_FILTER,
+        keepGoing);
+  }
+
+  protected static ResolvedTargets<Target> parseTargetPatternList(
+      PathFragment relativeWorkingDirectory,
       TargetPatternEvaluator parser,
       ExtendedEventHandler eventHandler,
       List<String> targetPatterns,
       FilteringPolicy policy,
       boolean keepGoing)
       throws TargetParsingException, InterruptedException {
-    return parser.parseTargetPatternList(eventHandler, targetPatterns, policy, keepGoing);
+    return parser.parseTargetPatternList(
+        relativeWorkingDirectory, eventHandler, targetPatterns, policy, keepGoing);
   }
 
   /**
@@ -77,7 +102,7 @@ public abstract class AbstractTargetPatternEvaluatorTest extends PackageLoadingT
   @Before
   public final void initializeParser() throws Exception {
     setUpSkyframe(ConstantRuleVisibility.PRIVATE, loadingMock.getDefaultsPackageContent());
-    parser = skyframeExecutor.getPackageManager().newTargetPatternEvaluator();
+    parser = skyframeExecutor.newTargetPatternEvaluator();
     parsingListener = new RecordingParsingListener(reporter);
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java
index d421e9229c..3c3a7739a3 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformerTest.java
@@ -24,6 +24,7 @@ import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.HashSet;
@@ -49,7 +50,7 @@ public class CompileOneDependencyTransformerTest extends PackageLoadingTestCase
 
   @Before
   public final void createTransformer() throws Exception {
-    parser = getPackageManager().newTargetPatternEvaluator();
+    parser = skyframeExecutor.newTargetPatternEvaluator();
     transformer = new CompileOneDependencyTransformer(getPackageManager());
   }
 
@@ -75,7 +76,8 @@ public class CompileOneDependencyTransformerTest extends PackageLoadingTestCase
       TargetPatternEvaluator parser, Reporter reporter,
       List<String> targetPatterns, FilteringPolicy policy,
       boolean keepGoing) throws Exception {
-    return parser.parseTargetPatternList(reporter, targetPatterns, policy, keepGoing);
+    return parser.parseTargetPatternList(
+        PathFragment.EMPTY_FRAGMENT, reporter, targetPatterns, policy, keepGoing);
   }
 
   private ResolvedTargets<Target> parseCompileOneDep(String... patterns) throws Exception {
@@ -91,12 +93,14 @@ public class CompileOneDependencyTransformerTest extends PackageLoadingTestCase
   private Set<Label> parseListCompileOneDepRelative(String... patterns)
       throws TargetParsingException, IOException, InterruptedException {
     Path foo = scratch.dir("foo");
-    TargetPatternEvaluator fooOffsetParser = getPackageManager().newTargetPatternEvaluator();
-    fooOffsetParser.updateOffset(foo.relativeTo(rootDirectory));
+    TargetPatternEvaluator fooOffsetParser = skyframeExecutor.newTargetPatternEvaluator();
     ResolvedTargets<Target> result;
     try {
       result = fooOffsetParser.parseTargetPatternList(
-          reporter, Arrays.asList(patterns), FilteringPolicies.NO_FILTER, false);
+          foo.relativeTo(rootDirectory),
+          reporter,
+          Arrays.asList(patterns),
+          FilteringPolicies.NO_FILTER, false);
     } catch (InterruptedException e) {
       throw new RuntimeException(e);
     }
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
index a7f81a23e0..94029ac5c7 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
@@ -152,11 +152,6 @@ public class TargetPatternEvaluatorTest extends AbstractTargetPatternEvaluatorTe
     skyframeExecutor.setDeletedPackages(deletedPackages);
   }
 
-  private TargetPatternEvaluator shiftOffset() {
-    parser.updateOffset(fooOffset);
-    return parser;
-  }
-
   private Set<Label> parseList(String... patterns)
       throws TargetParsingException, InterruptedException {
     return targetsToLabels(
@@ -176,13 +171,25 @@ public class TargetPatternEvaluatorTest extends AbstractTargetPatternEvaluatorTe
       FilteringPolicy policy, String... patterns)
       throws TargetParsingException, InterruptedException {
     return targetsToLabels(getFailFast(
-        parseTargetPatternList(parser, parsingListener, Arrays.asList(patterns), policy, false)));
+        parseTargetPatternList(
+            PathFragment.EMPTY_FRAGMENT,
+            parser,
+            parsingListener,
+            Arrays.asList(patterns),
+            policy,
+            false)));
+  }
+
+  private Set<Label> parseListRelative(PathFragment offset, String... patterns)
+      throws TargetParsingException, InterruptedException {
+    return targetsToLabels(getFailFast(parseTargetPatternList(
+        offset, parser, parsingListener, Arrays.asList(patterns), false)));
   }
 
   private Set<Label> parseListRelative(String... patterns)
       throws TargetParsingException, InterruptedException {
     return targetsToLabels(getFailFast(parseTargetPatternList(
-        shiftOffset(), parsingListener, Arrays.asList(patterns), false)));
+        fooOffset, parser, parsingListener, Arrays.asList(patterns), false)));
   }
 
   private static Set<Target> getFailFast(ResolvedTargets<Target> result) {
@@ -190,10 +197,14 @@ public class TargetPatternEvaluatorTest extends AbstractTargetPatternEvaluatorTe
     return result.getTargets();
   }
 
-  private void expectError(TargetPatternEvaluator parser, String expectedError,
-      String target) throws InterruptedException {
+  private void expectError(
+      PathFragment offset,
+      TargetPatternEvaluator parser,
+      String expectedError,
+      String target)
+          throws InterruptedException {
     try {
-      parser.parseTargetPattern(parsingListener, target, false);
+      parseTargetPatternList(offset, parser, parsingListener, ImmutableList.of(target), false);
       fail("target='" + target + "', expected error: " + expectedError);
     } catch (TargetParsingException e) {
       assertThat(e).hasMessageThat().contains(expectedError);
@@ -201,23 +212,27 @@ public class TargetPatternEvaluatorTest extends AbstractTargetPatternEvaluatorTe
   }
 
   private void expectError(String expectedError, String target) throws InterruptedException {
-    expectError(parser, expectedError, target);
+    expectError(PathFragment.EMPTY_FRAGMENT, parser, expectedError, target);
   }
 
   private void expectErrorRelative(String expectedError, String target)
       throws InterruptedException {
-    expectError(shiftOffset(), expectedError, target);
+    expectError(fooOffset, parser, expectedError, target);
   }
 
   private Label parseIndividualTarget(String targetLabel) throws Exception {
     return Iterables.getOnlyElement(
-        getFailFast(parser.parseTargetPattern(parsingListener, targetLabel, false))).getLabel();
+        getFailFast(
+            parseTargetPatternList(parser, parsingListener, ImmutableList.of(targetLabel), false)))
+        .getLabel();
   }
 
   private Label parseIndividualTargetRelative(String targetLabel) throws Exception {
     return Iterables.getOnlyElement(
         getFailFast(
-            shiftOffset().parseTargetPattern(parsingListener, targetLabel, false))).getLabel();
+            parseTargetPatternList(
+                fooOffset, parser, parsingListener, ImmutableList.of(targetLabel), false)))
+        .getLabel();
   }
 
   @Test
@@ -336,14 +351,10 @@ public class TargetPatternEvaluatorTest extends AbstractTargetPatternEvaluatorTe
     scratch.file("nest/nest/BUILD",
         "cc_library(name = 'nested2', srcs = [ ])");
 
-    updateOffset(PathFragment.create("nest"));
-    assertThat(parseList(":all")).containsExactlyElementsIn(labels("//nest:nested1"));
-    updateOffset(PathFragment.create("nest/nest"));
-    assertThat(parseList(":all")).containsExactlyElementsIn(labels("//nest/nest:nested2"));
-  }
-
-  protected void updateOffset(PathFragment rel) {
-    parser.updateOffset(rel);
+    assertThat(parseListRelative(PathFragment.create("nest"), ":all"))
+        .containsExactlyElementsIn(labels("//nest:nested1"));
+    assertThat(parseListRelative(PathFragment.create("nest/nest"), ":all"))
+        .containsExactlyElementsIn(labels("//nest/nest:nested2"));
   }
 
   private void runFindTargetsInPackage(String suffix) throws Exception {
@@ -517,9 +528,14 @@ public class TargetPatternEvaluatorTest extends AbstractTargetPatternEvaluatorTe
     scratch.file("x/z/BUILD", "cc_library(name='z')");
     setDeletedPackages(Sets.newHashSet(PackageIdentifier.createInMainRepo("x/y")));
 
-    parser.updateOffset(PathFragment.create("x"));
     assertThat(
-            targetsToLabels(getFailFast(parser.parseTargetPattern(parsingListener, "...", false))))
+            targetsToLabels(getFailFast(
+                parseTargetPatternList(
+                    PathFragment.create("x"),
+                    parser,
+                    parsingListener,
+                    ImmutableList.of("..."),
+                    false))))
         .isEqualTo(Sets.newHashSet(Label.parseAbsolute("//x/z")));
   }
 
@@ -599,27 +615,6 @@ public class TargetPatternEvaluatorTest extends AbstractTargetPatternEvaluatorTe
     assertThat(parseListRelative("//foo/...:*")).containsExactlyElementsIn(targetsBeneathFoo);
   }
 
-  @Test
-  public void testFactoryMethod() throws Exception {
-    Path workspace = scratch.dir("/client/workspace");
-    Path underWorkspace = scratch.dir("/client/workspace/foo");
-    Path notUnderWorkspace = scratch.dir("/client/otherclient");
-
-    updateOffset(workspace, underWorkspace);
-    updateOffset(workspace, workspace);
-
-    // The client must be equal to or underneath the workspace.
-    try {
-      updateOffset(workspace, notUnderWorkspace);
-      fail("Should have failed because client was not underneath the workspace");
-    } catch (IllegalArgumentException expected) {
-    }
-  }
-
-  private void updateOffset(Path workspace, Path workingDir) {
-    parser.updateOffset(workingDir.relativeTo(workspace));
-  }
-
   private void setupSubDirectoryCircularSymlink() throws Exception {
     Path parent = scratch.file("parent/BUILD", "sh_library(name = 'parent')").getParentDirectory();
     Path child = parent.getRelative("child");
@@ -725,7 +720,7 @@ public class TargetPatternEvaluatorTest extends AbstractTargetPatternEvaluatorTe
     // Ensure that validateTargetPattern's checking is strictly weaker than
     // that of parseTargetPattern.
     try {
-      parser.parseTargetPattern(parsingListener, target, false);
+      parseTargetPatternList(parser, parsingListener, ImmutableList.of(target), false);
       fail("parseTargetPattern(" + target + ") inconsistent with parseTargetPattern!");
     } catch (TargetParsingException expected) {
       /* ignore */
@@ -745,9 +740,7 @@ public class TargetPatternEvaluatorTest extends AbstractTargetPatternEvaluatorTe
   @Test
   public void testSetOffset() throws Exception {
     assertThat(parseIndividualTarget("foo:foo1").toString()).isEqualTo("//foo:foo1");
-
-    parser.updateOffset(PathFragment.create("foo"));
-    assertThat(parseIndividualTarget(":foo1").toString()).isEqualTo("//foo:foo1");
+    assertThat(parseIndividualTargetRelative(":foo1").toString()).isEqualTo("//foo:foo1");
   }
 
   @Test