Separate PackageSpecificationProvider from its target (PackageGroupCo…

…nfiguredTarget)

This is the first step towards implementing a public Starlark API for checking if a target exists in an allowlist.

PiperOrigin-RevId: 558726993
Change-Id: I6362e2f28fbbf0927e0438b6c00a9e16bff0ce1d

src/main/java/com/google/devtools/build/lib/analysis/Allowlist.java

@@ -22,7 +22,6 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
-import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.packages.Attribute;
@@ -51,7 +50,7 @@ public static Attribute.Builder<Label> getAttributeFromAllowlistName(String allo
     String attributeName = getAttributeNameFromAllowlistName(allowlistName).iterator().next();
     return attr(attributeName, LABEL)
         .cfg(ExecutionTransitionFactory.createFactory())
-        .mandatoryProviders(PackageGroupConfiguredTarget.PROVIDER.id());
+        .mandatoryBuiltinProviders(ImmutableList.of(PackageSpecificationProvider.class));
   }
 
   /**
@@ -116,7 +115,7 @@ public static PackageSpecificationProvider fetchPackageSpecificationProviderOrNu
       Preconditions.checkArgument(ruleContext.isAttrDefined(attributeName, LABEL), attributeName);
       TransitiveInfoCollection packageGroup = ruleContext.getPrerequisite(attributeName);
       PackageSpecificationProvider packageSpecificationProvider =
-          packageGroup.get(PackageGroupConfiguredTarget.PROVIDER);
+          packageGroup.get(PackageSpecificationProvider.PROVIDER);
       return requireNonNull(packageSpecificationProvider, packageGroup.getLabel().toString());
     }
     return null;

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -106,7 +106,6 @@ java_library(
         ":no_build_event",
         ":no_build_request_finished_event",
         ":options_diff_predicate",
-        ":package_specification_provider",
         ":platform_configuration",
         ":platform_options",
         ":print_action_visitor",
@@ -191,6 +190,7 @@ java_library(
         "LocationExpander.java",
         "LocationTemplateContext.java",
         "OutputGroupInfo.java",
+        "PackageSpecificationProvider.java",
         "PrerequisiteArtifacts.java",
         "PseudoAction.java",
         "RuleConfiguredTargetBuilder.java",
@@ -322,7 +322,6 @@ java_library(
         ":licenses_provider",
         ":make_variable_supplier",
         ":options_diff_predicate",
-        ":package_specification_provider",
         ":platform_configuration",
         ":provider_collection",
         ":repo_mapping_manifest_action",
@@ -886,15 +885,6 @@ java_library(
     ],
 )
 
-java_library(
-    name = "package_specification_provider",
-    srcs = ["PackageSpecificationProvider.java"],
-    deps = [
-        "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
-        "//src/main/java/com/google/devtools/build/lib/packages",
-    ],
-)
-
 java_library(
     name = "platform_configuration",
     srcs = ["PlatformConfiguration.java"],

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java

@@ -139,7 +139,7 @@ private static NestedSet<PackageGroupContents> convertVisibility(
       // ConfiguredTargetGraph, but for now, this is the minimally invasive way of providing a sane
       // error message in case a cycle is created by a visibility attribute.
       if (group != null) {
-        provider = group.get(PackageGroupConfiguredTarget.PROVIDER);
+        provider = group.get(PackageSpecificationProvider.PROVIDER);
       }
       if (provider != null) {
         result.addTransitive(provider.getPackageSpecifications());

src/main/java/com/google/devtools/build/lib/analysis/PackageSpecificationProvider.java

@@ -14,13 +14,80 @@
 
 package com.google.devtools.build.lib.analysis;
 
+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.collect.nestedset.Order;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.packages.BuiltinProvider;
+import com.google.devtools.build.lib.packages.NativeInfo;
+import com.google.devtools.build.lib.packages.PackageGroup;
 import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
+import com.google.devtools.build.lib.packages.Provider;
+import java.util.Optional;
 
 /**
  * A {@link TransitiveInfoProvider} that describes a set of transitive package specifications used
  * in package groups.
  */
-public interface PackageSpecificationProvider {
-  NestedSet<PackageGroupContents> getPackageSpecifications();
+public class PackageSpecificationProvider extends NativeInfo implements TransitiveInfoProvider {
+
+  private static final String STARLARK_NAME = "PackageSpecificationInfo";
+
+  public static final BuiltinProvider<PackageSpecificationProvider> PROVIDER =
+      new BuiltinProvider<>(STARLARK_NAME, PackageSpecificationProvider.class) {};
+
+  public static final PackageSpecificationProvider EMPTY =
+      new PackageSpecificationProvider(NestedSetBuilder.emptySet(Order.STABLE_ORDER));
+
+  private final NestedSet<PackageGroupContents> packageSpecifications;
+
+  private PackageSpecificationProvider(NestedSet<PackageGroupContents> packageSpecifications) {
+    this.packageSpecifications = packageSpecifications;
+  }
+
+  /**
+   * Creates a {@code PackageSpecificationProvider} by initializing transitive package
+   * specifications from {@code targetContext} and {@code packageGroup}.
+   */
+  public static PackageSpecificationProvider create(
+      TargetContext targetContext, PackageGroup packageGroup) {
+    return new PackageSpecificationProvider(getPackageSpecifications(targetContext, packageGroup));
+  }
+
+  @Override
+  public Provider getProvider() {
+    return PROVIDER;
+  }
+
+  /** Returns set of transitive package specifications used in package groups. */
+  public NestedSet<PackageGroupContents> getPackageSpecifications() {
+    return packageSpecifications;
+  }
+
+  private static NestedSet<PackageGroupContents> getPackageSpecifications(
+      TargetContext targetContext, PackageGroup packageGroup) {
+    NestedSetBuilder<PackageGroupContents> builder = NestedSetBuilder.stableOrder();
+    for (Label includeLabel : packageGroup.getIncludes()) {
+      TransitiveInfoCollection include =
+          targetContext.findDirectPrerequisite(
+              includeLabel, Optional.ofNullable(targetContext.getConfiguration()));
+      PackageSpecificationProvider provider = include == null ? null : include.get(PROVIDER);
+      if (provider == null) {
+        targetContext
+            .getAnalysisEnvironment()
+            .getEventHandler()
+            .handle(
+                Event.error(
+                    targetContext.getTarget().getLocation(),
+                    String.format("Label '%s' does not refer to a package group", includeLabel)));
+        continue;
+      }
+
+      builder.addTransitive(provider.getPackageSpecifications());
+    }
+
+    builder.add(packageGroup.getPackageSpecifications());
+    return builder.build();
+  }
 }

src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoProviderMapBuilder.java

@@ -61,12 +61,8 @@ public <T extends TransitiveInfoProvider> TransitiveInfoProviderMapBuilder put(
   @CanIgnoreReturnValue
   public TransitiveInfoProviderMapBuilder put(Info classObject) {
     Preconditions.checkNotNull(classObject);
-    // TODO(bazel-team): VisibilityProvider should be migrated to Info to avoid the
-    //  PackageSpecificationInfo check. Perhaps as part of a wider effort to migrate all native
-    //  TransitiveInfoProviders to Info.
     Preconditions.checkState(
-        !(classObject instanceof TransitiveInfoProvider)
-            || classObject.getProvider().getPrintableName().equals("PackageSpecificationInfo"),
+        !(classObject instanceof TransitiveInfoProvider),
         "Declared provider %s should not implement TransitiveInfoProvider",
         classObject.getClass());
 

src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java

@@ -48,7 +48,7 @@
 public abstract class AbstractConfiguredTarget implements ConfiguredTarget, VisibilityProvider {
   // This should really never be null, but is null in two cases.
   // 1. MergedConfiguredTarget: these are ephemeral and never added to the Skyframe graph.
-  // 2. TestActionBuilder.EmptyPackageProvider: it is used here only to inject an empty
+  // 2. PackageSpecificationProvider.EMPTY: it is used here only to inject an empty
   //    PackageSpecificationProvider.
   // TODO(b/281522692): The existence of these cases suggest that there should be some additional
   // abstraction that does not have a key.

src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/PackageGroupConfiguredTarget.java

@@ -19,20 +19,15 @@
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.PackageSpecificationProvider;
 import com.google.devtools.build.lib.analysis.TargetContext;
-import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 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.concurrent.ThreadSafety.Immutable;
-import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.packages.BuiltinProvider;
 import com.google.devtools.build.lib.packages.BuiltinRestriction;
 import com.google.devtools.build.lib.packages.Info;
 import com.google.devtools.build.lib.packages.PackageGroup;
 import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
 import com.google.devtools.build.lib.packages.Provider;
-import java.util.Optional;
 import javax.annotation.Nullable;
 import net.starlark.java.annot.Param;
 import net.starlark.java.annot.ParamType;
@@ -45,26 +40,16 @@
  * not really first-class Targets.
  */
 @Immutable
-public class PackageGroupConfiguredTarget extends AbstractConfiguredTarget
-    implements PackageSpecificationProvider, Info {
-
-  private final NestedSet<PackageGroupContents> packageSpecifications;
-
-  public static final BuiltinProvider<PackageGroupConfiguredTarget> PROVIDER =
-      new BuiltinProvider<>("PackageSpecificationInfo", PackageGroupConfiguredTarget.class) {};
-
-  // TODO(b/200065655): Only builtins should depend on a PackageGroupConfiguredTarget.
-  //  Allowlists should be migrated to a new rule type that isn't package_group. Do not expose this
-  //  to pure Starlark.
-  @Override
-  public Provider getProvider() {
-    return PROVIDER;
-  }
+public class PackageGroupConfiguredTarget extends AbstractConfiguredTarget {
+  private final PackageSpecificationProvider packageSpecificationProvider;
 
   @Override
   public <P extends TransitiveInfoProvider> P getProvider(Class<P> provider) {
     if (provider == FileProvider.class) {
       return provider.cast(FileProvider.EMPTY); // can't fail
+    }
+    if (provider == PackageSpecificationProvider.class) {
+      return provider.cast(packageSpecificationProvider);
     } else {
       return super.getProvider(provider);
     }
@@ -73,56 +58,23 @@ public <P extends TransitiveInfoProvider> P getProvider(Class<P> provider) {
   public PackageGroupConfiguredTarget(
       ActionLookupKey actionLookupKey,
       NestedSet<PackageGroupContents> visibility,
-      NestedSet<PackageGroupContents> packageSpecifications) {
+      TargetContext targetContext,
+      PackageGroup packageGroup) {
     super(actionLookupKey, visibility);
-    this.packageSpecifications = packageSpecifications;
+    this.packageSpecificationProvider =
+        PackageSpecificationProvider.create(targetContext, packageGroup);
   }
 
   public PackageGroupConfiguredTarget(
       ActionLookupKey actionLookupKey, TargetContext targetContext, PackageGroup packageGroup) {
-    this(
-        actionLookupKey,
-        targetContext.getVisibility(),
-        getPackageSpecifications(targetContext, packageGroup));
-  }
-
-  private static NestedSet<PackageGroupContents> getPackageSpecifications(
-      TargetContext targetContext, PackageGroup packageGroup) {
-    NestedSetBuilder<PackageGroupContents> builder = NestedSetBuilder.stableOrder();
-    for (Label label : packageGroup.getIncludes()) {
-      TransitiveInfoCollection include =
-          targetContext.findDirectPrerequisite(
-              label, Optional.ofNullable(targetContext.getConfiguration()));
-      PackageSpecificationProvider provider =
-          include == null ? null : include.get(PackageGroupConfiguredTarget.PROVIDER);
-      if (provider == null) {
-        targetContext
-            .getAnalysisEnvironment()
-            .getEventHandler()
-            .handle(
-                Event.error(
-                    targetContext.getTarget().getLocation(),
-                    String.format("Label '%s' does not refer to a package group", label)));
-        continue;
-      }
-
-      builder.addTransitive(provider.getPackageSpecifications());
-    }
-
-    builder.add(packageGroup.getPackageSpecifications());
-    return builder.build();
-  }
-
-  @Override
-  public NestedSet<PackageGroupContents> getPackageSpecifications() {
-    return packageSpecifications;
+    this(actionLookupKey, targetContext.getVisibility(), targetContext, packageGroup);
   }
 
   @Override
   @Nullable
   protected Info rawGetStarlarkProvider(Provider.Key providerKey) {
-    if (providerKey.equals(PROVIDER.getKey())) {
-      return this;
+    if (providerKey.equals(packageSpecificationProvider.getProvider().getKey())) {
+      return packageSpecificationProvider;
     }
     return null;
   }
@@ -144,6 +96,6 @@ protected Object rawGetStarlarkProvider(String providerKey) {
       useStarlarkThread = true)
   public boolean starlarkMatches(Label label, StarlarkThread thread) throws EvalException {
     BuiltinRestriction.failIfCalledOutsideBuiltins(thread);
-    return Allowlist.isAvailableFor(getPackageSpecifications(), label);
+    return Allowlist.isAvailableFor(packageSpecificationProvider.getPackageSpecifications(), label);
   }
 }

src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java

@@ -40,15 +40,11 @@
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.actions.LazyWriteNestedSetOfTupleAction;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
-import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget;
 import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams;
 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.PackageSpecification.PackageGroupContents;
 import com.google.devtools.build.lib.packages.TestTimeout;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
@@ -74,24 +70,6 @@ public final class TestActionBuilder {
   private static final String COVERAGE_REPORTED_TO_ACTUAL_SOURCES_FILE =
       "COVERAGE_REPORTED_TO_ACTUAL_SOURCES_FILE";
 
-  static class EmptyPackageProvider extends PackageGroupConfiguredTarget {
-    EmptyPackageProvider() {
-      // TODO(b/281522692): it's not good to pass a null key here.
-      super(
-          /* actionLookupKey= */ null,
-          (NestedSet<PackageGroupContents>) null,
-          (NestedSet<PackageGroupContents>) null);
-    }
-
-    @Override
-    public NestedSet<PackageGroupContents> getPackageSpecifications() {
-      return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
-    }
-  }
-
-  @VisibleForSerialization @SerializationConstant
-  static final PackageSpecificationProvider EMPTY_PACKAGES_PROVIDER = new EmptyPackageProvider();
-
   private final RuleContext ruleContext;
   private final ImmutableList.Builder<Artifact> additionalTools;
   private RunfilesSupport runfilesSupport;
@@ -476,7 +454,7 @@ private TestParams createTestAction(int shards)
                 MoreObjects.firstNonNull(
                     Allowlist.fetchPackageSpecificationProviderOrNull(
                         ruleContext, "external_network"),
-                    EMPTY_PACKAGES_PROVIDER),
+                    PackageSpecificationProvider.EMPTY),
                 ruleContext.isExecutedOnWindows());
 
         testOutputs.addAll(testRunnerAction.getSpawnOutputs());

src/main/java/com/google/devtools/build/lib/bazel/rules/CcRules.java

@@ -16,8 +16,8 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.RuleSet;
+import com.google.devtools.build.lib.analysis.PackageSpecificationProvider;
 import com.google.devtools.build.lib.analysis.StaticallyLinkedMarkerProvider;
-import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget;
 import com.google.devtools.build.lib.bazel.rules.cpp.BazelCcBinaryRule;
 import com.google.devtools.build.lib.bazel.rules.cpp.BazelCcImportRule;
 import com.google.devtools.build.lib.bazel.rules.cpp.BazelCcLibraryRule;
@@ -110,7 +110,7 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
         "PropellerOptimizeInfo", PropellerOptimizeProvider.PROVIDER);
     builder.addStarlarkBuiltinsInternal("MemProfProfileInfo", MemProfProfileProvider.PROVIDER);
     builder.addStarlarkBuiltinsInternal(
-        "PackageSpecificationInfo", PackageGroupConfiguredTarget.PROVIDER);
+        "PackageSpecificationInfo", PackageSpecificationProvider.PROVIDER);
     builder.addStarlarkBuiltinsInternal("cc_common", bazelCcModule);
     builder.addStarlarkBootstrap(
         new CcBootstrap(

src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD

@@ -68,7 +68,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:licenses_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:make_variable_supplier",
-        "//src/main/java/com/google/devtools/build/lib/analysis:package_specification_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_error_consumer",

src/main/java/com/google/devtools/build/lib/rules/java/BUILD

@@ -43,7 +43,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
-        "//src/main/java/com/google/devtools/build/lib/analysis:package_specification_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
         "//src/main/java/com/google/devtools/build/lib/analysis:template_variable_info",
         "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection",
@@ -140,7 +139,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
-        "//src/main/java/com/google/devtools/build/lib/analysis:package_specification_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:platform_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:provider_collection",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",