Change the preprocessor interface to take the byte[] contents of the BUILD file rather than a ParserInputSource.

This is part of a series of changes with the net result being that we open, read, and parse each BUILD file exactly once.

--
MOS_MIGRATED_REVID=105911557

6 files changed, 64 insertions, 41 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 11a9772121..0ce234dcee 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -60,6 +60,7 @@ import com.google.devtools.build.lib.syntax.Statement;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.syntax.Type.ConversionException;
 import com.google.devtools.build.lib.util.Pair;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.UnixGlob;
@@ -995,11 +996,9 @@ public final class PackageFactory {
       PackageIdentifier packageId,
       Path buildFile,
       Preprocessor.Result preprocessingResult,
-      Iterable<Event> preprocessingEvents,
       List<Statement> preludeStatements,
       Map<PathFragment, Extension> imports,
       ImmutableList<Label> skylarkFileDependencies,
-      CachingPackageLocator locator,
       RuleVisibility defaultVisibility,
       Globber globber) throws InterruptedException {
     StoredEventHandler localReporterForParsing = new StoredEventHandler();
@@ -1098,15 +1097,15 @@ public final class PackageFactory {
       throw new BuildFileNotFoundException(
           packageId, "illegal package name: '" + packageId + "' (" + error + ")");
     }
-    ParserInputSource inputSource = maybeGetParserInputSource(buildFile, eventHandler);
-    if (inputSource == null) {
+    byte[] buildFileBytes = maybeGetBuildFileBytes(buildFile, eventHandler);
+    if (buildFileBytes == null) {
       throw new BuildFileContainsErrorsException(packageId, "IOException occured");
     }
 
     Globber globber = createLegacyGlobber(buildFile.getParentDirectory(), packageId, locator);
     Preprocessor.Result preprocessingResult;
     try {
-      preprocessingResult = preprocess(packageId, inputSource, globber);
+      preprocessingResult = preprocess(buildFile, packageId, buildFileBytes, globber);
     } catch (IOException e) {
       eventHandler.handle(
           Event.error(Location.fromFile(buildFile), "preprocessing failed: " + e.getMessage()));
@@ -1119,11 +1118,9 @@ public final class PackageFactory {
                 packageId,
                 buildFile,
                 preprocessingResult,
-                /*preprocessingEvents=*/preprocessingResult.events,
                 /*preludeStatements=*/ImmutableList.<Statement>of(),
                 /*imports=*/ImmutableMap.<PathFragment, Extension>of(),
                 /*skylarkFileDependencies=*/ImmutableList.<Label>of(),
-                locator,
                 /*defaultVisibility=*/ConstantRuleVisibility.PUBLIC,
                 globber)
             .build();
@@ -1135,11 +1132,10 @@ public final class PackageFactory {
   public Preprocessor.Result preprocess(
       PackageIdentifier packageId, Path buildFile, CachingPackageLocator locator)
       throws InterruptedException, IOException {
-    ParserInputSource inputSource;
-    inputSource = ParserInputSource.create(buildFile);
+    byte[] buildFileBytes = FileSystemUtils.readWithKnownFileSize(buildFile, buildFile.getFileSize());
     Globber globber = createLegacyGlobber(buildFile.getParentDirectory(), packageId, locator);
     try {
-      return preprocess(packageId, inputSource, globber);
+      return preprocess(buildFile, packageId, buildFileBytes, globber);
     } finally {
       globber.onCompletion();
     }
@@ -1150,15 +1146,16 @@ public final class PackageFactory {
    * {@link InterruptedException}.
    */
   public Preprocessor.Result preprocess(
-      PackageIdentifier packageId, ParserInputSource inputSource, Globber globber)
-      throws InterruptedException, IOException {
+      Path buildFilePath, PackageIdentifier packageId, byte[] buildFileBytes,
+      Globber globber) throws InterruptedException, IOException {
     Preprocessor preprocessor = preprocessorFactory.getPreprocessor();
     if (preprocessor == null) {
-      return Preprocessor.Result.noPreprocessing(inputSource);
+      return Preprocessor.Result.noPreprocessing(buildFilePath.asFragment(), buildFileBytes);
     }
     try {
       return preprocessor.preprocess(
-          inputSource,
+          buildFilePath,
+          buildFileBytes,
           packageId.toString(),
           globber,
           Environment.BUILD,
@@ -1176,9 +1173,9 @@ public final class PackageFactory {
   }
 
   @Nullable
-  private ParserInputSource maybeGetParserInputSource(Path buildFile, EventHandler eventHandler) {
+  private byte[] maybeGetBuildFileBytes(Path buildFile, EventHandler eventHandler) {
     try {
-      return ParserInputSource.create(buildFile);
+      return FileSystemUtils.readWithKnownFileSize(buildFile, buildFile.getFileSize());
     } catch (IOException e) {
       eventHandler.handle(Event.error(Location.fromFile(buildFile), e.getMessage()));
       return null;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java b/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
index 57a2d848df..08bc380524 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
@@ -21,6 +21,8 @@ import com.google.devtools.build.lib.packages.PackageFactory.Globber;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
 import java.io.IOException;
@@ -113,6 +115,12 @@ public interface Preprocessor {
       this.events = ImmutableList.copyOf(events);
     }
 
+    public static Result noPreprocessing(PathFragment buildFilePathFragment,
+        byte[] buildFileBytes) {
+      return noPreprocessing(ParserInputSource.create(
+          FileSystemUtils.convertFromLatin1(buildFileBytes), buildFilePathFragment));
+    }
+
     /** Convenience factory for a {@link Result} wrapping non-preprocessed BUILD file contents. */
     public static Result noPreprocessing(ParserInputSource buildFileSource) {
       return new Result(
@@ -148,7 +156,8 @@ public interface Preprocessor {
    * preprocessing actually begins, any I/O problems encountered will be reflected in the return
    * value, not manifested as exceptions.
    *
-   * @param in the BUILD file to be preprocessed.
+   * @param buildFilePath the BUILD file to be preprocessed.
+   * @param buildFileBytes the raw contents of the BUILD file to be preprocessed.
    * @param packageName the BUILD file's package.
    * @param globber a globber for evaluating globs.
    * @param globals the global bindings for the Python environment.
@@ -157,7 +166,8 @@ public interface Preprocessor {
    * @return a pair of the ParserInputSource and a map of subincludes seen during the evaluation
    */
   Result preprocess(
-      ParserInputSource in,
+      Path buildFilePath,
+      byte[] buildFileBytes,
       String packageName,
       Globber globber,
       Environment.Frame globals,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index aa922913da..ee5ec1a0e4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -52,6 +52,7 @@ import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
 import com.google.devtools.build.lib.syntax.Statement;
 import com.google.devtools.build.lib.util.Pair;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.RootedPath;
@@ -876,14 +877,11 @@ public class PackageFunction implements SkyFunction {
           Preprocessor.Result preprocessingResult;
           if (replacementContents == null) {
             Preconditions.checkNotNull(buildFileValue, packageId);
-            // Even though we only open and read the file on a cache miss, note that the BUILD is
-            // still parsed two times. Also, the preprocessor may suboptimally open and read it
-            // again anyway.
-            ParserInputSource inputSource;
+            byte[] buildFileBytes;
             try {
-              inputSource = buildFileValue.isSpecialFile()
-                  ? ParserInputSource.create(buildFilePath)
-                  : ParserInputSource.create(buildFilePath, buildFileValue.getSize());
+              buildFileBytes = buildFileValue.isSpecialFile()
+                  ? FileSystemUtils.readContent(buildFilePath)
+                  : FileSystemUtils.readWithKnownFileSize(buildFilePath, buildFileValue.getSize());
             } catch (IOException e) {
               env.getListener().handle(Event.error(Location.fromFile(buildFilePath),
                   e.getMessage()));
@@ -893,7 +891,8 @@ public class PackageFunction implements SkyFunction {
                   packageId, e.getMessage()), Transience.TRANSIENT);
             }
             try {
-              preprocessingResult = packageFactory.preprocess(packageId, inputSource, globber);
+              preprocessingResult = packageFactory.preprocess(buildFilePath, packageId,
+                  buildFileBytes, globber);
             } catch (IOException e) {
               env.getListener().handle(Event.error(
                   Location.fromFile(buildFilePath),
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ParserInputSource.java b/src/main/java/com/google/devtools/build/lib/syntax/ParserInputSource.java
index dc29bf75d0..5f2ce3fa66 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ParserInputSource.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ParserInputSource.java
@@ -49,16 +49,8 @@ public abstract class ParserInputSource {
   }
 
   public static ParserInputSource create(Path path, long fileSize) throws IOException {
-    if (fileSize > Integer.MAX_VALUE) {
-      throw new IOException("Cannot parse file with size larger than 2GB");
-    }
-    char[] content = FileSystemUtils.readContentAsLatin1(path);
-    if (fileSize > content.length) {
-      // This assertion is to help diagnose problems arising from the
-      // filesystem;  see bugs and #859334 and #920195.
-      throw new IOException("Unexpected short read from file '" + path
-          + "' (expected " + fileSize + ", got " + content.length + " bytes)");
-    }
+    byte[] bytes = FileSystemUtils.readWithKnownFileSize(path, fileSize);
+    char[] content = FileSystemUtils.convertFromLatin1(bytes);
     return create(content, path.asFragment());
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
index fba03c5186..0fd54f591d 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
@@ -799,7 +799,10 @@ public class FileSystemUtils {
    * methods are not efficient and should not be used for large amounts of data!
    */
 
-  private static char[] convertFromLatin1(byte[] content) {
+  /**
+   * Decodes the given byte array assumed to be encoded with ISO-8859-1 encoding (isolatin1).
+   */
+  public static char[] convertFromLatin1(byte[] content) {
     char[] latin1 = new char[content.length];
     for (int i = 0; i < latin1.length; i++) { // yeah, latin1 is this easy! :-)
       latin1[i] = (char) (0xff & content[i]);
@@ -958,6 +961,28 @@ public class FileSystemUtils {
   }
 
   /**
+   * Reads the given file {@code path}, assumed to have size {@code fileSize}, and does a sanity
+   * check on the number of bytes read.
+   *
+   * <p>Use this method when you already know the size of the file. The sanity check is intended to
+   * catch issues where filesystems incorrectly truncate files.
+   *
+   * @throws IOException if there was an error, or if fewer than {@code fileSize} bytes were read.
+   */
+  public static byte[] readWithKnownFileSize(Path path, long fileSize) throws IOException {
+    if (fileSize > Integer.MAX_VALUE) {
+      throw new IOException("Cannot read file with size larger than 2GB");
+    }
+    int fileSizeInt = (int) fileSize;
+    byte[] bytes = readContentWithLimit(path, fileSizeInt);
+    if (fileSizeInt > bytes.length) {
+      throw new IOException("Unexpected short read from file '" + path
+          + "' (expected " + fileSizeInt + ", got " + bytes.length + " bytes)");
+    }
+    return bytes;
+  }
+
+  /**
    * Dumps diagnostic information about the specified filesystem to {@code out}.
    * This is the implementation of the filesystem part of the 'blaze dump'
    * command. It lives here, rather than in DumpCommand, because it requires
diff --git a/src/test/shell/bazel/local_repository_test.sh b/src/test/shell/bazel/local_repository_test.sh
index 536dbbd3e1..519baf4395 100755
--- a/src/test/shell/bazel/local_repository_test.sh
+++ b/src/test/shell/bazel/local_repository_test.sh
@@ -487,24 +487,24 @@ EOF
 genrule(
     name = "turtle",
     outs = ["tmnt"],
-    cmd = "echo 'Raphael' > \$@",
+    cmd = "echo 'Leonardo' > \$@",
     visibility = ["//visibility:public"],
 )
 EOF
   bazel fetch //external:best-turtle || fail "Fetch failed"
   bazel build //external:best-turtle &> $TEST_log || fail "First build failed"
-  assert_contains "Raphael" bazel-genfiles/external/mutant/tmnt
+  assert_contains "Leonardo" bazel-genfiles/external/mutant/tmnt
 
   cat > mutant.BUILD <<EOF
 genrule(
     name = "turtle",
     outs = ["tmnt"],
-    cmd = "echo 'Michaelangelo' > \$@",
+    cmd = "echo 'Donatello' > \$@",
     visibility = ["//visibility:public"],
 )
 EOF
   bazel build //external:best-turtle &> $TEST_log || fail "Second build failed"
-  assert_contains "Michaelangelo" bazel-genfiles/external/mutant/tmnt
+  assert_contains "Donatello" bazel-genfiles/external/mutant/tmnt
 }
 
 function test_external_deps_in_remote_repo() {