Improve fidelity of proto configured attribute serialization

--
MOS_MIGRATED_REVID=122889821

2 files changed, 41 insertions, 17 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/AttributeSerializer.java b/src/main/java/com/google/devtools/build/lib/packages/AttributeSerializer.java
index 4e67c39e8f..c38111d178 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AttributeSerializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AttributeSerializer.java
@@ -127,20 +127,28 @@ public class AttributeSerializer {
         Build.Attribute.SelectorList.newBuilder();
     selectorListBuilder.setType(ProtoUtils.getDiscriminatorFromType(type));
     for (Selector<?> selector : selectorList.getSelectors()) {
-      Build.Attribute.Selector.Builder selectorBuilder = Build.Attribute.Selector.newBuilder();
+      Build.Attribute.Selector.Builder selectorBuilder = Build.Attribute.Selector.newBuilder()
+          .setNoMatchError(selector.getNoMatchError())
+          .setHasDefaultValue(selector.hasDefault());
+
       // Note that the order of entries returned by selector.getEntries is stable. The map's
       // entries' order is preserved from the sorting performed by the SelectorValue constructor.
       for (Entry<Label, ?> entry : selector.getEntries().entrySet()) {
-        Builder selectorEntryBuilder = SelectorEntry.newBuilder();
-        selectorEntryBuilder.setLabel(entry.getKey().toString());
-        writeAttributeValueToBuilder(
-            new SelectorEntryBuilderAdapter(selectorEntryBuilder),
-            type,
-            entry.getValue(),
-            includeGlobs);
+        Label condition = entry.getKey();
+        Builder selectorEntryBuilder = SelectorEntry.newBuilder()
+            .setLabel(condition.toString())
+            .setIsDefaultValue(!selector.isValueSet(condition));
+
+        Object conditionValue = entry.getValue();
+        if (conditionValue != null) {
+          writeAttributeValueToBuilder(
+              new SelectorEntryBuilderAdapter(selectorEntryBuilder),
+              type,
+              conditionValue,
+              includeGlobs);
+        }
         selectorBuilder.addEntries(selectorEntryBuilder);
       }
-
       selectorListBuilder.addElements(selectorBuilder);
     }
     attrPb.setSelectorList(selectorListBuilder);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
index dd05744f5f..bbef95be91 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
@@ -385,7 +385,7 @@ public final class BuildType {
               context, originalType, ((SelectorValue) elem).getNoMatchError()));
         } else {
           T directValue = originalType.convert(elem, what, context);
-          builder.add(new Selector<T>(ImmutableMap.of(Selector.DEFAULT_CONDITION_KEY, directValue),
+          builder.add(new Selector<>(ImmutableMap.of(Selector.DEFAULT_CONDITION_KEY, directValue),
               what, context, originalType));
         }
       }
@@ -443,7 +443,8 @@ public final class BuildType {
         Label.parseAbsoluteUnchecked(DEFAULT_CONDITION_KEY);
 
     private final Type<T> originalType;
-    private final Map<Label, T> map; // Can hold null values.
+    // Can hold null values, underlying implementation should be ordered.
+    private final Map<Label, T> map;
     private final Set<Label> conditionsWithDefaultValues;
     private final String noMatchError;
     private final boolean hasDefaultCondition;
@@ -451,11 +452,9 @@ public final class BuildType {
     /**
      * Creates a new Selector using the default error message when no conditions match.
      */
-    @VisibleForTesting
     Selector(ImmutableMap<?, ?> x, String what, @Nullable Label context, Type<T> originalType)
         throws ConversionException {
       this(x, what, context, originalType, "");
-
     }
 
     /**
@@ -464,7 +463,7 @@ public final class BuildType {
     Selector(ImmutableMap<?, ?> x, String what, @Nullable Label context, Type<T> originalType,
         String noMatchError) throws ConversionException {
       this.originalType = originalType;
-      Map<Label, T> result = new LinkedHashMap<>();
+      LinkedHashMap<Label, T> result = new LinkedHashMap<>();
       ImmutableSet.Builder<Label> defaultValuesBuilder = ImmutableSet.builder();
       boolean foundDefaultCondition = false;
       for (Entry<?, ?> entry : x.entrySet()) {
@@ -480,10 +479,27 @@ public final class BuildType {
           result.put(key, originalType.convert(entry.getValue(), what, context));
         }
       }
-      map = Collections.unmodifiableMap(result);
+      this.map = Collections.unmodifiableMap(result);
+      this.noMatchError = noMatchError;
+      this.conditionsWithDefaultValues = defaultValuesBuilder.build();
+      this.hasDefaultCondition = foundDefaultCondition;
+    }
+
+    /**
+     * Create a new Selector from raw values. A defensive copy of the supplied map is <i>not</i>
+     * made, so it imperative that it is not modified following construction.
+     */
+    Selector(
+        LinkedHashMap<Label, T> map,
+        Type<T> originalType,
+        String noMatchError,
+        ImmutableSet<Label> conditionsWithDefaultValues,
+        boolean hasDefaultCondition) {
+      this.originalType = originalType;
+      this.map = Collections.unmodifiableMap(map);
       this.noMatchError = noMatchError;
-      conditionsWithDefaultValues = defaultValuesBuilder.build();
-      hasDefaultCondition = foundDefaultCondition;
+      this.conditionsWithDefaultValues = conditionsWithDefaultValues;
+      this.hasDefaultCondition = hasDefaultCondition;
     }
 
     /**