Merge PackageSerializer's and ProtoOutputFormatter's duplicated

serialization logic.

Among other things, this fixes an out-of-sync bug where
ProtoOutputFormatter's version knew how to handle configurable
attributes while PackageSerializer's version crashed.

The merged logic preserves ProtoOutputFormatter's semantics:
configurable attributes work, but their values are merged together
into a flattened list, so the original select structure can't be
reproduced later.

--
MOS_MIGRATED_REVID=89435116

4 files changed, 180 insertions, 399 deletions

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 83b8a64730..d7008eddb3 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
@@ -34,6 +34,7 @@ import static com.google.devtools.build.lib.packages.Type.STRING_LIST_DICT;
 import static com.google.devtools.build.lib.packages.Type.TRISTATE;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.License.DistributionType;
@@ -46,6 +47,7 @@ import com.google.devtools.build.lib.syntax.Label;
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 
@@ -110,16 +112,10 @@ public class PackageSerializer {
     result.setRuleClass(rule.getRuleClass());
     result.setParseableLocation(serializeLocation(rule.getLocation()));
     for (Attribute attribute : rule.getAttributes()) {
-      Object value = attribute.getName().equals("visibility")
-          ? rule.getVisibility().getDeclaredLabels()
-          // TODO(bazel-team): support configurable attributes. AggregatingAttributeMapper
-          // may make more sense here. Computed defaults may add complications.
-          : RawAttributeMapper.of(rule).get(attribute.getName(), attribute.getType());
-      if (value != null) {
-        PackageSerializer.addAttributeToProto(result, attribute, value,
-            rule.getAttributeLocation(attribute.getName()),
-            rule.isAttributeValueExplicitlySpecified(attribute),
-            true);
+      if (!RawAttributeMapper.of(rule).isNull(attribute.getName(), attribute.getType())) {
+        PackageSerializer.addAttributeToProto(result, attribute,
+            getAttributeValues(rule, attribute), rule.getAttributeLocation(attribute.getName()),
+          rule.isAttributeValueExplicitlySpecified(attribute), true);
       }
     }
 
@@ -180,7 +176,7 @@ public class PackageSerializer {
       case PROGRESS:
         kind = Build.Event.EventKind.PROGRESS;
         break;
-      default: throw new IllegalStateException();
+      default: throw new IllegalArgumentException("unexpected event type: " + event.getKind());
     }
 
     result.setKind(kind);
@@ -267,29 +263,48 @@ public class PackageSerializer {
   }
 
   /**
+   * Returns the possible values of the specified attribute in the specified rule. For
+   * non-configured attributes, this is a single value. For configurable attributes, this
+   * may be multiple values.
+   */
+  public static Iterable<Object> getAttributeValues(Rule rule, Attribute attr) {
+    List<Object> values = new LinkedList<>(); // Not an ImmutableList: may host null values.
+
+    if (attr.getName().equals("visibility")) {
+      values.add(rule.getVisibility().getDeclaredLabels());
+    } else {
+      for (Object o :
+          AggregatingAttributeMapper.of(rule).visitAttribute(attr.getName(), attr.getType())) {
+        values.add(o);
+      }
+    }
+
+    return values;
+  }
+
+  /**
    * Adds the serialized version of the specified attribute to the specified message.
    *
-   * @param result the message to amend
+   * @param rulePb the message to amend
    * @param attr the attribute to add
-   * @param value the value of the attribute
+   * @param values the possible values of the attribute (can be a multi-value list for
+   *              configurable attributes)
    * @param location the location of the attribute in the source file
    * @param explicitlySpecified whether the attribute was explicitly specified or not
    * @param includeGlobs add glob expression for attributes that contain them
    */
-  // TODO(bazel-team): This is a copy of the code in ProtoOutputFormatter.
   @SuppressWarnings("unchecked")
   public static void addAttributeToProto(
-      Build.Rule.Builder result, Attribute attr, Object value, Location location,
-      Boolean explicitlySpecified, boolean includeGlobs) {
+      Build.Rule.Builder rulePb, Attribute attr, Iterable<Object> values,
+      Location location, Boolean explicitlySpecified, boolean includeGlobs) {
     // Get the attribute type.  We need to convert and add appropriately
     com.google.devtools.build.lib.packages.Type<?> type = attr.getType();
 
     Build.Attribute.Builder attrPb = Build.Attribute.newBuilder();
 
     // Set the type, name and source
-    Build.Attribute.Discriminator attributeProtoType = ProtoUtils.getDiscriminatorFromType(type);
     attrPb.setName(attr.getName());
-    attrPb.setType(attributeProtoType);
+    attrPb.setType(ProtoUtils.getDiscriminatorFromType(type));
 
     if (location != null) {
       attrPb.setParseableLocation(serializeLocation(location));
@@ -299,6 +314,22 @@ public class PackageSerializer {
       attrPb.setExplicitlySpecified(explicitlySpecified);
     }
 
+    // Convenience binding for single-value attributes. Because those attributes can only
+    // have a single value, when we encounter configurable versions of them we need to
+    // react somehow to having multiple possible values to report. We currently just
+    // refrain from setting *any* value in that scenario. This variable is set to null
+    // to indicate that.
+    //
+    // For example, for "linkstatic = select({':foo': 0, ':bar': 1})", "values" will contain [0, 1].
+    // Since linkstatic is a single-value string element, its proto field (string_value) can't
+    // store both values. Since no use case today actually needs this, we just skip it.
+    //
+    // TODO(bazel-team): support this properly. This will require syntactic change to build.proto
+    // (or reinterpretation of its current fields).
+    Object singleAttributeValue = Iterables.size(values) == 1
+        ? Iterables.getOnlyElement(values)
+        : null;
+
     /*
      * Set the appropriate type and value.  Since string and string list store
      * values for multiple types, use the toString() method on the objects
@@ -306,142 +337,172 @@ public class PackageSerializer {
      * both an integer and string representation.
      */
     if (type == INTEGER) {
-      attrPb.setIntValue((Integer) value);
+      if (singleAttributeValue != null) {
+        attrPb.setIntValue((Integer) singleAttributeValue);
+      }
     } else if (type == STRING || type == LABEL || type == NODEP_LABEL || type == OUTPUT) {
-      attrPb.setStringValue(value.toString());
+      if (singleAttributeValue != null) {
+        attrPb.setStringValue(singleAttributeValue.toString());
+      }
     } else if (type == STRING_LIST || type == LABEL_LIST || type == NODEP_LABEL_LIST
         || type == OUTPUT_LIST || type == DISTRIBUTIONS) {
-      Collection<?> values = (Collection<?>) value;
-      for (Object entry : values) {
-        attrPb.addStringListValue(entry.toString());
+      for (Object value : values) {
+        for (Object entry : (Collection<?>) value) {
+          attrPb.addStringListValue(entry.toString());
+        }
       }
     } else if (type == INTEGER_LIST) {
-      Collection<Integer> values = (Collection<Integer>) value;
-      for (Integer entry : values) {
-        attrPb.addIntListValue(entry);
+      for (Object value : values) {
+        for (Integer entry : (Collection<Integer>) value) {
+          attrPb.addIntListValue(entry);
+        }
       }
     } else if (type == BOOLEAN) {
-      if ((Boolean) value) {
-        attrPb.setStringValue("true");
-        attrPb.setBooleanValue(true);
-      } else {
-        attrPb.setStringValue("false");
-        attrPb.setBooleanValue(false);
+      if (singleAttributeValue != null) {
+        if ((Boolean) singleAttributeValue) {
+          attrPb.setStringValue("true");
+          attrPb.setBooleanValue(true);
+        } else {
+          attrPb.setStringValue("false");
+          attrPb.setBooleanValue(false);
+        }
+        // This maintains partial backward compatibility for external users of the
+        // protobuf that were expecting an integer field and not a true boolean.
+        attrPb.setIntValue((Boolean) singleAttributeValue ? 1 : 0);
       }
-      // This maintains partial backward compatibility for external users of the
-      // protobuf that were expecting an integer field and not a true boolean.
-      attrPb.setIntValue((Boolean) value ? 1 : 0);
     } else if (type == TRISTATE) {
-      switch ((TriState) value) {
-        case AUTO:
+      if (singleAttributeValue != null) {
+        switch ((TriState) singleAttributeValue) {
+          case AUTO:
             attrPb.setIntValue(-1);
             attrPb.setStringValue("auto");
             attrPb.setTristateValue(Build.Attribute.Tristate.AUTO);
             break;
-        case NO:
+          case NO:
             attrPb.setIntValue(0);
             attrPb.setStringValue("no");
             attrPb.setTristateValue(Build.Attribute.Tristate.NO);
             break;
-        case YES:
+          case YES:
             attrPb.setIntValue(1);
             attrPb.setStringValue("yes");
             attrPb.setTristateValue(Build.Attribute.Tristate.YES);
             break;
+          default:
+            throw new AssertionError("Expected AUTO/NO/YES to cover all possible cases");
+        }
       }
     } else if (type == LICENSE) {
-      License license = (License) value;
-      Build.License.Builder licensePb = Build.License.newBuilder();
-      for (License.LicenseType licenseType : license.getLicenseTypes()) {
-        licensePb.addLicenseType(licenseType.toString());
-      }
-      for (Label exception : license.getExceptions()) {
-        licensePb.addException(exception.toString());
+      if (singleAttributeValue != null) {
+        License license = (License) singleAttributeValue;
+        Build.License.Builder licensePb = Build.License.newBuilder();
+        for (License.LicenseType licenseType : license.getLicenseTypes()) {
+          licensePb.addLicenseType(licenseType.toString());
+        }
+        for (Label exception : license.getExceptions()) {
+          licensePb.addException(exception.toString());
+        }
+        attrPb.setLicense(licensePb);
       }
-      attrPb.setLicense(licensePb);
     } else if (type == STRING_DICT) {
+      // TODO(bazel-team): support better de-duping here and in other dictionaries.
+      for (Object value : values) {
       Map<String, String> dict = (Map<String, String>) value;
-      for (Map.Entry<String, String> dictEntry : dict.entrySet()) {
-        Build.StringDictEntry entry = Build.StringDictEntry.newBuilder()
-            .setKey(dictEntry.getKey())
-            .setValue(dictEntry.getValue())
-            .build();
-        attrPb.addStringDictValue(entry);
+        for (Map.Entry<String, String> keyValueList : dict.entrySet()) {
+          Build.StringDictEntry entry = Build.StringDictEntry.newBuilder()
+              .setKey(keyValueList.getKey())
+              .setValue(keyValueList.getValue())
+              .build();
+          attrPb.addStringDictValue(entry);
+        }
       }
     } else if (type == STRING_DICT_UNARY) {
-      Map<String, String> dict = (Map<String, String>) value;
-      for (Map.Entry<String, String> dictEntry : dict.entrySet()) {
-        Build.StringDictUnaryEntry entry = Build.StringDictUnaryEntry.newBuilder()
-            .setKey(dictEntry.getKey())
-            .setValue(dictEntry.getValue())
-            .build();
-        attrPb.addStringDictUnaryValue(entry);
+      for (Object value : values) {
+        Map<String, String> dict = (Map<String, String>) value;
+        for (Map.Entry<String, String> dictEntry : dict.entrySet()) {
+          Build.StringDictUnaryEntry entry = Build.StringDictUnaryEntry.newBuilder()
+              .setKey(dictEntry.getKey())
+              .setValue(dictEntry.getValue())
+              .build();
+          attrPb.addStringDictUnaryValue(entry);
+        }
       }
     } else if (type == STRING_LIST_DICT) {
-      Map<String, List<String>> dict = (Map<String, List<String>>) value;
-      for (Map.Entry<String, List<String>> dictEntry : dict.entrySet()) {
-        Build.StringListDictEntry.Builder entry = Build.StringListDictEntry.newBuilder()
-            .setKey(dictEntry.getKey());
-        for (Object dictEntryValue : dictEntry.getValue()) {
-          entry.addValue(dictEntryValue.toString());
+      for (Object value : values) {
+        Map<String, List<String>> dict = (Map<String, List<String>>) value;
+        for (Map.Entry<String, List<String>> dictEntry : dict.entrySet()) {
+          Build.StringListDictEntry.Builder entry = Build.StringListDictEntry.newBuilder()
+              .setKey(dictEntry.getKey());
+          for (Object dictEntryValue : dictEntry.getValue()) {
+            entry.addValue(dictEntryValue.toString());
+          }
+          attrPb.addStringListDictValue(entry);
         }
-        attrPb.addStringListDictValue(entry);
       }
     } else if (type == LABEL_LIST_DICT) {
-      Map<String, List<Label>> dict = (Map<String, List<Label>>) value;
-      for (Map.Entry<String, List<Label>> dictEntry : dict.entrySet()) {
-        Build.LabelListDictEntry.Builder entry = Build.LabelListDictEntry.newBuilder()
-            .setKey(dictEntry.getKey());
-        for (Object dictEntryValue : dictEntry.getValue()) {
-          entry.addValue(dictEntryValue.toString());
+      for (Object value : values) {
+        Map<String, List<Label>> dict = (Map<String, List<Label>>) value;
+        for (Map.Entry<String, List<Label>> dictEntry : dict.entrySet()) {
+          Build.LabelListDictEntry.Builder entry = Build.LabelListDictEntry.newBuilder()
+              .setKey(dictEntry.getKey());
+          for (Object dictEntryValue : dictEntry.getValue()) {
+            entry.addValue(dictEntryValue.toString());
+          }
+          attrPb.addLabelListDictValue(entry);
         }
-        attrPb.addLabelListDictValue(entry);
       }
     } else if (type == FILESET_ENTRY_LIST) {
-      List<FilesetEntry> filesetEntries = (List<FilesetEntry>) value;
-      for (FilesetEntry filesetEntry : filesetEntries) {
-        Build.FilesetEntry.Builder filesetEntryPb = Build.FilesetEntry.newBuilder()
-            .setSource(filesetEntry.getSrcLabel().toString())
-            .setDestinationDirectory(filesetEntry.getDestDir().getPathString())
-            .setSymlinkBehavior(symlinkBehaviorToPb(filesetEntry.getSymlinkBehavior()))
-            .setStripPrefix(filesetEntry.getStripPrefix())
-            .setFilesPresent(filesetEntry.getFiles() != null);
-
-        if (filesetEntry.getFiles() != null) {
-          for (Label file : filesetEntry.getFiles()) {
-            filesetEntryPb.addFile(file.toString());
+      for (Object value : values) {
+        List<FilesetEntry> filesetEntries = (List<FilesetEntry>) value;
+        for (FilesetEntry filesetEntry : filesetEntries) {
+          Build.FilesetEntry.Builder filesetEntryPb = Build.FilesetEntry.newBuilder()
+              .setSource(filesetEntry.getSrcLabel().toString())
+              .setDestinationDirectory(filesetEntry.getDestDir().getPathString())
+              .setSymlinkBehavior(symlinkBehaviorToPb(filesetEntry.getSymlinkBehavior()))
+              .setStripPrefix(filesetEntry.getStripPrefix())
+              .setFilesPresent(filesetEntry.getFiles() != null);
+
+          if (filesetEntry.getFiles() != null) {
+            for (Label file : filesetEntry.getFiles()) {
+              filesetEntryPb.addFile(file.toString());
+            }
           }
-        }
 
-        if (filesetEntry.getExcludes() != null) {
-          for (String exclude : filesetEntry.getExcludes()) {
-            filesetEntryPb.addExclude(exclude);
+          if (filesetEntry.getExcludes() != null) {
+            for (String exclude : filesetEntry.getExcludes()) {
+              filesetEntryPb.addExclude(exclude);
+            }
           }
-        }
 
-        attrPb.addFilesetListValue(filesetEntryPb);
+          attrPb.addFilesetListValue(filesetEntryPb);
+        }
       }
     } else {
-      throw new IllegalStateException("Unknown type: " + type);
+      throw new AssertionError("Unknown type: " + type);
     }
 
-    if (includeGlobs && value instanceof GlobList<?>) {
-      GlobList<?> globList = (GlobList<?>) value;
-
-      for (GlobCriteria criteria : globList.getCriteria()) {
-        Build.GlobCriteria.Builder criteriaPb = Build.GlobCriteria.newBuilder();
-        criteriaPb.setGlob(criteria.isGlob());
-        for (String include : criteria.getIncludePatterns()) {
-          criteriaPb.addInclude(include);
-        }
-        for (String exclude : criteria.getExcludePatterns()) {
-          criteriaPb.addExclude(exclude);
+    if (includeGlobs) {
+      for (Object value : values) {
+        if (value instanceof GlobList<?>) {
+          GlobList<?> globList = (GlobList<?>) value;
+
+          for (GlobCriteria criteria : globList.getCriteria()) {
+            Build.GlobCriteria.Builder criteriaPb = Build.GlobCriteria.newBuilder()
+                .setGlob(criteria.isGlob());
+            for (String include : criteria.getIncludePatterns()) {
+              criteriaPb.addInclude(include);
+            }
+            for (String exclude : criteria.getExcludePatterns()) {
+              criteriaPb.addExclude(exclude);
+            }
+
+            attrPb.addGlobCriteria(criteriaPb);
+          }
         }
-
-        attrPb.addGlobCriteria(criteriaPb);
       }
     }
-    result.addAttribute(attrPb);
+
+    rulePb.addAttribute(attrPb);
   }
 
   // This is needed because I do not want to use the SymlinkBehavior from the
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 7de23d7c19..6cc12d03dd 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,6 +88,13 @@ 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/query2/output/OutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java
index 9272e1d285..e8247c540a 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/output/OutputFormatter.java
@@ -21,8 +21,8 @@ import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.graph.Digraph;
 import com.google.devtools.build.lib.graph.Node;
-import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
 import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.PackageSerializer;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.syntax.EvalUtils;
@@ -40,7 +40,6 @@ import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -450,10 +449,6 @@ public abstract class OutputFormatter implements Serializable {
    * non-configured attributes, this is a single value. For configurable attributes, this
    * may be multiple values.
    *
-   * <p>This is needed because the visibility attribute is replaced with an empty list
-   * during package loading if it is public or private in order not to visit
-   * the package called 'visibility'.
-   *
    * @return a pair, where the first value is the set of possible values and the
    *     second is an enum that tells where the values come from (declared on the
    *     rule, declared as a package level default or a
@@ -461,11 +456,9 @@ public abstract class OutputFormatter implements Serializable {
    */
   protected static Pair<Iterable<Object>, AttributeValueSource> getAttributeValues(
       Rule rule, Attribute attr) {
-    List<Object> values = new LinkedList<>(); // Not an ImmutableList: may host null values.
     AttributeValueSource source;
 
     if (attr.getName().equals("visibility")) {
-      values.add(rule.getVisibility().getDeclaredLabels());
       if (rule.isVisibilitySpecified()) {
         source = AttributeValueSource.RULE;
       } else if (rule.getPackage().isDefaultVisibilitySet()) {
@@ -474,15 +467,11 @@ public abstract class OutputFormatter implements Serializable {
         source = AttributeValueSource.DEFAULT;
       }
     } else {
-      for (Object o :
-          AggregatingAttributeMapper.of(rule).visitAttribute(attr.getName(), attr.getType())) {
-        values.add(o);
-      }
       source = rule.isAttributeValueExplicitlySpecified(attr)
           ? AttributeValueSource.RULE : AttributeValueSource.DEFAULT;
     }
 
-    return Pair.of((Iterable<Object>) values, source);
+    return Pair.of(PackageSerializer.getAttributeValues(rule, attr), source);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/query2/output/ProtoOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/output/ProtoOutputFormatter.java
index 0e22559d38..29f07ec804 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/output/ProtoOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/output/ProtoOutputFormatter.java
@@ -13,57 +13,29 @@
 // limitations under the License.
 package com.google.devtools.build.lib.query2.output;
 
-import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
-import static com.google.devtools.build.lib.packages.Type.DISTRIBUTIONS;
-import static com.google.devtools.build.lib.packages.Type.FILESET_ENTRY_LIST;
-import static com.google.devtools.build.lib.packages.Type.INTEGER;
-import static com.google.devtools.build.lib.packages.Type.INTEGER_LIST;
-import static com.google.devtools.build.lib.packages.Type.LABEL;
-import static com.google.devtools.build.lib.packages.Type.LABEL_LIST;
-import static com.google.devtools.build.lib.packages.Type.LABEL_LIST_DICT;
-import static com.google.devtools.build.lib.packages.Type.LICENSE;
-import static com.google.devtools.build.lib.packages.Type.NODEP_LABEL;
-import static com.google.devtools.build.lib.packages.Type.NODEP_LABEL_LIST;
-import static com.google.devtools.build.lib.packages.Type.OUTPUT;
-import static com.google.devtools.build.lib.packages.Type.OUTPUT_LIST;
-import static com.google.devtools.build.lib.packages.Type.STRING;
-import static com.google.devtools.build.lib.packages.Type.STRING_DICT;
-import static com.google.devtools.build.lib.packages.Type.STRING_DICT_UNARY;
-import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
-import static com.google.devtools.build.lib.packages.Type.STRING_LIST_DICT;
-import static com.google.devtools.build.lib.packages.Type.TRISTATE;
 import static com.google.devtools.build.lib.query2.proto.proto2api.Build.Target.Discriminator.GENERATED_FILE;
 import static com.google.devtools.build.lib.query2.proto.proto2api.Build.Target.Discriminator.PACKAGE_GROUP;
 import static com.google.devtools.build.lib.query2.proto.proto2api.Build.Target.Discriminator.RULE;
 import static com.google.devtools.build.lib.query2.proto.proto2api.Build.Target.Discriminator.SOURCE_FILE;
 
-import com.google.common.collect.Iterables;
-import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.graph.Digraph;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.InputFile;
-import com.google.devtools.build.lib.packages.License;
 import com.google.devtools.build.lib.packages.OutputFile;
 import com.google.devtools.build.lib.packages.PackageGroup;
+import com.google.devtools.build.lib.packages.PackageSerializer;
 import com.google.devtools.build.lib.packages.ProtoUtils;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
-import com.google.devtools.build.lib.packages.TriState;
 import com.google.devtools.build.lib.query2.FakeSubincludeTarget;
 import com.google.devtools.build.lib.query2.output.OutputFormatter.UnorderedFormatter;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build;
-import com.google.devtools.build.lib.syntax.FilesetEntry;
-import com.google.devtools.build.lib.syntax.GlobCriteria;
-import com.google.devtools.build.lib.syntax.GlobList;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.syntax.SkylarkEnvironment;
 import com.google.devtools.build.lib.util.BinaryPredicate;
 
 import java.io.IOException;
 import java.io.PrintStream;
-import java.util.Collection;
-import java.util.List;
-import java.util.Map;
 
 /**
  * An output formatter that outputs a protocol buffer representation
@@ -136,7 +108,8 @@ public class ProtoOutputFormatter extends OutputFormatter implements UnorderedFo
           .setLocation(location);
 
       for (Attribute attr : rule.getAttributes()) {
-        addAttributeToProto(rulePb, attr, getAttributeValues(rule, attr).first, null,
+        PackageSerializer.addAttributeToProto(rulePb, attr,
+            PackageSerializer.getAttributeValues(rule, attr), null,
             rule.isAttributeValueExplicitlySpecified(attr), false);
       }
 
@@ -242,253 +215,4 @@ public class ProtoOutputFormatter extends OutputFormatter implements UnorderedFo
 
     return targetPb.build();
   }
-
-  /**
-   * Adds the serialized version of the specified attribute to the specified message.
-   *
-   * @param rulePb the message to amend
-   * @param attr the attribute to add
-   * @param value the possible values of the attribute (can be a multi-value list for
-   *              configurable attributes)
-   * @param location the location of the attribute in the source file
-   * @param explicitlySpecified whether the attribute was explicitly specified or not
-   * @param includeGlobs add glob expression for attributes that contain them
-   */
-  @SuppressWarnings("unchecked")
-  public static void addAttributeToProto(
-      Build.Rule.Builder rulePb, Attribute attr, Iterable<Object> values,
-      Location location, Boolean explicitlySpecified, boolean includeGlobs) {
-    // Get the attribute type.  We need to convert and add appropriately
-    com.google.devtools.build.lib.packages.Type<?> type = attr.getType();
-
-    Build.Attribute.Builder attrPb = Build.Attribute.newBuilder();
-
-    // Set the type, name and source
-    attrPb.setName(attr.getName());
-    attrPb.setType(ProtoUtils.getDiscriminatorFromType(type));
-
-    if (location != null) {
-      attrPb.setParseableLocation(serialize(location));
-    }
-
-    if (explicitlySpecified != null) {
-      attrPb.setExplicitlySpecified(explicitlySpecified);
-    }
-
-    // Convenience binding for single-value attributes. Because those attributes can only
-    // have a single value, when we encounter configurable versions of them we need to
-    // react somehow to having multiple possible values to report. We currently just
-    // refrain from setting *any* value in that scenario. This variable is set to null
-    // to indicate that scenario.
-    Object singleAttributeValue = Iterables.size(values) == 1
-        ? Iterables.getOnlyElement(values)
-        : null;
-
-    /*
-     * Set the appropriate type and value.  Since string and string list store
-     * values for multiple types, use the toString() method on the objects
-     * instead of casting them.  Note that Boolean and TriState attributes have
-     * both an integer and string representation.
-     */
-    if (type == INTEGER) {
-      if (singleAttributeValue != null) {
-        attrPb.setIntValue((Integer) singleAttributeValue);
-      }
-    } else if (type == STRING || type == LABEL || type == NODEP_LABEL || type == OUTPUT) {
-      if (singleAttributeValue != null) {
-        attrPb.setStringValue(singleAttributeValue.toString());
-      }
-    } else if (type == STRING_LIST || type == LABEL_LIST || type == NODEP_LABEL_LIST
-        || type == OUTPUT_LIST || type == DISTRIBUTIONS) {
-      for (Object value : values) {
-        for (Object entry : (Collection<?>) value) {
-          attrPb.addStringListValue(entry.toString());
-        }
-      }
-    } else if (type == INTEGER_LIST) {
-      for (Object value : values) {
-        for (Integer entry : (Collection<Integer>) value) {
-          attrPb.addIntListValue(entry);
-        }
-      }
-    } else if (type == BOOLEAN) {
-      if (singleAttributeValue != null) {
-        if ((Boolean) singleAttributeValue) {
-          attrPb.setStringValue("true");
-          attrPb.setBooleanValue(true);
-        } else {
-          attrPb.setStringValue("false");
-          attrPb.setBooleanValue(false);
-        }
-        // This maintains partial backward compatibility for external users of the
-        // protobuf that were expecting an integer field and not a true boolean.
-        attrPb.setIntValue((Boolean) singleAttributeValue ? 1 : 0);
-      }
-    } else if (type == TRISTATE) {
-      if (singleAttributeValue != null) {
-        switch ((TriState) singleAttributeValue) {
-          case AUTO:
-            attrPb.setIntValue(-1);
-            attrPb.setStringValue("auto");
-            attrPb.setTristateValue(Build.Attribute.Tristate.AUTO);
-            break;
-          case NO:
-            attrPb.setIntValue(0);
-            attrPb.setStringValue("no");
-            attrPb.setTristateValue(Build.Attribute.Tristate.NO);
-            break;
-          case YES:
-            attrPb.setIntValue(1);
-            attrPb.setStringValue("yes");
-            attrPb.setTristateValue(Build.Attribute.Tristate.YES);
-            break;
-          default:
-            throw new IllegalStateException("Execpted AUTO/NO/YES to cover all possible cases");
-        }
-      }
-    } else if (type == LICENSE) {
-      if (singleAttributeValue != null) {
-        License license = (License) singleAttributeValue;
-        Build.License.Builder licensePb = Build.License.newBuilder();
-        for (License.LicenseType licenseType : license.getLicenseTypes()) {
-          licensePb.addLicenseType(licenseType.toString());
-        }
-        for (Label exception : license.getExceptions()) {
-          licensePb.addException(exception.toString());
-        }
-        attrPb.setLicense(licensePb);
-      }
-    } else if (type == STRING_DICT) {
-      // TODO(bazel-team): support better de-duping here and in other dictionaries.
-      for (Object value : values) {
-      Map<String, String> dict = (Map<String, String>) value;
-        for (Map.Entry<String, String> keyValueList : dict.entrySet()) {
-          Build.StringDictEntry entry = Build.StringDictEntry.newBuilder()
-              .setKey(keyValueList.getKey())
-              .setValue(keyValueList.getValue())
-              .build();
-          attrPb.addStringDictValue(entry);
-        }
-      }
-    } else if (type == STRING_DICT_UNARY) {
-      for (Object value : values) {
-        Map<String, String> dict = (Map<String, String>) value;
-        for (Map.Entry<String, String> dictEntry : dict.entrySet()) {
-          Build.StringDictUnaryEntry entry = Build.StringDictUnaryEntry.newBuilder()
-              .setKey(dictEntry.getKey())
-              .setValue(dictEntry.getValue())
-              .build();
-          attrPb.addStringDictUnaryValue(entry);
-        }
-      }
-    } else if (type == STRING_LIST_DICT) {
-      for (Object value : values) {
-        Map<String, List<String>> dict = (Map<String, List<String>>) value;
-        for (Map.Entry<String, List<String>> dictEntry : dict.entrySet()) {
-          Build.StringListDictEntry.Builder entry = Build.StringListDictEntry.newBuilder()
-              .setKey(dictEntry.getKey());
-          for (Object dictEntryValue : dictEntry.getValue()) {
-            entry.addValue(dictEntryValue.toString());
-          }
-          attrPb.addStringListDictValue(entry);
-        }
-      }
-    } else if (type == LABEL_LIST_DICT) {
-      for (Object value : values) {
-        Map<String, List<Label>> dict = (Map<String, List<Label>>) value;
-        for (Map.Entry<String, List<Label>> dictEntry : dict.entrySet()) {
-          Build.LabelListDictEntry.Builder entry = Build.LabelListDictEntry.newBuilder()
-              .setKey(dictEntry.getKey());
-          for (Object dictEntryValue : dictEntry.getValue()) {
-            entry.addValue(dictEntryValue.toString());
-          }
-          attrPb.addLabelListDictValue(entry);
-        }
-      }
-    } else if (type == FILESET_ENTRY_LIST) {
-      for (Object value : values) {
-        List<FilesetEntry> filesetEntries = (List<FilesetEntry>) value;
-        for (FilesetEntry filesetEntry : filesetEntries) {
-          Build.FilesetEntry.Builder filesetEntryPb = Build.FilesetEntry.newBuilder()
-              .setSource(filesetEntry.getSrcLabel().toString())
-              .setDestinationDirectory(filesetEntry.getDestDir().getPathString())
-              .setSymlinkBehavior(symlinkBehaviorToPb(filesetEntry.getSymlinkBehavior()))
-              .setStripPrefix(filesetEntry.getStripPrefix())
-              .setFilesPresent(filesetEntry.getFiles() != null);
-
-          if (filesetEntry.getFiles() != null) {
-            for (Label file : filesetEntry.getFiles()) {
-              filesetEntryPb.addFile(file.toString());
-            }
-          }
-
-          if (filesetEntry.getExcludes() != null) {
-            for (String exclude : filesetEntry.getExcludes()) {
-              filesetEntryPb.addExclude(exclude);
-            }
-          }
-
-          attrPb.addFilesetListValue(filesetEntryPb);
-        }
-      }
-    } else {
-      throw new IllegalStateException("Unknown type: " + type);
-    }
-
-    if (includeGlobs) {
-      for (Object value : values) {
-        if (value instanceof GlobList<?>) {
-          GlobList<?> globList = (GlobList<?>) value;
-
-          for (GlobCriteria criteria : globList.getCriteria()) {
-            Build.GlobCriteria.Builder criteriaPb = Build.GlobCriteria.newBuilder()
-                .setGlob(criteria.isGlob());
-            for (String include : criteria.getIncludePatterns()) {
-              criteriaPb.addInclude(include);
-            }
-            for (String exclude : criteria.getExcludePatterns()) {
-              criteriaPb.addExclude(exclude);
-            }
-
-            attrPb.addGlobCriteria(criteriaPb);
-          }
-        }
-      }
-    }
-
-    rulePb.addAttribute(attrPb);
-  }
-
-  // This is needed because I do not want to use the SymlinkBehavior from the
-  // protocol buffer all over the place, so there are two classes that do
-  // essentially the same thing.
-  private static Build.FilesetEntry.SymlinkBehavior symlinkBehaviorToPb(
-      FilesetEntry.SymlinkBehavior symlinkBehavior) {
-    switch (symlinkBehavior) {
-      case COPY:
-        return Build.FilesetEntry.SymlinkBehavior.COPY;
-      case DEREFERENCE:
-        return Build.FilesetEntry.SymlinkBehavior.DEREFERENCE;
-      default:
-        throw new AssertionError("Unhandled FilesetEntry.SymlinkBehavior");
-    }
-  }
-
-  private static Build.Location serialize(Location location) {
-    Build.Location.Builder result = Build.Location.newBuilder();
-
-    result.setStartOffset(location.getStartOffset());
-    if (location.getStartLineAndColumn() != null) {
-      result.setStartLine(location.getStartLineAndColumn().getLine());
-      result.setStartColumn(location.getStartLineAndColumn().getColumn());
-    }
-
-    result.setEndOffset(location.getEndOffset());
-    if (location.getEndLineAndColumn() != null) {
-      result.setEndLine(location.getEndLineAndColumn().getLine());
-      result.setEndColumn(location.getEndLineAndColumn().getColumn());
-    }
-
-    return result.build();
-  }
 }