PackageSerializer: include attributes with null values.

PackageDeserializer: handle null-value attributes (single-value attributes with
no value setting) without crashing.

Without this change, attributes with computed defaults can crash on serialization
because RawAttributeMapper.isNotNull isn't smart enough to check *indirect*
configurable attributes that the computed attribute depends on.

--
MOS_MIGRATED_REVID=89599145

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

@@ -58,7 +58,14 @@ public <T> T get(String attributeName, Type<T> type) {
     if (value instanceof Attribute.ComputedDefault) {
       value = ((Attribute.ComputedDefault) value).getDefault(this);
     }
-    return type.cast(value);
+    try {
+      return type.cast(value);
+    } catch (ClassCastException e) {
+      // getIndexWithTypeCheck checks the type is right, but unexpected configurable attributes
+      // can still trigger cast exceptions.
+      throw new IllegalArgumentException(
+          "wrong type for attribute \"" + attributeName + "\" in rule " + ruleLabel, e);
+    }
   }
 
   /**
@@ -174,7 +181,7 @@ protected <T> Type.Selector<T> getSelector(String attributeName, Type<T> type) {
     }
     if (((Type.Selector<?>) attrValue).getOriginalType() != type) {
       throw new IllegalArgumentException("Attribute " + attributeName
-          + " is not of type " + type + " in rule " + ruleLabel.getName());
+          + " is not of type " + type + " in rule " + ruleLabel);
     }
     return (Type.Selector<T>) attrValue;
   }
@@ -192,7 +199,7 @@ private int getIndexWithTypeCheck(String attrName, Type<?> type) {
     Attribute attr = ruleClass.getAttribute(index);
     if (attr.getType() != type) {
       throw new IllegalArgumentException("Attribute " + attrName
-          + " is not of type " + type + " in rule " + ruleLabel.getName());
+          + " is not of type " + type + " in rule " + ruleLabel);
     }
     return index;
   }

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

@@ -159,8 +159,8 @@ private ExplicitLocation(Path path, Build.Location location) {
           location.hasStartOffset() && location.hasEndOffset() ? location.getStartOffset() : 0,
           location.hasStartOffset() && location.hasEndOffset() ? location.getEndOffset() : 0);
       this.path = path.asFragment();
-      if (location.hasStartLine() && location.hasStartColumn() &&
-          location.hasEndLine() && location.hasEndColumn()) {
+      if (location.hasStartLine() && location.hasStartColumn()
+          && location.hasEndLine() && location.hasEndColumn()) {
         this.startLine = location.getStartLine();
         this.startColumn = location.getStartColumn();
         this.endLine = location.getEndLine();
@@ -282,8 +282,8 @@ private static List<FilesetEntry> deserializeFilesetEntries(
       List<Label> files =
           filesetPb.getFilesPresent() ? deserializeLabels(filesetPb.getFileList()) : null;
       List<String> excludes =
-          filesetPb.getExcludeList().isEmpty() ?
-              null : ImmutableList.copyOf(filesetPb.getExcludeList());
+          filesetPb.getExcludeList().isEmpty()
+              ? null : ImmutableList.copyOf(filesetPb.getExcludeList());
       String destDir = filesetPb.getDestinationDirectory();
       FilesetEntry.SymlinkBehavior symlinkBehavior =
           pbToSymlinkBehavior(filesetPb.getSymlinkBehavior());
@@ -442,18 +442,20 @@ private static Object deserializeAttributeValue(Type<?> expectedType,
       throws PackageDeserializationException {
     switch (attrPb.getType()) {
       case INTEGER:
-        return new Integer(attrPb.getIntValue());
+        return attrPb.hasIntValue() ? new Integer(attrPb.getIntValue()) : null;
 
       case STRING:
-        if (expectedType == Type.NODEP_LABEL) {
+        if (!attrPb.hasStringValue()) {
+          return null;
+        } else if (expectedType == Type.NODEP_LABEL) {
           return deserializeLabel(attrPb.getStringValue());
         } else {
           return attrPb.getStringValue();
         }
 
       case LABEL:
       case OUTPUT:
-        return deserializeLabel(attrPb.getStringValue());
+        return attrPb.hasStringValue() ? deserializeLabel(attrPb.getStringValue()) : null;
 
       case STRING_LIST:
         if (expectedType == Type.NODEP_LABEL_LIST) {
@@ -470,7 +472,7 @@ private static Object deserializeAttributeValue(Type<?> expectedType,
         return deserializeDistribs(attrPb.getStringListValueList());
 
       case LICENSE:
-        return deserializeLicense(attrPb.getLicense());
+        return attrPb.hasLicense() ? deserializeLicense(attrPb.getLicense()) : null;
 
       case STRING_DICT: {
         ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
@@ -508,10 +510,10 @@ private static Object deserializeAttributeValue(Type<?> expectedType,
       }
 
       case BOOLEAN:
-        return attrPb.getBooleanValue();
+        return attrPb.hasBooleanValue() ? attrPb.getBooleanValue() : null;
 
       case TRISTATE:
-        return deserializeTriStateValue(attrPb.getStringValue());
+        return attrPb.hasStringValue() ? deserializeTriStateValue(attrPb.getStringValue()) : null;
 
       default:
           throw new PackageDeserializationException("Invalid discriminator: " + attrPb.getType());

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

@@ -112,11 +112,9 @@ private static Build.Rule serializeRule(Rule rule) {
     result.setRuleClass(rule.getRuleClass());
     result.setParseableLocation(serializeLocation(rule.getLocation()));
     for (Attribute attribute : rule.getAttributes()) {
-      if (!RawAttributeMapper.of(rule).isNull(attribute.getName(), attribute.getType())) {
-        PackageSerializer.addAttributeToProto(result, attribute,
-            getAttributeValues(rule, attribute), rule.getAttributeLocation(attribute.getName()),
+      PackageSerializer.addAttributeToProto(result, attribute,
+          getAttributeValues(rule, attribute), rule.getAttributeLocation(attribute.getName()),
           rule.isAttributeValueExplicitlySpecified(attribute), true);
-      }
     }
 
     return result.build();

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

@@ -87,13 +87,6 @@ public <T> boolean isConfigurable(String attributeName, Type<T> type) {
     return getSelector(attributeName, type) != null;
   }
 
-  /**
-   * Returns true if this attribute has a null value.
-   */
-  public <T> boolean isNull(String attributeName, Type<T> type) {
-    return !isConfigurable(attributeName, type) && (get(attributeName, type) == null);
-  }
-
   /**
    * If the attribute is configurable for this rule instance, returns its configuration
    * keys. Else returns an empty list.

src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java

@@ -375,7 +375,7 @@ public static boolean appearsToHaveNoObjectFiles(AttributeMap rule) {
     // TODO(bazel-team): remove this hack for a more principled solution.
     try {
       rule.get("srcs", Type.LABEL_LIST);
-    } catch (ClassCastException e) {
+    } catch (IllegalArgumentException e) {
       // "srcs" is actually a configurable selector. Assume object files are possible somewhere.
       return false;
     }

src/main/protobuf/build.proto

@@ -137,24 +137,29 @@ message Attribute {
   // If this attribute has an integer value this will be populated.
   // Boolean and TriState also use this field as [0,1] and [-1,0,1]
   // for [false, true] and [auto, no, yes] respectively.
+  // Null-valued attributes will *not* set this value.
   optional int32 int_value = 3;
 
   // If the attribute has a string value this will be populated.  Label and
   // path attributes use this field as the value even though the type may
   // be LABEL or something else other than STRING.
+  // Null-valued attributes will *not* set this value.
   optional string string_value = 5;
 
-  // If the attribute has a boolean value this will be populated
+  // If the attribute has a boolean value this will be populated.
+  // Null-valued attributes will *not* set this value.
   optional bool boolean_value = 14;
 
   // If the attribute is a Tristate value, this will be populated.
+  // Null-valued attributes will *not* set this value.
   optional Tristate tristate_value = 15;
 
   // The value of the attribute has a list of string values (label and path
   // note from STRING applies here as well).
   repeated string string_list_value = 6;
 
   // If this is a license attribute, the license information is stored here.
+  // Null-valued attributes will *not* set this value.
   optional License license = 7;
 
   // If this is a string dict, each entry will be stored here.