diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index e2e7a3826c5e2d..bb17fb193f7b77 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -731,7 +731,7 @@ private static Optional<Root> computeSourceRoot(Metadata metadata) {
    * publicly.
    */
   private void finishInit(Builder builder) {
-    this.containsErrors |= builder.containsErrors;
+    this.containsErrors |= builder.containsErrors();
     if (directLoads == null && transitiveLoads == null) {
       Preconditions.checkState(containsErrors, "Loads not set for error-free package");
       builder.setLoads(ImmutableList.of());
@@ -740,13 +740,12 @@ private void finishInit(Builder builder) {
     this.workspaceName = builder.workspaceName;
 
     this.makeEnv = ImmutableMap.copyOf(builder.makeEnv);
-    this.targets = ImmutableSortedMap.copyOf(builder.targets);
-    this.macros = ImmutableSortedMap.copyOf(builder.macros);
+    this.targets = ImmutableSortedMap.copyOf(builder.getTargetMap());
+    this.macros = ImmutableSortedMap.copyOf(builder.getMacroMap());
     this.macroNamespaceViolatingTargets =
-        builder.macroNamespaceViolatingTargets != null
-            ? ImmutableMap.copyOf(builder.macroNamespaceViolatingTargets)
-            : ImmutableMap.of();
-    this.targetsToDeclaringMacros = ImmutableSortedMap.copyOf(builder.targetsToDeclaringMacros);
+        ImmutableMap.copyOf(builder.getMacroNamespaceViolatingTargets());
+    this.targetsToDeclaringMacros =
+        ImmutableSortedMap.copyOf(builder.getTargetsToDeclaringMacros());
     this.failureDetail = builder.getFailureDetail();
     this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
     this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
@@ -1642,7 +1641,8 @@ Rule createRule(
      */
     MacroInstance createMacro(
         MacroClass macroClass, Map<String, Object> attrValues, int sameNameDepth) {
-      MacroInstance parent = currentMacroFrame == null ? null : currentMacroFrame.macroInstance;
+      MacroInstance parent =
+          getCurrentMacroFrame() == null ? null : getCurrentMacroFrame().macroInstance;
       return new MacroInstance(pkg, parent, macroClass, attrValues, sameNameDepth);
     }
 
@@ -1667,9 +1667,9 @@ void replaceTarget(Target newTarget) {
     Map<String, Rule> getRulesSnapshotView() {
       if (rulesSnapshotViewForFinalizers != null) {
         return rulesSnapshotViewForFinalizers;
-      } else if (targets instanceof SnapshottableBiMap<?, ?>) {
+      } else if (getTargetMap() instanceof SnapshottableBiMap<?, ?>) {
         return Maps.transformValues(
-            ((SnapshottableBiMap<String, Target>) targets).getTrackedSnapshot(),
+            ((SnapshottableBiMap<String, Target>) getTargetMap()).getTrackedSnapshot(),
             target -> (Rule) target);
       } else {
         throw new IllegalStateException(
@@ -1691,7 +1691,7 @@ Map<String, Rule> getRulesSnapshotView() {
      * @throws IllegalArgumentException if the name is not a valid label
      */
     InputFile createInputFile(String targetName, Location location) throws NameConflictException {
-      Target existing = targets.get(targetName);
+      Target existing = getTargetMap().get(targetName);
 
       if (existing instanceof InputFile) {
         return (InputFile) existing; // idempotent
@@ -1721,7 +1721,7 @@ InputFile createInputFile(String targetName, Location location) throws NameConfl
     // visibility of :BUILD inside a symbolic macro.
     void setVisibilityAndLicense(InputFile inputFile, RuleVisibility visibility, License license) {
       String filename = inputFile.getName();
-      Target cacheInstance = targets.get(filename);
+      Target cacheInstance = getTargetMap().get(filename);
       if (!(cacheInstance instanceof InputFile)) {
         throw new IllegalArgumentException(
             "Can't set visibility for nonexistent FileTarget "
@@ -1808,7 +1808,7 @@ void addEnvironmentGroup(
         EventHandler eventHandler,
         Location location)
         throws NameConflictException, LabelSyntaxException {
-      Preconditions.checkState(currentMacroFrame == null);
+      Preconditions.checkState(getCurrentMacroFrame() == null);
 
       if (hasDuplicateLabels(environments, name, "environments", location, eventHandler)
           || hasDuplicateLabels(defaults, name, "defaults", location, eventHandler)) {
@@ -1852,9 +1852,15 @@ void addEnvironmentGroup(
     }
 
     @Override
-    protected void addRuleInternal(Rule rule) {
+    public void addRuleUnchecked(Rule rule) {
       Preconditions.checkArgument(rule.getPackage() == pkg);
-      super.addRuleInternal(rule);
+      super.addRuleUnchecked(rule);
+    }
+
+    @Override
+    public void addRule(Rule rule) throws NameConflictException {
+      Preconditions.checkArgument(rule.getPackage() == pkg);
+      super.addRule(rule);
     }
 
     @Override
@@ -1889,10 +1895,10 @@ public void markMacroComplete(MacroInstance macro) {
      * from which the macro's {@code MacroClass} was exported.
      */
     RuleVisibility copyAppendingCurrentMacroLocation(RuleVisibility visibility) {
-      if (currentMacroFrame == null) {
+      if (getCurrentMacroFrame() == null) {
         return visibility;
       }
-      MacroClass macroClass = currentMacroFrame.macroInstance.getMacroClass();
+      MacroClass macroClass = getCurrentMacroFrame().macroInstance.getMacroClass();
       PackageIdentifier macroLocation = macroClass.getDefiningBzlLabel().getPackageIdentifier();
       Label newVisibilityItem = Label.createUnvalidated(macroLocation, "__pkg__");
 
@@ -1946,7 +1952,8 @@ public void expandAllRemainingMacros(StarlarkSemantics semantics) throws Interru
       // Finalizer expansion step.
       if (!unexpandedMacros.isEmpty()) {
         Preconditions.checkState(
-            unexpandedMacros.stream().allMatch(id -> macros.get(id).getMacroClass().isFinalizer()),
+            unexpandedMacros.stream()
+                .allMatch(id -> getMacroMap().get(id).getMacroClass().isFinalizer()),
             "At the beginning of finalizer expansion, unexpandedMacros must contain only"
                 + " finalizers");
 
@@ -1955,14 +1962,14 @@ public void expandAllRemainingMacros(StarlarkSemantics semantics) throws Interru
         // include any rule instantiated by a finalizer or macro called from a finalizer.
         if (rulesSnapshotViewForFinalizers == null) {
           Preconditions.checkState(
-              targets instanceof SnapshottableBiMap<?, ?>,
+              getTargetMap() instanceof SnapshottableBiMap<?, ?>,
               "Cannot call expandAllRemainingMacros() after beforeBuild() has been called");
           rulesSnapshotViewForFinalizers = getRulesSnapshotView();
         }
 
         while (!unexpandedMacros.isEmpty()) { // NB: collection mutated by body
           String id = unexpandedMacros.iterator().next();
-          MacroInstance macro = macros.get(id);
+          MacroInstance macro = getMacroMap().get(id);
           MacroClass.executeMacroImplementation(macro, this, semantics);
         }
       }
@@ -1995,15 +2002,16 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
       // map to the SnapshottableBiMap's underlying bimap and thus stop tracking insertion order.
       // After this point, snapshots of targets should no longer be used, and any further
       // getRulesSnapshotView calls will throw.
-      if (targets instanceof SnapshottableBiMap<?, ?>) {
-        targets = ((SnapshottableBiMap<String, Target>) targets).getUnderlyingBiMap();
+      if (getTargetMap() instanceof SnapshottableBiMap<?, ?>) {
+        unwrapSnapshottableBiMap();
         rulesSnapshotViewForFinalizers = null;
       }
 
       // We create an InputFile corresponding to the BUILD file in Builder's constructor. However,
       // the visibility of this target may be overridden with an exports_files directive, so we wait
       // until now to obtain the current instance from the targets map.
-      pkg.buildFile = (InputFile) Preconditions.checkNotNull(targets.get(buildFileLabel.getName()));
+      pkg.buildFile =
+          (InputFile) Preconditions.checkNotNull(getTargetMap().get(buildFileLabel.getName()));
 
       // TODO(bazel-team): We run testSuiteImplicitTestsAccumulator here in beforeBuild(), but what
       // if one of the accumulated tests is later removed in PackageFunction, between the call to
@@ -2024,23 +2032,23 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
           // since we don't want the evaluation of the macro to affect the semantics of whether or
           // not this target was created (i.e. all implicitly created files are knowable without
           // necessarily evaluating symbolic macros).
-          if (rulesCreatedInMacros.contains(rule)) {
+          if (isRuleCreatedInMacro(rule)) {
             continue;
           }
           // We use a temporary map, newInputFiles, to avoid concurrent modification to this.targets
           // while iterating (via getRules() above).
-          List<Label> labels = (ruleLabels != null) ? ruleLabels.get(rule) : rule.getLabels();
+          List<Label> labels = getRuleLabels(rule);
           for (Label label : labels) {
             String name = label.getName();
             if (label.getPackageIdentifier().equals(metadata.packageIdentifier())
-                && !targets.containsKey(name)
+                && !getTargetMap().containsKey(name)
                 && !newInputFiles.containsKey(name)) {
               // Check for collision with a macro namespace. Currently this is a linear loop over
               // all symbolic macros in the package.
               // TODO(#19922): This is quadratic complexity, optimize with a trie or similar if
               // needed.
               boolean macroConflictsFound = false;
-              for (MacroInstance macro : macros.values()) {
+              for (MacroInstance macro : getMacroMap().values()) {
                 macroConflictsFound |= nameIsWithinMacroNamespace(name, macro.getName());
               }
               if (!macroConflictsFound) {
@@ -2096,7 +2104,7 @@ public Package finishBuild() {
 
       // Now all targets have been loaded, so we validate the group's member environments.
       for (EnvironmentGroup envGroup : ImmutableSet.copyOf(environmentGroups.values())) {
-        List<Event> errors = envGroup.processMemberEnvironments(targets);
+        List<Event> errors = envGroup.processMemberEnvironments(getTargetMap());
         if (!errors.isEmpty()) {
           Event.replayEventsOn(localEventHandler, errors);
           // TODO(bazel-team): Can't we automatically infer containsError from the presence of
diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetRegistrationEnvironment.java b/src/main/java/com/google/devtools/build/lib/packages/TargetRegistrationEnvironment.java
index b5911daa0281f9..9142c4bf51f41e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/TargetRegistrationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/TargetRegistrationEnvironment.java
@@ -17,6 +17,7 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.BiMap;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -40,7 +41,8 @@
  * construction.
  */
 // TODO: #19922 - Inheritance from TargetDefinitionContext isn't needed if we have Package.Builder
-// compose this class rather than inherit from it.
+// compose this class rather than inherit from it. If we change that, we can make the protected APIs
+// here public.
 public class TargetRegistrationEnvironment extends TargetDefinitionContext {
 
   /** Used for constructing macro namespace violation error messages. */
@@ -48,21 +50,18 @@ public class TargetRegistrationEnvironment extends TargetDefinitionContext {
       "Name must be the same as the macro's name, or the macro's name followed by '_'"
           + " (recommended), '-', or '.', and a non-empty string.";
 
-  // TODO: #19922 - Some of these fields are temporarily protected instead of private, to reduce the
-  // diff to Package.java.
-
-  protected boolean containsErrors = false;
+  private boolean containsErrors = false;
 
   // All targets added to the package.
   //
   // We use SnapshottableBiMap to help track insertion order of Rule targets, for use by
   // native.existing_rules().
-  protected BiMap<String, Target> targets =
+  private BiMap<String, Target> targetMap =
       new SnapshottableBiMap<>(target -> target instanceof Rule);
 
   // All instances of symbolic macros created during package construction, indexed by id (not
   // name).
-  protected final Map<String, MacroInstance> macros = new LinkedHashMap<>();
+  private final Map<String, MacroInstance> macroMap = new LinkedHashMap<>();
 
   /**
    * Represents the innermost currently executing symbolic macro, or null if none are running.
@@ -77,14 +76,14 @@ public class TargetRegistrationEnvironment extends TargetDefinitionContext {
    * documentation on {@code macro()} at {@link StarlarkRuleFunctionsApi#macro}), and to help
    * enforce naming requirements on targets and macros.
    */
-  @Nullable protected MacroFrame currentMacroFrame = null;
+  @Nullable private MacroFrame currentMacroFrame = null;
 
   /**
    * Represents the state of a running symbolic macro (see {@link #currentMacroFrame}). Semi-opaque.
    */
   static class MacroFrame {
     final MacroInstance macroInstance;
-    // Most name conflicts are caught by checking the keys of the `targets` and `macros` maps.
+    // Most name conflicts are caught by checking the keys of the `targetMap` and `macroMap` maps.
     // It is not a conflict for a target or macro to have the same name as the macro it is
     // declared in, yet such a target or macro may still conflict with siblings in the same macro.
     // We use this bool to track whether or not a newly introduced macro, M, having the same name
@@ -125,7 +124,7 @@ private enum NameConflictCheckingPolicy {
    */
   // TODO(#19922): Technically we don't need to store entries for rules that were created by
   // macros; see rulesCreatedInMacros, below.
-  @Nullable protected Map<Rule, List<Label>> ruleLabels = new HashMap<>();
+  @Nullable private Map<Rule, List<Label>> ruleLabels = new HashMap<>();
 
   /**
    * Stores labels of rule targets that were created in symbolic macros. We don't implicitly create
@@ -139,7 +138,7 @@ private enum NameConflictCheckingPolicy {
   // 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 protected Set<Rule> rulesCreatedInMacros = new HashSet<>();
+  @Nullable private Set<Rule> rulesCreatedInMacros = new HashSet<>();
 
   /**
    * A map from names of targets declared in a symbolic macro which violate macro naming rules, such
@@ -150,13 +149,14 @@ private enum NameConflictCheckingPolicy {
    * manipulated only in {@link #checkRuleAndOutputs}.
    */
   @Nullable
-  protected LinkedHashMap<String, String> macroNamespaceViolatingTargets = new LinkedHashMap<>();
+  private LinkedHashMap<String, String> macroNamespaceViolatingTargets = new LinkedHashMap<>();
 
   /**
    * A map from target name to the (innermost) macro instance that declared it. See {@link
    * Package#targetsToDeclaringMacros}.
    */
-  protected LinkedHashMap<String, MacroInstance> targetsToDeclaringMacros = new LinkedHashMap<>();
+  private final LinkedHashMap<String, MacroInstance> targetsToDeclaringMacros =
+      new LinkedHashMap<>();
 
   /**
    * The collection of the prefixes of every output file. Maps each prefix to an arbitrary output
@@ -171,6 +171,41 @@ protected TargetRegistrationEnvironment(RepositoryMapping mainRepositoryMapping)
     super(mainRepositoryMapping);
   }
 
+  protected Map<String, Target> getTargetMap() {
+    return targetMap;
+  }
+
+  protected Map<String, MacroInstance> getMacroMap() {
+    return macroMap;
+  }
+
+  protected List<Label> getRuleLabels(Rule rule) {
+    return (ruleLabels != null) ? ruleLabels.get(rule) : rule.getLabels();
+  }
+
+  protected boolean isRuleCreatedInMacro(Rule rule) {
+    return rulesCreatedInMacros.contains(rule);
+  }
+
+  /**
+   * Returns 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.
+   */
+  protected Map<String, String> getMacroNamespaceViolatingTargets() {
+    return macroNamespaceViolatingTargets != null
+        ? macroNamespaceViolatingTargets
+        : ImmutableMap.of();
+  }
+
+  /**
+   * A map from target name to the (innermost) macro instance that declared it. See {@link
+   * Package#targetsToDeclaringMacros}.
+   */
+  protected Map<String, MacroInstance> getTargetsToDeclaringMacros() {
+    return targetsToDeclaringMacros;
+  }
+
   /**
    * Declares that errors were encountering while loading this package.
    *
@@ -198,14 +233,15 @@ public boolean containsErrors() {
   }
 
   /**
-   * Inserts a target into the targets map. Returns the previous target if one was present, or null.
+   * Inserts a target into {@code targetMap}. Returns the previous target if one was present, or
+   * null.
    *
    * <p>No validation is done on the target's name.
    */
   @CanIgnoreReturnValue
   @Nullable
   private Target putTargetInternal(Target target) {
-    Target existing = targets.put(target.getName(), target);
+    Target existing = targetMap.put(target.getName(), target);
     if (currentMacroFrame != null) {
       targetsToDeclaringMacros.put(target.getName(), currentMacroFrame.macroInstance);
     }
@@ -254,7 +290,12 @@ protected void replaceInputFileUnchecked(InputFile file) {
 
   @Nullable
   Target getTarget(String name) {
-    return targets.get(name);
+    return targetMap.get(name);
+  }
+
+  protected void unwrapSnapshottableBiMap() {
+    Preconditions.checkState(targetMap instanceof SnapshottableBiMap<?, ?>);
+    this.targetMap = ((SnapshottableBiMap<String, Target>) targetMap).getUnderlyingBiMap();
   }
 
   /**
@@ -271,7 +312,7 @@ void replaceTarget(Target newTarget) {
     ensureNameConflictChecking();
 
     Preconditions.checkArgument(
-        targets.containsKey(newTarget.getName()),
+        targetMap.containsKey(newTarget.getName()),
         "No existing target with name '%s' in the targets map",
         newTarget.getName());
     Target oldTarget = putTargetInternal(newTarget);
@@ -287,7 +328,7 @@ void replaceTarget(Target newTarget) {
   }
 
   public Set<Target> getTargets() {
-    return targets.values();
+    return targetMap.values();
   }
 
   /**
@@ -297,7 +338,7 @@ public Set<Target> getTargets() {
    * instance targets were instantiated.
    */
   protected Iterable<Rule> getRules() {
-    return Iterables.filter(targets.values(), Rule.class);
+    return Iterables.filter(targetMap.values(), Rule.class);
   }
 
   /**
@@ -330,7 +371,7 @@ protected void ensureNameConflictChecking() {
    * Adds a rule and its outputs to the targets map, and propagates the error bit from the rule to
    * the package.
    */
-  protected void addRuleInternal(Rule rule) {
+  private void addRuleInternal(Rule rule) {
     for (OutputFile outputFile : rule.getOutputFiles()) {
       putTargetInternal(outputFile);
     }
@@ -369,7 +410,7 @@ void addRule(Rule rule) throws NameConflictException {
   /** Adds a symbolic macro instance to the package. */
   public void addMacro(MacroInstance macro) throws NameConflictException {
     checkMacroName(macro);
-    Object prev = macros.put(macro.getId(), macro);
+    Object prev = macroMap.put(macro.getId(), macro);
     Preconditions.checkState(prev == null);
 
     // Track whether a main submacro has been seen yet. Conflict checking for this is done in
@@ -475,8 +516,8 @@ private void checkRuleAndOutputs(Rule rule, List<Label> labels) throws NameConfl
         if (outputFilesByName.containsKey(prefix)) {
           throw overlappingOutputFilePrefixes(outputFile, outputFilesByName.get(prefix));
         }
-        if (targets.get(prefix) instanceof OutputFile) {
-          throw overlappingOutputFilePrefixes(outputFile, (OutputFile) targets.get(prefix));
+        if (targetMap.get(prefix) instanceof OutputFile) {
+          throw overlappingOutputFilePrefixes(outputFile, (OutputFile) targetMap.get(prefix));
         }
 
         // Store in persistent map, for checking when adding future rules.
@@ -512,7 +553,7 @@ private void checkRuleAndOutputs(Rule rule, List<Label> labels) throws NameConfl
    * <p>Note that just because a name is within a macro's namespace does not necessarily mean the
    * corresponding target or macro was declared within this macro.
    */
-  protected boolean nameIsWithinMacroNamespace(String name, String macroName) {
+  protected static boolean nameIsWithinMacroNamespace(String name, String macroName) {
     if (name.equals(macroName)) {
       return true;
     } else if (name.startsWith(macroName)) {
@@ -577,7 +618,7 @@ void putAllMacroNamespaceViolatingTargets(Map<String, String> macroNamespaceViol
    * <p>The given target must *not* have already been added.
    */
   private void checkForExistingTargetName(Target target) throws NameConflictException {
-    Target existing = targets.get(target.getName());
+    Target existing = targetMap.get(target.getName());
     if (existing == null) {
       return;
     }
@@ -610,7 +651,7 @@ private void checkMacroName(MacroInstance macro) throws NameConflictException {
     // A macro can share names with its main target but no other target. Since the macro hasn't
     // even been added yet, it hasn't run, and its main target is not yet defined. Therefore, any
     // match in the targets map represents a real conflict.
-    Target existingTarget = targets.get(name);
+    Target existingTarget = targetMap.get(name);
     if (existingTarget != null) {
       throw new NameConflictException(
           String.format("macro '%s' conflicts with an existing target.", name));
@@ -635,10 +676,10 @@ private void checkMacroName(MacroInstance macro) throws NameConflictException {
    * <p>{@code what} must be either "macro" or "target".
    */
   private void checkForExistingMacroName(String name, String what) throws NameConflictException {
-    // Macros are indexed by id, not name, so we can't just use macros.get() directly.
+    // Macros are indexed by id, not name, so we can't just use macroMap.get() directly.
     // Instead, we reason that if at least one macro by the given name exists, then there is one
     // with an id suffix of ":1".
-    MacroInstance existing = macros.get(name + ":1");
+    MacroInstance existing = macroMap.get(name + ":1");
     if (existing == null) {
       return;
     }