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

9 files changed, 29 insertions, 36 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;