Push some Package.Builder settings into the constructor

This makes the generatorMap, configSettingVisibilityPolicy, and globber into Builder() constructor args, instead of setter mutations. These kinds of settings are always known before BUILD file evaluation begins, so it's best to bake this guarantee into the builder's API. This makes it slightly easier to follow the rest of the builder's logic.

setFilename() will be done in a follow-up. We may or may not push setLoads() up in this manner.

This does make the builder's constructor signature longer, but I think that's preferable to mixing initial setup mutations with actual package construction mutations. One way to address this might be to have the caller supply the Package.Metadata object to the builder constructor, but first we'd have to migrate off Metadata the fields that are determined through BUILD file evaluation.

Work toward #19922.

PiperOrigin-RevId: 595407792
Change-Id: I5ed2b07b631fdbbf393a2095acd342a6e53d3c25

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -739,7 +739,10 @@ public static Builder newExternalPackageBuilder(
             mainRepoMapping,
             mainRepoMapping,
             /* cpuBoundSemaphore= */ null,
-            packageOverheadEstimator)
+            packageOverheadEstimator,
+            /* generatorMap= */ null,
+            /* configSettingVisibilityPolicy= */ null,
+            /* globber= */ null)
         .setFilename(workspacePath);
   }
 
@@ -761,7 +764,10 @@ public static Builder newExternalPackageBuilderForBzlmod(
             // seen by query, the particular value does not matter.
             RepositoryMapping.ALWAYS_FALLBACK,
             /* cpuBoundSemaphore= */ null,
-            PackageOverheadEstimator.NOOP_ESTIMATOR)
+            PackageOverheadEstimator.NOOP_ESTIMATOR,
+            /* generatorMap= */ null,
+            /* configSettingVisibilityPolicy= */ null,
+            /* globber= */ null)
         .setFilename(moduleFilePath)
         .setLoads(ImmutableList.of());
   }
@@ -878,7 +884,7 @@ default boolean precomputeTransitiveLoads() {
 
     // Used by glob(). Null for contexts where glob() is disallowed, including WORKSPACE files and
     // some tests.
-    @Nullable private Globber globber = null;
+    @Nullable private final Globber globber;
 
     // All targets added to the package. We use SnapshottableBiMap to help track insertion order of
     // Rule targets, for use by native.existing_rules().
@@ -923,7 +929,7 @@ default boolean precomputeTransitiveLoads() {
 
     private final Interner<ImmutableList<?>> listInterner = new ThreadCompatibleInterner<>();
 
-    private ImmutableMap<Location, String> generatorMap = ImmutableMap.of();
+    private final ImmutableMap<Location, String> generatorMap;
 
     private final TestSuiteImplicitTestsAccumulator testSuiteImplicitTestsAccumulator =
         new TestSuiteImplicitTestsAccumulator();
@@ -934,14 +940,6 @@ String getGeneratorNameByLocation(Location loc) {
       return generatorMap.get(loc);
     }
 
-    /** Sets the package's map of "generator_name" values keyed by the location of the call site. */
-    // TODO(#19922): Require this to be set before BUILD evaluation.
-    @CanIgnoreReturnValue
-    public Builder setGeneratorMap(ImmutableMap<Location, String> map) {
-      this.generatorMap = map;
-      return this;
-    }
-
     /**
      * Returns the value to use for {@code test_suite}s' {@code $implicit_tests} attribute, as-is,
      * when the {@code test_suite} doesn't specify an explicit, non-empty {@code tests} value. The
@@ -981,14 +979,20 @@ public T intern(T sample) {
         // TODO(#19922): Spurious parameter, delete.
         RepositoryMapping mainRepositoryMapping,
         @Nullable Semaphore cpuBoundSemaphore,
-        PackageOverheadEstimator packageOverheadEstimator) {
+        PackageOverheadEstimator packageOverheadEstimator,
+        @Nullable ImmutableMap<Location, String> generatorMap,
+        // TODO(bazel-team): Config policy is an enum, what is null supposed to mean?
+        // Maybe convert null -> LEGACY_OFF, assuming that's the correct default.
+        @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy,
+        @Nullable Globber globber) {
       Metadata metadata = new Metadata();
       metadata.packageIdentifier = Preconditions.checkNotNull(id);
       metadata.workspaceName = Preconditions.checkNotNull(workspaceName);
       metadata.repositoryMapping = Preconditions.checkNotNull(repositoryMapping);
       metadata.associatedModuleName = Preconditions.checkNotNull(associatedModuleName);
       metadata.associatedModuleVersion = Preconditions.checkNotNull(associatedModuleVersion);
       metadata.succinctTargetNotFoundErrors = packageSettings.succinctTargetNotFoundErrors();
+      metadata.configSettingVisibilityPolicy = configSettingVisibilityPolicy;
 
       this.pkg = new Package(metadata);
       this.precomputeTransitiveLoads = packageSettings.precomputeTransitiveLoads();
@@ -999,6 +1003,8 @@ public T intern(T sample) {
       }
       this.cpuBoundSemaphore = cpuBoundSemaphore;
       this.packageOverheadEstimator = packageOverheadEstimator;
+      this.generatorMap = (generatorMap == null) ? ImmutableMap.of() : generatorMap;
+      this.globber = globber;
     }
 
     PackageIdentifier getPackageIdentifier() {
@@ -1146,14 +1152,6 @@ public PackageArgs getPartialPackageArgs() {
       return pkg.metadata.packageArgs;
     }
 
-    /** Sets visibility enforcement policy for <code>config_setting</code>. */
-    // TODO(#19922): Require this to be set before BUILD evaluation.
-    @CanIgnoreReturnValue
-    public Builder setConfigSettingVisibilityPolicy(ConfigSettingVisibilityPolicy policy) {
-      this.pkg.metadata.configSettingVisibilityPolicy = policy;
-      return this;
-    }
-
     /** Uses the workspace name from {@code //external} to set this package's workspace name. */
     // TODO(#19922): Seems like this is only used for WORKSPACE logic (`workspace()` callable), but
     // it clashes with the notion that, for BUILD files, the workspace name should be supplied to
@@ -1272,15 +1270,6 @@ private void checkLoadsNotSet() {
           pkg.metadata.transitiveLoads);
     }
 
-    /** Sets the {@link Globber}. */
-    // TODO(#19922): Move this to Builder() constructor.
-    @CanIgnoreReturnValue
-    public Builder setGlobber(Globber globber) {
-      Preconditions.checkState(this.globber == null);
-      this.globber = globber;
-      return this;
-    }
-
     /**
      * Returns the {@link Globber} used to implement {@code glob()} functionality during BUILD
      * evaluation. Null for contexts where globbing is not possible, including WORKSPACE files and
@@ -2009,7 +1998,7 @@ public RepositoryMapping getRepositoryMapping() {
 
     private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;
 
-    /** Returns the visibility policy. */
+    /** Returns the visibility enforcement policy for {@code config_setting}. */
     public ConfigSettingVisibilityPolicy getConfigSettingVisibilityPolicy() {
       return configSettingVisibilityPolicy;
     }

src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java

@@ -29,6 +29,7 @@
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.Globber.BadGlobException;
 import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings;
+import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
 import com.google.devtools.build.lib.packages.PackageValidator.InvalidPackageException;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.profiler.Profiler;
@@ -251,7 +252,10 @@ public Package.Builder newPackageBuilder(
       StarlarkSemantics starlarkSemantics,
       RepositoryMapping repositoryMapping,
       RepositoryMapping mainRepositoryMapping,
-      @Nullable Semaphore cpuBoundSemaphore) {
+      @Nullable Semaphore cpuBoundSemaphore,
+      @Nullable ImmutableMap<Location, String> generatorMap,
+      @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy,
+      @Nullable Globber globber) {
     return new Package.Builder(
         packageSettings,
         packageId,
@@ -262,7 +266,10 @@ public Package.Builder newPackageBuilder(
         repositoryMapping,
         mainRepositoryMapping,
         cpuBoundSemaphore,
-        packageOverheadEstimator);
+        packageOverheadEstimator,
+        generatorMap,
+        configSettingVisibilityPolicy,
+        globber);
   }
 
   /** Returns a new {@link NonSkyframeGlobber}. */

src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java

@@ -1415,10 +1415,11 @@ private LoadedPackage loadPackage(
                   starlarkBuiltinsValue.starlarkSemantics,
                   repositoryMapping,
                   mainRepositoryMappingValue.getRepositoryMapping(),
-                  cpuBoundSemaphore.get())
-              .setFilename(buildFileRootedPath)
-              .setConfigSettingVisibilityPolicy(configSettingVisibilityPolicy)
-              .setGlobber(globber);
+                  cpuBoundSemaphore.get(),
+                  /* (Nullable) */ compiled.generatorMap,
+                  configSettingVisibilityPolicy,
+                  globber)
+              .setFilename(buildFileRootedPath);
 
       pkgBuilder
           .mergePackageArgsFrom(PackageArgs.builder().setDefaultVisibility(defaultVisibility))
@@ -1428,8 +1429,6 @@ private LoadedPackage loadPackage(
 
       // OK to execute BUILD program?
       if (compiled.ok()) {
-        pkgBuilder.setGeneratorMap(compiled.generatorMap);
-
         packageFactory.executeBuildFile(
             pkgBuilder,
             compiled.prog,

src/test/java/com/google/devtools/build/lib/packages/PackageTest.java

@@ -169,7 +169,10 @@ private Package.Builder pkgBuilder(String name) {
             RepositoryMapping.ALWAYS_FALLBACK,
             RepositoryMapping.ALWAYS_FALLBACK,
             /* cpuBoundSemaphore= */ null,
-            PackageOverheadEstimator.NOOP_ESTIMATOR);
+            PackageOverheadEstimator.NOOP_ESTIMATOR,
+            /* generatorMap= */ null,
+            /* configSettingVisibilityPolicy= */ null,
+            /* globber= */ null);
     result.setFilename(
         RootedPath.toRootedPath(
             Root.fromPath(fileSystem.getPath("/irrelevantRoot")), PathFragment.create(name)));

src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java

@@ -261,7 +261,10 @@ private Package.Builder createDummyPackageBuilder() {
             StarlarkSemantics.DEFAULT,
             RepositoryMapping.ALWAYS_FALLBACK,
             RepositoryMapping.ALWAYS_FALLBACK,
-            /* cpuBoundSemaphore= */ null)
+            /* cpuBoundSemaphore= */ null,
+            /* generatorMap= */ null,
+            /* configSettingVisibilityPolicy= */ null,
+            /* globber= */ null)
         .setFilename(RootedPath.toRootedPath(root, testBuildfilePath));
   }
 

src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java

@@ -65,7 +65,10 @@ private Package.Builder newBuilder(PackageIdentifier id, Path filename) {
             StarlarkSemantics.DEFAULT,
             RepositoryMapping.ALWAYS_FALLBACK,
             RepositoryMapping.ALWAYS_FALLBACK,
-            /* cpuBoundSemaphore= */ null)
+            /* cpuBoundSemaphore= */ null,
+            /* generatorMap= */ null,
+            /* configSettingVisibilityPolicy= */ null,
+            /* globber= */ null)
         .setFilename(RootedPath.toRootedPath(root, filename))
         .setLoads(ImmutableList.of());
   }