Remove separate eventHandler arg to createRule()-like methods

This is made possible by the change in d8d9078 to have Package.Builder own its own eventHandler.

Also delete an unused mainRepositoryMapping param of Package.Builder's constructor, which was made obsolete by ef98ef9.

RuleClass.java
- createRule(): delete eventHandler param, always use the Builder's eventHandler
- createRuleUnchecked(): possibly write events to the Builder's eventHandler that were previously discarded -- maybe not a no-op, though since this is an internal method hopefully it's not being used in ways where this would matter.
- update various signatures to not take the separate handler

Rule.java
- Narrow arg type from Builder to PackageIdentifier

BzlmodRepoRuleCreator.java
- event handler might now get events from Builder.build(), not just from createAndAddRule(). I'm not expecting this to matter.

WorkspaceFactoryHelper.java
- addBindRule() no longer discards events

Work toward #19922.

PiperOrigin-RevId: 595462762
Change-Id: I5bd3209aaeb3030a6e8d18d193294e7d3c356b23

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -1147,9 +1147,6 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
             thread
                 .getSemantics()
                 .getBool(BuildLanguageOptions.INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES),
-            // TODO(#19922): Delete this arg once it's definitely redundant (when the builder owns
-            // its eventHandler).
-            pkgBuilder.getLocalEventHandler(),
             thread.getCallStack());
       } catch (InvalidRuleException | NameConflictException e) {
         throw new EvalException(e);

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleCreator.java

@@ -77,17 +77,20 @@ public static Rule createRule(
     Rule rule;
     try {
       rule =
-          RuleFactory.createAndAddRule(
-              packageBuilder, ruleClass, attributeValues, true, eventHandler, callStack);
+          RuleFactory.createAndAddRule(packageBuilder, ruleClass, attributeValues, true, callStack);
+      if (rule.containsErrors()) {
+        throw Starlark.errorf(
+            "failed to instantiate '%s' from this module extension", ruleClass.getName());
+      }
+      packageBuilder.build();
     } catch (NameConflictException e) {
       // This literally cannot happen -- we just created the package!
       throw new IllegalStateException(e);
+    } finally {
+      // Make sure we propagate any errors reported by the rule,
+      // from the builder to the event handler.
+      packageBuilder.getLocalEventHandler().replayOn(eventHandler);
     }
-    if (rule.containsErrors()) {
-      throw Starlark.errorf(
-          "failed to instantiate '%s' from this module extension", ruleClass.getName());
-    }
-    packageBuilder.build();
     return rule;
   }
 }

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

@@ -737,8 +737,7 @@ public static Builder newExternalPackageBuilder(
         /* associatedModuleName= */ Optional.empty(),
         /* associatedModuleVersion= */ Optional.empty(),
         noImplicitFileExport,
-        mainRepoMapping,
-        mainRepoMapping,
+        /* repositoryMapping= */ mainRepoMapping,
         /* cpuBoundSemaphore= */ null,
         packageOverheadEstimator,
         /* generatorMap= */ null,
@@ -760,10 +759,6 @@ public static Builder newExternalPackageBuilderForBzlmod(
             /* associatedModuleVersion= */ Optional.empty(),
             noImplicitFileExport,
             repoMapping,
-            // This mapping is *not* the main repository's mapping, but since it is only used to
-            // construct a query command in an error message and the package built here can't be
-            // seen by query, the particular value does not matter.
-            RepositoryMapping.ALWAYS_FALLBACK,
             /* cpuBoundSemaphore= */ null,
             PackageOverheadEstimator.NOOP_ESTIMATOR,
             /* generatorMap= */ null,
@@ -977,8 +972,6 @@ public T intern(T sample) {
         Optional<String> associatedModuleVersion,
         boolean noImplicitFileExport,
         RepositoryMapping repositoryMapping,
-        // TODO(#19922): Spurious parameter, delete.
-        RepositoryMapping mainRepositoryMapping,
         @Nullable Semaphore cpuBoundSemaphore,
         PackageOverheadEstimator packageOverheadEstimator,
         @Nullable ImmutableMap<Location, String> generatorMap,

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

@@ -252,7 +252,6 @@ public Package.Builder newPackageBuilder(
       Optional<String> associatedModuleVersion,
       StarlarkSemantics starlarkSemantics,
       RepositoryMapping repositoryMapping,
-      RepositoryMapping mainRepositoryMapping,
       @Nullable Semaphore cpuBoundSemaphore,
       @Nullable ImmutableMap<Location, String> generatorMap,
       @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy,
@@ -266,7 +265,6 @@ public Package.Builder newPackageBuilder(
         associatedModuleVersion,
         starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
         repositoryMapping,
-        mainRepositoryMapping,
         cpuBoundSemaphore,
         packageOverheadEstimator,
         generatorMap,

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

@@ -915,11 +915,11 @@ void checkValidityPredicate(EventHandler eventHandler) {
    * Collects the output files (both implicit and explicit). Must be called before the output
    * accessors methods can be used, and must be called only once.
    */
-  void populateOutputFiles(EventHandler eventHandler, Package.Builder pkgBuilder)
+  void populateOutputFiles(EventHandler eventHandler, PackageIdentifier pkgId)
       throws LabelSyntaxException, InterruptedException {
     populateOutputFilesInternal(
         eventHandler,
-        pkgBuilder.getPackageIdentifier(),
+        pkgId,
         ruleClass.getDefaultImplicitOutputsFunction(),
         /* checkLabels= */ true);
   }

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

@@ -39,7 +39,6 @@
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.events.NullEventHandler;
 import com.google.devtools.build.lib.packages.Attribute.ComputedDefault;
 import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate;
 import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate.CannotPrecomputeDefaultsException;
@@ -2117,15 +2116,14 @@ <T> Rule createRule(
       Label ruleLabel,
       AttributeValues<T> attributeValues,
       boolean failOnUnknownAttributes,
-      // TODO(#19922): elim eventHandler param, it's redundant with pkgBuilder
-      EventHandler eventHandler,
       List<StarlarkThread.CallStackEntry> callstack)
       throws LabelSyntaxException, InterruptedException, CannotPrecomputeDefaultsException {
+    EventHandler eventHandler = pkgBuilder.getLocalEventHandler();
+
     Rule rule = pkgBuilder.createRule(ruleLabel, this, callstack);
-    populateRuleAttributeValues(
-        rule, pkgBuilder, attributeValues, failOnUnknownAttributes, eventHandler);
+    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, failOnUnknownAttributes);
     checkAspectAllowedValues(rule, eventHandler);
-    rule.populateOutputFiles(eventHandler, pkgBuilder);
+    rule.populateOutputFiles(eventHandler, pkgBuilder.getPackageIdentifier());
     checkForDuplicateLabels(rule, eventHandler);
 
     checkForValidSizeAndTimeoutValues(rule, eventHandler);
@@ -2146,7 +2144,7 @@ <T> Rule createRuleUnchecked(
       ImplicitOutputsFunction implicitOutputsFunction)
       throws InterruptedException, CannotPrecomputeDefaultsException {
     Rule rule = pkgBuilder.createRule(ruleLabel, this, callstack.toLocation(), callstack.next());
-    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, true, NullEventHandler.INSTANCE);
+    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, true);
     rule.populateOutputFilesUnchecked(pkgBuilder, implicitOutputsFunction);
     return rule;
   }
@@ -2162,8 +2160,7 @@ private <T> void populateRuleAttributeValues(
       Rule rule,
       Package.Builder pkgBuilder,
       AttributeValues<T> attributeValues,
-      boolean failOnUnknownAttributes,
-      EventHandler eventHandler)
+      boolean failOnUnknownAttributes)
       throws InterruptedException, CannotPrecomputeDefaultsException {
 
     BitSet definedAttrIndices =
@@ -2173,8 +2170,8 @@ private <T> void populateRuleAttributeValues(
             attributeValues,
             failOnUnknownAttributes,
             pkgBuilder.getListInterner(),
-            eventHandler);
-    populateDefaultRuleAttributeValues(rule, pkgBuilder, definedAttrIndices, eventHandler);
+            pkgBuilder.getLocalEventHandler());
+    populateDefaultRuleAttributeValues(rule, pkgBuilder, definedAttrIndices);
     // Now that all attributes are bound to values, collect and store configurable attribute keys.
     populateConfigDependenciesAttribute(rule);
   }
@@ -2285,7 +2282,7 @@ private <T> BitSet populateDefinedRuleAttributeValues(
    * <p>Errors are reported on {@code eventHandler}.
    */
   private void populateDefaultRuleAttributeValues(
-      Rule rule, Package.Builder pkgBuilder, BitSet definedAttrIndices, EventHandler eventHandler)
+      Rule rule, Package.Builder pkgBuilder, BitSet definedAttrIndices)
       throws InterruptedException, CannotPrecomputeDefaultsException {
     // Set defaults; ensure that every mandatory attribute has a value. Use the default if none
     // is specified.
@@ -2300,10 +2297,8 @@ private void populateDefaultRuleAttributeValues(
         rule.reportError(
             String.format(
                 "%s: missing value for mandatory attribute '%s' in '%s' rule",
-                rule.getLabel(),
-                attr.getName(),
-                name),
-            eventHandler);
+                rule.getLabel(), attr.getName(), name),
+            pkgBuilder.getLocalEventHandler());
       }
 
       // We must check both the name and the type of each attribute below in case a Starlark rule
@@ -2397,7 +2392,7 @@ private void populateDefaultRuleAttributeValues(
       Object defaultValue = attr.getDefaultValue(null);
       if (defaultValue instanceof StarlarkComputedDefaultTemplate) {
         StarlarkComputedDefaultTemplate template = (StarlarkComputedDefaultTemplate) defaultValue;
-        valueToSet = template.computePossibleValues(attr, rule, eventHandler);
+        valueToSet = template.computePossibleValues(attr, rule, pkgBuilder.getLocalEventHandler());
       } else if (defaultValue instanceof ComputedDefault) {
         // Compute all possible values to verify that the ComputedDefault is well-defined. This was
         // previously done implicitly as part of visiting all labels to check for null-ness in

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

@@ -19,7 +19,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
-import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate.CannotPrecomputeDefaultsException;
 import com.google.devtools.build.lib.packages.Package.NameConflictException;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
@@ -56,8 +55,6 @@ public static Rule createRule(
       RuleClass ruleClass,
       BuildLangTypedAttributeValuesMap attributeValues,
       boolean failOnUnknownAttributes,
-      // TODO(#19922): elim eventHandler param, it's redundant with pkgBuilder
-      EventHandler eventHandler,
       ImmutableList<StarlarkThread.CallStackEntry> callstack)
       throws InvalidRuleException, InterruptedException {
     Preconditions.checkNotNull(ruleClass);
@@ -95,7 +92,7 @@ public static Rule createRule(
 
     try {
       return ruleClass.createRule(
-          pkgBuilder, label, attributes, failOnUnknownAttributes, eventHandler, callstack);
+          pkgBuilder, label, attributes, failOnUnknownAttributes, callstack);
     } catch (LabelSyntaxException | CannotPrecomputeDefaultsException e) {
       throw new RuleFactory.InvalidRuleException(ruleClass + " " + e.getMessage());
     }
@@ -126,17 +123,10 @@ public static Rule createAndAddRule(
       RuleClass ruleClass,
       BuildLangTypedAttributeValuesMap attributeValues,
       boolean failOnUnknownAttributes,
-      EventHandler eventHandler,
       ImmutableList<StarlarkThread.CallStackEntry> callstack)
       throws InvalidRuleException, NameConflictException, InterruptedException {
     Rule rule =
-        createRule(
-            pkgBuilder,
-            ruleClass,
-            attributeValues,
-            failOnUnknownAttributes,
-            eventHandler,
-            callstack);
+        createRule(pkgBuilder, ruleClass, attributeValues, failOnUnknownAttributes, callstack);
     pkgBuilder.addRule(rule);
     return rule;
   }
@@ -300,8 +290,6 @@ public NoneType call(StarlarkThread thread, Tuple args, Dict<String, Object> kwa
             thread
                 .getSemantics()
                 .getBool(BuildLanguageOptions.INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES),
-            // TODO(#19922): createAndAddRule() should get this from the builder arg directly.
-            pkgBuilder.getLocalEventHandler(),
             thread.getCallStack());
       } catch (RuleFactory.InvalidRuleException | Package.NameConflictException e) {
         throw new EvalException(e);

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

@@ -200,7 +200,7 @@ public void setParent(
                 rule.getLocation(),
                 rule.getInteriorCallStack());
         newRule.copyAttributesFrom(rule);
-        newRule.populateOutputFiles(NullEventHandler.INSTANCE, builder);
+        newRule.populateOutputFiles(NullEventHandler.INSTANCE, builder.getPackageIdentifier());
         if (rule.containsErrors()) {
           newRule.setContainsErrors();
         }

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

@@ -24,7 +24,6 @@
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.cmdline.TargetPattern;
-import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.util.Map;
@@ -55,14 +54,7 @@ public static Rule createAndAddRepositoryRule(
           LabelSyntaxException,
           InterruptedException {
     BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
-    Rule rule =
-        RuleFactory.createRule(
-            pkgBuilder,
-            ruleClass,
-            attributeValues,
-            true,
-            pkgBuilder.getLocalEventHandler(),
-            callstack);
+    Rule rule = RuleFactory.createRule(pkgBuilder, ruleClass, attributeValues, true, callstack);
     overwriteRule(pkgBuilder, rule);
     for (Map.Entry<String, Label> entry :
         ruleClass.getExternalBindingsFunction().apply(rule).entrySet()) {
@@ -172,11 +164,9 @@ static void addBindRule(
     if (actual != null) {
       attributes.put("actual", actual);
     }
-    StoredEventHandler handler = new StoredEventHandler();
     BuildLangTypedAttributeValuesMap attributeValues =
         new BuildLangTypedAttributeValuesMap(attributes);
-    Rule rule =
-        RuleFactory.createRule(pkg, bindRuleClass, attributeValues, true, handler, callstack);
+    Rule rule = RuleFactory.createRule(pkg, bindRuleClass, attributeValues, true, callstack);
     overwriteRule(pkg, rule);
   }
 

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

@@ -33,7 +33,6 @@
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
-import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.io.FileSymlinkException;
 import com.google.devtools.build.lib.io.InconsistentFilesystemException;
@@ -1242,8 +1241,6 @@ private LoadedPackage loadPackage(
     RepositoryMappingValue repositoryMappingValue =
         (RepositoryMappingValue)
             env.getValue(RepositoryMappingValue.key(packageId.getRepository()));
-    RepositoryMappingValue mainRepositoryMappingValue =
-        (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN));
     RootedPath buildFileRootedPath = packageLookupValue.getRootedPath(packageId);
     FileValue buildFileValue = getBuildFileValue(env, buildFileRootedPath);
     RuleVisibility defaultVisibility = PrecomputedValue.DEFAULT_VISIBILITY.get(env);
@@ -1414,7 +1411,6 @@ private LoadedPackage loadPackage(
               repositoryMappingValue.getAssociatedModuleVersion(),
               starlarkBuiltinsValue.starlarkSemantics,
               repositoryMapping,
-              mainRepositoryMappingValue.getRepositoryMapping(),
               cpuBoundSemaphore.get(),
               /* (Nullable) */ compiled.generatorMap,
               configSettingVisibilityPolicy,

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

@@ -167,8 +167,7 @@ private Package.Builder pkgBuilder(String name) {
         Optional.empty(),
         Optional.empty(),
         /* noImplicitFileExport= */ true,
-        RepositoryMapping.ALWAYS_FALLBACK,
-        RepositoryMapping.ALWAYS_FALLBACK,
+        /* repositoryMapping= */ RepositoryMapping.ALWAYS_FALLBACK,
         /* cpuBoundSemaphore= */ null,
         PackageOverheadEstimator.NOOP_ESTIMATOR,
         /* generatorMap= */ null,
@@ -179,7 +178,7 @@ private Package.Builder pkgBuilder(String name) {
   private static Rule addRule(Package.Builder pkgBuilder, Label label, RuleClass ruleClass)
       throws Exception {
     Rule rule = pkgBuilder.createRule(label, ruleClass, /* callstack= */ ImmutableList.of());
-    rule.populateOutputFiles(new StoredEventHandler(), pkgBuilder);
+    rule.populateOutputFiles(new StoredEventHandler(), pkgBuilder.getPackageIdentifier());
     pkgBuilder.addRule(rule);
     return rule;
   }

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

@@ -259,8 +259,7 @@ private Package.Builder createDummyPackageBuilder() {
         Optional.empty(),
         Optional.empty(),
         StarlarkSemantics.DEFAULT,
-        RepositoryMapping.ALWAYS_FALLBACK,
-        RepositoryMapping.ALWAYS_FALLBACK,
+        /* repositoryMapping= */ RepositoryMapping.ALWAYS_FALLBACK,
         /* cpuBoundSemaphore= */ null,
         /* generatorMap= */ null,
         /* configSettingVisibilityPolicy= */ null,
@@ -932,14 +931,16 @@ private Rule createRule(RuleClass ruleClass, String name, Map<String, Object> at
     } catch (LabelSyntaxException e) {
       throw new IllegalArgumentException("Rule has illegal label", e);
     }
-    return ruleClass.createRule(
-        pkgBuilder,
-        ruleLabel,
-        new BuildLangTypedAttributeValuesMap(attributeValues),
-        true,
-        reporter,
-        ImmutableList.of(
-            StarlarkThread.callStackEntry(StarlarkThread.TOP_LEVEL, testRuleLocation)));
+    Rule rule =
+        ruleClass.createRule(
+            pkgBuilder,
+            ruleLabel,
+            new BuildLangTypedAttributeValuesMap(attributeValues),
+            true,
+            ImmutableList.of(
+                StarlarkThread.callStackEntry(StarlarkThread.TOP_LEVEL, testRuleLocation)));
+    pkgBuilder.getLocalEventHandler().replayOn(reporter);
+    return rule;
   }
 
   @Test