Simplify CompileOneDependencyTransformer. This is the first step of improving

it more with respect to the new parse_headers feature, ideas I have about
configurable attributes etc.

I am trying to keep functional changes to a minimum. Functional changes
that couldn't really be easily avoided:
- Find correct rule for headers through filegroups
- Find correct target to build even if sources are missing. A proper error is
  reported in the subsequent loading phase

Also remove SrcTargetUtil as it is no longer used.

--
MOS_MIGRATED_REVID=118589517

2 files changed, 84 insertions, 179 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java b/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java
index 7caddf6329..b5c76b7749 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java
@@ -14,26 +14,27 @@
 package com.google.devtools.build.lib.pkgcache;
 
 import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
-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.BuildType;
-import com.google.devtools.build.lib.packages.DependencyFilter;
 import com.google.devtools.build.lib.packages.FileTarget;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
+import com.google.devtools.build.lib.packages.OutputFile;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.RawAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.util.BinaryPredicate;
 
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
 
 /**
  * Implementation of --compile_one_dependency.
@@ -83,82 +84,90 @@ public final class CompileOneDependencyTransformer {
     return orderedList;
   }
 
-  private Target transformCompileOneDependency(EventHandler eventHandler, Target target)
+  /**
+   * Returns true if a specific rule compiles a specific source. Looks through genrules and
+   * filegroups.
+   */
+  private boolean listContainsFile(
+      EventHandler eventHandler,
+      Collection<Label> srcLabels,
+      Label source,
+      Set<Label> visitedRuleLabels)
       throws TargetParsingException {
-    if (!(target instanceof FileTarget)) {
-      throw new TargetParsingException("--compile_one_dependency target '" +
-                                       target.getLabel() + "' must be a file");
+    if (srcLabels.contains(source)) {
+      return true;
     }
-
-    Package pkg = target.getPackage();
-
-    Iterable<Rule> orderedRuleList = getOrderedRuleList(pkg);
-    // Consuming rule to return if no "preferred" rules have been found.
-    Rule fallbackRule = null;
-
-    for (Rule rule : orderedRuleList) {
+    for (Label label : srcLabels) {
+      if (!visitedRuleLabels.add(label)) {
+        continue;
+      }
+      Target target = null;
       try {
-        // The call to getSrcTargets here can be removed in favor of the
-        // rule.getLabels() call below once we update "srcs" for all rules.
-        if (SrcTargetUtil.getSrcTargets(eventHandler, rule, targetProvider).contains(target)) {
-          if (rule.getRuleClassObject().isPreferredDependency(target.getName())) {
-            return rule;
-          } else if (fallbackRule == null) {
-            fallbackRule = rule;
-          }
-        }
+        target = targetProvider.getTarget(eventHandler, label);
       } catch (NoSuchThingException e) {
-        // Nothing to see here. Move along.
+        // Just ignore failing sources/packages. We could report them here, but as long as we do
+        // early return, the presence of this error would then be determined by the order of items
+        // in the srcs attribute. A proper error will be created by the subsequent loading.
       } catch (InterruptedException e) {
+        Thread.currentThread().interrupt();
         throw new TargetParsingException("interrupted");
       }
+      if (target == null || target instanceof FileTarget) {
+        continue;
+      }
+      Rule targetRule = target.getAssociatedRule();
+      if ("filegroup".equals(targetRule.getRuleClass())) {
+        RawAttributeMapper attributeMapper = RawAttributeMapper.of(targetRule);
+        Collection<Label> srcs = attributeMapper.getMergedValues("srcs", BuildType.LABEL_LIST);
+        if (listContainsFile(eventHandler, srcs, source, visitedRuleLabels)) {
+          return true;
+        }
+      } else if ("genrule".equals(targetRule.getRuleClass())) {
+        // TODO(djasper): Likely, it makes much more sense to look at the inputs of a genrule.
+        for (OutputFile file : targetRule.getOutputFiles()) {
+          if (file.getLabel().equals(source)) {
+            return true;
+          }
+        }
+      }
     }
+    return false;
+  }
 
-    Rule result = null;
+  private Target transformCompileOneDependency(EventHandler eventHandler, Target target)
+      throws TargetParsingException {
+    if (!(target instanceof FileTarget)) {
+      throw new TargetParsingException(
+          "--compile_one_dependency target '" + target.getLabel() + "' must be a file");
+    }
 
-    // For each rule, see if it has directCompileTimeInputAttribute,
-    // and if so check the targets listed in that attribute match the label.
-    BinaryPredicate<Rule, Attribute> directCompileTimeInput =
-        new BinaryPredicate<Rule, Attribute>() {
-          @Override
-          public boolean apply(Rule rule, Attribute attribute) {
-            return DependencyFilter.DIRECT_COMPILE_TIME_INPUT.apply(rule, attribute)
-                // We don't know which path to follow for configurable attributes, so skip them.
-                && !rule.isConfigurableAttribute(attribute.getName());
-          }
-        };
+    Rule result = null;
+    Iterable<Rule> orderedRuleList = getOrderedRuleList(target.getPackage());
     for (Rule rule : orderedRuleList) {
-      if (rule.getLabels(directCompileTimeInput).contains(target.getLabel())) {
+      Set<Label> labels = getInputLabels(rule);
+      if (listContainsFile(eventHandler, labels, target.getLabel(), Sets.<Label>newHashSet())) {
         if (rule.getRuleClassObject().isPreferredDependency(target.getName())) {
           result = rule;
-        } else if (fallbackRule == null) {
-          fallbackRule = rule;
+          break;
+        }
+        if (result == null) {
+          result = rule;
         }
       }
     }
 
     if (result == null) {
-      result = fallbackRule;
-    }
-
-    if (result == null) {
       throw new TargetParsingException(
           "Couldn't find dependency on target '" + target.getLabel() + "'");
     }
 
-    try {
-      // If the rule has source targets, return it.
-      if (!SrcTargetUtil.getSrcTargets(eventHandler, result, targetProvider).isEmpty()) {
-        return result;
-      }
-    } catch (NoSuchThingException e) {
-      eventHandler.handle(Event.error(e.getMessage()));
-      throw new TargetParsingException(
-          "Couldn't find dependency on target '" + target.getLabel() + "'");
-    } catch (InterruptedException e) {
-      throw new TargetParsingException("interrupted");
+    // TODO(djasper): Check whether parse_headers is disabled and just return if not.
+    // If the rule has source targets, return it.
+    if (!RawAttributeMapper.of(result).getMergedValues("srcs", BuildType.LABEL_LIST).isEmpty()) {
+      return result;
     }
 
+    // Try to find a rule in the same package that has 'result' as a dependency.
     for (Rule rule : orderedRuleList) {
       RawAttributeMapper attributes = RawAttributeMapper.of(rule);
       // We don't know which path to follow for configurable attributes, so skip them.
@@ -167,8 +176,8 @@ public final class CompileOneDependencyTransformer {
         continue;
       }
       RuleClass ruleClass = rule.getRuleClassObject();
-      if (ruleClass.hasAttr("deps", BuildType.LABEL_LIST) &&
-          ruleClass.hasAttr("srcs", BuildType.LABEL_LIST)) {
+      if (ruleClass.hasAttr("deps", BuildType.LABEL_LIST)
+          && ruleClass.hasAttr("srcs", BuildType.LABEL_LIST)) {
         for (Label dep : attributes.get("deps", BuildType.LABEL_LIST)) {
           if (dep.equals(result.getLabel())) {
             if (!attributes.get("srcs", BuildType.LABEL_LIST).isEmpty()) {
@@ -182,4 +191,21 @@ public final class CompileOneDependencyTransformer {
     throw new TargetParsingException(
         "Couldn't find dependency on target '" + target.getLabel() + "'");
   }
+
+  /** Returns all labels that are contained in direct compile time inputs of {@code rule}. */
+  private static Set<Label> getInputLabels(Rule rule) {
+    RawAttributeMapper attributeMapper = RawAttributeMapper.of(rule);
+    Set<Label> labels = new TreeSet<>();
+    for (String attrName : attributeMapper.getAttributeNames()) {
+      if (!attributeMapper.getAttributeDefinition(attrName).isDirectCompileTimeInput()) {
+        continue;
+      }
+      // TODO(djasper): We might also want to look at LABEL types, but there currently is the
+      // attribute xcode_config, which leads to test errors in Bazel tests.
+      if (rule.isAttrDefined(attrName, BuildType.LABEL_LIST)) {
+        labels.addAll(attributeMapper.getMergedValues(attrName, BuildType.LABEL_LIST));
+      }
+    }
+    return labels;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/SrcTargetUtil.java b/src/main/java/com/google/devtools/build/lib/pkgcache/SrcTargetUtil.java
deleted file mode 100644
index 3aeab0a46d..0000000000
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/SrcTargetUtil.java
+++ /dev/null
@@ -1,121 +0,0 @@
-// Copyright 2014 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.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Sets;
-import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.concurrent.ThreadSafety;
-import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.packages.BuildType;
-import com.google.devtools.build.lib.packages.FileTarget;
-import com.google.devtools.build.lib.packages.NoSuchPackageException;
-import com.google.devtools.build.lib.packages.NoSuchTargetException;
-import com.google.devtools.build.lib.packages.RawAttributeMapper;
-import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.packages.Target;
-
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Set;
-
-/**
- * A helper class for getting source and header files from a given {@link Rule}.
- */
-public final class SrcTargetUtil {
-  private SrcTargetUtil() {
-  }
-
-  /**
-   * Given a Rule, returns an immutable list of FileTarget for its sources, in the order they appear
-   * in its "srcs", "src" or "srcjar" attribute, and any filegroups or other rules it references.
-   * An empty list is returned if no "srcs" or "src" attribute exists for this rule. The list may
-   * contain OutputFiles if the sources were generated by another rule.
-   *
-   * <p>This method should be considered only a heuristic, and should not be used during the
-   * analysis phase.
-   *
-   * <p>(We could remove the throws clauses if we restrict the results to srcs within the same
-   * package.)
-   *
-   * @throws NoSuchTargetException or NoSuchPackageException when a source label cannot be resolved
-   *         to a Target
-   */
-  @ThreadSafety.ThreadSafe
-  public static List<FileTarget> getSrcTargets(EventHandler eventHandler, Rule rule,
-                                               TargetProvider provider)
-      throws NoSuchTargetException, NoSuchPackageException, InterruptedException  {
-    return getTargets(eventHandler, rule, SOURCE_ATTRIBUTES, Sets.newHashSet(rule.getLabel()),
-        provider);
-  }
-
-  // Attributes referring to "sources".
-  private static final ImmutableSet<String> SOURCE_ATTRIBUTES =
-      ImmutableSet.of("srcs", "src", "srcjar");
-
-  // The attribute to search in filegroups.
-  private static final ImmutableSet<String> FILEGROUP_ATTRIBUTES =
-      ImmutableSet.of("srcs");
-
-  /**
-   * @see #getSrcTargets(EventHandler, Rule, TargetProvider)
-   */
-  private static List<FileTarget> getTargets(EventHandler eventHandler,
-      Rule rule,
-      ImmutableSet<String> attributes,
-      Set<Label> visitedRuleLabels,
-      TargetProvider targetProvider)
-      throws NoSuchTargetException, NoSuchPackageException, InterruptedException {
-    List<Label> srcLabels = Lists.newArrayList();
-    RawAttributeMapper attributeMapper = RawAttributeMapper.of(rule);
-    for (String attrName : attributes) {
-      if (rule.isAttrDefined(attrName, BuildType.LABEL_LIST)) {
-        // Note that we simply flatten configurable attributes here, which is not the correct
-        // behavior. However, it seems to be a good approximation of what is needed in most use
-        // cases.
-        srcLabels.addAll(attributeMapper.getMergedValues(attrName, BuildType.LABEL_LIST));
-      } else if (rule.isAttrDefined(attrName, BuildType.LABEL)
-          && !rule.isConfigurableAttribute(attrName)) {
-        Label srcLabel = attributeMapper.get(attrName, BuildType.LABEL);
-        if (srcLabel != null) {
-          srcLabels.add(srcLabel);
-        }
-      }
-    }
-    if (srcLabels.isEmpty()) {
-      return ImmutableList.of();
-    }
-    List<FileTarget> srcTargets = new ArrayList<>();
-    for (Label label : srcLabels) {
-      Target target = targetProvider.getTarget(eventHandler, label);
-      if (target instanceof FileTarget) {
-        srcTargets.add((FileTarget) target);
-      } else {
-        Rule srcRule = target.getAssociatedRule();
-        if (srcRule != null && !visitedRuleLabels.contains(srcRule.getLabel())) {
-          visitedRuleLabels.add(srcRule.getLabel());
-          if ("filegroup".equals(srcRule.getRuleClass())) {
-            srcTargets.addAll(getTargets(eventHandler, srcRule, FILEGROUP_ATTRIBUTES,
-                visitedRuleLabels, targetProvider));
-          } else {
-            srcTargets.addAll(srcRule.getOutputFiles());
-          }
-        }
-      }
-    }
-    return ImmutableList.copyOf(srcTargets);
-  }
-}