Do not put OutputGroupProvider into SkylarkProviders.

Almost every target has an OutputGroupProvider. Putting an
("output_groups", value) pair into SkylarkProviders creates an
unneccessary map. This CL removes it.

RELNOTES: None.
PiperOrigin-RevId: 154940624

11 files changed, 89 insertions, 95 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 cb7daee618..cedb87bd32 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
@@ -79,15 +79,6 @@ public abstract class AbstractConfiguredTarget
     return configuration;
   }
 
-  @Nullable
-  @Override
-  public Object get(SkylarkProviderIdentifier id) {
-    if (id.isLegacy()) {
-      return get(id.getLegacyId());
-    }
-    return get(id.getKey());
-  }
-
   @Override
   public Label getLabel() {
     return getTarget().getLabel();
@@ -159,12 +150,22 @@ public abstract class AbstractConfiguredTarget
 
   @Override
   public ImmutableCollection<String> getKeys() {
-    return ImmutableList.of(
-        DATA_RUNFILES_FIELD,
-        DEFAULT_RUNFILES_FIELD,
-        LABEL_FIELD,
-        FILES_FIELD,
-        FilesToRunProvider.SKYLARK_NAME);
+    if (getProvider(OutputGroupProvider.class) != null) {
+      return ImmutableList.of(
+          DATA_RUNFILES_FIELD,
+          DEFAULT_RUNFILES_FIELD,
+          LABEL_FIELD,
+          FILES_FIELD,
+          FilesToRunProvider.SKYLARK_NAME,
+          OutputGroupProvider.SKYLARK_NAME);
+    } else {
+      return ImmutableList.of(
+          DATA_RUNFILES_FIELD,
+          DEFAULT_RUNFILES_FIELD,
+          LABEL_FIELD,
+          FILES_FIELD,
+          FilesToRunProvider.SKYLARK_NAME);
+    }
   }
 
   private DefaultProvider getDefaultProvider() {
@@ -179,6 +180,16 @@ public abstract class AbstractConfiguredTarget
     return defaultProvider.get();
   }
 
+  @Nullable
+  @Override
+  public Object get(SkylarkProviderIdentifier id) {
+    if (id.isLegacy()) {
+      return get(id.getLegacyId());
+    }
+    return get(id.getKey());
+  }
+
+
   /** Returns a declared provider provided by this target. Only meant to use from Skylark. */
   @Nullable
   @Override
@@ -195,4 +206,17 @@ public abstract class AbstractConfiguredTarget
     }
     return null;
   }
+
+  /**
+   * Returns a value provided by this target. Only meant to use from Skylark.
+   */
+  @Override
+  public final Object get(String providerKey) {
+    if (OutputGroupProvider.SKYLARK_NAME.equals(providerKey)) {
+      return getProvider(OutputGroupProvider.class);
+    }
+    SkylarkProviders skylarkProviders = getProvider(SkylarkProviders.class);
+    return skylarkProviders != null ? skylarkProviders.getValue(providerKey) : null;
+  }
+
 }
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 18c00f9156..116df6153d 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
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.packages.ClassObjectConstructor;
 import com.google.devtools.build.lib.packages.ClassObjectConstructor.Key;
 import com.google.devtools.build.lib.packages.SkylarkClassObject;
 import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor;
+import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.util.Preconditions;
 import java.util.Arrays;
@@ -92,6 +93,33 @@ public final class ConfiguredAspect implements Iterable<TransitiveInfoProvider>
     return providers.getProvider(providerClass);
   }
 
+  public Object getProvider(SkylarkProviderIdentifier id) {
+    if (id.isLegacy()) {
+      return get(id.getLegacyId());
+    } else {
+      return get(id.getKey());
+    }
+  }
+
+  public SkylarkClassObject get(ClassObjectConstructor.Key key) {
+    if (OutputGroupProvider.SKYLARK_CONSTRUCTOR.getKey().equals(key)) {
+      return getProvider(OutputGroupProvider.class);
+    }
+    SkylarkProviders skylarkProviders = getProvider(SkylarkProviders.class);
+    return skylarkProviders != null ? skylarkProviders.getDeclaredProvider(key) : null;
+  }
+
+  public Object get(String legacyKey) {
+    if (OutputGroupProvider.SKYLARK_NAME.equals(legacyKey)) {
+      return getProvider(OutputGroupProvider.class);
+    }
+    SkylarkProviders skylarkProviders = getProvider(SkylarkProviders.class);
+    return skylarkProviders != null
+        ? skylarkProviders.get(SkylarkProviderIdentifier.forLegacy(legacyKey))
+        : null;
+  }
+
+
   @Override
   public UnmodifiableIterator<TransitiveInfoProvider> iterator() {
     return providers.values().iterator();
@@ -210,14 +238,23 @@ public final class ConfiguredAspect implements Iterable<TransitiveInfoProvider>
         throw new EvalException(
             constructor.getLocation(), "All providers must be top level values");
       }
-      skylarkDeclaredProvidersBuilder.put(constructor.getKey(), declaredProvider);
+      ClassObjectConstructor.Key key = constructor.getKey();
+      addDeclaredProvider(key, declaredProvider);
       return this;
     }
 
+    private void addDeclaredProvider(Key key, SkylarkClassObject declaredProvider) {
+      if (OutputGroupProvider.SKYLARK_CONSTRUCTOR.getKey().equals(key)) {
+        addProvider(OutputGroupProvider.class, (OutputGroupProvider) declaredProvider);
+      } else {
+        skylarkDeclaredProvidersBuilder.put(key, declaredProvider);
+      }
+    }
+
     public Builder addNativeDeclaredProvider(SkylarkClassObject declaredProvider) {
       ClassObjectConstructor constructor = declaredProvider.getConstructor();
       Preconditions.checkState(constructor.isExported());
-      skylarkDeclaredProvidersBuilder.put(constructor.getKey(), declaredProvider);
+      addDeclaredProvider(constructor.getKey(), declaredProvider);
       return this;
     }
 
@@ -234,8 +271,7 @@ public final class ConfiguredAspect implements Iterable<TransitiveInfoProvider>
           throw new IllegalStateException(
               "OutputGroupProvider was provided explicitly; do not use addOutputGroup");
         }
-        skylarkDeclaredProvidersBuilder.put(
-            OutputGroupProvider.SKYLARK_CONSTRUCTOR.getKey(),
+        addDeclaredProvider(OutputGroupProvider.SKYLARK_CONSTRUCTOR.getKey(),
             new OutputGroupProvider(outputGroups.build()));
       }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index d560923c8d..b1ec35db8b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -388,8 +388,7 @@ public final class ConfiguredTargetFactory {
     }
 
     for (SkylarkProviderIdentifier providerId : advertisedProviders.getSkylarkProviders()) {
-      SkylarkProviders skylarkProviders = configuredAspect.getProvider(SkylarkProviders.class);
-      if (skylarkProviders == null || skylarkProviders.get(providerId) == null) {
+      if (configuredAspect.getProvider(providerId) == null) {
         eventHandler.handle(Event.error(
             target.getLocation(),
             String.format(
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 5d3ca37b32..eb7a1e1a32 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
@@ -35,12 +35,6 @@ public final class EnvironmentGroupConfiguredTarget extends AbstractConfiguredTa
     return (EnvironmentGroup) super.getTarget();
   }
 
-  @Override
-  public Object get(String providerKey) {
-    // No providers.
-    return null;
-  }
-
   @Nullable
   @Override
   public SkylarkClassObject get(ClassObjectConstructor.Key providerKey) {
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 75495da725..7f3f9f6887 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
@@ -77,9 +77,4 @@ public abstract class FileConfiguredTarget extends AbstractConfiguredTarget
     AnalysisUtils.checkProvider(provider);
     return providers.getProvider(provider);
   }
-
-  @Override
-  public Object get(String providerKey) {
-    return null;
-  }
 }
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
index bff4b1cd2c..76223780b2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/MergedConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/MergedConfiguredTarget.java
@@ -15,10 +15,7 @@ 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.devtools.build.lib.packages.SkylarkClassObject;
-import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -50,12 +47,6 @@ public final class MergedConfiguredTarget extends AbstractConfiguredTarget {
     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);
@@ -90,22 +81,8 @@ public final class MergedConfiguredTarget extends AbstractConfiguredTarget {
         OutputGroupProvider.merge(getAllOutputGroupProviders(base, aspects));
 
     // Merge Skylark providers.
-    ImmutableMap<String, Object> premergedLegacyProviders =
-        mergedOutputGroupProvider == null
-            ? ImmutableMap.<String, Object>of()
-            : ImmutableMap.<String, Object>of(
-                OutputGroupProvider.SKYLARK_NAME, mergedOutputGroupProvider);
-
-    ImmutableMap<SkylarkClassObjectConstructor.Key, SkylarkClassObject> premergedProviders =
-        mergedOutputGroupProvider == null
-        ? ImmutableMap.<SkylarkClassObjectConstructor.Key, SkylarkClassObject>of()
-        : ImmutableMap.<SkylarkClassObjectConstructor.Key, SkylarkClassObject>of(
-            OutputGroupProvider.SKYLARK_CONSTRUCTOR.getKey(), mergedOutputGroupProvider);
     SkylarkProviders mergedSkylarkProviders =
-        SkylarkProviders.merge(
-            premergedLegacyProviders,
-            premergedProviders,
-            getAllProviders(base, aspects, SkylarkProviders.class));
+        SkylarkProviders.merge(getAllProviders(base, aspects, SkylarkProviders.class));
 
     // Merge extra-actions provider.
     ExtraActionArtifactsProvider mergedExtraActionProviders = ExtraActionArtifactsProvider.merge(
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupProvider.java
index 0606a1fda4..06bbfdcee2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/OutputGroupProvider.java
@@ -127,17 +127,12 @@ public final class OutputGroupProvider extends SkylarkClassObject
 
   @Nullable
   public static OutputGroupProvider get(TransitiveInfoCollection collection) {
-    return (OutputGroupProvider) collection.getProvider(OutputGroupProvider.class);
+    return collection.getProvider(OutputGroupProvider.class);
   }
 
   @Nullable
   public static OutputGroupProvider get(ConfiguredAspect aspect) {
-    SkylarkProviders skylarkProviders = aspect.getProvider(SkylarkProviders.class);
-
-
-    return skylarkProviders != null
-        ? (OutputGroupProvider) skylarkProviders.getDeclaredProvider(SKYLARK_CONSTRUCTOR.getKey())
-        : null;
+    return (OutputGroupProvider) aspect.get(SKYLARK_CONSTRUCTOR.getKey());
   }
 
 
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 7a3a33a7d9..3e0b855904 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
@@ -67,11 +67,6 @@ public final class PackageGroupConfiguredTarget extends AbstractConfiguredTarget
     return packageSpecifications;
   }
 
-  @Override
-  public Object get(String providerKey) {
-    // No providers.
-    return null;
-  }
 
   @Nullable
   @Override
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 287923673a..54d1e46b1d 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
@@ -100,14 +100,6 @@ public final class RuleConfiguredTarget extends AbstractConfiguredTarget {
     return providers.getProvider(providerClass);
   }
 
-  /**
-   * 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 final Rule getTarget() {
     return (Rule) super.getTarget();
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 16baad7e4c..512f57654b 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
@@ -132,7 +132,6 @@ public final class RuleConfiguredTargetBuilder {
 
       OutputGroupProvider outputGroupProvider = new OutputGroupProvider(outputGroups.build());
       addProvider(OutputGroupProvider.class, outputGroupProvider);
-      addSkylarkTransitiveInfo(OutputGroupProvider.SKYLARK_NAME, outputGroupProvider);
     }
 
     TransitiveInfoProviderMap providers = providersBuilder.build();
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 99a4534ae9..b306b7791b 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
@@ -20,7 +20,6 @@ import com.google.devtools.build.lib.analysis.MergedConfiguredTarget.DuplicateEx
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.ClassObjectConstructor;
 import com.google.devtools.build.lib.packages.SkylarkClassObject;
-import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor;
 import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
 import com.google.devtools.build.lib.rules.SkylarkApiProvider;
 import com.google.devtools.build.lib.syntax.EvalException;
@@ -120,45 +119,34 @@ public final class SkylarkProviders implements TransitiveInfoProvider {
   /**
    * Merges skylark providers. The set of providers must be disjoint.
    *
-   * @param premergedProviders providers that has already been merged. They will
-   *        be put into the result as-is and their presence will be ignored among {@code providers}.
    * @param providers providers to merge {@code this} with.
    */
-  public static SkylarkProviders merge(
-      ImmutableMap<String, Object> premergedLegacyProviders,
-      ImmutableMap<SkylarkClassObjectConstructor.Key, SkylarkClassObject> premergedProviders,
-      List<SkylarkProviders> providers)
+  public static SkylarkProviders merge(List<SkylarkProviders> providers)
       throws DuplicateException {
-    if (premergedProviders.size() == 0 && providers.size() == 0) {
+    if (providers.size() == 0) {
       return null;
     }
-    if (premergedProviders.size() == 0 && providers.size() == 1) {
+    if (providers.size() == 1) {
       return providers.get(0);
     }
 
     ImmutableMap<String, Object> skylarkProviders = mergeMaps(providers,
-        SKYLARK_PROVIDERS_MAP_FUNCTION,
-        premergedLegacyProviders);
+        SKYLARK_PROVIDERS_MAP_FUNCTION);
 
     ImmutableMap<ClassObjectConstructor.Key, SkylarkClassObject> declaredProviders =
-        mergeMaps(providers, DECLARED_PROVIDERS_MAP_FUNCTION,
-            premergedProviders);
+        mergeMaps(providers, DECLARED_PROVIDERS_MAP_FUNCTION);
 
     return new SkylarkProviders(skylarkProviders, declaredProviders);
   }
 
   private static <K, V> ImmutableMap<K, V> mergeMaps(List<SkylarkProviders> providers,
-      Function<SkylarkProviders, Map<K, V>> mapGetter, Map<K, V> premerged)
+      Function<SkylarkProviders, Map<K, V>> mapGetter)
       throws DuplicateException {
     Set<K> seenKeys = new HashSet<>();
     ImmutableMap.Builder<K, V> resultBuilder = ImmutableMap.builder();
-    resultBuilder.putAll(premerged);
     for (SkylarkProviders provider : providers) {
       Map<K, V> map = mapGetter.apply(provider);
       for (K key : map.keySet()) {
-        if (premerged.containsKey(key)) {
-          continue;
-        }
         if (!seenKeys.add(key)) {
           // TODO(dslomov): add better diagnostics.
           throw new DuplicateException("Provider " + key + " provided twice");