From a46125ca0e4569a41d2d078c3168dbfd8950534a Mon Sep 17 00:00:00 2001
From: Janak Ramakrishnan <janakr@google.com>
Date: Wed, 11 Feb 2015 16:51:37 +0000
Subject: Extract an EvaluableBlazeQueryEnvironment abstract class from
 BlazeQueryEnvironment that exposes an evaluateQuery method, and also
 implements the non-LabelVisitor-specific parts of BlazeQueryEnvironment, for
 other implementations' uses.

Most of the code is just a straight refactoring of BlazeQueryEnvironment into EvaluableBlazeQueryEnvironment (and BlazeTargetAccessor). Ignoring whitespace changes in [] may be your friend for seeing that it's a straight move.

This also allows us to change tests to use QueryCommand.newQueryEnvironment, in preparation for newQueryEnvironment potentially returning a different EvaluableBlazeQueryEnvironment subclass depending on the circumstances.

--
MOS_MIGRATED_REVID=86088953
---
 .../lib/query2/AbstractBlazeQueryEnvironment.java  | 217 +++++++++++++++
 .../build/lib/query2/BlazeQueryEnvironment.java    | 292 +--------------------
 .../build/lib/query2/BlazeTargetAccessor.java      | 154 +++++++++++
 .../build/lib/runtime/commands/QueryCommand.java   |  44 +++-
 4 files changed, 414 insertions(+), 293 deletions(-)
 create mode 100644 src/main/java/com/google/devtools/build/lib/query2/AbstractBlazeQueryEnvironment.java
 create mode 100644 src/main/java/com/google/devtools/build/lib/query2/BlazeTargetAccessor.java

(limited to 'src/main/java/com/google')

diff --git a/src/main/java/com/google/devtools/build/lib/query2/AbstractBlazeQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/AbstractBlazeQueryEnvironment.java
new file mode 100644
index 0000000000..48840eac7c
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/query2/AbstractBlazeQueryEnvironment.java
@@ -0,0 +1,217 @@
+// Copyright 2015 Google Inc. 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.query2;
+
+import com.google.common.base.Preconditions;
+import com.google.common.base.Predicate;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Sets;
+import com.google.devtools.build.lib.cmdline.ResolvedTargets;
+import com.google.devtools.build.lib.cmdline.TargetParsingException;
+import com.google.devtools.build.lib.events.ErrorSensingEventHandler;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
+import com.google.devtools.build.lib.query2.engine.QueryEnvironment;
+import com.google.devtools.build.lib.query2.engine.QueryEvalResult;
+import com.google.devtools.build.lib.query2.engine.QueryException;
+import com.google.devtools.build.lib.query2.engine.QueryExpression;
+import com.google.devtools.build.lib.syntax.Label;
+import com.google.devtools.build.lib.util.BinaryPredicate;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * {@link QueryEnvironment} that can evaluate queries to produce a result, and implements as much
+ * of QueryEnvironment as possible while remaining mostly agnostic as to the objects being stored.
+ */
+public abstract class AbstractBlazeQueryEnvironment<T> implements QueryEnvironment<T> {
+  protected final ErrorSensingEventHandler eventHandler;
+  private final TargetPatternEvaluator targetPatternEvaluator;
+  private final Map<String, Set<T>> letBindings = new HashMap<>();
+  protected final Map<String, ResolvedTargets<Target>> resolvedTargetPatterns = new HashMap<>();
+  protected final boolean keepGoing;
+  protected final boolean strictScope;
+
+  protected final BinaryPredicate<Rule, Attribute> dependencyFilter;
+  private final Predicate<Label> labelFilter;
+
+  private final Set<Setting> settings;
+  private final List<QueryFunction> extraFunctions;
+
+  protected AbstractBlazeQueryEnvironment(TargetPatternEvaluator targetPatternEvaluator,
+      boolean keepGoing,
+      boolean strictScope,
+      Predicate<Label> labelFilter,
+      EventHandler eventHandler,
+      Set<Setting> settings,
+      Iterable<QueryFunction> extraFunctions) {
+    this.eventHandler = new ErrorSensingEventHandler(eventHandler);
+    this.targetPatternEvaluator = targetPatternEvaluator;
+    this.keepGoing = keepGoing;
+    this.strictScope = strictScope;
+    this.dependencyFilter = constructDependencyFilter(settings);
+    this.labelFilter = labelFilter;
+    this.settings = Sets.immutableEnumSet(settings);
+    this.extraFunctions = ImmutableList.copyOf(extraFunctions);
+  }
+
+  private static BinaryPredicate<Rule, Attribute> constructDependencyFilter(Set<Setting> settings) {
+    BinaryPredicate<Rule, Attribute> specifiedFilter =
+        settings.contains(Setting.NO_HOST_DEPS) ? Rule.NO_HOST_DEPS : Rule.ALL_DEPS;
+    if (settings.contains(Setting.NO_IMPLICIT_DEPS)) {
+      specifiedFilter = Rule.and(specifiedFilter, Rule.NO_IMPLICIT_DEPS);
+    }
+    if (settings.contains(Setting.NO_NODEP_DEPS)) {
+      specifiedFilter = Rule.and(specifiedFilter, Rule.NO_NODEP_ATTRIBUTES);
+    }
+    return specifiedFilter;
+  }
+
+  /**
+   * Evaluate the specified query expression in this environment.
+   *
+   * @return a {@link QueryEvalResult} object that contains the resulting set of targets and a bit
+   *   to indicate whether errors occured during evaluation; note that the
+   *   success status can only be false if {@code --keep_going} was in effect
+   * @throws QueryException if the evaluation failed and {@code --nokeep_going} was in
+   *   effect
+   */
+  public QueryEvalResult<T> evaluateQuery(QueryExpression expr) throws QueryException {
+    // Some errors are reported as QueryExceptions and others as ERROR events
+    // (if --keep_going).
+    eventHandler.resetErrors();
+    resolvedTargetPatterns.clear();
+
+    // In the --nokeep_going case, errors are reported in the order in which the patterns are
+    // specified; using a linked hash set here makes sure that the left-most error is reported.
+    Set<String> targetPatternSet = new LinkedHashSet<>();
+    expr.collectTargetPatterns(targetPatternSet);
+    try {
+      resolvedTargetPatterns.putAll(preloadOrThrow(targetPatternSet));
+    } catch (TargetParsingException e) {
+      // Unfortunately, by evaluating the patterns in parallel, we lose some location information.
+      throw new QueryException(expr, e.getMessage());
+    }
+
+    Set<T> resultNodes;
+    try {
+      resultNodes = expr.eval(this);
+    } catch (QueryException e) {
+      throw new QueryException(e, expr);
+    }
+
+    if (eventHandler.hasErrors()) {
+      if (!keepGoing) {
+        // This case represents loading-phase errors reported during evaluation
+        // of target patterns that don't cause evaluation to fail per se.
+        throw new QueryException("Evaluation of query \"" + expr
+            + "\" failed due to BUILD file errors");
+      } else {
+        eventHandler.handle(Event.warn("--keep_going specified, ignoring errors.  "
+            + "Results may be inaccurate"));
+      }
+    }
+
+    return new QueryEvalResult<>(!eventHandler.hasErrors(), resultNodes);
+  }
+
+  public QueryEvalResult<T> evaluateQuery(String query) throws QueryException {
+    return evaluateQuery(QueryExpression.parse(query, this));
+  }
+
+  @Override
+  public void reportBuildFileError(QueryExpression caller, String message) throws QueryException {
+    if (!keepGoing) {
+      throw new QueryException(caller, message);
+    } else {
+      // Keep consistent with evaluateQuery() above.
+      eventHandler.handle(Event.error("Evaluation of query \"" + caller + "\" failed: " + message));
+    }
+  }
+
+  public abstract Target getTarget(Label label) throws TargetNotFoundException, QueryException;
+
+  @Override
+  public Set<T> getVariable(String name) {
+    return letBindings.get(name);
+  }
+
+  @Override
+  public Set<T> setVariable(String name, Set<T> value) {
+    return letBindings.put(name, value);
+  }
+
+  protected boolean validateScope(Label label, boolean strict) throws QueryException {
+    if (!labelFilter.apply(label)) {
+      String error = String.format("target '%s' is not within the scope of the query", label);
+      if (strict) {
+        throw new QueryException(error);
+      } else {
+        eventHandler.handle(Event.warn(error + ". Skipping"));
+        return false;
+      }
+    }
+    return true;
+  }
+
+  public Set<T> evalTargetPattern(QueryExpression caller, String pattern)
+      throws QueryException {
+    if (!resolvedTargetPatterns.containsKey(pattern)) {
+      try {
+        resolvedTargetPatterns.putAll(preloadOrThrow(ImmutableList.of(pattern)));
+      } catch (TargetParsingException e) {
+        // Will skip the target and keep going if -k is specified.
+        resolvedTargetPatterns.put(pattern, ResolvedTargets.<Target>empty());
+        reportBuildFileError(caller, e.getMessage());
+      }
+    }
+    return getTargetsMatchingPattern(caller, pattern);
+  }
+
+  private Map<String, ResolvedTargets<Target>> preloadOrThrow(Collection<String> patterns)
+      throws TargetParsingException {
+    try {
+      // Note that this may throw a RuntimeException if deps are missing in Skyframe and this is
+      // being called from within a SkyFunction.
+      return targetPatternEvaluator.preloadTargetPatterns(
+          eventHandler, patterns, keepGoing);
+    } catch (InterruptedException e) {
+      // TODO(bazel-team): Propagate the InterruptedException from here [skyframe-loading].
+      throw new TargetParsingException("interrupted");
+    }
+  }
+
+  @Override
+  public boolean isSettingEnabled(Setting setting) {
+    return settings.contains(Preconditions.checkNotNull(setting));
+  }
+
+  @Override
+  public Iterable<QueryFunction> getFunctions() {
+    ImmutableList.Builder<QueryFunction> builder = ImmutableList.builder();
+    builder.addAll(DEFAULT_QUERY_FUNCTIONS);
+    builder.addAll(extraFunctions);
+    return builder.build();
+  }
+
+}
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 7fabdc75aa..0cc6da0b12 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
@@ -13,76 +13,47 @@
 // limitations under the License.
 package com.google.devtools.build.lib.query2;
 
-import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
-import static com.google.devtools.build.lib.packages.Type.TRISTATE;
-
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Sets;
-import com.google.devtools.build.lib.cmdline.ResolvedTargets;
-import com.google.devtools.build.lib.cmdline.TargetParsingException;
-import com.google.devtools.build.lib.events.ErrorSensingEventHandler;
-import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.graph.Digraph;
 import com.google.devtools.build.lib.graph.Node;
-import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
-import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
 import com.google.devtools.build.lib.packages.OutputFile;
 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.packages.TargetUtils;
-import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.pkgcache.PackageProvider;
 import com.google.devtools.build.lib.pkgcache.TargetEdgeObserver;
 import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
 import com.google.devtools.build.lib.pkgcache.TargetProvider;
 import com.google.devtools.build.lib.query2.engine.BlazeQueryEvalResult;
-import com.google.devtools.build.lib.query2.engine.QueryEnvironment;
+import com.google.devtools.build.lib.query2.engine.QueryEvalResult;
 import com.google.devtools.build.lib.query2.engine.QueryException;
 import com.google.devtools.build.lib.query2.engine.QueryExpression;
 import com.google.devtools.build.lib.query2.engine.SkyframeRestartQueryException;
 import com.google.devtools.build.lib.syntax.Label;
-import com.google.devtools.build.lib.util.BinaryPredicate;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
-import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
-import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
 /**
  * The environment of a Blaze query. Not thread-safe.
  */
-public class BlazeQueryEnvironment implements QueryEnvironment<Target> {
-  protected final ErrorSensingEventHandler eventHandler;
+public class BlazeQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
   private final TargetProvider targetProvider;
-  private final TargetPatternEvaluator targetPatternEvaluator;
   private final Digraph<Target> graph = new Digraph<>();
   private final ErrorPrintingTargetEdgeErrorObserver errorObserver;
   private final LabelVisitor labelVisitor;
-  private final Map<String, Set<Target>> letBindings = new HashMap<>();
-  private final Map<String, ResolvedTargets<Target>> resolvedTargetPatterns = new HashMap<>();
-  protected final boolean keepGoing;
-  private final boolean strictScope;
   protected final int loadingPhaseThreads;
 
-  private final BinaryPredicate<Rule, Attribute> dependencyFilter;
-  private final Predicate<Label> labelFilter;
-
-  private final Set<Setting> settings;
-  private final List<QueryFunction> extraFunctions;
-  private final BlazeTargetAccessor accessor = new BlazeTargetAccessor();
+  private final BlazeTargetAccessor accessor = new BlazeTargetAccessor(this);
 
   /**
    * Note that the correct operation of this class critically depends on the Reporter being a
@@ -104,18 +75,13 @@ public class BlazeQueryEnvironment implements QueryEnvironment<Target> {
       EventHandler eventHandler,
       Set<Setting> settings,
       Iterable<QueryFunction> extraFunctions) {
-    this.eventHandler = new ErrorSensingEventHandler(eventHandler);
+    super(targetPatternEvaluator, keepGoing, strictScope, labelFilter, eventHandler, settings,
+        extraFunctions
+    );
     this.targetProvider = packageProvider;
-    this.targetPatternEvaluator = targetPatternEvaluator;
     this.errorObserver = new ErrorPrintingTargetEdgeErrorObserver(this.eventHandler);
-    this.keepGoing = keepGoing;
-    this.strictScope = strictScope;
     this.loadingPhaseThreads = loadingPhaseThreads;
-    this.dependencyFilter = constructDependencyFilter(settings);
     this.labelVisitor = new LabelVisitor(packageProvider, dependencyFilter);
-    this.labelFilter = labelFilter;
-    this.settings = Sets.immutableEnumSet(settings);
-    this.extraFunctions = ImmutableList.copyOf(extraFunctions);
   }
 
   /**
@@ -136,78 +102,11 @@ public class BlazeQueryEnvironment implements QueryEnvironment<Target> {
         loadingPhaseThreads, Rule.ALL_LABELS, eventHandler, settings, extraFunctions);
   }
 
-  private static BinaryPredicate<Rule, Attribute> constructDependencyFilter(Set<Setting> settings) {
-    BinaryPredicate<Rule, Attribute> specifiedFilter =
-        settings.contains(Setting.NO_HOST_DEPS) ? Rule.NO_HOST_DEPS : Rule.ALL_DEPS;
-    if (settings.contains(Setting.NO_IMPLICIT_DEPS)) {
-      specifiedFilter = Rule.and(specifiedFilter, Rule.NO_IMPLICIT_DEPS);
-    }
-    if (settings.contains(Setting.NO_NODEP_DEPS)) {
-      specifiedFilter = Rule.and(specifiedFilter, Rule.NO_NODEP_ATTRIBUTES);
-    }
-    return specifiedFilter;
-  }
-
-  /**
-   * Evaluate the specified query expression in this environment.
-   *
-   * @return a {@link BlazeQueryEvalResult} object that contains the resulting set of targets, the
-   *   partial graph, and a bit to indicate whether errors occured during evaluation; note that the
-   *   success status can only be false if {@code --keep_going} was in effect
-   * @throws QueryException if the evaluation failed and {@code --nokeep_going} was in
-   *   effect
-   */
-  public BlazeQueryEvalResult<Target> evaluateQuery(QueryExpression expr) throws QueryException {
-    // Some errors are reported as QueryExceptions and others as ERROR events
-    // (if --keep_going).
-    eventHandler.resetErrors();
-    resolvedTargetPatterns.clear();
-
-    // In the --nokeep_going case, errors are reported in the order in which the patterns are
-    // specified; using a linked hash set here makes sure that the left-most error is reported.
-    Set<String> targetPatternSet = new LinkedHashSet<>();
-    expr.collectTargetPatterns(targetPatternSet);
-    try {
-      resolvedTargetPatterns.putAll(preloadOrThrow(targetPatternSet));
-    } catch (TargetParsingException e) {
-      // Unfortunately, by evaluating the patterns in parallel, we lose some location information.
-      throw new QueryException(expr, e.getMessage());
-    }
-
-    Set<Target> resultNodes;
-    try {
-      resultNodes = expr.eval(this);
-    } catch (QueryException e) {
-      throw new QueryException(e, expr);
-    }
-
-    if (eventHandler.hasErrors()) {
-      if (!keepGoing) {
-        // This case represents loading-phase errors reported during evaluation
-        // of target patterns that don't cause evaluation to fail per se.
-        throw new QueryException("Evaluation of query \"" + expr
-            + "\" failed due to BUILD file errors");
-      } else {
-        eventHandler.handle(Event.warn("--keep_going specified, ignoring errors.  "
-                      + "Results may be inaccurate"));
-      }
-    }
-
-    return new BlazeQueryEvalResult<>(!eventHandler.hasErrors(), resultNodes, graph);
-  }
-
-  public BlazeQueryEvalResult<Target> evaluateQuery(String query) throws QueryException {
-    return evaluateQuery(QueryExpression.parse(query, this));
-  }
-
   @Override
-  public void reportBuildFileError(QueryExpression caller, String message) throws QueryException {
-    if (!keepGoing) {
-      throw new QueryException(caller, message);
-    } else {
-      // Keep consistent with evaluateQuery() above.
-      eventHandler.handle(Event.error("Evaluation of query \"" + caller + "\" failed: " + message));
-    }
+  public BlazeQueryEvalResult<Target> evaluateQuery(QueryExpression expr) throws QueryException {
+    QueryEvalResult<Target> queryEvalResult = super.evaluateQuery(expr);
+    return new BlazeQueryEvalResult<>(queryEvalResult.getSuccess(), queryEvalResult.getResultSet(),
+        graph);
   }
 
   @Override
@@ -362,16 +261,6 @@ public class BlazeQueryEnvironment implements QueryEnvironment<Target> {
     return getTargetsFromNodes(graph.getShortestPath(getNode(from), getNode(to)));
   }
 
-  @Override
-  public Set<Target> getVariable(String name) {
-    return letBindings.get(name);
-  }
-
-  @Override
-  public Set<Target> setVariable(String name, Set<Target> value) {
-    return letBindings.put(name, value);
-  }
-
   /**
    * It suffices to synchronize the modifications of this.graph from within the
    * GraphBuildingObserver, because that's the only concurrent part.
@@ -403,45 +292,6 @@ public class BlazeQueryEnvironment implements QueryEnvironment<Target> {
     graph.addEdge(from, to);
   }
 
-  private boolean validateScope(Label label, boolean strict) throws QueryException {
-    if (!labelFilter.apply(label)) {
-      String error = String.format("target '%s' is not within the scope of the query", label);
-      if (strict) {
-        throw new QueryException(error);
-      } else {
-        eventHandler.handle(Event.warn(error + ". Skipping"));
-        return false;
-      }
-    }
-    return true;
-  }
-
-  public Set<Target> evalTargetPattern(QueryExpression caller, String pattern)
-      throws QueryException {
-    if (!resolvedTargetPatterns.containsKey(pattern)) {
-      try {
-        resolvedTargetPatterns.putAll(preloadOrThrow(ImmutableList.of(pattern)));
-      } catch (TargetParsingException e) {
-        // Will skip the target and keep going if -k is specified.
-        resolvedTargetPatterns.put(pattern, ResolvedTargets.<Target>empty());
-        reportBuildFileError(caller, e.getMessage());
-      }
-    }
-    return getTargetsMatchingPattern(caller, pattern);
-  }
-
-  private Map<String, ResolvedTargets<Target>> preloadOrThrow(Collection<String> patterns)
-      throws TargetParsingException {
-    try {
-      // Note that this may throw a RuntimeException if deps are missing in Skyframe.
-      return targetPatternEvaluator.preloadTargetPatterns(
-          eventHandler, patterns, keepGoing);
-    } catch (InterruptedException e) {
-      // TODO(bazel-team): Propagate the InterruptedException from here [skyframe-loading].
-      throw new TargetParsingException("interrupted");
-    }
-  }
-
   private Target getTargetOrThrow(Label label)
       throws NoSuchThingException, SkyframeRestartQueryException, InterruptedException {
     Target target = targetProvider.getTarget(eventHandler, label);
@@ -500,128 +350,6 @@ public class BlazeQueryEnvironment implements QueryEnvironment<Target> {
     return accessor;
   }
 
-  @Override
-  public boolean isSettingEnabled(Setting setting) {
-    return settings.contains(Preconditions.checkNotNull(setting));
-  }
-
-  @Override
-  public Iterable<QueryFunction> getFunctions() {
-    ImmutableList.Builder<QueryFunction> builder = ImmutableList.builder();
-    builder.addAll(DEFAULT_QUERY_FUNCTIONS);
-    builder.addAll(extraFunctions);
-    return builder.build();
-  }
-
-  private final class BlazeTargetAccessor implements TargetAccessor<Target> {
-
-    @Override
-    public String getTargetKind(Target target) {
-      return target.getTargetKind();
-    }
-
-    @Override
-    public String getLabel(Target target) {
-      return target.getLabel().toString();
-    }
-
-    @Override
-    public List<Target> getLabelListAttr(QueryExpression caller, Target target, String attrName,
-        String errorMsgPrefix) throws QueryException {
-      Preconditions.checkArgument(target instanceof Rule);
-
-      List<Target> result = new ArrayList<>();
-      Rule rule = (Rule) target;
-
-      AggregatingAttributeMapper attrMap = AggregatingAttributeMapper.of(rule);
-      Type<?> attrType = attrMap.getAttributeType(attrName);
-      if (attrType == null) {
-        // Return an empty list if the attribute isn't defined for this rule.
-        return ImmutableList.of();
-      }
-      for (Object value : attrMap.visitAttribute(attrName, attrType)) {
-        // Computed defaults may have null values.
-        if (value != null) {
-          for (Label label : attrType.getLabels(value)) {
-            try {
-              result.add(getTarget(label));
-            } catch (TargetNotFoundException e) {
-              reportBuildFileError(caller, errorMsgPrefix + e.getMessage());
-            }
-          }
-        }
-      }
-
-      return result;
-    }
-
-    @Override
-    public List<String> getStringListAttr(Target target, String attrName) {
-      Preconditions.checkArgument(target instanceof Rule);
-      return NonconfigurableAttributeMapper.of((Rule) target).get(attrName, Type.STRING_LIST);
-    }
-
-    @Override
-    public String getStringAttr(Target target, String attrName) {
-      Preconditions.checkArgument(target instanceof Rule);
-      return NonconfigurableAttributeMapper.of((Rule) target).get(attrName, Type.STRING);
-    }
-
-    @Override
-    public Iterable<String> getAttrAsString(Target target, String attrName) {
-      Preconditions.checkArgument(target instanceof Rule);
-      List<String> values = new ArrayList<>(); // May hold null values.
-      Attribute attribute = ((Rule) target).getAttributeDefinition(attrName);
-      if (attribute != null) {
-        Type<?> attributeType = attribute.getType();
-        for (Object attrValue : AggregatingAttributeMapper.of((Rule) target).visitAttribute(
-            attribute.getName(), attributeType)) {
-
-          // Ugly hack to maintain backward 'attr' query compatibility for BOOLEAN and TRISTATE
-          // attributes. These are internally stored as actual Boolean or TriState objects but were
-          // historically queried as integers. To maintain compatibility, we inspect their actual
-          // value and return the integer equivalent represented as a String. This code is the
-          // opposite of the code in BooleanType and TriStateType respectively.
-          if (attributeType == BOOLEAN) {
-            values.add(Type.BOOLEAN.cast(attrValue) ? "1" : "0");
-          } else if (attributeType == TRISTATE) {
-              switch (Type.TRISTATE.cast(attrValue)) {
-                case AUTO :
-                  values.add("-1");
-                  break;
-                case NO :
-                  values.add("0");
-                  break;
-                case YES :
-                  values.add("1");
-                  break;
-                default :
-                  throw new AssertionError("This can't happen!");
-              }
-          } else {
-            values.add(attrValue == null ? null : attrValue.toString());
-          }
-        }
-      }
-      return values;
-    }
-
-    @Override
-    public boolean isRule(Target target) {
-      return target instanceof Rule;
-    }
-
-    @Override
-    public boolean isTestRule(Target target) {
-      return TargetUtils.isTestRule(target);
-    }
-
-    @Override
-    public boolean isTestSuite(Target target) {
-      return TargetUtils.isTestSuiteRule(target);
-    }
-  }
-
   /** Given a set of target nodes, returns the targets. */
   private static Set<Target> getTargetsFromNodes(Iterable<Node<Target>> input) {
     Set<Target> result = new LinkedHashSet<>();
diff --git a/src/main/java/com/google/devtools/build/lib/query2/BlazeTargetAccessor.java b/src/main/java/com/google/devtools/build/lib/query2/BlazeTargetAccessor.java
new file mode 100644
index 0000000000..026bfde723
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/query2/BlazeTargetAccessor.java
@@ -0,0 +1,154 @@
+// Copyright 2015 Google Inc. 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.query2;
+
+import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
+import static com.google.devtools.build.lib.packages.Type.TRISTATE;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
+import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
+import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.packages.TargetUtils;
+import com.google.devtools.build.lib.packages.Type;
+import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetAccessor;
+import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetNotFoundException;
+import com.google.devtools.build.lib.query2.engine.QueryException;
+import com.google.devtools.build.lib.query2.engine.QueryExpression;
+import com.google.devtools.build.lib.syntax.Label;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Implementation of {@link TargetAccessor&lt;Target&gt;} that uses an
+ * {@link AbstractBlazeQueryEnvironment &lt;Target&gt;} internally to report issues and resolve
+ * targets.
+ */
+final class BlazeTargetAccessor implements TargetAccessor<Target> {
+  private final AbstractBlazeQueryEnvironment<Target> queryEnvironment;
+
+  BlazeTargetAccessor(AbstractBlazeQueryEnvironment<Target> queryEnvironment) {
+    this.queryEnvironment = queryEnvironment;
+  }
+
+  @Override
+  public String getTargetKind(Target target) {
+    return target.getTargetKind();
+  }
+
+  @Override
+  public String getLabel(Target target) {
+    return target.getLabel().toString();
+  }
+
+  @Override
+  public List<Target> getLabelListAttr(QueryExpression caller, Target target, String attrName,
+      String errorMsgPrefix) throws QueryException {
+    Preconditions.checkArgument(target instanceof Rule);
+
+    List<Target> result = new ArrayList<>();
+    Rule rule = (Rule) target;
+
+    AggregatingAttributeMapper attrMap = AggregatingAttributeMapper.of(rule);
+    Type<?> attrType = attrMap.getAttributeType(attrName);
+    if (attrType == null) {
+      // Return an empty list if the attribute isn't defined for this rule.
+      return ImmutableList.of();
+    }
+    for (Object value : attrMap.visitAttribute(attrName, attrType)) {
+      // Computed defaults may have null values.
+      if (value != null) {
+        for (Label label : attrType.getLabels(value)) {
+          try {
+            result.add(queryEnvironment.getTarget(label));
+          } catch (TargetNotFoundException e) {
+            queryEnvironment.reportBuildFileError(caller, errorMsgPrefix + e.getMessage());
+          }
+        }
+      }
+    }
+
+    return result;
+  }
+
+  @Override
+  public List<String> getStringListAttr(Target target, String attrName) {
+    Preconditions.checkArgument(target instanceof Rule);
+    return NonconfigurableAttributeMapper.of((Rule) target).get(attrName, Type.STRING_LIST);
+  }
+
+  @Override
+  public String getStringAttr(Target target, String attrName) {
+    Preconditions.checkArgument(target instanceof Rule);
+    return NonconfigurableAttributeMapper.of((Rule) target).get(attrName, Type.STRING);
+  }
+
+  @Override
+  public Iterable<String> getAttrAsString(Target target, String attrName) {
+    Preconditions.checkArgument(target instanceof Rule);
+    List<String> values = new ArrayList<>(); // May hold null values.
+    Attribute attribute = ((Rule) target).getAttributeDefinition(attrName);
+    if (attribute != null) {
+      Type<?> attributeType = attribute.getType();
+      for (Object attrValue : AggregatingAttributeMapper.of((Rule) target).visitAttribute(
+          attribute.getName(), attributeType)) {
+
+        // Ugly hack to maintain backward 'attr' query compatibility for BOOLEAN and TRISTATE
+        // attributes. These are internally stored as actual Boolean or TriState objects but were
+        // historically queried as integers. To maintain compatibility, we inspect their actual
+        // value and return the integer equivalent represented as a String. This code is the
+        // opposite of the code in BooleanType and TriStateType respectively.
+        if (attributeType == BOOLEAN) {
+          values.add(Type.BOOLEAN.cast(attrValue) ? "1" : "0");
+        } else if (attributeType == TRISTATE) {
+            switch (Type.TRISTATE.cast(attrValue)) {
+              case AUTO :
+                values.add("-1");
+                break;
+              case NO :
+                values.add("0");
+                break;
+              case YES :
+                values.add("1");
+                break;
+              default :
+                throw new AssertionError("This can't happen!");
+            }
+        } else {
+          values.add(attrValue == null ? null : attrValue.toString());
+        }
+      }
+    }
+    return values;
+  }
+
+  @Override
+  public boolean isRule(Target target) {
+    return target instanceof Rule;
+  }
+
+  @Override
+  public boolean isTestRule(Target target) {
+    return TargetUtils.isTestRule(target);
+  }
+
+  @Override
+  public boolean isTestSuite(Target target) {
+    return TargetUtils.isTestSuiteRule(target);
+  }
+}
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 5062813f4a..db11cd5064 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
@@ -17,9 +17,13 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
-import com.google.devtools.build.lib.query2.BlazeQueryEnvironment;
+import com.google.devtools.build.lib.pkgcache.PackageManager;
+import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
+import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader;
+import com.google.devtools.build.lib.query2.AbstractBlazeQueryEnvironment;
 import com.google.devtools.build.lib.query2.SkyframeQueryEnvironment;
 import com.google.devtools.build.lib.query2.engine.BlazeQueryEvalResult;
 import com.google.devtools.build.lib.query2.engine.QueryEnvironment.QueryFunction;
@@ -102,9 +106,10 @@ public final class QueryCommand implements BlazeCommand {
     String query = Joiner.on(' ').join(options.getResidue());
 
     Set<Setting> settings = queryOptions.toSettings();
-    BlazeQueryEnvironment env = newQueryEnvironment(
+    AbstractBlazeQueryEnvironment<Target> env = newQueryEnvironment(
         runtime,
         queryOptions.keepGoing,
+        orderResults(queryOptions, formatter),
         queryOptions.loadingPhaseThreads,
         settings);
 
@@ -119,7 +124,7 @@ public final class QueryCommand implements BlazeCommand {
     }
 
     // 2. Evaluate expression:
-    BlazeQueryEvalResult<Target> result;
+    QueryEvalResult<Target> result;
     try {
       result = env.evaluateQuery(expr);
     } catch (QueryException e) {
@@ -145,11 +150,16 @@ public final class QueryCommand implements BlazeCommand {
     return result.getSuccess() ? ExitCode.SUCCESS : ExitCode.PARTIAL_ANALYSIS_FAILURE;
   }
 
+  @VisibleForTesting
+  public static boolean orderResults(QueryOptions queryOptions, OutputFormatter formatter) {
+    return queryOptions.orderResults || !(formatter instanceof UnorderedFormatter);
+  }
+
   @VisibleForTesting
   public static void output(QueryOptions queryOptions, QueryEvalResult<Target> result,
       OutputFormatter formatter, PrintStream outputStream)
       throws IOException {
-    if (queryOptions.orderResults || !(formatter instanceof UnorderedFormatter)) {
+    if (orderResults(queryOptions, formatter)) {
       formatter.output(queryOptions, ((BlazeQueryEvalResult<Target>) result).getResultGraph(),
           outputStream);
     } else {
@@ -159,16 +169,28 @@ public final class QueryCommand implements BlazeCommand {
   }
 
   @VisibleForTesting // for com.google.devtools.deps.gquery.test.QueryResultTestUtil
-  public static BlazeQueryEnvironment newQueryEnvironment(BlazeRuntime runtime,
-      boolean keepGoing, int loadingPhaseThreads, Set<Setting> settings) {
+  public static AbstractBlazeQueryEnvironment<Target> newQueryEnvironment(BlazeRuntime runtime,
+      boolean keepGoing, boolean orderedResults,  int loadingPhaseThreads,
+      Set<Setting> settings) {
     ImmutableList.Builder<QueryFunction> functions = ImmutableList.builder();
     for (BlazeModule module : runtime.getBlazeModules()) {
       functions.addAll(module.getQueryFunctions());
     }
-    return new SkyframeQueryEnvironment(
-            runtime.getPackageManager().newTransitiveLoader(),
-            runtime.getPackageManager(),
-            runtime.getTargetPatternEvaluator(),
-            keepGoing, loadingPhaseThreads, runtime.getReporter(), settings, functions.build());
+    return newQueryEnvironment(
+        runtime.getPackageManager().newTransitiveLoader(),
+        runtime.getPackageManager(),
+        runtime.getTargetPatternEvaluator(),
+        keepGoing, orderedResults, loadingPhaseThreads, runtime.getReporter(), settings,
+        functions.build());
+  }
+
+  @VisibleForTesting
+  public static AbstractBlazeQueryEnvironment<Target> newQueryEnvironment(
+      TransitivePackageLoader transitivePackageLoader, PackageManager packageManager,
+      TargetPatternEvaluator targetPatternEvaluator, boolean keepGoing, boolean orderedResults,
+      int loadingPhaseThreads,
+      EventHandler eventHandler, Set<Setting> settings, Iterable<QueryFunction> functions) {
+    return new SkyframeQueryEnvironment(transitivePackageLoader, packageManager,
+        targetPatternEvaluator, keepGoing, loadingPhaseThreads, eventHandler, settings, functions);
   }
 }
-- 
cgit v1.2.3