bazel syntax: delete StarlarkFile.contentHashCode
It has moved to skyframe.ASTFileLookupValue.
Also, improve error messages related to ASTFileLookupValue.
PiperOrigin-RevId: 310543093
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
index 6a37406..5b45bda 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
@@ -95,7 +95,8 @@
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 @@
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 @@
// 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 @@
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(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java
index c35ada0..7b67b80 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupValue.java
+++ b/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 @@
}
@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 @@
}
@Override
- public String getErrorMsg() {
+ public byte[] getDigest() {
+ throw new IllegalStateException("attempted to retrieve digest for successful lookup");
+ }
+
+ @Override
+ public String getError() {
return this.errorMsg;
}
}
- static ASTFileLookupValue forBadPackage(Label fileLabel, String reason) {
- return new ASTLookupNoFile(
- String.format("Unable to load package for '%s': %s", fileLabel, reason));
+ /** 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));
}
- static ASTFileLookupValue forMissingFile(Label fileLabel) {
- return new ASTLookupNoFile(
- String.format("Unable to load file '%s': file doesn't exist", fileLabel));
+ /** Constructs a value from a parsed file. */
+ public static ASTFileLookupValue withFile(StarlarkFile ast, byte[] digest) {
+ return new ASTLookupWithFile(ast, digest);
}
- static ASTFileLookupValue forBadFile(Label fileLabel) {
- return new ASTLookupNoFile(
- String.format("Unable to load file '%s': it isn't a regular file", fileLabel));
- }
-
- public static ASTFileLookupValue withFile(StarlarkFile ast) {
- return new ASTLookupWithFile(ast);
- }
-
- public static Key key(Label astFileLabel) {
- return ASTFileLookupValue.Key.create(astFileLabel);
+ public static Key key(Label label) {
+ return ASTFileLookupValue.Key.create(label);
}
@AutoCodec.VisibleForSerialization
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index 9e97667..c6d3118 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -833,6 +833,7 @@
"//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",
],
)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java
index 501cd38..721ddc9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java
@@ -398,8 +398,8 @@
@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 @@
// 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());
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java
index 8fed417..e7858f8 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java
+++ b/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 @@
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 @@
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 @@
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 @@
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 @@
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 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 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;
- }
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
index 82e7136..3d6ff85 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
@@ -174,9 +174,9 @@
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");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageErrorMessageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageErrorMessageFunctionTest.java
index 110e89e..262866c 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageErrorMessageFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageErrorMessageFunctionTest.java
@@ -57,9 +57,7 @@
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)
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
index 3f7988f..71ebab7 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
@@ -615,7 +615,7 @@
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 @@
.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
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkDefinedAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkDefinedAspectsTest.java
index c852456..fa87b2d 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkDefinedAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkDefinedAspectsTest.java
@@ -1146,7 +1146,7 @@
} 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
diff --git a/src/test/shell/integration/loading_phase_test.sh b/src/test/shell/integration/loading_phase_test.sh
index 102d625..b4a9d1b 100755
--- a/src/test/shell/integration/loading_phase_test.sh
+++ b/src/test/shell/integration/loading_phase_test.sh
@@ -108,7 +108,7 @@
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:
