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
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 6ae21d9..f3875b6 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 @@ 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 @@ } 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 @@ 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 1c56072..960432d 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ } 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 d7008ed..9d40bb4 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 @@ 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 6cc12d0..7de23d7 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 @@ } /** - * 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 fe93ba4..b23f0c0 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 @@ // 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; }
diff --git a/src/main/protobuf/build.proto b/src/main/protobuf/build.proto index b5d6346..d212cbb 100644 --- a/src/main/protobuf/build.proto +++ b/src/main/protobuf/build.proto
@@ -137,17 +137,21 @@ // 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 @@ -155,6 +159,7 @@ 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.