Stop serializing/deserializing Locations as part of Packages

They're not cheap to serialize or reconstitute and we don't really need them.

This does leave some odd ends around, in particular i decided to keep deserialization context around as i can picture use for it soon. Also return non-null values from all
of EmptyLocation's method since while the javadocs declare that LineAndColumn and Path
can be null, there does exist code which does not take this into account, this is for
another change.

--
MOS_MIGRATED_REVID=102758810

4 files changed, 46 insertions, 137 deletions

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 e0ee1924ca..2537693f24 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
@@ -25,7 +25,6 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.events.NullEventHandler;
@@ -52,7 +51,6 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.logging.Level;
@@ -103,10 +101,8 @@ public class PackageDeserializer {
   /** Class encapsulating state for a single package deserialization. */
   private static class DeserializationContext {
     private final Package.Builder packageBuilder;
-    private final PathFragment buildFilePath;
 
-    public DeserializationContext(Path buildFilePath, Package.Builder packageBuilder) {
-      this.buildFilePath = buildFilePath.asFragment();
+    public DeserializationContext(Package.Builder packageBuilder) {
       this.packageBuilder = packageBuilder;
     }
   }
@@ -125,17 +121,12 @@ public class PackageDeserializer {
         Preconditions.checkNotNull(packageDeserializationEnvironment);
   }
 
-  private static Location deserializeLocation(DeserializationContext context,
-      Build.Location location) {
-    return new ExplicitLocation(context.buildFilePath, location);
-  }
-
-  private static ParsedAttributeValue deserializeAttribute(DeserializationContext context,
+  private static ParsedAttributeValue deserializeAttribute(
       Type<?> expectedType, Build.Attribute attrPb) throws PackageDeserializationException {
     Object value = deserializeAttributeValue(expectedType, attrPb);
     return new ParsedAttributeValue(
         attrPb.hasExplicitlySpecified() && attrPb.getExplicitlySpecified(), value,
-        deserializeLocation(context, attrPb.getParseableLocation()));
+        EmptyLocation.INSTANCE);
   }
 
   private void deserializeInputFile(DeserializationContext context, Build.SourceFile sourceFile)
@@ -143,8 +134,7 @@ public class PackageDeserializer {
     InputFile inputFile;
     try {
       inputFile = context.packageBuilder.createInputFile(
-          deserializeLabel(sourceFile.getName()).getName(),
-          deserializeLocation(context, sourceFile.getParseableLocation()));
+          deserializeLabel(sourceFile.getName()).getName(), EmptyLocation.INSTANCE);
     } catch (GeneratedLabelConflict e) {
       throw new PackageDeserializationException(e);
     }
@@ -169,7 +159,7 @@ public class PackageDeserializer {
           specifications,
           deserializeLabels(packageGroupPb.getIncludedPackageGroupList()),
           NullEventHandler.INSTANCE,  // TODO(bazel-team): Handle errors properly
-          deserializeLocation(context, packageGroupPb.getParseableLocation()));
+          EmptyLocation.INSTANCE);
     } catch (Label.SyntaxException | Package.NameConflictException e) {
       throw new PackageDeserializationException(e);
     }
@@ -177,12 +167,12 @@ public class PackageDeserializer {
 
   private void deserializeRule(DeserializationContext context, Build.Rule rulePb)
       throws PackageDeserializationException {
-    Location ruleLocation = deserializeLocation(context, rulePb.getParseableLocation());
+    Location ruleLocation = EmptyLocation.INSTANCE;
     RuleClass ruleClass = packageDeserializationEnvironment.getRuleClass(rulePb, ruleLocation);
     Map<String, ParsedAttributeValue> attributeValues = new HashMap<>();
     for (Build.Attribute attrPb : rulePb.getAttributeList()) {
       Type<?> type = ruleClass.getAttributeByName(attrPb.getName()).getType();
-      attributeValues.put(attrPb.getName(), deserializeAttribute(context, type, attrPb));
+      attributeValues.put(attrPb.getName(), deserializeAttribute(type, attrPb));
     }
 
     Label ruleLabel = deserializeLabel(rulePb.getName());
@@ -198,66 +188,40 @@ public class PackageDeserializer {
     }
   }
 
-  @Immutable
-  private static final class ExplicitLocation extends Location {
-    private final PathFragment path;
-    private final int startLine;
-    private final int startColumn;
-    private final int endLine;
-    private final int endColumn;
-
-    private ExplicitLocation(PathFragment path, Build.Location location) {
-      super(
-          location.hasStartOffset() && location.hasEndOffset() ? location.getStartOffset() : 0,
-          location.hasStartOffset() && location.hasEndOffset() ? location.getEndOffset() : 0);
-      this.path = path;
-      if (location.hasStartLine() && location.hasStartColumn()
-          && location.hasEndLine() && location.hasEndColumn()) {
-        this.startLine = location.getStartLine();
-        this.startColumn = location.getStartColumn();
-        this.endLine = location.getEndLine();
-        this.endColumn = location.getEndColumn();
-      } else {
-        this.startLine = 0;
-        this.startColumn = 0;
-        this.endLine = 0;
-        this.endColumn = 0;
-      }
+  /** "Empty" location implementation, all methods should return non-null, but empty, values. */
+  private static class EmptyLocation extends Location {
+    private static final EmptyLocation INSTANCE = new EmptyLocation();
+
+    private static final PathFragment DEV_NULL = new PathFragment("/dev/null");
+    private static final LineAndColumn EMPTY_LINE_AND_COLUMN = new LineAndColumn(0, 0);
+
+    private EmptyLocation() {
+      super(0, 0);
     }
 
     @Override
     public PathFragment getPath() {
-      return path;
+      return DEV_NULL;
     }
 
     @Override
     public LineAndColumn getStartLineAndColumn() {
-      return new LineAndColumn(startLine, startColumn);
+      return EMPTY_LINE_AND_COLUMN;
     }
 
     @Override
     public LineAndColumn getEndLineAndColumn() {
-      return new LineAndColumn(endLine, endColumn);
+      return EMPTY_LINE_AND_COLUMN;
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(
-          path.hashCode(), startLine, startColumn, endLine, endColumn, internalHashCode());
+      return 42;
     }
 
     @Override
     public boolean equals(Object other) {
-      if (other == null || !other.getClass().equals(getClass())) {
-        return false;
-      }
-      ExplicitLocation that = (ExplicitLocation) other;
-      return this.startLine == that.startLine
-          && this.startColumn == that.startColumn
-          && this.endLine == that.endLine
-          && this.endColumn == that.endColumn
-          && internalEquals(that)
-          && Objects.equals(this.path, that.path);
+      return other instanceof EmptyLocation;
     }
   }
 
@@ -360,14 +324,14 @@ public class PackageDeserializer {
 
   /**
    * Deserialize a package from its representation as a protocol message. The inverse of
-   * {@link PackageSerializer#serializePackage}.
+   * {@link PackageSerializer#serialize}.
    * @throws IOException
    */
   private void deserializeInternal(Build.Package packagePb, StoredEventHandler eventHandler,
       Package.Builder builder, InputStream in) throws PackageDeserializationException, IOException {
     Path buildFile = packageDeserializationEnvironment.getPath(packagePb.getBuildFilePath());
     Preconditions.checkNotNull(buildFile);
-    DeserializationContext context = new DeserializationContext(buildFile, builder);
+    DeserializationContext context = new DeserializationContext(builder);
     builder.setFilename(buildFile);
 
     if (packagePb.hasDefaultVisibilitySet() && packagePb.getDefaultVisibilitySet()) {
@@ -416,7 +380,7 @@ public class PackageDeserializer {
     builder.setMakeEnv(makeEnvBuilder);
 
     for (Build.Event event : packagePb.getEventList()) {
-      deserializeEvent(context, eventHandler, event);
+      deserializeEvent(eventHandler, event);
     }
 
     if (packagePb.hasContainsErrors() && packagePb.getContainsErrors()) {
@@ -452,7 +416,7 @@ public class PackageDeserializer {
 
   /**
    * Deserializes a {@link Package} from {@code in}. The inverse of
-   * {@link PackageSerializer#serializePackage}.
+   * {@link PackageSerializer#serialize}.
    *
    * <p>Expects {@code in} to contain a single
    * {@link com.google.devtools.build.lib.query2.proto.proto2api.Build.Package} message followed
@@ -493,19 +457,13 @@ public class PackageDeserializer {
     return builder.build();
   }
 
-  private static void deserializeEvent(
-      DeserializationContext context, StoredEventHandler eventHandler, Build.Event event) {
-    Location location = null;
-    if (event.hasLocation()) {
-      location = deserializeLocation(context, event.getLocation());
-    }
-
+  private static void deserializeEvent(StoredEventHandler eventHandler, Build.Event event) {
     String message = event.getMessage();
     switch (event.getKind()) {
-      case ERROR: eventHandler.handle(Event.error(location, message)); break;
-      case WARNING: eventHandler.handle(Event.warn(location, message)); break;
-      case INFO: eventHandler.handle(Event.info(location, message)); break;
-      case PROGRESS: eventHandler.handle(Event.progress(location, message)); break;
+      case ERROR: eventHandler.handle(Event.error(message)); break;
+      case WARNING: eventHandler.handle(Event.warn(message)); break;
+      case INFO: eventHandler.handle(Event.info(message)); break;
+      case PROGRESS: eventHandler.handle(Event.progress(message)); break;
       default: break;  // Ignore
     }
   }
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 65da467bc5..835a185ceb 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
@@ -37,7 +37,6 @@ 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;
 import com.google.devtools.build.lib.packages.MakeEnvironment.Binding;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build;
@@ -59,8 +58,7 @@ import java.util.Map;
  */
 public class PackageSerializer {
 
-  public static final PackageSerializer DEFAULT =
-      new PackageSerializer(/*serializefullLocations=*/true);
+  public static final PackageSerializer DEFAULT = new PackageSerializer();
 
   /**
    * Get protocol buffer representation of the specified attribute.
@@ -68,13 +66,12 @@ public class PackageSerializer {
    * @param attr the attribute to add
    * @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
    */
   public static Build.Attribute getAttributeProto(Attribute attr, Iterable<Object> values,
-      Location location, Boolean explicitlySpecified, boolean includeGlobs) {
-    return DEFAULT.serializeAttribute(attr, values, location, explicitlySpecified, includeGlobs);
+      Boolean explicitlySpecified, boolean includeGlobs) {
+    return DEFAULT.serializeAttribute(attr, values, explicitlySpecified, includeGlobs);
   }
 
   /**
@@ -97,18 +94,6 @@ public class PackageSerializer {
     return values;
   }
 
-  private final boolean serializeFullLocations;
-
-  /**
-   * Initialize with the specified configuration.
-   *
-   * @param serializeFullLocations if true will include start and end offset, line, and column
-   *    in serialized locations, if false will only include start and end offset
-   */
-  public PackageSerializer(boolean serializeFullLocations) {
-    this.serializeFullLocations = serializeFullLocations;
-  }
-
   /**
    * Serialize a package to {@code out}. The inverse of {@link PackageDeserializer#deserialize}.
    *
@@ -193,7 +178,7 @@ public class PackageSerializer {
 
   @SuppressWarnings("unchecked")
   private Build.Attribute serializeAttribute(Attribute attr, Iterable<Object> values,
-      Location location, Boolean explicitlySpecified, boolean includeGlobs) {
+      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();
 
@@ -203,10 +188,6 @@ public class PackageSerializer {
     attrPb.setName(attr.getName());
     attrPb.setType(ProtoUtils.getDiscriminatorFromType(type));
 
-    if (location != null) {
-      attrPb.setParseableLocation(serializeLocation(location));
-    }
-
     if (explicitlySpecified != null) {
       attrPb.setExplicitlySpecified(explicitlySpecified);
     }
@@ -427,42 +408,16 @@ public class PackageSerializer {
       builder.setLicense(serializeLicense(inputFile.getLicense()));
     }
 
-    builder.setParseableLocation(serializeLocation(inputFile.getLocation()));
-
     return Build.Target.newBuilder()
         .setType(Build.Target.Discriminator.SOURCE_FILE)
         .setSourceFile(builder.build())
         .build();
   }
 
-  private Build.Location serializeLocation(Location location) {
-    Build.Location.Builder result = Build.Location.newBuilder();
-
-    result.setStartOffset(location.getStartOffset());
-    result.setEndOffset(location.getEndOffset());
-
-    if (serializeFullLocations) {
-      Location.LineAndColumn startLineAndColumn = location.getStartLineAndColumn();
-      if (startLineAndColumn != null) {
-        result.setStartLine(startLineAndColumn.getLine());
-        result.setStartColumn(startLineAndColumn.getColumn());
-      }
-
-      Location.LineAndColumn endLineAndColumn = location.getEndLineAndColumn();
-      if (endLineAndColumn != null) {
-        result.setEndLine(endLineAndColumn.getLine());
-        result.setEndColumn(endLineAndColumn.getColumn());
-      }
-    }
-
-    return result.build();
-  }
-
   private Build.Target serializePackageGroup(PackageGroup packageGroup) {
     Build.PackageGroup.Builder builder = Build.PackageGroup.newBuilder();
 
     builder.setName(packageGroup.getLabel().toString());
-    builder.setParseableLocation(serializeLocation(packageGroup.getLocation()));
 
     for (PackageSpecification packageSpecification : packageGroup.getPackageSpecifications()) {
       builder.addContainedPackage(packageSpecification.toString());
@@ -482,12 +437,11 @@ public class PackageSerializer {
     Build.Rule.Builder builder = Build.Rule.newBuilder();
     builder.setName(rule.getLabel().toString());
     builder.setRuleClass(rule.getRuleClass());
-    builder.setParseableLocation(serializeLocation(rule.getLocation()));
     builder.setPublicByDefault(rule.getRuleClassObject().isPublicByDefault());
     for (Attribute attribute : rule.getAttributes()) {
-      builder.addAttribute(serializeAttribute(attribute,
-          getAttributeValues(rule, attribute), rule.getAttributeLocation(attribute.getName()),
-          rule.isAttributeValueExplicitlySpecified(attribute), true));
+      builder.addAttribute(
+          serializeAttribute(attribute, getAttributeValues(rule, attribute),
+              rule.isAttributeValueExplicitlySpecified(attribute), /*includeGlobs=*/ true));
     }
 
     return Build.Target.newBuilder()
@@ -531,12 +485,8 @@ public class PackageSerializer {
   private Build.Event serializeEvent(Event event) {
     Build.Event.Builder result = Build.Event.newBuilder();
     result.setMessage(event.getMessage());
-    if (event.getLocation() != null) {
-      result.setLocation(serializeLocation(event.getLocation()));
-    }
 
     Build.Event.EventKind kind;
-
     switch (event.getKind()) {
       case ERROR:
         kind = Build.Event.EventKind.ERROR;
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 639e124eba..787e43f35f 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
@@ -137,8 +137,8 @@ public class ProtoOutputFormatter extends OutputFormatter implements UnorderedFo
           continue;
         }
         rulePb.addAttribute(PackageSerializer.getAttributeProto(attr,
-            PackageSerializer.getAttributeValues(rule, attr), null,
-            rule.isAttributeValueExplicitlySpecified(attr), false));
+            PackageSerializer.getAttributeValues(rule, attr),
+            rule.isAttributeValueExplicitlySpecified(attr), /*includeGlobs=*/ false));
       }
 
       SkylarkEnvironment env = rule.getRuleClassObject().getRuleDefinitionEnvironment();
@@ -158,7 +158,8 @@ public class ProtoOutputFormatter extends OutputFormatter implements UnorderedFo
       // Add information about additional attributes from aspects.
       for (Entry<Attribute, Collection<Label>> entry : aspectsDependencies.asMap().entrySet()) {
         rulePb.addAttribute(PackageSerializer.getAttributeProto(entry.getKey(),
-            Lists.<Object>newArrayList(entry.getValue()), null, false, false));
+            Lists.<Object>newArrayList(entry.getValue()),
+            /*explicitlySpecified=*/ false, /*includeGlobs=*/ false));
       }
       // Add all deps from aspects as rule inputs of current target.
       for (Label label : aspectsDependencies.values()) {
diff --git a/src/main/protobuf/build.proto b/src/main/protobuf/build.proto
index 72006cebd9..b77dd221c7 100644
--- a/src/main/protobuf/build.proto
+++ b/src/main/protobuf/build.proto
@@ -131,7 +131,7 @@ message Attribute {
   required string name = 1;
 
   // The location of the target in the BUILD file in a machine-parseable form.
-  optional Location parseable_location = 12;
+  optional Location DEPRECATED_parseable_location = 12;
 
   // Whether the attribute was explicitly specified
   optional bool explicitly_specified = 13;
@@ -235,7 +235,7 @@ message Rule {
   repeated string default_setting = 7;
 
   // The location of the target in the BUILD file in a machine-parseable form.
-  optional Location parseable_location = 8;
+  optional Location DEPRECATED_parseable_location = 8;
 
   // The rule's class's public by default value.
   optional bool public_by_default = 9;
@@ -266,7 +266,7 @@ message PackageGroup {
   repeated string included_package_group = 3;
 
   // The location of the target in the BUILD file in a machine-parseable form.
-  optional Location parseable_location = 4;
+  optional Location DEPRECATED_parseable_location = 4;
 }
 
 // An environment group.
@@ -294,7 +294,7 @@ message SourceFile {
 
   // The location of the corresponding label in the BUILD file in a
   // machine-parseable form.
-  optional Location parseable_location = 7;
+  optional Location DEPRECATED_parseable_location = 7;
 
   // Labels of files that are transitively subincluded in this BUILD file. This
   // is present only when the SourceFile represents a BUILD file that
@@ -472,7 +472,7 @@ message Event {
   }
 
   required EventKind kind = 1;
-  optional Location location = 2;
+  optional Location DEPRECATED_location = 2;
   optional string message = 3;
 }