For native rule classes, (de)serialize only explicit attrs

Native rule classes can provide default values for rules after they're
deserialized, so there isn't a need to serialize those default values.

This doesn't apply yet to rules with Skylark-defined rule classes, due
to the non-serializablity of Skylark rule classes.

--
MOS_MIGRATED_REVID=112066930

6 files changed, 412 insertions, 215 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java b/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java
index 8570899691..b771763eb2 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ExternalPackageBuilder.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.events.StoredEventHandler;
+import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.util.Preconditions;
 
@@ -40,8 +41,10 @@ public class ExternalPackageBuilder {
           InterruptedException {
 
     StoredEventHandler eventHandler = new StoredEventHandler();
+    BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
     Rule tempRule =
-        RuleFactory.createRule(pkg, ruleClass, kwargs, eventHandler, ast, ast.getLocation(), null);
+        RuleFactory.createRule(
+            pkg, ruleClass, attributeValues, eventHandler, ast, ast.getLocation(), /*env=*/ null);
     pkg.addEvents(eventHandler.getEvents());
     overwriteRule(pkg, tempRule);
     for (Map.Entry<String, Label> entry :
@@ -64,8 +67,11 @@ public class ExternalPackageBuilder {
       attributes.put("actual", actual);
     }
     StoredEventHandler handler = new StoredEventHandler();
+    BuildLangTypedAttributeValuesMap attributeValues =
+        new BuildLangTypedAttributeValuesMap(attributes);
     Rule rule =
-        RuleFactory.createRule(pkg, bindRuleClass, attributes, handler, null, location, null);
+        RuleFactory.createRule(
+            pkg, bindRuleClass, attributeValues, handler, /*ast=*/ null, location, /*env=*/ null);
     overwriteRule(pkg, rule);
     rule.setVisibility(ConstantRuleVisibility.PUBLIC);
   }
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 ecf1df70dc..bc7804475c 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
@@ -1001,17 +1001,16 @@ public class Package {
     }
 
     /**
-     * Returns a new Rule belonging to this package instance, and uses the given Label.
+     * Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package}
+     * associated with this {@link Builder}.
      *
-     * <p>Useful for RuleClass instantiation, where the rule name is checked by trying to create a
-     * Label. This label can then be used again here.
+     * <p>The created {@link Rule} will have no attribute values, no output files, and therefore
+     * will be in an invalid state.
      */
-    Rule newRuleWithLabel(Label label, RuleClass ruleClass, Location location) {
-      return newRuleWithLabelAndAttrContainer(label, ruleClass, location,
-          new AttributeContainer(ruleClass));
-    }
-
-    Rule newRuleWithLabelAndAttrContainer(Label label, RuleClass ruleClass, Location location,
+    Rule createRule(
+        Label label,
+        RuleClass ruleClass,
+        Location location,
         AttributeContainer attributeContainer) {
       return new Rule(pkg, label, ruleClass, location, attributeContainer);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index ba9111c375..1de065eed8 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -32,6 +32,7 @@ import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.GlobCache.BadGlobException;
 import com.google.devtools.build.lib.packages.License.DistributionType;
 import com.google.devtools.build.lib.packages.Preprocessor.AstAfterPreprocessing;
+import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature.Param;
 import com.google.devtools.build.lib.syntax.AssignmentStatement;
@@ -923,7 +924,8 @@ public final class PackageFactory {
                               Environment env)
       throws RuleFactory.InvalidRuleException, Package.NameConflictException, InterruptedException {
     RuleClass ruleClass = getBuiltInRuleClass(ruleClassName, ruleFactory);
-    RuleFactory.createAndAddRule(context, ruleClass, kwargs, ast, env);
+    BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
+    RuleFactory.createAndAddRule(context, ruleClass, attributeValues, ast, env);
   }
 
   private static RuleClass getBuiltInRuleClass(String ruleClassName, RuleFactory ruleFactory) {
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 1b07ae0bba..8e00db47b6 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
@@ -36,6 +36,7 @@ import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.BuildType.SelectorList;
 import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
+import com.google.devtools.build.lib.packages.RuleFactory.AttributeValuesMap;
 import com.google.devtools.build.lib.syntax.Argument;
 import com.google.devtools.build.lib.syntax.BaseFunction;
 import com.google.devtools.build.lib.syntax.Environment;
@@ -45,6 +46,7 @@ import com.google.devtools.build.lib.syntax.GlobList;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.syntax.Type.ConversionException;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.StringUtil;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -1273,104 +1275,189 @@ public final class RuleClass {
   }
 
   /**
-   * Helper function for {@link RuleFactory#createAndAddRule}.
+   * Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package}
+   * associated with {@code pkgBuilder}.
+   *
+   * <p>The created {@link Rule} will be populated with attribute values from {@code
+   * attributeValues} or the default attribute values associated with this {@link RuleClass} and
+   * {@code pkgBuilder}.
+   *
+   * <p>The created {@link Rule} will also be populated with output files. These output files
+   * will have been collected from the explicitly provided values of type {@link BuildType#OUTPUT}
+   * and {@link BuildType#OUTPUT_LIST} as well as from the implicit outputs determined by this
+   * {@link RuleClass} and the values in {@code attributeValues}.
+   *
+   * <p>This performs several validity checks. Invalid output file labels result in a thrown {@link
+   * LabelSyntaxException}. All other errors are reported on {@code eventHandler}.
    */
-  Rule createRuleWithLabel(Package.Builder pkgBuilder, Label ruleLabel,
-      Map<String, Object> attributeValues, EventHandler eventHandler, FuncallExpression ast,
-      Location location) throws LabelSyntaxException, InterruptedException {
-    Rule rule = pkgBuilder.newRuleWithLabel(ruleLabel, this, location);
-    createRuleCommon(rule, pkgBuilder, attributeValues, eventHandler, ast);
-    return rule;
-  }
-
-  private void createRuleCommon(Rule rule, Package.Builder pkgBuilder,
-      Map<String, Object> attributeValues, EventHandler eventHandler, FuncallExpression ast)
+  Rule createRule(
+      Package.Builder pkgBuilder,
+      Label ruleLabel,
+      AttributeValuesMap attributeValues,
+      EventHandler eventHandler,
+      @Nullable FuncallExpression ast,
+      Location location,
+      AttributeContainer attributeContainer)
       throws LabelSyntaxException, InterruptedException {
-    populateRuleAttributeValues(
-        rule, pkgBuilder, attributeValues, eventHandler, ast);
+    Rule rule = pkgBuilder.createRule(ruleLabel, this, location, attributeContainer);
+    populateRuleAttributeValues(rule, pkgBuilder, attributeValues, eventHandler);
     rule.populateOutputFiles(eventHandler, pkgBuilder);
+    if (ast != null) {
+      populateAttributeLocations(rule, ast);
+    }
+    checkForDuplicateLabels(rule, eventHandler);
+    checkThirdPartyRuleHasLicense(rule, pkgBuilder, eventHandler);
+    checkForValidSizeAndTimeoutValues(rule, eventHandler);
     rule.checkForNullLabels();
     rule.checkValidityPredicate(eventHandler);
+    return rule;
+  }
+
+  /**
+   * Populates the attributes table of the new {@link Rule} with the values in the {@code
+   * attributeValues} map and with default values provided by this {@link RuleClass} and the {@code
+   * pkgBuilder}.
+   *
+   * <p>Errors are reported on {@code eventHandler}.
+   */
+  private void populateRuleAttributeValues(
+      Rule rule,
+      Package.Builder pkgBuilder,
+      AttributeValuesMap attributeValues,
+      EventHandler eventHandler) {
+    BitSet definedAttrIndices =
+        populateDefinedRuleAttributeValues(rule, attributeValues, eventHandler);
+    populateDefaultRuleAttributeValues(rule, pkgBuilder, definedAttrIndices, eventHandler);
+    // Now that all attributes are bound to values, collect and store configurable attribute keys.
+    populateConfigDependenciesAttribute(rule);
   }
 
   /**
-   * Populates the attributes table of new rule "rule" from the
-   * "attributeValues" mapping from attribute names to values in the build
-   * language.  Errors are reported on "reporter".  "ast" is used to associate
-   * location information with each rule attribute.
+   * Populates the attributes table of the new {@link Rule} with the values in the {@code
+   * attributeValues} map.
+   *
+   * <p>Handles the special cases of the attribute named {@code "name"} and attributes with value
+   * {@link Runtime#NONE}.
+   *
+   * <p>Returns a bitset {@code b} where {@code b.get(i)} is {@code true} if this method set a
+   * value for the attribute with index {@code i} in this {@link RuleClass}. Errors are reported
+   * on {@code eventHandler}.
    */
-  private void populateRuleAttributeValues(Rule rule,
-                                           Package.Builder pkgBuilder,
-                                           Map<String, Object> attributeValues,
-                                           EventHandler eventHandler,
-                                           FuncallExpression ast)
-                                               throws InterruptedException {
-    BitSet definedAttrs = new BitSet(); //  set of attr indices
-
-    for (Map.Entry<String, Object> entry : attributeValues.entrySet()) {
-      String attributeName = entry.getKey();
-      Object attributeValue = entry.getValue();
-      if (attributeValue == Runtime.NONE) {  // Ignore all None values.
+  private BitSet populateDefinedRuleAttributeValues(
+      Rule rule, AttributeValuesMap attributeValues, EventHandler eventHandler) {
+    BitSet definedAttrIndices = new BitSet();
+    for (String attributeName : attributeValues.getAttributeNames()) {
+      // The attribute named "name" was handled in a special way already.
+      if (attributeName.equals("name")) {
         continue;
       }
-      Integer attrIndex = setRuleAttributeValue(rule, eventHandler, attributeName, attributeValue);
-      if (attrIndex != null) {
-        definedAttrs.set(attrIndex);
-        checkAttrValNonEmpty(rule, eventHandler, attributeValue, attrIndex);
+
+      Object attributeValue = attributeValues.getAttributeValue(attributeName);
+      // Ignore all None values.
+      if (attributeValue == Runtime.NONE) {
+        continue;
+      }
+
+      // Check that the attribute's name belongs to a valid attribute for this rule class.
+      Integer attrIndex = getAttributeIndex(attributeName);
+      if (attrIndex == null) {
+        rule.reportError(
+            String.format(
+                "%s: no such attribute '%s' in '%s' rule", rule.getLabel(), attributeName, name),
+            eventHandler);
+        continue;
+      }
+      Attribute attr = getAttribute(attrIndex);
+
+      // Convert the build-lang value to a native value, if necessary.
+      Object nativeAttributeValue;
+      if (attributeValues.valuesAreBuildLanguageTyped()) {
+        try {
+          nativeAttributeValue = convertFromBuildLangType(rule, attr, attributeValue);
+        } catch (ConversionException e) {
+          rule.reportError(String.format("%s: %s", rule.getLabel(), e.getMessage()), eventHandler);
+          continue;
+        }
+      } else {
+        nativeAttributeValue = attributeValue;
       }
+
+      boolean explicit = attributeValues.isAttributeExplicitlySpecified(attributeName);
+      setRuleAttributeValue(rule, eventHandler, attr, nativeAttributeValue, explicit);
+      definedAttrIndices.set(attrIndex);
+      checkAttrValNonEmpty(rule, eventHandler, attributeValue, attr);
     }
+    return definedAttrIndices;
+  }
 
-    // Save the location of each non-default attribute definition:
-    if (ast != null) {
-      for (Argument.Passed arg : ast.getArguments()) {
-        if (arg.isKeyword()) {
-          String name = arg.getName();
-          Integer attrIndex = getAttributeIndex(name);
-          if (attrIndex != null) {
-            rule.setAttributeLocation(attrIndex, arg.getValue().getLocation());
-          }
+  /** Populates attribute locations for attributes defined in {@code ast}. */
+  private void populateAttributeLocations(Rule rule, FuncallExpression ast) {
+    for (Argument.Passed arg : ast.getArguments()) {
+      if (arg.isKeyword()) {
+        String name = arg.getName();
+        Integer attrIndex = getAttributeIndex(name);
+        if (attrIndex != null) {
+          rule.setAttributeLocation(attrIndex, arg.getValue().getLocation());
         }
       }
     }
+  }
 
+  /**
+   * Populates the attributes table of the new {@link Rule} with default values provided by this
+   * {@link RuleClass} and the {@code pkgBuilder}. This will only provide values for attributes
+   * that haven't already been populated, using {@code definedAttrIndices} to determine whether an
+   * attribute was populated.
+   *
+   * <p>Errors are reported on {@code eventHandler}.
+   */
+  private void populateDefaultRuleAttributeValues(
+      Rule rule, Package.Builder pkgBuilder, BitSet definedAttrIndices, EventHandler eventHandler) {
+    // Set defaults; ensure that every mandatory attribute has a value. Use the default if none
+    // is specified.
     List<Attribute> attrsWithComputedDefaults = new ArrayList<>();
-
-    // Set defaults; ensure that every mandatory attribute has a value.  Use
-    // the default if none is specified.
     int numAttributes = getAttributeCount();
     for (int attrIndex = 0; attrIndex < numAttributes; ++attrIndex) {
-      if (!definedAttrs.get(attrIndex)) {
-        Attribute attr = getAttribute(attrIndex);
-        if (attr.isMandatory()) {
-          rule.reportError(rule.getLabel() + ": missing value for mandatory "
-                           + "attribute '" + attr.getName() + "' in '"
-                           + name + "' rule", eventHandler);
-        }
+      if (definedAttrIndices.get(attrIndex)) {
+        continue;
+      }
+      Attribute attr = getAttribute(attrIndex);
+      if (attr.isMandatory()) {
+        rule.reportError(
+            String.format(
+                "%s: missing value for mandatory attribute '%s' in '%s' rule",
+                rule.getLabel(),
+                attr.getName(),
+                name),
+            eventHandler);
+      }
 
-        if (attr.hasComputedDefault()) {
-          attrsWithComputedDefaults.add(attr);
-        } else {
-          Object defaultValue = getAttributeNoncomputedDefaultValue(attr, pkgBuilder);
-          checkAttrValNonEmpty(rule, eventHandler, defaultValue, attrIndex);
-          rule.setAttributeValue(attr, defaultValue, /*explicit=*/false);
-          checkAllowedValues(rule, attr, eventHandler);
-        }
+      if (attr.hasComputedDefault()) {
+        // Note that it is necessary to set all non-computed default values before calling
+        // Attribute#getDefaultValue for computed default attributes. Computed default attributes
+        // may have a condition predicate (i.e. the predicate returned by Attribute#getCondition)
+        // that depends on non-computed default attribute values, and that condition predicate is
+        // evaluated by the call to Attribute#getDefaultValue.
+        attrsWithComputedDefaults.add(attr);
+      } else {
+        Object defaultValue = getAttributeNoncomputedDefaultValue(attr, pkgBuilder);
+        checkAttrValNonEmpty(rule, eventHandler, defaultValue, attr);
+        rule.setAttributeValue(attr, defaultValue, /*explicit=*/ false);
+        checkAllowedValues(rule, attr, eventHandler);
       }
     }
 
-    // Evaluate and set any computed defaults now that all non-computed
-    // TODO(bazel-team): remove this special casing. Thanks to configurable attributes refactoring,
-    // computed defaults don't get bound to their final values at this point, so we no longer
-    // have to wait until regular attributes have been initialized.
+    // Set computed default attribute values now that all other (i.e. non-computed) default values
+    // have been set.
     for (Attribute attr : attrsWithComputedDefaults) {
-      rule.setAttributeValue(attr, attr.getDefaultValue(rule), /*explicit=*/false);
+      // If Attribute#hasComputedDefault is true, Attribute#getDefaultValue returns the computed
+      // default function object. Note that we don't evaluate the computed default function here
+      // because it may depend on other attribute values that are configurable (i.e. they came
+      // from select({..}) expressions in the build language, and they require configuration data
+      // from the analysis phase to be resolved). We're setting the attribute value to a
+      // reference to the computed default function.
+      rule.setAttributeValue(attr, attr.getDefaultValue(rule), /*explicit=*/ false);
     }
-
-    // Now that all attributes are bound to values, collect and store configurable attribute keys.
-    populateConfigDependenciesAttribute(rule);
-    checkForDuplicateLabels(rule, eventHandler);
-    checkThirdPartyRuleHasLicense(rule, pkgBuilder, eventHandler);
-    checkForValidSizeAndTimeoutValues(rule, eventHandler);
   }
 
   /**
@@ -1398,9 +1485,7 @@ public final class RuleClass {
   }
 
   private void checkAttrValNonEmpty(
-      Rule rule, EventHandler eventHandler, Object attributeValue, Integer attrIndex) {
-
-    Attribute attr = getAttribute(attrIndex);
+      Rule rule, EventHandler eventHandler, Object attributeValue, Attribute attr) {
     if (!attr.isNonEmpty()) {
       return;
     }
@@ -1525,65 +1610,64 @@ public final class RuleClass {
   }
 
   /**
-   * Sets the value of attribute "attrName" in rule "rule", by converting the
-   * build-language value "attrVal" to the appropriate type for the attribute.
-   * Returns the attribute index iff successful, null otherwise.
+   * Sets the value of attribute {@code attr} in {@code rule} to the native value {@code
+   * nativeAttrVal}, and sets the value's explicitness to {@code explicit}.
+   *
+   * <p>Handles the special case of the "visibility" attribute by also setting the rule's
+   * visibility with {@link Rule#setVisibility}.
    *
-   * <p>In case of failure, error messages are reported on "handler", and "rule"
-   * is marked as containing errors.
+   * <p>Checks that {@code nativeAttrVal} is an allowed value via {@link #checkAllowedValues}.
    */
-  @SuppressWarnings("unchecked")
-  private Integer setRuleAttributeValue(Rule rule,
-                                        EventHandler eventHandler,
-                                        String attrName,
-                                        Object attrVal) {
-    if (attrName.equals("name")) {
-      return null; // "name" is handled specially
-    }
-
-    Integer attrIndex = getAttributeIndex(attrName);
-    if (attrIndex == null) {
-      rule.reportError(rule.getLabel() + ": no such attribute '" + attrName
-          + "' in '" + name + "' rule", eventHandler);
-      return null;
-    }
-
-    Attribute attr = getAttribute(attrIndex);
-    Object converted;
-    try {
-      String what = "attribute '" + attrName + "' in '" + name + "' rule";
-      converted = BuildType.selectableConvert(attr.getType(), attrVal, what, rule.getLabel());
-
-      if ((converted instanceof SelectorList<?>) && !attr.isConfigurable()) {
-        rule.reportError(rule.getLabel() + ": attribute \"" + attr.getName()
-            + "\" is not configurable", eventHandler);
-        return null;
+  private static void setRuleAttributeValue(
+      Rule rule,
+      EventHandler eventHandler,
+      Attribute attr,
+      Object nativeAttrVal,
+      boolean explicit) {
+    if (attr.getName().equals("visibility")) {
+      @SuppressWarnings("unchecked")
+      List<Label> attrList = (List<Label>) nativeAttrVal;
+      if (!attrList.isEmpty()
+          && ConstantRuleVisibility.LEGACY_PUBLIC_LABEL.equals(attrList.get(0))) {
+        rule.reportError(
+            rule.getLabel() + ": //visibility:legacy_public only allowed in package declaration",
+            eventHandler);
       }
+      rule.setVisibility(PackageFactory.getVisibility(rule.getLabel(), attrList));
+    }
+    rule.setAttributeValue(attr, nativeAttrVal, explicit);
+    checkAllowedValues(rule, attr, eventHandler);
+  }
 
-      if ((converted instanceof List<?>) && !(converted instanceof GlobList<?>)) {
-        if (attr.isOrderIndependent()) {
-          converted = Ordering.natural().sortedCopy((List<? extends Comparable<?>>) converted);
-        }
-        converted = ImmutableList.copyOf((List<?>) converted);
-      }
-    } catch (Type.ConversionException e) {
-      rule.reportError(rule.getLabel() + ": " + e.getMessage(), eventHandler);
-      return null;
+  /**
+   * Converts the build-language-typed {@code buildLangValue} to a native value via {@link
+   * BuildType#selectableConvert}. Canonicalizes the value's order if it is a {@link List} type
+   * (but not a {@link GlobList}) and {@code attr.isOrderIndependent()} returns {@code true}.
+   *
+   * <p>Throws {@link ConversionException} if the conversion fails, or if {@code buildLangValue}
+   * is a selector expression but {@code attr.isConfigurable()} is {@code false}.
+   */
+  private static Object convertFromBuildLangType(Rule rule, Attribute attr, Object buildLangValue)
+      throws ConversionException {
+    String what = String.format("attribute '%s' in '%s' rule", attr.getName(), rule.getRuleClass());
+    Object converted =
+        BuildType.selectableConvert(attr.getType(), buildLangValue, what, rule.getLabel());
+
+    if ((converted instanceof SelectorList<?>) && !attr.isConfigurable()) {
+      throw new ConversionException(
+          String.format("attribute \"%s\" is not configurable", attr.getName()));
     }
 
-    if (attrName.equals("visibility")) {
-      List<Label> attrList = (List<Label>) converted;
-      if (!attrList.isEmpty()
-          && ConstantRuleVisibility.LEGACY_PUBLIC_LABEL.equals(attrList.get(0))) {
-        rule.reportError(rule.getLabel() + ": //visibility:legacy_public only allowed in package "
-            + "declaration", eventHandler);
+    if ((converted instanceof List<?>) && !(converted instanceof GlobList<?>)) {
+      if (attr.isOrderIndependent()) {
+        @SuppressWarnings("unchecked")
+        List<? extends Comparable<?>> list = (List<? extends Comparable<?>>) converted;
+        converted = Ordering.natural().sortedCopy(list);
       }
-      rule.setVisibility(PackageFactory.getVisibility(rule.getLabel(), attrList));
+      converted = ImmutableList.copyOf((List<?>) converted);
     }
 
-    rule.setAttributeValue(attr, converted, /*explicit=*/true);
-    checkAllowedValues(rule, attr, eventHandler);
-    return attrIndex;
+    return converted;
   }
 
   /**
@@ -1595,7 +1679,8 @@ public final class RuleClass {
    * <p>If the rule is configurable, all of its potential values are evaluated, and errors for each
    * of the invalid values are reported.
    */
-  void checkAllowedValues(Rule rule, Attribute attribute, EventHandler eventHandler) {
+  private static void checkAllowedValues(
+      Rule rule, Attribute attribute, EventHandler eventHandler) {
     if (attribute.checkAllowedValues()) {
       PredicateWithMessage<Object> allowedValues = attribute.getAllowedValues();
       Iterable<?> values =
@@ -1603,9 +1688,13 @@ public final class RuleClass {
               attribute.getName(), attribute.getType());
       for (Object value : values) {
         if (!allowedValues.apply(value)) {
-          rule.reportError(String.format(rule.getLabel() + ": invalid value in '%s' attribute: %s",
-              attribute.getName(),
-              allowedValues.getErrorReason(value)), eventHandler);
+          rule.reportError(
+              String.format(
+                  "%s: invalid value in '%s' attribute: %s",
+                  rule.getLabel(),
+                  attribute.getName(),
+                  allowedValues.getErrorReason(value)),
+              eventHandler);
         }
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
index 7982a62386..4837b04763 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
@@ -34,12 +34,12 @@ import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
- * Given a rule class and a set of attributes, returns a Rule instance. Also
- * performs a number of checks and associates the rule and the owning package
+ * Given a {@link RuleClass} and a set of attribute values, returns a {@link Rule} instance. Also
+ * performs a number of checks and associates the {@link Rule} and the owning {@link Package}
  * with each other.
  *
- * <p>Note: the code that actually populates the RuleClass map has been moved
- * to {@link RuleClassProvider}.
+ * <p>Note: the code that actually populates the RuleClass map has been moved to {@link
+ * RuleClassProvider}.
  */
 public class RuleFactory {
 
@@ -73,20 +73,20 @@ public class RuleFactory {
    * Creates and returns a rule instance.
    *
    * <p>It is the caller's responsibility to add the rule to the package (the caller may choose not
-   * to do so if, for example, the rule has errors).</p>
+   * to do so if, for example, the rule has errors).
    */
   static Rule createRule(
       Package.Builder pkgBuilder,
       RuleClass ruleClass,
-      Map<String, Object> attributeValues,
+      BuildLangTypedAttributeValuesMap attributeValues,
       EventHandler eventHandler,
-      FuncallExpression ast,
+      @Nullable FuncallExpression ast,
       Location location,
       @Nullable Environment env)
       throws InvalidRuleException, InterruptedException {
     Preconditions.checkNotNull(ruleClass);
     String ruleClassName = ruleClass.getName();
-    Object nameObject = attributeValues.get("name");
+    Object nameObject = attributeValues.getAttributeValue("name");
     if (nameObject == null) {
       throw new InvalidRuleException(ruleClassName + " rule has no 'name' attribute");
     } else if (!(nameObject instanceof String)) {
@@ -113,38 +113,48 @@ public class RuleFactory {
     AttributesAndLocation generator =
         generatorAttributesForMacros(attributeValues, env, location, label);
     try {
-      return ruleClass.createRuleWithLabel(
-          pkgBuilder, label, generator.attributes, eventHandler, ast, generator.location);
+      return ruleClass.createRule(
+          pkgBuilder,
+          label,
+          generator.attributes,
+          eventHandler,
+          ast,
+          generator.location,
+          new AttributeContainer(ruleClass));
     } catch (LabelSyntaxException e) {
       throw new RuleFactory.InvalidRuleException(ruleClass + " " + e.getMessage());
     }
   }
 
   /**
-   * Creates a rule instance, adds it to the package and returns it.
+   * Creates a {@link Rule} instance, adds it to the {@link Package.Builder} and returns it.
    *
-   * @param pkgBuilder the under-construction package to which the rule belongs
-   * @param ruleClass the class of the rule; this must not be null
-   * @param attributeValues a map of attribute names to attribute values. Each
-   *        attribute must be defined for this class of rule, and have a value
-   *        of the appropriate type. There must be a map entry for each
-   *        non-optional attribute of this class of rule.
+   * @param pkgBuilder the under-construction {@link Package.Builder} to which the rule belongs
+   * @param ruleClass the {@link RuleClass} of the rule
+   * @param attributeValues a {@link BuildLangTypedAttributeValuesMap} mapping attribute names to
+   *     attribute values of build-language type. Each attribute must be defined for this class of
+   *     rule, and have a build-language-typed value which can be converted to the appropriate
+   *     native type of the attribute (i.e. via {@link BuildType#selectableConvert}). There must
+   *     be a map entry for each non-optional attribute of this class of rule.
    * @param eventHandler a eventHandler on which errors and warnings are reported during
-   *        rule creation
+   *     rule creation
    * @param ast the abstract syntax tree of the rule expression (optional)
    * @param location the location at which this rule was declared
+   * @param env the lexical environment of the function call which declared this rule (optional)
    * @throws InvalidRuleException if the rule could not be constructed for any
-   *         reason (e.g. no <code>name</code> attribute is defined)
-   * @throws InvalidRuleException, NameConflictException
+   *     reason (e.g. no {@code name} attribute is defined)
+   * @throws NameConflictException if the rule's name or output files conflict with others in this
+   *     package
+   * @throws InterruptedException if interrupted
    */
   static Rule createAndAddRule(
       Package.Builder pkgBuilder,
       RuleClass ruleClass,
-      Map<String, Object> attributeValues,
+      BuildLangTypedAttributeValuesMap attributeValues,
       EventHandler eventHandler,
-      FuncallExpression ast,
+      @Nullable FuncallExpression ast,
       Location location,
-      Environment env)
+      @Nullable Environment env)
       throws InvalidRuleException, NameConflictException, InterruptedException {
     Rule rule = createRule(
         pkgBuilder, ruleClass, attributeValues, eventHandler, ast, location, env);
@@ -152,12 +162,31 @@ public class RuleFactory {
     return rule;
   }
 
+  /**
+   * Creates a {@link Rule} instance, adds it to the {@link Package.Builder} and returns it.
+   *
+   * @param context the package-building context in which this rule was declared
+   * @param ruleClass the {@link RuleClass} of the rule
+   * @param attributeValues a {@link BuildLangTypedAttributeValuesMap} mapping attribute names to
+   *     attribute values of build-language type. Each attribute must be defined for this class
+   *     of rule, and have a build-language-typed value which can be converted to the appropriate
+   *     native type of the attribute (i.e. via {@link BuildType#selectableConvert}). There must
+   *     be a map entry for each non-optional attribute of this class of rule.
+   * @param ast the abstract syntax tree of the rule expression (mandatory because this looks up a
+   *     {@link Location} from the {@code ast})
+   * @param env the lexical environment of the function call which declared this rule (optional)
+   * @throws InvalidRuleException if the rule could not be constructed for any reason (e.g. no
+   *     {@code name} attribute is defined)
+   * @throws NameConflictException if the rule's name or output files conflict with others in this
+   *     package
+   * @throws InterruptedException if interrupted
+   */
   public static Rule createAndAddRule(
       PackageContext context,
       RuleClass ruleClass,
-      Map<String, Object> attributeValues,
+      BuildLangTypedAttributeValuesMap attributeValues,
       FuncallExpression ast,
-      Environment env)
+      @Nullable Environment env)
       throws InvalidRuleException, NameConflictException, InterruptedException {
     return createAndAddRule(
         context.pkgBuilder,
@@ -170,8 +199,8 @@ public class RuleFactory {
   }
 
   /**
-   * InvalidRuleException is thrown by createRule() if the Rule could not be
-   * constructed. It contains an error message.
+   * InvalidRuleException is thrown by {@link Rule} creation methods if the {@link Rule} could
+   * not be constructed. It contains an error message.
    */
   public static class InvalidRuleException extends Exception {
     private InvalidRuleException(String message) {
@@ -179,32 +208,89 @@ public class RuleFactory {
     }
   }
 
-  /** Pair of attributes and location */
+  /** A pair of attributes and location. */
   private static final class AttributesAndLocation {
-    final Map<String, Object> attributes;
+    final BuildLangTypedAttributeValuesMap attributes;
     final Location location;
 
-    AttributesAndLocation(Map<String, Object> attributes, Location location) {
+    AttributesAndLocation(BuildLangTypedAttributeValuesMap attributes, Location location) {
       this.attributes = attributes;
       this.location = location;
     }
   }
 
   /**
+   * A wrapper around an map of named attribute values that specifies whether the map's values
+   * are of "build-language" or of "native" types.
+   */
+  public interface AttributeValuesMap {
+    /**
+     * Returns {@code true} if all the map's values are "build-language typed", i.e., resulting
+     * from the evaluation of an expression in the build language. Returns {@code false} if all
+     * the map's values are "natively typed", i.e. of a type returned by {@link
+     * BuildType#selectableConvert}.
+     */
+    boolean valuesAreBuildLanguageTyped();
+
+    Iterable<String> getAttributeNames();
+
+    Object getAttributeValue(String attributeName);
+
+    boolean isAttributeExplicitlySpecified(String attributeName);
+  }
+
+  /** A {@link AttributeValuesMap} of explicit "build-language" values. */
+  public static final class BuildLangTypedAttributeValuesMap implements AttributeValuesMap {
+
+    private final Map<String, Object> attributeValues;
+
+    public BuildLangTypedAttributeValuesMap(Map<String, Object> attributeValues) {
+      this.attributeValues = attributeValues;
+    }
+
+    private boolean containsAttributeNamed(String attributeName) {
+      return attributeValues.containsKey(attributeName);
+    }
+
+    @Override
+    public boolean valuesAreBuildLanguageTyped() {
+      return true;
+    }
+
+    @Override
+    public Iterable<String> getAttributeNames() {
+      return attributeValues.keySet();
+    }
+
+    @Override
+    public Object getAttributeValue(String attributeName) {
+      return attributeValues.get(attributeName);
+    }
+
+    @Override
+    public boolean isAttributeExplicitlySpecified(String attributeName) {
+      return true;
+    }
+  }
+
+  /**
    * If the rule was created by a macro, this method sets the appropriate values for the
    * attributes generator_{name, function, location} and returns all attributes.
    *
    * <p>Otherwise, it returns the given attributes without any changes.
    */
   private static AttributesAndLocation generatorAttributesForMacros(
-      Map<String, Object> args, @Nullable Environment env, Location location, Label label) {
+      BuildLangTypedAttributeValuesMap args,
+      @Nullable Environment env,
+      Location location,
+      Label label) {
     // Returns the original arguments if a) there is only the rule itself on the stack
     // trace (=> no macro) or b) the attributes have already been set by Python pre-processing.
     if (env == null) {
       return new AttributesAndLocation(args, location);
     }
-    boolean hasName = args.containsKey("generator_name");
-    boolean hasFunc = args.containsKey("generator_function");
+    boolean hasName = args.containsAttributeNamed("generator_name");
+    boolean hasFunc = args.containsAttributeNamed("generator_function");
     // TODO(bazel-team): resolve cases in our code where hasName && !hasFunc, or hasFunc && !hasName
     if (hasName || hasFunc) {
       return new AttributesAndLocation(args, location);
@@ -217,23 +303,25 @@ public class RuleFactory {
     FuncallExpression generator = topCall.first;
     BaseFunction function = topCall.second;
     String name = generator.getNameArg();
+    
     ImmutableMap.Builder<String, Object> builder = ImmutableMap.builder();
-
-    builder.putAll(args);
-    builder.put("generator_name", (name == null) ? args.get("name") : name);
+    for (String attributeName : args.getAttributeNames()) {
+      builder.put(attributeName, args.getAttributeValue(attributeName));
+    }
+    builder.put("generator_name", (name == null) ? args.getAttributeValue("name") : name);
     builder.put("generator_function", function.getName());
 
     if (generator.getLocation() != null) {
       location = generator.getLocation();
     }
-
     String relativePath = maybeGetRelativeLocation(location, label);
     if (relativePath != null) {
       builder.put("generator_location", relativePath);
     }
 
     try {
-      return new AttributesAndLocation(builder.build(), location);
+      return new AttributesAndLocation(
+          new BuildLangTypedAttributeValuesMap(builder.build()), location);
     } catch (IllegalArgumentException ex) {
       // We just fall back to the default case and swallow any messages.
       return new AttributesAndLocation(args, location);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java b/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java
index 658515cb49..f7470e92e6 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java
@@ -36,48 +36,34 @@ public class RuleSerializer {
     builder.setName(rule.getLabel().getName());
     builder.setRuleClass(rule.getRuleClass());
     builder.setPublicByDefault(rule.getRuleClassObject().isPublicByDefault());
+
     RawAttributeMapper rawAttributeMapper = RawAttributeMapper.of(rule);
+    boolean isSkylark = rule.getRuleClassObject().isSkylark();
+
     for (Attribute attr : rule.getAttributes()) {
       Object rawAttributeValue = rawAttributeMapper.getRawAttributeValue(rule, attr);
+      boolean isExplicit = rule.isAttributeValueExplicitlySpecified(attr);
+
+      if (!isSkylark && !isExplicit) {
+        // If the rule class is native (i.e. not Skylark-defined), then we can skip serialization
+        // of implicit attribute values. The native rule class can provide the same default value
+        // for the attribute after deserialization.
+        continue;
+      }
 
       Object valueToSerialize;
-      if (rawAttributeValue instanceof ComputedDefault) {
-        if (rule.getRuleClassObject().isSkylark()) {
-          // If the rule class is Skylark-defined (i.e. rule.getRuleClassObject().isSkylark() is
-          // true), and the attribute has a ComputedDefault value, we must serialize it. The
-          // Skylark-defined ComputedDefault function won't be available after deserialization due
-          // to Skylark's non-serializability. Fortunately (from the perspective of rule
-          // serialization), Skylark doesn't support defining rule classes with ComputedDefault
-          // attributes, and so the only ComputedDefault attributes we need to worry about for
-          // Skylark-defined rule classes are those declared in those rule classes' natively
-          // defined base rule classes.
-          //
-          // See the comment for SKYLARK_RULE_CLASS_COMPUTED_DEFAULT_ATTRIBUTES for the locations
-          // of these expected attributes.
-          //
-          // The RawAttributeMapper#get method, inherited from AbstractAttributeMapper, evaluates
-          // the ComputedDefault function, so we use that, after verifying the attribute's name is
-          // expected.
-          Preconditions.checkState(
-              SKYLARK_RULE_CLASS_COMPUTED_DEFAULT_ATTRIBUTES.contains(attr.getName()),
-              "Unexpected ComputedDefault value for %s in %s",
-              attr,
-              rule);
-          valueToSerialize = rawAttributeMapper.get(attr.getName(), attr.getType());
-        } else {
-          // If the rule class is native (i.e. not Skylark-defined), we can skip serialization of
-          // attributes with ComputedDefault values. The native rule class can provide the same
-          // ComputedDefault value for the attribute after deserialization.
-          //
-          // TODO(mschaller): While the native rule class *could* provide it, it doesn't yet. Make
-          // it so! For now, we fall back to flattening the set of all possible values, computed
-          // using AggregatingAttributeMapper.
-          Iterable<Object> possibleValues =
-              AggregatingAttributeMapper.of(rule).getPossibleAttributeValues(rule, attr);
-          valueToSerialize =
-              AggregatingAttributeMapper.flattenAttributeValues(attr.getType(), possibleValues);
-        }
+      if (isExplicit) {
+        valueToSerialize = rawAttributeValue;
+      } else if (rawAttributeValue instanceof ComputedDefault) {
+        // If the rule class is Skylark-defined (i.e. rule.getRuleClassObject().isSkylark() is
+        // true), and the attribute has a ComputedDefault value, then we must serialize what it
+        // evaluates to. The Skylark-defined ComputedDefault function won't be available after
+        // deserialization due to Skylark's non-serializability.
+        valueToSerialize = evaluateSkylarkComputedDefault(rule, rawAttributeMapper, attr);
       } else {
+        // If the rule class is Skylark-defined and the attribute value is implicit, then we
+        // must serialize it. The Skylark-defined rule class won't be available after
+        // deserialization due to Skylark's non-serializability.
         valueToSerialize = rawAttributeValue;
       }
 
@@ -85,11 +71,38 @@ public class RuleSerializer {
           AttributeSerializer.getAttributeProto(
               attr,
               valueToSerialize,
-              rule.isAttributeValueExplicitlySpecified(attr),
+              isExplicit,
               /*includeGlobs=*/ true,
               /*encodeBooleanAndTriStateAsIntegerAndString=*/ false));
     }
     return builder;
   }
+
+  /**
+   * Evaluates a {@link ComputedDefault} attribute value for a {@link Rule} with a
+   * Skylark-defined {@link RuleClass}.
+   *
+   * <p>Fortunately (from the perspective of rule serialization), Skylark doesn't support defining
+   * rule classes with {@link ComputedDefault} attributes, and so the only {@link
+   * ComputedDefault} attributes we need to worry about for Skylark-defined rule classes are
+   * declared in those rule classes' natively-defined base rule classes.
+   *
+   * <p>See the comment for {@link #SKYLARK_RULE_CLASS_COMPUTED_DEFAULT_ATTRIBUTES} for the
+   * locations of these expected attributes. None of them have dependencies on other attributes
+   * which are configurable, so they can be evaluated here without loss of fidelity.
+   *
+   * <p>The {@link RawAttributeMapper#get} method, inherited from {@link
+   * AbstractAttributeMapper}, evaluates the {@link ComputedDefault} function, so we use that,
+   * after verifying the attribute's name is expected.
+   */
+  private static Object evaluateSkylarkComputedDefault(
+      Rule rule, RawAttributeMapper rawAttributeMapper, Attribute attr) {
+    Preconditions.checkState(
+        SKYLARK_RULE_CLASS_COMPUTED_DEFAULT_ATTRIBUTES.contains(attr.getName()),
+        "Unexpected ComputedDefault value for %s in %s",
+        attr,
+        rule);
+    return rawAttributeMapper.get(attr.getName(), attr.getType());
+  }
 }