bazel syntax: delete StarlarkFile.contentHashCode

It has moved to skyframe.ASTFileLookupValue.
Also, improve error messages related to ASTFileLookupValue.

PiperOrigin-RevId: 310543093

src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java

@@ -95,7 +95,8 @@ static ASTFileLookupValue computeInline(
       return null;
     }
     if (!pkgLookupValue.packageExists()) {
-      return ASTFileLookupValue.forBadPackage(fileLabel, pkgLookupValue.getErrorMsg());
+      return ASTFileLookupValue.noFile(
+          "cannot load '%s': %s", fileLabel, pkgLookupValue.getErrorMsg());
     }
 
     // Determine whether the file designated by fileLabel exists.
@@ -112,10 +113,13 @@ static ASTFileLookupValue computeInline(
       return null;
     }
     if (!fileValue.exists()) {
-      return ASTFileLookupValue.forMissingFile(fileLabel);
+      return ASTFileLookupValue.noFile("cannot load '%s': no such file", fileLabel);
     }
     if (!fileValue.isFile()) {
-      return ASTFileLookupValue.forBadFile(fileLabel);
+      return fileValue.isDirectory()
+          ? ASTFileLookupValue.noFile("cannot load '%s': is a directory", fileLabel)
+          : ASTFileLookupValue.noFile(
+              "cannot load '%s': not a regular file (dangling link?)", fileLabel);
     }
     StarlarkSemantics semantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
     if (semantics == null) {
@@ -130,16 +134,16 @@ static ASTFileLookupValue computeInline(
 
     // Both the package and the file exist; load and parse the file.
     Path path = rootedPath.asPath();
-    StarlarkFile file = null;
+    StarlarkFile file;
+    byte[] digest;
     try {
       byte[] bytes =
           fileValue.isSpecialFile()
               ? FileSystemUtils.readContent(path)
               : FileSystemUtils.readWithKnownFileSize(path, fileValue.getSize());
-      byte[] digest =
-          getDigestFromFileValueOrFromKnownFileContents(fileValue, bytes, digestHashFunction);
+      digest = getDigestFromFileValueOrFromKnownFileContents(fileValue, bytes, digestHashFunction);
       ParserInput input = ParserInput.create(bytes, path.toString());
-      file = StarlarkFile.parseWithDigest(input, digest, options);
+      file = StarlarkFile.parse(input, options);
     } catch (IOException e) {
       throw new ErrorReadingSkylarkExtensionException(e, Transience.TRANSIENT);
     }
@@ -148,7 +152,7 @@ static ASTFileLookupValue computeInline(
     Resolver.resolveFile(file, Module.createForBuiltins(ruleClassProvider.getEnvironment()));
     Event.replayEventsOn(env.getListener(), file.errors()); // TODO(adonovan): fail if !ok()?
 
-    return ASTFileLookupValue.withFile(file);
+    return ASTFileLookupValue.withFile(file, digest);
   }
 
   private static byte[] getDigestFromFileValueOrFromKnownFileContents(

src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java

@@ -23,37 +23,48 @@
 import com.google.devtools.build.skyframe.AbstractSkyKey;
 import com.google.devtools.build.skyframe.NotComparableSkyValue;
 import com.google.devtools.build.skyframe.SkyFunctionName;
+import com.google.errorprone.annotations.FormatMethod;
 
 /**
  * A value that represents an AST file lookup result. There are two subclasses: one for the case
  * where the file is found, and another for the case where the file is missing (but there are no
  * other errors).
  */
-// In practice, if a ASTFileLookupValue is re-computed (i.e. not changed pruned), then it will
-// almost certainly be unequal to the previous value. This is because of (i) the change-pruning
-// semantics of the PackageLookupValue dep and the FileValue dep; consider the latter: if the
-// FileValue for the bzl file has changed, then the contents of the bzl file probably changed and
-// (ii) we don't currently have Starlark-semantic-equality in StarlarkFile, so two StarlarkFile
-// instances representing two different contents of a bzl file will be different.
-// TODO(bazel-team): Consider doing better here. As a pre-req, we would need
-// Starlark-semantic-equality in StarlarkFile, rather than equality naively based on the contents of
-// the bzl file. For a concrete example, the contents of comment lines do not currently impact
-// Starlark semantics.
+// In practice, almost any change to a .bzl causes the ASTFileLookupValue to be recomputed.
+// We could do better with a finer-grained notion of equality for StarlarkFile than "the source
+// files differ". In particular, a trivial change such as fixing a typo in a comment should not
+// cause invalidation. (Changes that are only slightly more substantial may be semantically
+// significant. For example, inserting a blank line affects subsequent line numbers, which appear
+// in error messages and query output.)
+//
+// Comparing syntax trees for equality is complex and expensive, so the most practical
+// implementation of this optimization will have to wait until Starlark files are compiled,
+// at which point byte-equality of the compiled representation (which is simple to compute)
+// will serve. (At that point, ASTFileLookup should be renamed CompileStarlark.)
+//
 public abstract class ASTFileLookupValue implements NotComparableSkyValue {
+
+  // TODO(adonovan): flatten this hierarchy into a single class.
+  // It would only cost one word per Starlark file.
+  // Eliminate lookupSuccessful; use getAST() != null.
+
   public abstract boolean lookupSuccessful();
 
   public abstract StarlarkFile getAST();
 
-  public abstract String getErrorMsg();
+  public abstract byte[] getDigest();
+
+  public abstract String getError();
 
   /** If the file is found, this class encapsulates the parsed AST. */
   @AutoCodec.VisibleForSerialization
   public static class ASTLookupWithFile extends ASTFileLookupValue {
     private final StarlarkFile ast;
+    private final byte[] digest;
 
-    private ASTLookupWithFile(StarlarkFile ast) {
-      Preconditions.checkNotNull(ast);
-      this.ast = ast;
+    private ASTLookupWithFile(StarlarkFile ast, byte[] digest) {
+      this.ast = Preconditions.checkNotNull(ast);
+      this.digest = Preconditions.checkNotNull(digest);
     }
 
     @Override
@@ -67,7 +78,12 @@ public StarlarkFile getAST() {
     }
 
     @Override
-    public String getErrorMsg() {
+    public byte[] getDigest() {
+      return this.digest;
+    }
+
+    @Override
+    public String getError() {
       throw new IllegalStateException(
           "attempted to retrieve unsuccessful lookup reason for successful lookup");
     }
@@ -93,32 +109,29 @@ public StarlarkFile getAST() {
     }
 
     @Override
-    public String getErrorMsg() {
-      return this.errorMsg;
+    public byte[] getDigest() {
+      throw new IllegalStateException("attempted to retrieve digest for successful lookup");
     }
-  }
-
-  static ASTFileLookupValue forBadPackage(Label fileLabel, String reason) {
-    return new ASTLookupNoFile(
-        String.format("Unable to load package for '%s': %s", fileLabel, reason));
-  }
 
-  static ASTFileLookupValue forMissingFile(Label fileLabel) {
-    return new ASTLookupNoFile(
-        String.format("Unable to load file '%s': file doesn't exist", fileLabel));
+    @Override
+    public String getError() {
+      return this.errorMsg;
+    }
   }
 
-  static ASTFileLookupValue forBadFile(Label fileLabel) {
-    return new ASTLookupNoFile(
-        String.format("Unable to load file '%s': it isn't a regular file", fileLabel));
+  /** Constructs a value from a failure before parsing a file. */
+  @FormatMethod
+  static ASTFileLookupValue noFile(String format, Object... args) {
+    return new ASTLookupNoFile(String.format(format, args));
   }
 
-  public static ASTFileLookupValue withFile(StarlarkFile ast) {
-    return new ASTLookupWithFile(ast);
+  /** Constructs a value from a parsed file. */
+  public static ASTFileLookupValue withFile(StarlarkFile ast, byte[] digest) {
+    return new ASTLookupWithFile(ast, digest);
   }
 
-  public static Key key(Label astFileLabel) {
-    return ASTFileLookupValue.Key.create(astFileLabel);
+  public static Key key(Label label) {
+    return ASTFileLookupValue.Key.create(label);
   }
 
   @AutoCodec.VisibleForSerialization

src/main/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -833,6 +833,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/syntax:frontend",
         "//src/main/java/com/google/devtools/build/skyframe",
         "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
+        "//third_party:error_prone_annotations",
         "//third_party:guava",
     ],
 )

src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java

@@ -398,8 +398,8 @@ private StarlarkImportLookupValue computeInternalWithAst(
       @Nullable InliningState inliningState)
       throws InconsistentFilesystemException, StarlarkImportFailedException, InterruptedException {
     if (!astLookupValue.lookupSuccessful()) {
-      // Starlark import files have to exist.
-      throw new StarlarkImportFailedException(astLookupValue.getErrorMsg());
+      // Starlark import files must exist.
+      throw new StarlarkImportFailedException(astLookupValue.getError());
     }
     StarlarkFile file = astLookupValue.getAST();
     if (!file.ok()) {
@@ -445,8 +445,7 @@ private StarlarkImportLookupValue computeInternalWithAst(
     // Compute a digest of the file itself plus the transitive hashes of the modules it directly
     // loads. Loop iteration order matches the source order of load statements.
     Fingerprint fp = new Fingerprint();
-    // TODO(adonovan): save file.getContentHashCode in ASTFileLookupValue, not the syntax tree.
-    fp.addBytes(file.getContentHashCode());
+    fp.addBytes(astLookupValue.getDigest());
     Map<String, Module> loadedModules = Maps.newHashMapWithExpectedSize(loadMap.size());
     ImmutableList.Builder<StarlarkFileDependency> fileDependencies =
         ImmutableList.builderWithExpectedSize(loadMap.size());

src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java

@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.syntax;
 
 import com.google.common.collect.ImmutableList;
-import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
 import javax.annotation.Nullable;
@@ -31,7 +30,6 @@ public final class StarlarkFile extends Node {
   private final FileOptions options;
   private final ImmutableList<Comment> comments;
   final List<SyntaxError> errors; // appended to by Resolver
-  @Nullable private final byte[] contentHashCode;
 
   // set by resolver
   @Nullable Resolver.Function resolved;
@@ -51,14 +49,12 @@ private StarlarkFile(
       ImmutableList<Statement> statements,
       FileOptions options,
       ImmutableList<Comment> comments,
-      List<SyntaxError> errors,
-      byte[] contentHashCode) {
+      List<SyntaxError> errors) {
     super(locs);
     this.statements = statements;
     this.options = options;
     this.comments = comments;
     this.errors = errors;
-    this.contentHashCode = contentHashCode;
   }
 
   // Creates a StarlarkFile from the given effective list of statements,
@@ -67,15 +63,9 @@ private static StarlarkFile create(
       FileLocations locs,
       ImmutableList<Statement> statements,
       FileOptions options,
-      Parser.ParseResult result,
-      byte[] contentHashCode) {
+      Parser.ParseResult result) {
     return new StarlarkFile(
-        locs,
-        statements,
-        options,
-        ImmutableList.copyOf(result.comments),
-        result.errors,
-        contentHashCode);
+        locs, statements, options, ImmutableList.copyOf(result.comments), result.errors);
   }
 
   /** Extract a subtree containing only statements from i (included) to j (excluded). */
@@ -85,8 +75,7 @@ public StarlarkFile subTree(int i, int j) {
         this.statements.subList(i, j),
         this.options,
         /*comments=*/ ImmutableList.of(),
-        errors,
-        /*contentHashCode=*/ null);
+        errors);
   }
   /**
    * Returns an unmodifiable view of the list of scanner, parser, and (perhaps) resolver errors
@@ -133,14 +122,7 @@ public static StarlarkFile parseWithPrelude(
     stmts.addAll(prelude);
     stmts.addAll(result.statements);
 
-    return create(result.locs, stmts.build(), options, result, /*contentHashCode=*/ null);
-  }
-
-  // TODO(adonovan): move the digest into skyframe and delete this.
-  public static StarlarkFile parseWithDigest(ParserInput input, byte[] digest, FileOptions options)
-      throws IOException {
-    Parser.ParseResult result = Parser.parseFile(input, options);
-    return create(result.locs, ImmutableList.copyOf(result.statements), options, result, digest);
+    return create(result.locs, stmts.build(), options, result);
   }
 
   /**
@@ -159,12 +141,7 @@ public static StarlarkFile parseWithDigest(ParserInput input, byte[] digest, Fil
    */
   public static StarlarkFile parse(ParserInput input, FileOptions options) {
     Parser.ParseResult result = Parser.parseFile(input, options);
-    return create(
-        result.locs,
-        ImmutableList.copyOf(result.statements),
-        options,
-        result,
-        /*contentHashCode=*/ null);
+    return create(result.locs, ImmutableList.copyOf(result.statements), options, result);
   }
 
   /** Parse a Starlark file with default options. */
@@ -176,13 +153,4 @@ public static StarlarkFile parse(ParserInput input) {
   public FileOptions getOptions() {
     return options;
   }
-
-  /**
-   * Returns the digest of the source file, if this StarlarkFile was constructed by parseWithDigest,
-   * null otherwise.
-   */
-  @Nullable
-  public byte[] getContentHashCode() {
-    return contentHashCode;
-  }
 }

src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java

@@ -174,9 +174,9 @@ public void testLoadWithNonExistentBuildFile() throws Exception {
         SkyframeExecutorTestUtils.evaluate(
             getSkyframeExecutor(), skyKey, /*keepGoing=*/ false, reporter);
     assertThat(result.get(skyKey).lookupSuccessful()).isFalse();
-    assertThat(result.get(skyKey).getErrorMsg())
-    .contains("Unable to load package for '@a_remote_repo//remote_pkg:BUILD'");
-    assertThat(result.get(skyKey).getErrorMsg())
+    assertThat(result.get(skyKey).getError())
+        .contains("cannot load '@a_remote_repo//remote_pkg:BUILD':");
+    assertThat(result.get(skyKey).getError())
         .contains("The repository '@a_remote_repo' could not be resolved");
   }
 }

src/test/java/com/google/devtools/build/lib/skyframe/PackageErrorMessageFunctionTest.java

@@ -57,9 +57,7 @@ public void testNoSuchPackageException() throws Exception {
         getPackageErrorMessageValue(/*keepGoing=*/ true);
     assertThat(packageErrorMessageValue.getResult()).isEqualTo(Result.NO_SUCH_PACKAGE_EXCEPTION);
     assertThat(packageErrorMessageValue.getNoSuchPackageExceptionMessage())
-        .isEqualTo(
-            "error loading package 'a': Unable to load file "
-                + "'//a:does_not_exist.bzl': file doesn't exist");
+        .isEqualTo("error loading package 'a': cannot load '//a:does_not_exist.bzl': no such file");
   }
 
   private PackageErrorMessageValue getPackageErrorMessageValue(boolean keepGoing)

src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java

@@ -615,7 +615,7 @@ public void testNonExistingSkylarkExtension() throws Exception {
     ErrorInfo errorInfo = result.getError(skyKey);
     String expectedMsg =
         "error loading package 'test/skylark': "
-            + "Unable to load file '//test/skylark:bad_extension.bzl': file doesn't exist";
+            + "cannot load '//test/skylark:bad_extension.bzl': no such file";
     assertThat(errorInfo.getException()).hasMessageThat().isEqualTo(expectedMsg);
   }
 
@@ -645,7 +645,7 @@ public void testNonExistingSkylarkExtensionFromExtension() throws Exception {
         .isEqualTo(
             "error loading package 'test/skylark': "
                 + "in /workspace/test/skylark/extension.bzl: "
-                + "Unable to load file '//test/skylark:bad_extension.bzl': file doesn't exist");
+                + "cannot load '//test/skylark:bad_extension.bzl': no such file");
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/skylark/SkylarkDefinedAspectsTest.java

@@ -1146,7 +1146,7 @@ public void topLevelAspectDoesNotExist2() throws Exception {
     } catch (ViewCreationFailedException e) {
       // expect to fail.
     }
-    assertContainsEvent("Unable to load file '//test:aspect.bzl': file doesn't exist");
+    assertContainsEvent("cannot load '//test:aspect.bzl': no such file");
   }
 
   @Test

src/test/shell/integration/loading_phase_test.sh

@@ -108,7 +108,7 @@ function test_query_buildfiles_with_load() {
     rm -f $pkg/y/rules.bzl
     bazel query --noshow_progress "buildfiles(//$pkg/x)" 2>$TEST_log &&
         fail "Expected error"
-    expect_log "Unable to load file '//$pkg/y:rules.bzl'"
+    expect_log "cannot load '//$pkg/y:rules.bzl'"
 }
 
 # Regression test for: