Make aspects work through bind().

bind() is assumed to be able to provide any provider. This is suboptimal, but beats the alternative of traversing the dependency graph to an arbitrary depth.

The reason for the removal of the iteration ability in TransitiveInfoCollection is that now aspects can be attached to BindConfiguredTarget, too, which is not a RuleConfiguredTarget. Whereas I could have implemented the iterator, it was used only in BindConfiguredTarget anyway, so there didn't seem to be much reason to.

Some work towards #952.

--
MOS_MIGRATED_REVID=120549877

8 files changed, 154 insertions, 174 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
index f438c99204..645cd6d889 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
@@ -87,6 +87,10 @@ public final class ConfiguredAspect implements Iterable<TransitiveInfoProvider>
     return providers.values().iterator();
   }
 
+  public static ConfiguredAspect forAlias(ConfiguredAspect real) {
+    return new ConfiguredAspect(real.getName(), real.getProviders());
+  }
+
   /**
    * Builder for {@link ConfiguredAspect}.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index fe8bddc4a0..ef48488e4f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -61,7 +61,6 @@ import javax.annotation.Nullable;
  * <p>Includes logic to derive the right configurations depending on transition type.
  */
 public abstract class DependencyResolver {
-
   protected DependencyResolver() {
   }
 
@@ -450,7 +449,7 @@ public abstract class DependencyResolver {
   }
 
   private ImmutableSet<AspectDescriptor> requiredAspects(
-      Aspect aspect, Attribute attribute, Target target, Rule originalRule) {
+      Aspect aspect, Attribute attribute, final Target target, Rule originalRule) {
     if (!(target instanceof Rule)) {
       return ImmutableSet.of();
     }
@@ -459,7 +458,7 @@ public abstract class DependencyResolver {
     RuleClass ruleClass = ((Rule) target).getRuleClassObject();
     ImmutableSet.Builder<AspectDescriptor> result = ImmutableSet.builder();
     for (Aspect aspectCandidate : aspectCandidates) {
-      if (Sets.difference(
+      if (ruleClass.canHaveAnyProvider() || Sets.difference(
               aspectCandidate.getDefinition().getRequiredProviders(),
               ruleClass.getAdvertisedProviders())
           .isEmpty()) {
@@ -499,8 +498,9 @@ public abstract class DependencyResolver {
     return aspectCandidates;
   }
 
-  private void visitRule(Rule rule, ListMultimap<Attribute, LabelAndConfiguration> labelMap,
-      NestedSetBuilder<Label> rootCauses, ListMultimap<Attribute, Dependency> outgoingEdges) {
+  private void visitRule(Rule rule,
+      ListMultimap<Attribute, LabelAndConfiguration> labelMap, NestedSetBuilder<Label> rootCauses,
+      ListMultimap<Attribute, Dependency> outgoingEdges) {
     visitRule(rule, /*aspect=*/ null, labelMap, rootCauses, outgoingEdges);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/EnvironmentGroupConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/EnvironmentGroupConfiguredTarget.java
index dd1c29fb21..3465a1ddda 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/EnvironmentGroupConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/EnvironmentGroupConfiguredTarget.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.analysis;
 
-import com.google.common.collect.UnmodifiableIterator;
 import com.google.devtools.build.lib.packages.EnvironmentGroup;
 import com.google.devtools.build.lib.util.Preconditions;
 
@@ -38,9 +37,4 @@ public final class EnvironmentGroupConfiguredTarget extends AbstractConfiguredTa
     // No providers.
     return null;
   }
-
-  @Override
-  public UnmodifiableIterator<TransitiveInfoProvider> iterator() {
-    throw new UnsupportedOperationException();
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/FileConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/FileConfiguredTarget.java
index 86b837fbed..42178bf531 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/FileConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/FileConfiguredTarget.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.analysis;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMap.Builder;
-import com.google.common.collect.UnmodifiableIterator;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -83,9 +82,4 @@ public abstract class FileConfiguredTarget extends AbstractConfiguredTarget
   public Object get(String providerKey) {
     return null;
   }
-
-  @Override
-  public UnmodifiableIterator<TransitiveInfoProvider> iterator() {
-    return providers.values().iterator();
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/MergedConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/MergedConfiguredTarget.java
new file mode 100644
index 0000000000..6e01d8ec1f
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/MergedConfiguredTarget.java
@@ -0,0 +1,141 @@
+// Copyright 2016 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.analysis;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * A single dependency with its configured target and aspects merged together.
+ *
+ * <p>This is an ephemeral object created only for the analysis of a single configured target.
+ * After that configured target is analyzed, this is thrown away.
+ */
+public class MergedConfiguredTarget extends AbstractConfiguredTarget {
+  private final ConfiguredTarget base;
+  private final ImmutableMap<Class<? extends TransitiveInfoProvider>, TransitiveInfoProvider>
+      providers;
+
+  private MergedConfiguredTarget(ConfiguredTarget base,
+      ImmutableMap<Class<? extends TransitiveInfoProvider>, TransitiveInfoProvider> providers) {
+    super(base.getTarget(), base.getConfiguration());
+    this.base = base;
+    this.providers = providers;
+  }
+
+  /**
+   * Returns a value provided by this target. Only meant to use from Skylark.
+   */
+  @Override
+  public Object get(String providerKey) {
+    return getProvider(SkylarkProviders.class).getValue(providerKey);
+  }
+
+  @Override
+  public <P extends TransitiveInfoProvider> P getProvider(Class<P> providerClass) {
+    AnalysisUtils.checkProvider(providerClass);
+
+    Object provider = providers.get(providerClass);
+    if (provider == null) {
+      provider = base.getProvider(providerClass);
+    }
+
+    return providerClass.cast(provider);
+  }
+
+  /**
+   * Creates an instance based on a configured target and a set of aspects.
+   */
+  public static ConfiguredTarget of(ConfiguredTarget base,
+      Iterable<ConfiguredAspect> aspects) {
+    if (Iterables.isEmpty(aspects)) {
+      // If there are no aspects, don't bother with creating a proxy object
+      return base;
+    }
+
+    Set<Class<? extends TransitiveInfoProvider>> providers = new HashSet<>();
+
+    ImmutableSet<Class<? extends TransitiveInfoProvider>> baseProviders =
+        ImmutableSet.copyOf(providers);
+
+    // Merge output group providers.
+    OutputGroupProvider mergedOutputGroupProvider =
+        OutputGroupProvider.merge(getAllProviders(base, aspects, OutputGroupProvider.class));
+
+    // Merge Skylark providers.
+    SkylarkProviders mergedSkylarkProviders =
+        SkylarkProviders.merge(getAllProviders(base, aspects, SkylarkProviders.class));
+
+    // Merge extra-actions provider.
+    ExtraActionArtifactsProvider mergedExtraActionProviders = ExtraActionArtifactsProvider.merge(
+        getAllProviders(base, aspects, ExtraActionArtifactsProvider.class));
+
+    Map<Class<? extends TransitiveInfoProvider>, TransitiveInfoProvider> aspectProviders =
+        new LinkedHashMap<>();
+    if (mergedOutputGroupProvider != null) {
+      aspectProviders.put(OutputGroupProvider.class, mergedOutputGroupProvider);
+    }
+    if (mergedSkylarkProviders != null) {
+      aspectProviders.put(SkylarkProviders.class, mergedSkylarkProviders);
+    }
+    if (mergedExtraActionProviders != null) {
+      aspectProviders.put(ExtraActionArtifactsProvider.class, mergedExtraActionProviders);
+    }
+
+    for (ConfiguredAspect aspect : aspects) {
+      for (Map.Entry<Class<? extends TransitiveInfoProvider>, TransitiveInfoProvider> entry :
+          aspect.getProviders().entrySet()) {
+        if (OutputGroupProvider.class.equals(entry.getKey())
+            || SkylarkProviders.class.equals(entry.getKey())
+            || ExtraActionArtifactsProvider.class.equals(entry.getKey())) {
+          continue;
+        }
+
+        if (base.getProvider(entry.getKey()) != null
+            || aspectProviders.containsKey(entry.getKey())) {
+          throw new IllegalStateException("Provider " + entry.getKey() + " provided twice");
+        }
+
+        aspectProviders.put(entry.getKey(), entry.getValue());
+      }
+    }
+    return new MergedConfiguredTarget(base, ImmutableMap.copyOf(aspectProviders));
+  }
+
+  private static <T extends TransitiveInfoProvider> List<T> getAllProviders(
+      ConfiguredTarget base, Iterable<ConfiguredAspect> aspects, Class<T> providerClass) {
+    T baseProvider = base.getProvider(providerClass);
+    List<T> providers = new ArrayList<>();
+    if (baseProvider != null) {
+      providers.add(baseProvider);
+    }
+
+    for (ConfiguredAspect configuredAspect : aspects) {
+      final T aspectProvider = configuredAspect.getProvider(providerClass);
+      if (aspectProvider == null) {
+        continue;
+      }
+      providers.add(aspectProvider);
+    }
+    return providers;
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java
index c148ecd0bf..549932daa0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.analysis;
 
-import com.google.common.collect.UnmodifiableIterator;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -70,9 +69,4 @@ public final class PackageGroupConfiguredTarget extends AbstractConfiguredTarget
     // No providers.
     return null;
   }
-
-  @Override
-  public UnmodifiableIterator<TransitiveInfoProvider> iterator() {
-    throw new UnsupportedOperationException();
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTarget.java
index 9773cc1062..211097f45d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTarget.java
@@ -16,8 +16,6 @@ package com.google.devtools.build.lib.analysis;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.UnmodifiableIterator;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.packages.OutputFile;
@@ -25,10 +23,7 @@ import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.rules.SkylarkApiProvider;
 import com.google.devtools.build.lib.util.Preconditions;
 
-import java.util.ArrayList;
-import java.util.HashSet;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -51,7 +46,6 @@ public final class RuleConfiguredTarget extends AbstractConfiguredTarget {
 
   private final ImmutableMap<Class<? extends TransitiveInfoProvider>, Object> providers;
   private final Set<ConfigMatchingProvider> configConditions;
-  private final ImmutableList<ConfiguredAspect> configuredAspects;
 
   RuleConfiguredTarget(RuleContext ruleContext,
       ImmutableMap<String, Object> skylarkProviders,
@@ -76,7 +70,6 @@ public final class RuleConfiguredTarget extends AbstractConfiguredTarget {
 
     this.providers = ImmutableMap.copyOf(providerBuilder);
     this.configConditions = ruleContext.getConfigConditions();
-    this.configuredAspects = ImmutableList.of();
 
     // If this rule is the run_under target, then check that we have an executable; note that
     // run_under is only set in the target configuration, and the target must also be analyzed for
@@ -98,117 +91,6 @@ public final class RuleConfiguredTarget extends AbstractConfiguredTarget {
   }
 
   /**
-   * Merge a configured target with its associated aspects.
-   *
-   * <p>If aspects are present, the configured target must be created from a rule (instead of e.g.
-   * an input or an output file).
-   */
-  public static ConfiguredTarget mergeAspects(
-      ConfiguredTarget base, Iterable<ConfiguredAspect> aspects) {
-    if (Iterables.isEmpty(aspects)) {
-      // If there are no aspects, don't bother with creating a proxy object
-      return base;
-    } else {
-      // Aspects can only be attached to rules for now. This invariant is upheld by
-      // DependencyResolver#requiredAspects()
-      return new RuleConfiguredTarget((RuleConfiguredTarget) base, aspects);
-    }
-  }
-
-  /**
-   * Creates an instance based on a configured target and a set of aspects.
-   */
-  private RuleConfiguredTarget(RuleConfiguredTarget base, Iterable<ConfiguredAspect> aspects) {
-    super(base.getTarget(), base.getConfiguration());
-
-    Set<Class<? extends TransitiveInfoProvider>> providers = new HashSet<>();
-
-    providers.addAll(base.providers.keySet());
-
-    // Merge output group providers.
-    OutputGroupProvider mergedOutputGroupProvider =
-        OutputGroupProvider.merge(getAllProviders(base, aspects, OutputGroupProvider.class));
-
-    // Merge Skylark providers.
-    SkylarkProviders mergedSkylarkProviders =
-        SkylarkProviders.merge(getAllProviders(base, aspects, SkylarkProviders.class));
-
-    // Merge extra-actions provider.
-    ExtraActionArtifactsProvider mergedExtraActionProviders = ExtraActionArtifactsProvider.merge(
-        getAllProviders(base, aspects, ExtraActionArtifactsProvider.class));
-
-    // Validate that all other providers are only provided once.
-    for (ConfiguredAspect configuredAspect : aspects) {
-      for (TransitiveInfoProvider aspectProvider : configuredAspect) {
-        Class<? extends TransitiveInfoProvider> aClass = aspectProvider.getClass();
-        if (OutputGroupProvider.class.equals(aClass)) {
-          continue;
-        }
-        if (SkylarkProviders.class.equals(aClass)) {
-          continue;
-        }
-        if (ExtraActionArtifactsProvider.class.equals(aClass)) {
-          continue;
-        }
-        if (!providers.add(aClass)) {
-          throw new IllegalStateException("Provider " + aClass + " provided twice");
-        }
-      }
-    }
-
-    if (base.getProvider(OutputGroupProvider.class) == mergedOutputGroupProvider
-        && base.getProvider(SkylarkProviders.class) == mergedSkylarkProviders
-        && base.getProvider(ExtraActionArtifactsProvider.class) == mergedExtraActionProviders) {
-      this.providers = base.providers;
-    } else {
-      ImmutableMap.Builder<Class<? extends TransitiveInfoProvider>, Object> builder =
-          new ImmutableMap.Builder<>();
-      for (Class<? extends TransitiveInfoProvider> aClass : base.providers.keySet()) {
-        if (OutputGroupProvider.class.equals(aClass)) {
-          continue;
-        }
-        if (SkylarkProviders.class.equals(aClass)) {
-          continue;
-        }
-        if (ExtraActionArtifactsProvider.class.equals(aClass)) {
-          continue;
-        }
-        builder.put(aClass, base.providers.get(aClass));
-      }
-      if (mergedOutputGroupProvider != null) {
-        builder.put(OutputGroupProvider.class, mergedOutputGroupProvider);
-      }
-      if (mergedSkylarkProviders != null) {
-        builder.put(SkylarkProviders.class, mergedSkylarkProviders);
-      }
-      if (mergedExtraActionProviders != null) {
-        builder.put(ExtraActionArtifactsProvider.class, mergedExtraActionProviders);
-      }
-      this.providers = builder.build();
-    }
-    this.configConditions = base.configConditions;
-    this.configuredAspects = ImmutableList.copyOf(aspects);
-  }
-
-  private static <T extends TransitiveInfoProvider> List<T> getAllProviders(
-      RuleConfiguredTarget base, Iterable<ConfiguredAspect> aspects, Class<T> providerClass) {
-    T baseProvider = base.getProvider(providerClass);
-    List<T> providers = new ArrayList<>();
-    if (baseProvider != null) {
-      providers.add(baseProvider);
-    }
-
-    for (ConfiguredAspect configuredAspect : aspects) {
-      final T aspectProvider = configuredAspect.getProvider(providerClass);
-      if (aspectProvider == null) {
-        continue;
-      }
-      providers.add(aspectProvider);
-    }
-    return providers;
-  }
-
-  /**
    * The configuration conditions that trigger this rule's configurable attributes.
    */
   Set<ConfigMatchingProvider> getConfigConditions() {
@@ -222,15 +104,10 @@ public final class RuleConfiguredTarget extends AbstractConfiguredTarget {
     // class?
     Object provider = providers.get(providerClass);
     if (provider == null) {
-      for (ConfiguredAspect configuredAspect : configuredAspects) {
-        provider = configuredAspect.getProviders().get(providerClass);
-        if (provider != null) {
-          break;
-        }
-      }
+      return null;
+    } else {
+      return providerClass.cast(provider);
     }
-
-    return providerClass.cast(provider);
   }
 
   /**
@@ -247,23 +124,6 @@ public final class RuleConfiguredTarget extends AbstractConfiguredTarget {
   }
 
   @Override
-  public UnmodifiableIterator<TransitiveInfoProvider> iterator() {
-    Map<Class<? extends TransitiveInfoProvider>, TransitiveInfoProvider> allProviders =
-        new LinkedHashMap<>();
-    for (int i = configuredAspects.size() - 1; i >= 0; i--) {
-      for (TransitiveInfoProvider tip : configuredAspects.get(i)) {
-        allProviders.put(tip.getClass(), tip);
-      }
-    }
-
-    for (Map.Entry<Class<? extends TransitiveInfoProvider>, Object> entry : providers.entrySet()) {
-      allProviders.put(entry.getKey(), entry.getKey().cast(entry.getValue()));
-    }
-
-    return ImmutableList.copyOf(allProviders.values()).iterator();
-  }
-
-  @Override
   public String errorMessage(String name) {
     return String.format("target (rule class of '%s') doesn't have provider '%s'.",
         getTarget().getRuleClass(), name);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java
index 2264d8ff17..42b829785f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.analysis;
 
-import com.google.common.collect.UnmodifiableIterator;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
@@ -95,7 +94,7 @@ import javax.annotation.Nullable;
     + "These extra providers are defined in the <code>struct</code> returned by the rule "
     + "implementation function.</li>"
     + "</ul>")
-public interface TransitiveInfoCollection extends Iterable<TransitiveInfoProvider> {
+public interface TransitiveInfoCollection  {
 
   /**
    * Returns the transitive information provider requested, or null if the provider is not found.
@@ -121,10 +120,4 @@ public interface TransitiveInfoCollection extends Iterable<TransitiveInfoProvide
    * The transitive information has to have been added using the Skylark framework.
    */
   @Nullable Object get(String providerKey);
-
-  /**
-   * Returns an unmodifiable iterator over the transitive info providers in the collections.
-   */
-  @Override
-  UnmodifiableIterator<TransitiveInfoProvider> iterator();
 }