Do not access SkylarkProviders anywhere outside of ConfiguredTarget implementation.

A first step towards applying the same memory optimizations we do for
native provider representation to Skylark providers (declared and
legacy).

RELNOTES: None.
PiperOrigin-RevId: 156111749

13 files changed, 62 insertions, 63 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java
index f3e1535916..12fd6b9224 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java
@@ -89,7 +89,6 @@ public abstract class AbstractConfiguredTarget
     return "ConfiguredTarget(" + getTarget().getLabel() + ", " + getConfiguration() + ")";
   }
 
-  @Nullable
   @Override
   public <P extends TransitiveInfoProvider> P getProvider(Class<P> provider) {
     AnalysisUtils.checkProvider(provider);
@@ -117,7 +116,7 @@ public abstract class AbstractConfiguredTarget
   }
 
   @Override
-  public Object getIndex(Object key, Location loc) throws EvalException {
+  public final Object getIndex(Object key, Location loc) throws EvalException {
     if (!(key instanceof ClassObjectConstructor)) {
       throw new EvalException(loc, String.format(
           "Type Target only supports indexing by object constructors, got %s instead",
@@ -149,7 +148,7 @@ public abstract class AbstractConfiguredTarget
   }
 
   @Override
-  public ImmutableCollection<String> getKeys() {
+  public final ImmutableCollection<String> getKeys() {
     ImmutableList.Builder<String> result = ImmutableList.builder();
     result.addAll(ImmutableList.of(
         DATA_RUNFILES_FIELD,
@@ -180,7 +179,7 @@ public abstract class AbstractConfiguredTarget
 
   @Nullable
   @Override
-  public Object get(SkylarkProviderIdentifier id) {
+  public final Object get(SkylarkProviderIdentifier id) {
     if (id.isLegacy()) {
       return get(id.getLegacyId());
     }
@@ -191,7 +190,7 @@ public abstract class AbstractConfiguredTarget
   /** Returns a declared provider provided by this target. Only meant to use from Skylark. */
   @Nullable
   @Override
-  public SkylarkClassObject get(ClassObjectConstructor.Key providerKey) {
+  public final SkylarkClassObject get(ClassObjectConstructor.Key providerKey) {
     if (providerKey.equals(DefaultProvider.SKYLARK_CONSTRUCTOR.getKey())) {
       return getDefaultProvider();
     }
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 be92473463..f1c4b4d515 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
@@ -88,9 +88,14 @@ public final class ConfiguredAspect {
   @Nullable
   @VisibleForTesting
   public <P extends TransitiveInfoProvider> P getProvider(Class<P> providerClass) {
+    AnalysisUtils.checkProvider(providerClass);
     return providers.getProvider(providerClass);
   }
 
+  SkylarkProviders getSkylarkProviders() {
+    return providers.getProvider(SkylarkProviders.class);
+  }
+
   public Object getProvider(SkylarkProviderIdentifier id) {
     if (id.isLegacy()) {
       return get(id.getLegacyId());
@@ -103,7 +108,7 @@ public final class ConfiguredAspect {
     if (OutputGroupProvider.SKYLARK_CONSTRUCTOR.getKey().equals(key)) {
       return getProvider(OutputGroupProvider.class);
     }
-    SkylarkProviders skylarkProviders = getProvider(SkylarkProviders.class);
+    SkylarkProviders skylarkProviders = providers.getProvider(SkylarkProviders.class);
     return skylarkProviders != null ? skylarkProviders.getDeclaredProvider(key) : null;
   }
 
@@ -111,7 +116,7 @@ public final class ConfiguredAspect {
     if (OutputGroupProvider.SKYLARK_NAME.equals(legacyKey)) {
       return getProvider(OutputGroupProvider.class);
     }
-    SkylarkProviders skylarkProviders = getProvider(SkylarkProviders.class);
+    SkylarkProviders skylarkProviders = providers.getProvider(SkylarkProviders.class);
     return skylarkProviders != null
         ? skylarkProviders.get(SkylarkProviderIdentifier.forLegacy(legacyKey))
         : null;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java
index 5605f953bf..cee9b8544a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.analysis;
 
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.syntax.ClassObject;
 import javax.annotation.Nullable;
 
 /**
@@ -28,7 +29,7 @@ import javax.annotation.Nullable;
  * {@link TransitiveInfoCollection}s. Also, {@link ConfiguredTarget} objects should not be
  * accessible from the action graph.
  */
-public interface ConfiguredTarget extends TransitiveInfoCollection {
+public interface ConfiguredTarget extends TransitiveInfoCollection, ClassObject {
 
   /**
    *  All <code>ConfiguredTarget</code>s have a "label" field.
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 eb7a1e1a32..e67a2b6395 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,11 +14,8 @@
 
 package com.google.devtools.build.lib.analysis;
 
-import com.google.devtools.build.lib.packages.ClassObjectConstructor;
 import com.google.devtools.build.lib.packages.EnvironmentGroup;
-import com.google.devtools.build.lib.packages.SkylarkClassObject;
 import com.google.devtools.build.lib.util.Preconditions;
-import javax.annotation.Nullable;
 
 /**
  * Dummy ConfiguredTarget for environment groups. Contains no functionality, since
@@ -34,11 +31,4 @@ public final class EnvironmentGroupConfiguredTarget extends AbstractConfiguredTa
   public EnvironmentGroup getTarget() {
     return (EnvironmentGroup) super.getTarget();
   }
-
-  @Nullable
-  @Override
-  public SkylarkClassObject get(ClassObjectConstructor.Key providerKey) {
-    // No providers.
-    return null;
-  }
 }
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 399ef6afe6..7b68c7b2b3 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
@@ -65,7 +65,7 @@ public abstract class FileConfiguredTarget extends AbstractConfiguredTarget
   }
 
   /**
-   *  Returns the file type of this file target.
+   *  Returns the file name of this file target.
    */
   @Override
   public String getFilename() {
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 3e0b855904..181162c97a 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
@@ -18,12 +18,9 @@ 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;
 import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.packages.ClassObjectConstructor;
 import com.google.devtools.build.lib.packages.PackageGroup;
 import com.google.devtools.build.lib.packages.PackageSpecification;
-import com.google.devtools.build.lib.packages.SkylarkClassObject;
 import com.google.devtools.build.lib.util.Preconditions;
-import javax.annotation.Nullable;
 
 /**
  * Dummy ConfiguredTarget for package groups. Contains no functionality, since
@@ -66,12 +63,4 @@ public final class PackageGroupConfiguredTarget extends AbstractConfiguredTarget
   public NestedSet<PackageSpecification> getPackageSpecifications() {
     return packageSpecifications;
   }
-
-
-  @Nullable
-  @Override
-  public SkylarkClassObject get(ClassObjectConstructor.Key providerKey) {
-    // No providers.
-    return null;
-  }
 }
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 993b812f95..85e65940fd 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
@@ -19,6 +19,8 @@ import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.OutputFile;
 import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.SkylarkClassObject;
+import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor;
 import com.google.devtools.build.lib.util.Preconditions;
 import javax.annotation.Nullable;
 
@@ -48,7 +50,8 @@ public final class RuleConfiguredTarget extends AbstractConfiguredTarget {
   RuleConfiguredTarget(
       RuleContext ruleContext,
       TransitiveInfoProviderMap providers,
-      SkylarkProviders skylarkProviders) {
+      ImmutableMap<String, Object> legacySkylarkProviders,
+      ImmutableMap<SkylarkClassObjectConstructor.Key, SkylarkClassObject> skylarkProviders) {
     super(ruleContext);
     // We don't use ImmutableMap.Builder here to allow augmenting the initial list of 'default'
     // providers by passing them in.
@@ -59,9 +62,11 @@ public final class RuleConfiguredTarget extends AbstractConfiguredTarget {
     Preconditions.checkState(providerBuilder.contains(FilesToRunProvider.class));
 
     // Initialize every SkylarkApiProvider
-    if (!skylarkProviders.isEmpty()) {
-      skylarkProviders.init(this);
-      providerBuilder.add(skylarkProviders);
+    if (!legacySkylarkProviders.isEmpty() || !skylarkProviders.isEmpty()) {
+      SkylarkProviders allSkylarkProviders = new SkylarkProviders(legacySkylarkProviders,
+          skylarkProviders);
+      allSkylarkProviders.init(this);
+      providerBuilder.add(allSkylarkProviders);
     }
 
     this.providers = providerBuilder.build();
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index 84fa02700c..552122a3a5 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -140,7 +140,7 @@ public final class RuleConfiguredTargetBuilder {
     return new RuleConfiguredTarget(
         ruleContext,
         providers,
-        new SkylarkProviders(skylarkProviders.build(), skylarkDeclaredProviders.build()));
+        skylarkProviders.build(), skylarkDeclaredProviders.build());
   }
 
   /** Adds skylark providers from a skylark provider registry, and checks for collisions. */
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java
index 78cb247164..a9046eb03e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java
@@ -32,9 +32,13 @@ import java.util.Set;
 
 /**
  * A helper class for transitive infos provided by Skylark rule implementations.
+ *
+ * DO NOT USE THIS CLASS to access Skylark providers.
+ * Use {@link ConfiguredTarget#get(SkylarkProviderIdentifier)} or
+ * {@link ConfiguredAspect#getProvider(SkylarkProviderIdentifier)}.
  */
 @Immutable
-public final class SkylarkProviders implements TransitiveInfoProvider {
+final class SkylarkProviders implements TransitiveInfoProvider {
   private final ImmutableMap<ClassObjectConstructor.Key, SkylarkClassObject>
       declaredProviders;
   private final ImmutableMap<String, Object> skylarkProviders;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/AliasConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/rules/AliasConfiguredTarget.java
index 7be13d2dda..3ac06faff7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/AliasConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/AliasConfiguredTarget.java
@@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
-import com.google.devtools.build.lib.analysis.SkylarkProviders;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -127,19 +126,15 @@ public final class AliasConfiguredTarget implements ConfiguredTarget, ClassObjec
           ? NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER)
           : getProvider(FileProvider.class).getFilesToBuild());
     }
-    if (actual instanceof ClassObject) {
-      return ((ClassObject) actual).getValue(name);
-    }
-    return actual == null ? null : actual.get(name);
+    return actual == null ? null : actual.getValue(name);
   }
 
   @Override
   public ImmutableCollection<String> getKeys() {
-    ImmutableList.Builder<String> result = ImmutableList.<String>builder().add("label", "files");
     if (actual != null) {
-        result.addAll(actual.getProvider(SkylarkProviders.class).getKeys());
+        return actual.getKeys();
     }
-    return result.build();
+    return ImmutableList.of();
   }
 
   @Override
@@ -147,4 +142,12 @@ public final class AliasConfiguredTarget implements ConfiguredTarget, ClassObjec
     // Use the default error message.
     return null;
   }
+
+  /**
+   *  Returns a target this target aliases.
+   */
+  @Nullable
+  public ConfiguredTarget getActual() {
+    return actual;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
index dd1dece72a..848ef4419c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
@@ -37,6 +37,8 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.BuildType;
+import com.google.devtools.build.lib.packages.ClassObjectConstructor;
+import com.google.devtools.build.lib.packages.SkylarkClassObject;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.rules.cpp.CppCompilationContext;
@@ -832,6 +834,15 @@ public class JavaCommon {
   }
 
   /**
+   * Gets all the deps that implement a particular provider.
+   */
+  public final <P extends SkylarkClassObject> Iterable<P> getDependencies(
+      ClassObjectConstructor.Key provider, Class<P> resultClass) {
+    return AnalysisUtils.getProviders(getDependencies(), provider, resultClass);
+  }
+
+
+  /**
    * Returns true if and only if this target has the neverlink attribute set to
    * 1, or false if the neverlink attribute does not exist (for example, on
    * *_binary targets)
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaProvider.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaProvider.java
index f8c6daec8f..1a07ff3d0e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaProvider.java
@@ -18,7 +18,6 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.Runfiles;
-import com.google.devtools.build.lib.analysis.SkylarkProviders;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMap;
@@ -133,12 +132,8 @@ public final class JavaProvider extends SkylarkClassObject implements Transitive
     if (provider != null) {
       return provider;
     }
-    SkylarkProviders skylarkProviders = target.getProvider(SkylarkProviders.class);
-    if (skylarkProviders == null) {
-      return null;
-    }
     JavaProvider javaProvider =
-        (JavaProvider) skylarkProviders.getDeclaredProvider(JavaProvider.JAVA_PROVIDER.getKey());
+        (JavaProvider) target.get(JavaProvider.JAVA_PROVIDER.getKey());
     if (javaProvider == null) {
       return null;
     }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
index 2036401394..3ea0849634 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
@@ -30,7 +30,6 @@ import com.google.devtools.build.lib.analysis.PseudoAction;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleContext;
-import com.google.devtools.build.lib.analysis.SkylarkProviders;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.Util;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -294,12 +293,13 @@ public final class PyCommon {
   private NestedSet<Artifact> getTransitivePythonSourcesFromSkylarkProvider(
       TransitiveInfoCollection dep) {
     SkylarkClassObject pythonSkylarkProvider = null;
-    SkylarkProviders skylarkProviders = dep.getProvider(SkylarkProviders.class);
     try {
-      if (skylarkProviders != null) {
-        pythonSkylarkProvider = skylarkProviders.getValue(PYTHON_SKYLARK_PROVIDER_NAME,
-            SkylarkClassObject.class);
-      }
+      pythonSkylarkProvider = SkylarkType.cast(
+              dep.get(PYTHON_SKYLARK_PROVIDER_NAME),
+              SkylarkClassObject.class,
+              null,
+              "%s should be a struct", PYTHON_SKYLARK_PROVIDER_NAME
+      );
 
       if (pythonSkylarkProvider != null) {
         Object sourceFiles = pythonSkylarkProvider.getValue(TRANSITIVE_PYTHON_SRCS);
@@ -472,13 +472,10 @@ public final class PyCommon {
   public static boolean checkForSharedLibraries(Iterable<TransitiveInfoCollection> deps)
           throws EvalException{
     for (TransitiveInfoCollection dep : deps) {
-      SkylarkProviders providers = dep.getProvider(SkylarkProviders.class);
-      SkylarkClassObject provider = null;
-      if (providers != null) {
-        provider = providers.getValue(PYTHON_SKYLARK_PROVIDER_NAME,
-                SkylarkClassObject.class);
-      }
-      if (provider != null) {
+      Object providerObject = dep.get(PYTHON_SKYLARK_PROVIDER_NAME);
+      if (providerObject != null) {
+        SkylarkType.checkType(providerObject, SkylarkClassObject.class, null);
+        SkylarkClassObject provider = (SkylarkClassObject) providerObject;
         Boolean isUsingSharedLibrary = provider.getValue(IS_USING_SHARED_LIBRARY, Boolean.class);
         if (Boolean.TRUE.equals(isUsingSharedLibrary)) {
           return true;