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

5 files changed, 25 insertions, 25 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
index 6ae21d9a1d..f3875b65a9 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
@@ -58,7 +58,14 @@ public abstract class AbstractAttributeMapper implements AttributeMap {
     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 @@ public abstract class AbstractAttributeMapper implements AttributeMap {
     }
     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 @@ public abstract class AbstractAttributeMapper implements AttributeMap {
     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;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java b/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
index 1c56072dc3..960432db85 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java
@@ -159,8 +159,8 @@ public class PackageDeserializer {
           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 @@ public class PackageDeserializer {
       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,10 +442,12 @@ public class PackageDeserializer {
       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();
@@ -453,7 +455,7 @@ public class PackageDeserializer {
 
       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 @@ public class PackageDeserializer {
         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 @@ public class PackageDeserializer {
       }
 
       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());
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java b/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
index d7008eddb3..9d40bb4aca 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java
@@ -112,11 +112,9 @@ public class PackageSerializer {
     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();
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java
index 6cc12d03dd..7de23d7c19 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java
@@ -88,13 +88,6 @@ public class RawAttributeMapper extends AbstractAttributeMapper {
   }
 
   /**
-   * 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.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
index fe93ba41d2..b23f0c060b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
@@ -375,7 +375,7 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
     // 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;
     }