Simplify TargetRecorder API for disabling name conflict checking

Name conflict checking is disabled for package deserialization but enabled in all other contexts where we are building a `Package` object. Previously, this disabling was done by a stateful mutation on the `Package.Builder`, with associated precondition checks to confirm that this mutation did or did not occur before calling certain methods, and that this mutation was only attempted at most once.

This is a bit convoluted and is the result of my original attempt to understand `Package.Builder`'s sprawling API (now part of `TargetRecorder`). It's clear though that this can be better expressed as a constructor arg, removing the need for the extra consistency checking.

Also updated some javadoc to remove mention of fields being nullified after package construction -- that was removed in 8977891. The field `macroNamespaceViolatingTargets` is also now never null -- even during package deserialization it still eventually gets a value due to the `putAll...()` method, so we may as well initialize it to a collection object. With these changes, the fields in question are now able to be marked `final`.

Drive-by cleanup during #19922.

PiperOrigin-RevId: 681065954
Change-Id: I060ae2fbaca9589ed95a45568127538a6aafe537

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

@@ -882,7 +882,8 @@ public static Builder newPackageBuilder(
       @Nullable ImmutableMap<Location, String> generatorMap,
       // TODO(bazel-team): See Builder() constructor comment about use of null for this param.
       @Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy,
-      @Nullable Globber globber) {
+      @Nullable Globber globber,
+      boolean enableNameConflictChecking) {
     // Determine whether this is for a repo rule package. We shouldn't actually have to do this
     // because newPackageBuilder() is supposed to only be called for normal packages. Unfortunately
     // serialization still uses the same code path for deserializing BUILD and WORKSPACE files,
@@ -912,7 +913,8 @@ public static Builder newPackageBuilder(
         cpuBoundSemaphore,
         packageOverheadEstimator,
         generatorMap,
-        globber);
+        globber,
+        enableNameConflictChecking);
   }
 
   public static Builder newExternalPackageBuilder(
@@ -944,7 +946,8 @@ public static Builder newExternalPackageBuilder(
         /* cpuBoundSemaphore= */ null,
         packageOverheadEstimator,
         /* generatorMap= */ null,
-        /* globber= */ null);
+        /* globber= */ null,
+        /* enableNameConflictChecking= */ true);
   }
 
   public static Builder newExternalPackageBuilderForBzlmod(
@@ -973,7 +976,8 @@ public static Builder newExternalPackageBuilderForBzlmod(
             /* cpuBoundSemaphore= */ null,
             PackageOverheadEstimator.NOOP_ESTIMATOR,
             /* generatorMap= */ null,
-            /* globber= */ null)
+            /* globber= */ null,
+            /* enableNameConflictChecking= */ true)
         .setLoads(ImmutableList.of());
   }
 
@@ -1090,7 +1094,8 @@ private Builder(
         @Nullable Semaphore cpuBoundSemaphore,
         PackageOverheadEstimator packageOverheadEstimator,
         @Nullable ImmutableMap<Location, String> generatorMap,
-        @Nullable Globber globber) {
+        @Nullable Globber globber,
+        boolean enableNameConflictChecking) {
       super(
           metadata,
           new Package(metadata),
@@ -1100,7 +1105,8 @@ private Builder(
           mainRepositoryMapping,
           cpuBoundSemaphore,
           generatorMap,
-          globber);
+          globber,
+          enableNameConflictChecking);
       this.precomputeTransitiveLoads = precomputeTransitiveLoads;
       this.noImplicitFileExport = noImplicitFileExport;
       this.packageOverheadEstimator = packageOverheadEstimator;
@@ -1422,11 +1428,6 @@ Rule getNonFinalizerInstantiatedRule(String name) {
       }
     }
 
-    // For Package deserialization.
-    void disableNameConflictChecking() {
-      recorder.disableNameConflictChecking();
-    }
-
     public void addRuleUnchecked(Rule rule) {
       Preconditions.checkArgument(rule.getPackage() == pkg);
       recorder.addRuleUnchecked(rule);

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

@@ -257,7 +257,8 @@ public Package.Builder newPackageBuilder(
         packageOverheadEstimator,
         generatorMap,
         configSettingVisibilityPolicy,
-        globber);
+        globber,
+        /* enableNameConflictChecking= */ true);
   }
 
   /** Returns a new {@link NonSkyframeGlobber}. */

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

@@ -87,7 +87,7 @@ public abstract class TargetDefinitionContext extends StarlarkThreadContext {
 
   // The container object on which targets and macro instances are added and conflicts are
   // detected.
-  protected final TargetRecorder recorder = new TargetRecorder();
+  protected final TargetRecorder recorder;
 
   // Initialized from outside but also potentially set by `workspace()` function in WORKSPACE
   // file.
@@ -175,7 +175,8 @@ public T intern(T sample) {
       RepositoryMapping mainRepositoryMapping,
       @Nullable Semaphore cpuBoundSemaphore,
       @Nullable ImmutableMap<Location, String> generatorMap,
-      @Nullable Globber globber) {
+      @Nullable Globber globber,
+      boolean enableNameConflictChecking) {
     super(() -> mainRepositoryMapping);
     this.metadata = metadata;
     this.pkg = pkg;
@@ -203,6 +204,8 @@ public T intern(T sample) {
     this.generatorMap = (generatorMap == null) ? ImmutableMap.of() : generatorMap;
     this.globber = globber;
 
+    this.recorder = new TargetRecorder(enableNameConflictChecking);
+
     // Add target for the BUILD file itself.
     // (This may be overridden by an exports_file declaration.)
     // TODO: #19922 - Figure out exactly where this line belongs once TargetDefinitionContext is a

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

@@ -91,61 +91,41 @@ static class MacroFrame {
     }
   }
 
-  private enum NameConflictCheckingPolicy {
-    UNKNOWN,
-    NOT_GUARANTEED,
-    ENABLED;
-  }
-
-  /**
-   * Whether to do all validation checks for name clashes among targets, macros, and output file
-   * prefixes.
-   *
-   * <p>The {@code NOT_GUARANTEED} value should only be used when the package data has already been
-   * validated, e.g. in package deserialization.
-   *
-   * <p>Setting it to {@code NOT_GUARANTEED} does not necessarily turn off *all* checking, just some
-   * of the more expensive ones. Do not rely on being able to violate these checks.
-   */
-  private NameConflictCheckingPolicy nameConflictCheckingPolicy =
-      NameConflictCheckingPolicy.UNKNOWN;
-
   /**
    * Stores labels for each rule so that we don't have to call the costly {@link Rule#getLabels}
    * twice (once for {@link Package.Builder#checkForInputOutputConflicts} and once for {@link
    * Package.Builder#beforeBuild}).
    *
-   * <p>This field is null if name conflict checking is disabled. It is also null after the package
-   * is built.
+   * <p>This field is null if name conflict checking is disabled.
    */
   // TODO(#19922): Technically we don't need to store entries for rules that were created by
   // macros; see rulesCreatedInMacros, below.
-  @Nullable private Map<Rule, List<Label>> ruleLabels = new HashMap<>();
+  @Nullable private final Map<Rule, List<Label>> ruleLabels;
 
   /**
    * Stores labels of rule targets that were created in symbolic macros. We don't implicitly create
    * input files on behalf of such targets (though they may still be created on behalf of other
    * targets not in macros).
    *
-   * <p>This field is null if name conflict checking is disabled. It is also null after the package
-   * is built.
+   * <p>This field is null if name conflict checking is disabled.
    */
   // TODO(#19922): This can be eliminated once we have Targets directly store a reference to the
   // MacroInstance that instantiated them. (This is a little nontrivial because we'd like to avoid
   // simply adding a new field to Target subclasses, and instead want to combine it with the
   // existing Package-typed field.)
-  @Nullable private Set<Rule> rulesCreatedInMacros = new HashSet<>();
+  @Nullable private final Set<Rule> rulesCreatedInMacros;
 
   /**
    * A map from names of targets declared in a symbolic macro which violate macro naming rules, such
    * as "lib%{name}-src.jar" implicit outputs in java rules, to the name of the macro instance where
    * they were declared.
    *
-   * <p>This field is null if name conflict checking is disabled. The content of the map is
-   * manipulated only in {@link #checkRuleAndOutputs}.
+   * <p>Outside of package deserialization, the content of the map is manipulated only in {@link
+   * #checkRuleAndOutputs}. During deserialization, this map may also be populated by calling {@link
+   * #putAllMacroNamespaceViolatingTargets}.
    */
-  @Nullable
-  private LinkedHashMap<String, String> macroNamespaceViolatingTargets = new LinkedHashMap<>();
+  private final LinkedHashMap<String, String> macroNamespaceViolatingTargets =
+      new LinkedHashMap<>();
 
   /**
    * A map from target name to the (innermost) macro instance that declared it. See {@link
@@ -158,10 +138,31 @@ private enum NameConflictCheckingPolicy {
    * The collection of the prefixes of every output file. Maps each prefix to an arbitrary output
    * file having that prefix. Used for error reporting.
    *
-   * <p>This field is null if name conflict checking is disabled. It is also null after the package
-   * is built. The content of the map is manipulated only in {@link #checkRuleAndOutputs}.
+   * <p>This field is null if name conflict checking is disabled. The content of the map is
+   * manipulated only in {@link #checkRuleAndOutputs}.
+   */
+  @Nullable private final Map<String, OutputFile> outputFilePrefixes;
+
+  /**
+   * Constructs a {@link TargetRecorder}.
+   *
+   * @param enableNameConflictChecking whether to perform all validation checks for name clashes
+   *     among targets, macros, and output file prefixes. This should only be disabled when the
+   *     package has already been validated, e.g. in package deserialization. Setting it to false
+   *     does not necessarily turn off *all* checking, just some of the more expensive ones. Do not
+   *     rely on being able to violate these checks.
    */
-  @Nullable private Map<String, OutputFile> outputFilePrefixes = new HashMap<>();
+  public TargetRecorder(boolean enableNameConflictChecking) {
+    if (enableNameConflictChecking) {
+      this.ruleLabels = new HashMap<>();
+      this.rulesCreatedInMacros = new HashSet<>();
+      this.outputFilePrefixes = new HashMap<>();
+    } else {
+      this.ruleLabels = null;
+      this.rulesCreatedInMacros = null;
+      this.outputFilePrefixes = null;
+    }
+  }
 
   public Map<String, Target> getTargetMap() {
     return targetMap;
@@ -224,6 +225,10 @@ public boolean containsErrors() {
     return containsErrors;
   }
 
+  private boolean isNameConflictCheckingEnabled() {
+    return ruleLabels != null;
+  }
+
   /**
    * Inserts a target into {@code targetMap}. Returns the previous target if one was present, or
    * null.
@@ -296,12 +301,14 @@ public void unwrapSnapshottableBiMap() {
    *
    * <p>There must already be an existing target by the same name.
    *
-   * <p>Requires that {@link #disableNameConflictChecking} was not called.
+   * <p>Requires that the constructor was called with {@code enableNameConflictChecking} set to
+   * true.
    *
    * <p>A hack needed for {@link WorkspaceFactoryHelper}.
    */
   public void replaceTarget(Target newTarget) {
-    ensureNameConflictChecking();
+    Preconditions.checkState(
+        isNameConflictCheckingEnabled(), "Expected name conflict checking to be enabled");
 
     Preconditions.checkArgument(
         targetMap.containsKey(newTarget.getName()),
@@ -336,32 +343,6 @@ public Iterable<Rule> getRules() {
     return Iterables.filter(targetMap.values(), Rule.class);
   }
 
-  /**
-   * Turns off (some) conflict checking for name clashes between targets, macros, and output file
-   * prefixes. (It is not guaranteed to disable all checks, since it is intended as an optimization
-   * and not for semantic effect.)
-   *
-   * <p>This should only be done for data that has already been validated, e.g. during package
-   * deserialization. Do not call this unless you know what you're doing.
-   *
-   * <p>This method must be called prior to {@link #addRuleUnchecked}. It may not be called, neither
-   * before nor after, a call to {@link #addRule} or {@link #replaceTarget}.
-   */
-  public void disableNameConflictChecking() {
-    Preconditions.checkState(nameConflictCheckingPolicy == NameConflictCheckingPolicy.UNKNOWN);
-    this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.NOT_GUARANTEED;
-    this.ruleLabels = null;
-    this.rulesCreatedInMacros = null;
-    this.macroNamespaceViolatingTargets = null;
-    this.outputFilePrefixes = null;
-  }
-
-  public void ensureNameConflictChecking() {
-    Preconditions.checkState(
-        nameConflictCheckingPolicy != NameConflictCheckingPolicy.NOT_GUARANTEED);
-    this.nameConflictCheckingPolicy = NameConflictCheckingPolicy.ENABLED;
-  }
-
   /**
    * Adds a rule and its outputs to the targets map, and propagates the error bit from the rule to
    * the package.
@@ -377,21 +358,22 @@ private void addRuleInternal(Rule rule) {
   }
 
   /**
-   * Adds a rule without certain validation checks. Requires that {@link
-   * #disableNameConflictChecking} was already called.
+   * Adds a rule without certain validation checks. Requires that the constructor was called with
+   * {@code enableNameConflictChecking} set to false.
    */
   public void addRuleUnchecked(Rule rule) {
     Preconditions.checkState(
-        nameConflictCheckingPolicy == NameConflictCheckingPolicy.NOT_GUARANTEED);
+        !isNameConflictCheckingEnabled(), "Expected name conflict checking to be disabled");
     addRuleInternal(rule);
   }
 
   /**
-   * Adds a rule, subject to the usual validation checks. Requires that {@link
-   * #disableNameConflictChecking} was not called.
+   * Adds a rule, subject to the usual validation checks. Requires that the constructor was called
+   * with {@code enableNameConflictChecking} set to true.
    */
   public void addRule(Rule rule) throws NameConflictException {
-    ensureNameConflictChecking();
+    Preconditions.checkState(
+        isNameConflictCheckingEnabled(), "Expected name conflict checking to be enabled");
 
     List<Label> labels = rule.getLabels();
     checkRuleAndOutputs(rule, labels);
@@ -601,9 +583,6 @@ private void checkTargetName(Target target) throws NameConflictException {
    */
   public void putAllMacroNamespaceViolatingTargets(
       Map<String, String> macroNamespaceViolatingTargets) {
-    if (this.macroNamespaceViolatingTargets == null) {
-      this.macroNamespaceViolatingTargets = new LinkedHashMap<>();
-    }
     this.macroNamespaceViolatingTargets.putAll(macroNamespaceViolatingTargets);
   }
 

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

@@ -178,7 +178,8 @@ private Package.Builder pkgBuilder(String name) {
         PackageOverheadEstimator.NOOP_ESTIMATOR,
         /* generatorMap= */ null,
         /* configSettingVisibilityPolicy= */ null,
-        /* globber= */ null);
+        /* globber= */ null,
+        /* enableNameConflictChecking= */ true);
   }
 
   private static Rule addRule(Package.Builder pkgBuilder, Label label, RuleClass ruleClass)