From 1f18b1911b28347157e9250deb5e223a3de3d032 Mon Sep 17 00:00:00 2001
From: Googler <wyv@google.com>
Date: Mon, 12 Jun 2023 12:23:56 -0700
Subject: [PATCH] Add a PackageArgs class in preparation for REPO.bazel

- Created a PackageArgs class that stores the values passed to the attrs of the `package()` function, so that they can be applied to a package builder in one go
  - These attr values used to be applied as soon as each attr is processed (i.e. evaluated). We can't do that anymore for repo(), since at the time of REPO.bazel evaluation, the package hasn't started being built yet.
  - Instead of storing the individual fields, Package now just holds on to a PackageArgs object.
  - In a follow-up CL, we store all the repo() values in a PackageArgs class, and apply those before the package() ones.
- Also removed `Package#isDefaultVisibilitySet` as nobody calls it anymore.

Work towards https://github.com/bazelbuild/bazel/issues/18077

PiperOrigin-RevId: 539728608
Change-Id: Ie4a081fa0e52e833ac2a6be50033f95728da3a9b
---
 .../build/lib/packages/BuildGlobals.java      |   4 +-
 .../devtools/build/lib/packages/Package.java  | 214 ++---------------
 .../build/lib/packages/PackageArgs.java       | 226 ++++++++++++++++++
 .../build/lib/packages/PackageCallable.java   |  82 +------
 .../build/lib/packages/RuleClass.java         |   9 +-
 .../build/lib/pkgcache/PackageOptions.java    |  15 +-
 .../build/lib/skyframe/PackageFunction.java   |   9 +-
 .../lib/packages/PackageFactoryTest.java      |   4 +-
 8 files changed, 277 insertions(+), 286 deletions(-)
 create mode 100644 src/main/java/com/google/devtools/build/lib/packages/PackageArgs.java

diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java
index 72097be48f9846..03c87efcd1ca1c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java
@@ -121,7 +121,7 @@ public NoneType licenses(
     PackageContext context = PackageFactory.getContext(thread);
     try {
       License license = BuildType.LICENSE.convert(licensesList, "'licenses' operand");
-      context.pkgBuilder.setDefaultLicense(license);
+      context.pkgBuilder.mergePackageArgsFrom(PackageArgs.builder().setLicense(license));
     } catch (ConversionException e) {
       context.eventHandler.handle(
           Package.error(thread.getCallerLocation(), e.getMessage(), Code.LICENSE_PARSE_FAILURE));
@@ -144,7 +144,7 @@ public NoneType distribs(Object object, StarlarkThread thread) throws EvalExcept
     try {
       Set<DistributionType> distribs =
           BuildType.DISTRIBUTIONS.convert(object, "'distribs' operand");
-      context.pkgBuilder.setDefaultDistribs(distribs);
+      context.pkgBuilder.mergePackageArgsFrom(PackageArgs.builder().setDistribs(distribs));
     } catch (ConversionException e) {
       context.eventHandler.handle(
           Package.error(
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 da4a12308ab4ab..a46dafde605ae6 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
@@ -44,7 +44,6 @@
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
-import com.google.devtools.build.lib.packages.License.DistributionType;
 import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
@@ -66,7 +65,6 @@
 import java.io.PrintStream;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
@@ -171,11 +169,7 @@ private NameConflictException(String message) {
   /** The collection of all targets defined in this package, indexed by name. */
   private ImmutableSortedMap<String, Target> targets;
 
-  /**
-   * Default visibility for rules that do not specify it.
-   */
-  private RuleVisibility defaultVisibility;
-  private boolean defaultVisibilitySet;
+  private PackageArgs packageArgs = PackageArgs.DEFAULT;
 
   /**
    * How to enforce config_setting visibility settings.
@@ -195,16 +189,6 @@ public enum ConfigSettingVisibilityPolicy {
 
   private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;
 
-  /**
-   * Default package-level 'testonly' value for rules that do not specify it.
-   */
-  private boolean defaultTestOnly = false;
-
-  /**
-   * Default package-level 'deprecation' value for rules that do not specify it.
-   */
-  private String defaultDeprecation;
-
   /**
    * Default header strictness checking for rules that do not specify it.
    */
@@ -232,18 +216,6 @@ public enum ConfigSettingVisibilityPolicy {
    */
   @Nullable private FailureDetail failureDetail;
 
-  /** The package's default "package_metadata" attribute. */
-  private ImmutableList<Label> defaultPackageMetadata = ImmutableList.of();
-
-  /**
-   * The package's default "licenses" and "distribs" attributes, as specified
-   * in calls to licenses() and distribs() in the BUILD file.
-   */
-  // These sets contain the values specified by the most recent licenses() or
-  // distribs() declarations encountered during package parsing:
-  private License defaultLicense;
-  private Set<License.DistributionType> defaultDistributionSet;
-
   /**
    * The map from each repository to that repository's remappings map. This is only used in the
    * //external package, it is an empty map for all other packages. For example, an entry of {"@foo"
@@ -265,11 +237,6 @@ public enum ConfigSettingVisibilityPolicy {
    */
   private RepositoryMapping mainRepositoryMapping;
 
-  private Set<Label> defaultCompatibleWith = ImmutableSet.of();
-  private Set<Label> defaultRestrictedTo = ImmutableSet.of();
-
-  private FeatureSet features;
-
   private ImmutableList<TargetPattern> registeredExecutionPlatforms;
   private ImmutableList<TargetPattern> registeredToolchains;
 
@@ -369,36 +336,6 @@ private void setDefaultHdrsCheck(String defaultHdrsCheck) {
     this.defaultHdrsCheck = defaultHdrsCheck;
   }
 
-  /**
-   * Set the default 'testonly' value for this package.
-   */
-  private void setDefaultTestOnly(boolean testOnly) {
-    defaultTestOnly = testOnly;
-  }
-
-  /**
-   * Set the default 'deprecation' value for this package.
-   */
-  private void setDefaultDeprecation(String deprecation) {
-    defaultDeprecation = deprecation;
-  }
-
-  /**
-   * Sets the default value to use for a rule's {@link RuleClass#COMPATIBLE_ENVIRONMENT_ATTR}
-   * attribute when not explicitly specified by the rule.
-   */
-  private void setDefaultCompatibleWith(Set<Label> environments) {
-    defaultCompatibleWith = environments;
-  }
-
-  /**
-   * Sets the default value to use for a rule's {@link RuleClass#RESTRICTED_ENVIRONMENT_ATTR}
-   * attribute when not explicitly specified by the rule.
-   */
-  private void setDefaultRestrictedTo(Set<Label> environments) {
-    defaultRestrictedTo = environments;
-  }
-
   /**
    * Returns the source root (a directory) beneath which this package's BUILD file was found, or
    * {@link Optional#empty} if this package was derived from a workspace file.
@@ -471,15 +408,10 @@ private void finishInit(Builder builder) {
 
     this.makeEnv = ImmutableMap.copyOf(builder.makeEnv);
     this.targets = ImmutableSortedMap.copyOf(builder.targets);
-    this.defaultVisibility = builder.defaultVisibility;
-    this.defaultVisibilitySet = builder.defaultVisibilitySet;
+    this.packageArgs = builder.packageArgs;
     this.configSettingVisibilityPolicy = builder.configSettingVisibilityPolicy;
     this.buildFile = builder.buildFile;
     this.failureDetail = builder.getFailureDetail();
-    this.defaultLicense = builder.defaultLicense;
-    this.defaultDistributionSet = builder.defaultDistributionSet;
-    this.defaultPackageMetadata = builder.defaultPackageMetadata;
-    this.features = builder.features;
     this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
     this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
     this.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping);
@@ -744,7 +676,7 @@ public String getWorkspaceName() {
 
   /** Returns the features specified in the <code>package()</code> declaration. */
   public FeatureSet getFeatures() {
-    return features;
+    return packageArgs.features();
   }
 
   /**
@@ -826,7 +758,7 @@ private String getAlternateTargetSuggestion(String targetName) {
    * Returns the default visibility for this package.
    */
   public RuleVisibility getDefaultVisibility() {
-    return defaultVisibility;
+    return packageArgs.defaultVisibility();
   }
 
   /**
@@ -841,14 +773,14 @@ public ConfigSettingVisibilityPolicy getConfigSettingVisibilityPolicy() {
    * Returns the default testonly value.
    */
   public Boolean getDefaultTestOnly() {
-    return defaultTestOnly;
+    return packageArgs.defaultTestOnly();
   }
 
   /**
    * Returns the default deprecation value.
    */
   public String getDefaultDeprecation() {
-    return defaultDeprecation;
+    return packageArgs.defaultDeprecation();
   }
 
   /** Gets the default header checking mode. */
@@ -864,23 +796,19 @@ public boolean isDefaultHdrsCheckSet() {
     return defaultHdrsCheck != null;
   }
 
-  public boolean isDefaultVisibilitySet() {
-    return defaultVisibilitySet;
-  }
-
   /** Gets the package metadata list for the default metadata declared by this package. */
   ImmutableList<Label> getDefaultPackageMetadata() {
-    return defaultPackageMetadata;
+    return packageArgs.defaultPackageMetadata();
   }
 
   /** Gets the parsed license object for the default license declared by this package. */
   License getDefaultLicense() {
-    return defaultLicense;
+    return packageArgs.license();
   }
 
   /** Returns the parsed set of distributions declared as the default for this package. */
   Set<License.DistributionType> getDefaultDistribs() {
-    return defaultDistributionSet;
+    return packageArgs.distribs();
   }
 
   /**
@@ -888,7 +816,7 @@ Set<License.DistributionType> getDefaultDistribs() {
    * attribute when not explicitly specified by the rule.
    */
   public Set<Label> getDefaultCompatibleWith() {
-    return defaultCompatibleWith;
+    return packageArgs.defaultCompatibleWith();
   }
 
   /**
@@ -896,7 +824,7 @@ public Set<Label> getDefaultCompatibleWith() {
    * attribute when not explicitly specified by the rule.
    */
   public Set<Label> getDefaultRestrictedTo() {
-    return defaultRestrictedTo;
+    return packageArgs.defaultRestrictedTo();
   }
 
   public ImmutableList<TargetPattern> getRegisteredExecutionPlatforms() {
@@ -1072,10 +1000,8 @@ default boolean precomputeTransitiveLoads() {
     // TreeMap so that the iteration order of variables is predictable. This is useful so that the
     // serialized representation is deterministic.
     private final TreeMap<String, String> makeEnv = new TreeMap<>();
-    private RuleVisibility defaultVisibility = RuleVisibility.PRIVATE;
+    private PackageArgs packageArgs = PackageArgs.DEFAULT;
     private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;
-    private boolean defaultVisibilitySet;
-    private FeatureSet features = FeatureSet.EMPTY;
     private final List<Event> events = Lists.newArrayList();
     private final List<Postable> posts = Lists.newArrayList();
     @Nullable private String ioExceptionMessage = null;
@@ -1094,10 +1020,6 @@ default boolean precomputeTransitiveLoads() {
     // serialize events emitted during its construction/evaluation.
     @Nullable private FailureDetail failureDetailOverride = null;
 
-    private ImmutableList<Label> defaultPackageMetadata = ImmutableList.of();
-    private License defaultLicense = License.NO_LICENSE;
-    private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;
-
     // All targets added to the package. We use SnapshottableBiMap to help track insertion order of
     // Rule targets, for use by native.existing_rules().
     private BiMap<String, Target> targets =
@@ -1201,7 +1123,7 @@ public T intern(T sample) {
       this.mainRepositoryMapping = mainRepositoryMapping;
       this.labelConverter = new LabelConverter(id, repositoryMapping);
       if (pkg.getName().startsWith("javatests/")) {
-        setDefaultTestonly(true);
+        mergePackageArgsFrom(PackageArgs.builder().setDefaultTestOnly(true));
       }
     }
 
@@ -1347,22 +1269,19 @@ Builder setMakeVariable(String name, String value) {
       return this;
     }
 
-    /**
-     * Sets the default visibility for this package. Called at most once per package from
-     * PackageFactory.
-     */
     @CanIgnoreReturnValue
-    public Builder setDefaultVisibility(RuleVisibility visibility) {
-      this.defaultVisibility = visibility;
-      this.defaultVisibilitySet = true;
+    public Builder mergePackageArgsFrom(PackageArgs packageArgs) {
+      this.packageArgs = this.packageArgs.mergeWith(packageArgs);
       return this;
     }
 
-    /** Sets whether the default visibility is set in the BUILD file. */
     @CanIgnoreReturnValue
-    public Builder setDefaultVisibilitySet(boolean defaultVisibilitySet) {
-      this.defaultVisibilitySet = defaultVisibilitySet;
-      return this;
+    public Builder mergePackageArgsFrom(PackageArgs.Builder builder) {
+      return mergePackageArgsFrom(builder.build());
+    }
+
+    public PackageArgs getPackageArgs() {
+      return packageArgs;
     }
 
     /** Sets visibility enforcement policy for <code>config_setting</code>. */
@@ -1372,20 +1291,6 @@ public Builder setConfigSettingVisibilityPolicy(ConfigSettingVisibilityPolicy po
       return this;
     }
 
-    /** Sets the default value of 'testonly'. Rule-level 'testonly' will override this. */
-    @CanIgnoreReturnValue
-    Builder setDefaultTestonly(boolean defaultTestonly) {
-      pkg.setDefaultTestOnly(defaultTestonly);
-      return this;
-    }
-
-    /** Sets the default value of 'deprecation'. Rule-level 'deprecation' will append to this. */
-    @CanIgnoreReturnValue
-    Builder setDefaultDeprecation(String defaultDeprecation) {
-      pkg.setDefaultDeprecation(defaultDeprecation);
-      return this;
-    }
-
     /** Uses the workspace name from {@code //external} to set this package's workspace name. */
     @CanIgnoreReturnValue
     @VisibleForTesting
@@ -1420,12 +1325,6 @@ public Builder setDefaultHdrsCheck(String hdrsCheck) {
       return this;
     }
 
-    @CanIgnoreReturnValue
-    public Builder addFeatures(Iterable<String> features) {
-      this.features = FeatureSet.merge(this.features, FeatureSet.parse(features));
-      return this;
-    }
-
     Builder setIOException(IOException e, String message, DetailedExitCode detailedExitCode) {
       this.ioException = e;
       this.ioExceptionMessage = message;
@@ -1528,75 +1427,6 @@ private void checkLoadsNotSet() {
           pkg.transitiveLoads == null, "Transitive loads already set: %s", pkg.transitiveLoads);
     }
 
-    /**
-     * Sets the default value to use for a rule's {@link RuleClass#APPLICABLE_LICENSES_ATTR}
-     * attribute when not explicitly specified by the rule. Records a package error if any labels
-     * are duplicated.
-     */
-    void setDefaultPackageMetadata(List<Label> licenses, String attrName, Location location) {
-      if (hasDuplicateLabels(
-          licenses, "package " + pkg.getName(), attrName, location, this::addEvent)) {
-        setContainsErrors();
-      }
-      this.defaultPackageMetadata = ImmutableList.copyOf(licenses);
-    }
-
-    ImmutableList<Label> getDefaultPackageMetadata() {
-      return defaultPackageMetadata;
-    }
-
-    /**
-     * Sets the default license for this package.
-     */
-    void setDefaultLicense(License license) {
-      this.defaultLicense = license;
-    }
-
-    License getDefaultLicense() {
-      return defaultLicense;
-    }
-
-    /**
-     * Initializes the default set of distributions for targets in this package.
-     *
-     * <p> TODO(bazel-team): (2011) consider moving the license & distribs info into Metadata--maybe
-     * even in the Build language.
-     */
-    void setDefaultDistribs(Set<DistributionType> dists) {
-      this.defaultDistributionSet = dists;
-    }
-
-    Set<DistributionType> getDefaultDistribs() {
-      return defaultDistributionSet;
-    }
-
-    /**
-     * Sets the default value to use for a rule's {@link RuleClass#COMPATIBLE_ENVIRONMENT_ATTR}
-     * attribute when not explicitly specified by the rule. Records a package error if any labels
-     * are duplicated.
-     */
-    void setDefaultCompatibleWith(List<Label> environments, String attrName, Location location) {
-      if (hasDuplicateLabels(
-          environments, "package " + pkg.getName(), attrName, location, this::addEvent)) {
-        setContainsErrors();
-      }
-      pkg.setDefaultCompatibleWith(ImmutableSet.copyOf(environments));
-    }
-
-    /**
-     * Sets the default value to use for a rule's {@link RuleClass#RESTRICTED_ENVIRONMENT_ATTR}
-     * attribute when not explicitly specified by the rule. Records a package error if
-     * any labels are duplicated.
-     */
-    void setDefaultRestrictedTo(List<Label> environments, String attrName, Location location) {
-      if (hasDuplicateLabels(
-          environments, "package " + pkg.getName(), attrName, location, this::addEvent)) {
-        setContainsErrors();
-      }
-
-      pkg.setDefaultRestrictedTo(ImmutableSet.copyOf(environments));
-    }
-
     /**
      * Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package}
      * associated with this {@link Builder}.
@@ -1989,8 +1819,6 @@ public Package finishBuild() {
       }
       ruleLabels = null;
       targets = Maps.unmodifiableBiMap(targets);
-      defaultDistributionSet =
-          Collections.unmodifiableSet(defaultDistributionSet);
 
       // Now all targets have been loaded, so we validate the group's member environments.
       for (EnvironmentGroup envGroup : ImmutableSet.copyOf(environmentGroups.values())) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageArgs.java b/src/main/java/com/google/devtools/build/lib/packages/PackageArgs.java
new file mode 100644
index 00000000000000..cd8b5f26fe28ad
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageArgs.java
@@ -0,0 +1,226 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.packages;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
+import com.google.devtools.build.lib.analysis.config.FeatureSet;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.collect.CollectionUtils;
+import com.google.devtools.build.lib.packages.License.DistributionType;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import java.util.List;
+import java.util.Set;
+import javax.annotation.Nullable;
+import net.starlark.java.eval.EvalException;
+import net.starlark.java.eval.Starlark;
+
+/**
+ * A group of {@link Package} argument values that may be provided by `package()` or `repo()` calls.
+ */
+@AutoValue
+public abstract class PackageArgs {
+  public static final PackageArgs DEFAULT =
+      PackageArgs.builder()
+          .setDefaultVisibility(RuleVisibility.PRIVATE)
+          .setDefaultTestOnly(false)
+          .setFeatures(FeatureSet.EMPTY)
+          .setLicense(License.NO_LICENSE)
+          .setDistribs(License.DEFAULT_DISTRIB)
+          .setDefaultCompatibleWith(ImmutableSet.of())
+          .setDefaultRestrictedTo(ImmutableSet.of())
+          .setDefaultPackageMetadata(ImmutableList.of())
+          .build();
+
+  /** See {@link Package#getDefaultVisibility()}. */
+  @Nullable
+  abstract RuleVisibility defaultVisibility();
+
+  /** See {@link Package#getDefaultTestOnly()}. */
+  @Nullable
+  abstract Boolean defaultTestOnly();
+
+  /** See {@link Package#getDefaultDeprecation()}. */
+  @Nullable
+  abstract String defaultDeprecation();
+
+  /** See {@link Package#getFeatures()}. */
+  abstract FeatureSet features();
+
+  /** See {@link Package#getDefaultLicense()}. */
+  @Nullable
+  abstract License license();
+
+  /** See {@link Package#getDefaultDistribs()}. */
+  @Nullable
+  abstract ImmutableSet<DistributionType> distribs();
+
+  /** See {@link Package#getDefaultCompatibleWith()}. */
+  @Nullable
+  abstract ImmutableSet<Label> defaultCompatibleWith();
+
+  /** See {@link Package#getDefaultRestrictedTo()}. */
+  @Nullable
+  abstract ImmutableSet<Label> defaultRestrictedTo();
+
+  /** See {@link Package#getDefaultPackageMetadata()}. */
+  @Nullable
+  abstract ImmutableList<Label> defaultPackageMetadata();
+
+  public static Builder builder() {
+    return new AutoValue_PackageArgs.Builder().setFeatures(FeatureSet.EMPTY);
+  }
+
+  abstract Builder toBuilder();
+
+  /** Builder type for {@link PackageArgs}. */
+  @AutoValue.Builder
+  public abstract static class Builder {
+    public abstract Builder setDefaultVisibility(RuleVisibility x);
+
+    public abstract Builder setDefaultTestOnly(Boolean x);
+
+    public abstract Builder setDefaultDeprecation(String x);
+
+    abstract FeatureSet features();
+
+    public abstract Builder setFeatures(FeatureSet x);
+
+    @CanIgnoreReturnValue
+    public final Builder mergeFeatures(FeatureSet x) {
+      return setFeatures(FeatureSet.merge(features(), x));
+    }
+
+    public abstract Builder setLicense(License x);
+
+    public abstract Builder setDistribs(Set<DistributionType> x);
+
+    /** Note that we don't check dupes in this method. Check beforehand! */
+    public abstract Builder setDefaultCompatibleWith(Iterable<Label> x);
+
+    /** Note that we don't check dupes in this method. Check beforehand! */
+    public abstract Builder setDefaultRestrictedTo(Iterable<Label> x);
+
+    @Nullable
+    abstract ImmutableList<Label> defaultPackageMetadata();
+
+    /** Note that we don't check dupes in this method. Check beforehand! */
+    public abstract Builder setDefaultPackageMetadata(List<Label> x);
+
+    public abstract PackageArgs build();
+  }
+
+  private static void throwIfHasDupes(List<Label> labels, String what) throws EvalException {
+    var dupes = ImmutableSortedSet.copyOf(CollectionUtils.duplicatedElementsOf(labels));
+    if (!dupes.isEmpty()) {
+      throw Starlark.errorf("duplicate label(s) in %s: %s", what, Joiner.on(", ").join(dupes));
+    }
+  }
+
+  /**
+   * Processes the given Starlark parameter to the {@code package()/repo()} call into a field on a
+   * {@link Builder} object.
+   */
+  public static void processParam(
+      String name, Object rawValue, String what, LabelConverter labelConverter, Builder builder)
+      throws EvalException {
+    if (name.equals("default_visibility")) {
+      List<Label> value = BuildType.LABEL_LIST.convert(rawValue, what, labelConverter);
+      builder.setDefaultVisibility(RuleVisibility.parse(value));
+
+    } else if (name.equals("default_testonly")) {
+      Boolean value = Type.BOOLEAN.convert(rawValue, what, labelConverter);
+      builder.setDefaultTestOnly(value);
+
+    } else if (name.equals("default_deprecation")) {
+      String value = Type.STRING.convert(rawValue, what, labelConverter);
+      builder.setDefaultDeprecation(value);
+
+    } else if (name.equals("features")) {
+      List<String> value = Type.STRING_LIST.convert(rawValue, what, labelConverter);
+      builder.mergeFeatures(FeatureSet.parse(value));
+
+    } else if (name.equals("licenses")) {
+      License value = BuildType.LICENSE.convert(rawValue, what, labelConverter);
+      builder.setLicense(value);
+
+    } else if (name.equals("distribs")) {
+      Set<DistributionType> value = BuildType.DISTRIBUTIONS.convert(rawValue, what, labelConverter);
+      builder.setDistribs(value);
+
+    } else if (name.equals("default_compatible_with")) {
+      List<Label> value = BuildType.LABEL_LIST.convert(rawValue, what, labelConverter);
+      throwIfHasDupes(value, name);
+      builder.setDefaultCompatibleWith(value);
+
+    } else if (name.equals("default_restricted_to")) {
+      List<Label> value = BuildType.LABEL_LIST.convert(rawValue, what, labelConverter);
+      throwIfHasDupes(value, name);
+      builder.setDefaultRestrictedTo(value);
+
+    } else if (name.equals("default_applicable_licenses")
+        || name.equals("default_package_metadata")) {
+      List<Label> value = BuildType.LABEL_LIST.convert(rawValue, what, labelConverter);
+      if (builder.defaultPackageMetadata() != null) {
+        throw Starlark.errorf(
+            "Can not set both default_package_metadata and default_applicable_licenses."
+                + " Move all declarations to default_package_metadata.");
+      }
+      throwIfHasDupes(value, name);
+      builder.setDefaultPackageMetadata(value);
+
+    } else {
+      throw Starlark.errorf("unexpected keyword argument: %s", name);
+    }
+  }
+
+  /**
+   * Returns a new {@link PackageArgs} containing the result of merging {@code other} into {@code
+   * this}. {@code other}'s fields take precedence if specified.
+   */
+  public PackageArgs mergeWith(PackageArgs other) {
+    Builder builder = toBuilder();
+    if (other.defaultVisibility() != null) {
+      builder.setDefaultVisibility(other.defaultVisibility());
+    }
+    if (other.defaultTestOnly() != null) {
+      builder.setDefaultTestOnly(other.defaultTestOnly());
+    }
+    if (other.defaultDeprecation() != null) {
+      builder.setDefaultDeprecation(other.defaultDeprecation());
+    }
+    if (!other.features().equals(FeatureSet.EMPTY)) {
+      builder.mergeFeatures(other.features());
+    }
+    if (other.license() != null) {
+      builder.setLicense(other.license());
+    }
+    if (other.distribs() != null) {
+      builder.setDistribs(other.distribs());
+    }
+    if (other.defaultCompatibleWith() != null) {
+      builder.setDefaultCompatibleWith(other.defaultCompatibleWith());
+    }
+    if (other.defaultRestrictedTo() != null) {
+      builder.setDefaultRestrictedTo(other.defaultRestrictedTo());
+    }
+    if (other.defaultPackageMetadata() != null) {
+      builder.setDefaultPackageMetadata(other.defaultPackageMetadata());
+    }
+    return builder.build();
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java b/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java
index 3f8db53ca1fdd8..3fae8dacafb360 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java
@@ -14,18 +14,12 @@
 
 package com.google.devtools.build.lib.packages;
 
-import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.packages.License.DistributionType;
-import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code;
-import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import net.starlark.java.annot.Param;
 import net.starlark.java.annot.StarlarkMethod;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkThread;
-import net.starlark.java.syntax.Location;
 
 /**
  * Utility class encapsulating the standard definition of the {@code package()} function of BUILD
@@ -54,12 +48,13 @@ public Object packageCallable(Map<String, Object> kwargs, StarlarkThread thread)
       throw new EvalException("at least one argument must be given to the 'package' function");
     }
 
-    Location loc = thread.getCallerLocation();
+    PackageArgs.Builder pkgArgsBuilder = PackageArgs.builder();
     for (Map.Entry<String, Object> kwarg : kwargs.entrySet()) {
       String name = kwarg.getKey();
       Object rawValue = kwarg.getValue();
-      processParam(name, rawValue, pkgBuilder, loc);
+      processParam(name, rawValue, pkgBuilder, pkgArgsBuilder);
     }
+    pkgBuilder.mergePackageArgsFrom(pkgArgsBuilder);
     return Starlark.NONE;
   }
 
@@ -68,70 +63,13 @@ public Object packageCallable(Map<String, Object> kwargs, StarlarkThread thread)
    * back on the super method when the parameter does not match.
    */
   protected void processParam(
-      String name, Object rawValue, Package.Builder pkgBuilder, Location loc) throws EvalException {
-    if (name.equals("default_visibility")) {
-      List<Label> value = convert(BuildType.LABEL_LIST, rawValue, pkgBuilder);
-      pkgBuilder.setDefaultVisibility(RuleVisibility.parse(value));
-
-    } else if (name.equals("default_testonly")) {
-      Boolean value = convert(Type.BOOLEAN, rawValue, pkgBuilder);
-      pkgBuilder.setDefaultTestonly(value);
-
-    } else if (name.equals("default_deprecation")) {
-      String value = convert(Type.STRING, rawValue, pkgBuilder);
-      pkgBuilder.setDefaultDeprecation(value);
-
-    } else if (name.equals("features")) {
-      List<String> value = convert(Type.STRING_LIST, rawValue, pkgBuilder);
-      pkgBuilder.addFeatures(value);
-
-    } else if (name.equals("licenses")) {
-      License value = convert(BuildType.LICENSE, rawValue, pkgBuilder);
-      pkgBuilder.setDefaultLicense(value);
-
-    } else if (name.equals("distribs")) {
-      Set<DistributionType> value = convert(BuildType.DISTRIBUTIONS, rawValue, pkgBuilder);
-      pkgBuilder.setDefaultDistribs(value);
-
-    } else if (name.equals("default_compatible_with")) {
-      List<Label> value = convert(BuildType.LABEL_LIST, rawValue, pkgBuilder);
-      pkgBuilder.setDefaultCompatibleWith(value, name, loc);
-
-    } else if (name.equals("default_restricted_to")) {
-      List<Label> value = convert(BuildType.LABEL_LIST, rawValue, pkgBuilder);
-      pkgBuilder.setDefaultRestrictedTo(value, name, loc);
-
-    } else if (name.equals("default_applicable_licenses")) {
-      List<Label> value = convert(BuildType.LABEL_LIST, rawValue, pkgBuilder);
-      if (!pkgBuilder.getDefaultPackageMetadata().isEmpty()) {
-        pkgBuilder.addEvent(
-            Package.error(
-                loc,
-                "Can not set both default_package_metadata and default_applicable_licenses."
-                    + " Move all declarations to default_package_metadata.",
-                Code.INVALID_PACKAGE_SPECIFICATION));
-      }
-      pkgBuilder.setDefaultPackageMetadata(value, name, loc);
-
-    } else if (name.equals("default_package_metadata")) {
-      List<Label> value = convert(BuildType.LABEL_LIST, rawValue, pkgBuilder);
-      if (!pkgBuilder.getDefaultPackageMetadata().isEmpty()) {
-        pkgBuilder.addEvent(
-            Package.error(
-                loc,
-                "Can not set both default_package_metadata and default_applicable_licenses."
-                    + " Move all declarations to default_package_metadata.",
-                Code.INVALID_PACKAGE_SPECIFICATION));
-      }
-      pkgBuilder.setDefaultPackageMetadata(value, name, loc);
-
-    } else {
-      throw Starlark.errorf("unexpected keyword argument: %s", name);
-    }
-  }
-
-  protected static <T> T convert(Type<T> type, Object value, Package.Builder pkgBuilder)
+      String name, Object rawValue, Package.Builder pkgBuilder, PackageArgs.Builder pkgArgsBuilder)
       throws EvalException {
-    return type.convert(value, "'package' argument", pkgBuilder.getLabelConverter());
+    PackageArgs.processParam(
+        name,
+        rawValue,
+        "package() argument '" + name + "'",
+        pkgBuilder.getLabelConverter(),
+        pkgArgsBuilder);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 8ee1d5229f5f0d..bac37243b88da1 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -2188,17 +2188,18 @@ private void populateDefaultRuleAttributeValues(
         if (rule.getRuleClassObject().isPackageMetadataRule()) {
           // Do nothing
         } else {
-          rule.setAttributeValue(attr, pkgBuilder.getDefaultPackageMetadata(), /*explicit=*/ false);
+          rule.setAttributeValue(
+              attr, pkgBuilder.getPackageArgs().defaultPackageMetadata(), /* explicit= */ false);
         }
 
       } else if (attr.getName().equals("licenses") && attr.getType() == BuildType.LICENSE) {
         rule.setAttributeValue(
             attr,
-            ignoreLicenses ? License.NO_LICENSE : pkgBuilder.getDefaultLicense(),
-            /*explicit=*/ false);
+            ignoreLicenses ? License.NO_LICENSE : pkgBuilder.getPackageArgs().license(),
+            /* explicit= */ false);
 
       } else if (attr.getName().equals("distribs") && attr.getType() == BuildType.DISTRIBUTIONS) {
-        rule.setAttributeValue(attr, pkgBuilder.getDefaultDistribs(), /*explicit=*/ false);
+        rule.setAttributeValue(attr, pkgBuilder.getPackageArgs().distribs(), /* explicit= */ false);
       }
       // Don't store default values, querying materializes them at read time.
     }
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java
index ed7dec91d5403e..210f7c28564155 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java
@@ -110,14 +110,13 @@ public ParallelismConverter() throws OptionsParsingException {
   public List<PackageIdentifier> deletedPackages;
 
   @Option(
-    name = "default_visibility",
-    defaultValue = "private",
-    converter = DefaultVisibilityConverter.class,
-    documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
-    effectTags = {OptionEffectTag.UNKNOWN},
-    help =
-        "Default visibility for packages that don't set it explicitly ('public' or " + "'private')."
-  )
+      name = "default_visibility",
+      defaultValue = "private",
+      converter = DefaultVisibilityConverter.class,
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help =
+          "Default visibility for packages that don't set it explicitly ('public' or 'private').")
   public RuleVisibility defaultVisibility;
 
   @Option(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 30d84e8692a289..00f1ac5b815565 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -49,6 +49,7 @@
 import com.google.devtools.build.lib.packages.NonSkyframeGlobber;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy;
+import com.google.devtools.build.lib.packages.PackageArgs;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.PackageValidator.InvalidPackageException;
 import com.google.devtools.build.lib.packages.RuleVisibility;
@@ -1372,13 +1373,11 @@ private LoadedPackage loadPackage(
                   repositoryMapping,
                   mainRepositoryMappingValue.getRepositoryMapping())
               .setFilename(buildFileRootedPath)
-              .setDefaultVisibility(defaultVisibility)
-              // "defaultVisibility" comes from the command line.
-              // Let's give the BUILD file a chance to set default_visibility once,
-              // by resetting the PackageBuilder.defaultVisibilitySet flag.
-              .setDefaultVisibilitySet(false)
               .setConfigSettingVisibilityPolicy(configSettingVisibilityPolicy);
 
+      pkgBuilder.mergePackageArgsFrom(
+          PackageArgs.builder().setDefaultVisibility(defaultVisibility));
+
       Set<SkyKey> globDepKeys = ImmutableSet.of();
 
       // OK to execute BUILD program?
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 1d670ee0e746ae..abf73f6a87f32f 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -1136,14 +1136,14 @@ public void testPackageDefaultEnvironments() throws Exception {
   @Test
   public void testPackageDefaultCompatibilityDuplicates() throws Exception {
     expectEvalError(
-        "'//foo:foo' is duplicated in the 'default_compatible_with' list",
+        "duplicate label(s) in default_compatible_with: //foo:foo",
         "package(default_compatible_with=['//foo', '//bar', '//foo'])");
   }
 
   @Test
   public void testPackageDefaultRestrictionDuplicates() throws Exception {
     expectEvalError(
-        "'//foo:foo' is duplicated in the 'default_restricted_to' list",
+        "duplicate label(s) in default_restricted_to: //foo:foo",
         "package(default_restricted_to=['//foo', '//bar', '//foo'])");
   }