bazel syntax: break dependence on EventHandler
This change removes nearly all uses of EventHandler in the interpreter.
(The remaining uses are for events created by the Starlark 'print' statement.
A follow-up change will create a dedicated StarlarkThread.PrintHandler.)
All parse functions now report errors in one of two ways:
1) by throwing a SyntaxError exception containing a list of events.
This approach is used by "all-or-nothing" parse operations related
to expressions. Clients may catch the exception, extract the events,
and replay them on their handler should they choose. Example:
try {
Expression expr = Expression.parse(input)
...
} catch (SyntaxError ex) {
Event.replayEventsOn(handler, ex.errors());
}
2) by recording a list of scan/parse errors in StarlarkFile.errors.
The result of parsing a file is both a syntax tree (possibly incomplete),
and a list of errors. If parsing is followed by validation,
the validator appends its errors to this list. (Validation is now
capable of reporting multiple errors per file. ValidationException is gone.)
StarlarkFile file = StarlarkFile.parse(input)
if (!file.ok()) {
Event.replayEventsOn(handler, file.errors());
return;
}
Or:
StarlarkFile file = StarlarkFile.parse(input)
if (!file.ok()) {
throw new SyntaxError(file.errors());
}
Details:
- error helpers for Identifiers have moved from Eval to ValidationEnvironment
and no longer depend on EvalException.
- The EvalException.incompleteAST concept is irrelevant now that
clients do not proceed to evaluation if the parser or validation
step produced errors.
- StarlarkFile: exec + execTopLevelStatement have moved to EvalUtils, and will
change further.
- replayLexerEvents is gone from the API. The validator (which has access
to a StarlarkSemantics) can optionally copy the string escape events
into the main error bucket, or ignore them.
- StarlarkFile: eliminate parseWithoutImports
- Parser: delete dead code parseStatement{,s}
- Lexer: delete dead code stringAtLine
Numerous fixes were required in the tests, because they are such a muddle
of different phases (scan, parse, validate, evaluate)
and modes (BUILD vs .bzl vs core Starlark).
RELNOTES: Multiple Starlark validation errors are reported in a single pass.
PiperOrigin-RevId: 272205103
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 b4ea17a..2be21ac 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
@@ -1397,10 +1397,10 @@
ExtendedEventHandler eventHandler) {
// Logged messages are used as a testability hook tracing the parsing progress
logger.fine("Starting to parse " + packageId);
- StarlarkFile buildFileAST =
- StarlarkFile.parseWithPrelude(input, preludeStatements, eventHandler);
+ StarlarkFile file = StarlarkFile.parseWithPrelude(input, preludeStatements);
+ Event.replayEventsOn(eventHandler, file.errors());
logger.fine("Finished parsing of " + packageId);
- return buildFileAST;
+ return file;
}
public Package.Builder createPackageFromAst(
@@ -1773,10 +1773,6 @@
pkgBuilder.setContainsErrors();
}
- if (file.containsErrors()) {
- pkgBuilder.setContainsErrors();
- }
-
pkgBuilder.setThirdPartyLicenceExistencePolicy(
ruleClassProvider.getThirdPartyLicenseExistencePolicy());
@@ -1799,19 +1795,36 @@
}
}
- if (!ValidationEnvironment.validateFile(file, pkgThread, /*isBuildFile=*/ true, eventHandler)
- || !checkBuildSyntax(file, eventHandler)) {
- pkgBuilder.setContainsErrors();
+ boolean ok = true;
+
+ // Reject forbidden BUILD syntax.
+ if (!checkBuildSyntax(file, eventHandler)) {
+ ok = false;
}
- // TODO(bazel-team): (2009) the invariant "if errors are reported, mark the package
- // as containing errors" is strewn all over this class. Refactor to use an
- // event sensor--and see if we can simplify the calling code in
- // createPackage().
- if (!pkgBuilder.containsErrors()) {
- if (!file.exec(pkgThread, eventHandler)) {
- pkgBuilder.setContainsErrors();
+ // Attempt validation only if the file parsed clean.
+ if (file.ok()) {
+ ValidationEnvironment.validateFile(file, pkgThread, /*isBuildFile=*/ true);
+ if (!file.ok()) {
+ Event.replayEventsOn(eventHandler, file.errors());
+ ok = false;
}
+
+ // Attempt execution only if the file parsed, validated, and checked clean.
+ if (ok) {
+ try {
+ EvalUtils.exec(file, pkgThread);
+ } catch (EvalException ex) {
+ eventHandler.handle(Event.error(ex.getLocation(), ex.getMessage()));
+ ok = false;
+ }
+ }
+ } else {
+ ok = false;
+ }
+
+ if (!ok) {
+ pkgBuilder.setContainsErrors();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index dc59b49..99c7d15 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -33,6 +33,7 @@
import com.google.devtools.build.lib.syntax.CallUtils;
import com.google.devtools.build.lib.syntax.ClassObject;
import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.FuncallExpression;
import com.google.devtools.build.lib.syntax.FunctionSignature;
import com.google.devtools.build.lib.syntax.Mutability;
@@ -134,13 +135,14 @@
if (localReporter == null) {
localReporter = new StoredEventHandler();
}
- StarlarkFile buildFileAST = StarlarkFile.parse(source, localReporter);
- if (buildFileAST.containsErrors()) {
+ StarlarkFile file = StarlarkFile.parse(source);
+ if (!file.ok()) {
+ Event.replayEventsOn(localReporter, file.errors());
throw new BuildFileContainsErrorsException(
LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER, "Failed to parse " + source.getPath());
}
execute(
- buildFileAST,
+ file,
null,
starlarkSemantics,
localReporter,
@@ -207,11 +209,16 @@
}
}
- if (!ValidationEnvironment.validateFile(
- file, workspaceThread, /*isBuildFile=*/ true, localReporter)
- || !PackageFactory.checkBuildSyntax(file, localReporter)
- || !file.exec(workspaceThread, localReporter)) {
- localReporter.handle(Event.error("Error evaluating WORKSPACE file"));
+ // Validate the file, apply BUILD dialect checks, then execute.
+ ValidationEnvironment.validateFile(file, workspaceThread, /*isBuildFile=*/ true);
+ if (!file.ok()) {
+ Event.replayEventsOn(localReporter, file.errors());
+ } else if (PackageFactory.checkBuildSyntax(file, localReporter)) {
+ try {
+ EvalUtils.exec(file, workspaceThread);
+ } catch (EvalException ex) {
+ localReporter.handle(Event.error(ex.getLocation(), ex.getMessage()));
+ }
}
// Save the list of variable bindings for the next part of the workspace file. The list of
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/ResolvedFileFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/ResolvedFileFunction.java
index ec711e8..86d738a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/ResolvedFileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/ResolvedFileFunction.java
@@ -18,11 +18,14 @@
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.cmdline.LabelConstants;
+import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.BazelLibrary;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.rules.repository.ResolvedFileValue.ResolvedFileKey;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
+import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.ParserInput;
import com.google.devtools.build.lib.syntax.StarlarkFile;
@@ -63,14 +66,11 @@
byte[] bytes =
FileSystemUtils.readWithKnownFileSize(
key.getPath().asPath(), key.getPath().asPath().getFileSize());
- StarlarkFile ast =
- StarlarkFile.parse(
- ParserInput.create(bytes, key.getPath().asPath().asFragment()), env.getListener());
- if (ast.containsErrors()) {
- throw new ResolvedFileFunctionException(
- new BuildFileContainsErrorsException(
- LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
- "Failed to parse file resolved file " + key.getPath()));
+ StarlarkFile file =
+ StarlarkFile.parse(ParserInput.create(bytes, key.getPath().asPath().asFragment()));
+ if (!file.ok()) {
+ Event.replayEventsOn(env.getListener(), file.errors());
+ throw resolvedValueError("Failed to parse file resolved file " + key.getPath());
}
StarlarkThread resolvedThread;
try (Mutability mutability = Mutability.create("resolved file %s", key.getPath())) {
@@ -79,48 +79,40 @@
.setSemantics(starlarkSemantics)
.setGlobals(BazelLibrary.GLOBALS)
.build();
- if (!ast.exec(resolvedThread, env.getListener())) {
- throw new ResolvedFileFunctionException(
- new BuildFileContainsErrorsException(
- LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
- "Failed to evaluate resolved file " + key.getPath()));
+ try {
+ EvalUtils.exec(file, resolvedThread);
+ } catch (EvalException ex) {
+ env.getListener().handle(Event.error(ex.getLocation(), ex.getMessage()));
+ throw resolvedValueError("Failed to evaluate resolved file " + key.getPath());
}
}
Object resolved = resolvedThread.moduleLookup("resolved");
if (resolved == null) {
- throw new ResolvedFileFunctionException(
- new BuildFileContainsErrorsException(
- LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
- "Symbol 'resolved' not exported in resolved file " + key.getPath()));
+ throw resolvedValueError(
+ "Symbol 'resolved' not exported in resolved file " + key.getPath());
}
if (!(resolved instanceof List)) {
- throw new ResolvedFileFunctionException(
- new BuildFileContainsErrorsException(
- LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
- "Symbol 'resolved' in resolved file " + key.getPath() + " not a list"));
+ throw resolvedValueError(
+ "Symbol 'resolved' in resolved file " + key.getPath() + " not a list");
}
ImmutableList.Builder<Map<String, Object>> result
= new ImmutableList.Builder<Map<String, Object>>();
for (Object entry : (List) resolved) {
if (!(entry instanceof Map)) {
- throw new ResolvedFileFunctionException(
- new BuildFileContainsErrorsException(
- LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
- "Symbol 'resolved' in resolved file "
- + key.getPath()
- + " contains a non-map entry"));
+ throw resolvedValueError(
+ "Symbol 'resolved' in resolved file "
+ + key.getPath()
+ + " contains a non-map entry");
}
ImmutableMap.Builder<String, Object> entryBuilder
= new ImmutableMap.Builder<String, Object>();
for (Map.Entry<Object, Object> keyValue : ((Map<Object, Object>) entry).entrySet()) {
Object attribute = keyValue.getKey();
if (!(attribute instanceof String)) {
- throw new ResolvedFileFunctionException(
- new BuildFileContainsErrorsException(
- LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
- "Symbol 'resolved' in resolved file "
- + key.getPath()
- + " contains a non-string key in one of its entries"));
+ throw resolvedValueError(
+ "Symbol 'resolved' in resolved file "
+ + key.getPath()
+ + " contains a non-string key in one of its entries");
}
entryBuilder.put((String) attribute, keyValue.getValue());
}
@@ -133,6 +125,11 @@
}
}
+ private static ResolvedFileFunctionException resolvedValueError(String message) {
+ return new ResolvedFileFunctionException(
+ new BuildFileContainsErrorsException(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER, message));
+ }
+
@Override
@Nullable
public String extractTag(SkyKey skyKey) {
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 5cb106c..a21e2e1 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
@@ -18,6 +18,7 @@
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.actions.InconsistentFilesystemException;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.syntax.Mutability;
@@ -25,6 +26,7 @@
import com.google.devtools.build.lib.syntax.StarlarkFile;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
+import com.google.devtools.build.lib.syntax.ValidationEnvironment;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -127,8 +129,9 @@
/*repoMapping=*/ ImmutableMap.of());
byte[] bytes = FileSystemUtils.readWithKnownFileSize(path, astFileSize);
ParserInput input = ParserInput.create(bytes, path.asFragment());
- file = StarlarkFile.parseWithDigest(input, path.getDigest(), env.getListener());
- file = file.validate(thread, /*isBuildFile=*/ false, env.getListener());
+ file = StarlarkFile.parseWithDigest(input, path.getDigest());
+ ValidationEnvironment.validateFile(file, thread, /*isBuildFile=*/ false);
+ Event.replayEventsOn(env.getListener(), file.errors());
}
} catch (IOException e) {
throw new ASTLookupFunctionException(new ErrorReadingSkylarkExtensionException(e),
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index b467f95c..caace66 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -45,6 +45,7 @@
import com.google.devtools.build.lib.skyframe.SkylarkImportLookupValue.SkylarkImportLookupKey;
import com.google.devtools.build.lib.syntax.AssignmentStatement;
import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.Identifier;
import com.google.devtools.build.lib.syntax.LoadStatement;
import com.google.devtools.build.lib.syntax.Mutability;
@@ -320,7 +321,7 @@
throw new SkylarkImportFailedException(astLookupValue.getErrorMsg());
}
StarlarkFile file = astLookupValue.getAST();
- if (file.containsErrors()) {
+ if (!file.ok()) {
throw SkylarkImportFailedException.skylarkErrors(filePath);
}
@@ -615,19 +616,18 @@
}
}
+ // Precondition: file is validated and error-free.
public static void execAndExport(
- StarlarkFile ast,
- Label extensionLabel,
- EventHandler eventHandler,
- StarlarkThread extensionThread)
+ StarlarkFile file, Label extensionLabel, EventHandler handler, StarlarkThread thread)
throws InterruptedException {
- ast.replayLexerEvents(extensionThread, eventHandler);
- ImmutableList<Statement> statements = ast.getStatements();
- for (Statement statement : statements) {
- if (!ast.execTopLevelStatement(statement, extensionThread, eventHandler)) {
+ for (Statement stmt : file.getStatements()) {
+ try {
+ EvalUtils.execToplevelStatement(stmt, thread);
+ } catch (EvalException ex) {
+ handler.handle(Event.error(ex.getLocation(), ex.getMessage()));
break;
}
- possiblyExport(statement, extensionLabel, eventHandler, extensionThread);
+ possiblyExport(stmt, extensionLabel, handler, thread);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceASTFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceASTFunction.java
index e33ade1..96d3c4c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceASTFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceASTFunction.java
@@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.cmdline.LabelConstants;
+import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
@@ -83,35 +84,26 @@
StarlarkFile.parse(
ParserInput.create(
ruleClassProvider.getDefaultWorkspacePrefix(),
- PathFragment.create("/DEFAULT.WORKSPACE")),
- env.getListener());
- if (file.containsErrors()) {
- throw new WorkspaceASTFunctionException(
- new BuildFileContainsErrorsException(
- LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
- "Failed to parse default WORKSPACE file"),
- Transience.PERSISTENT);
+ PathFragment.create("/DEFAULT.WORKSPACE")));
+ if (!file.ok()) {
+ Event.replayEventsOn(env.getListener(), file.errors());
+ throw resolvedValueError("Failed to parse default WORKSPACE file");
}
if (newWorkspaceFileContents != null) {
file =
StarlarkFile.parseVirtualBuildFile(
ParserInput.create(
newWorkspaceFileContents, resolvedFile.get().asPath().asFragment()),
- file.getStatements(),
- env.getListener());
+ file.getStatements());
} else if (workspaceFileValue.exists()) {
byte[] bytes =
FileSystemUtils.readWithKnownFileSize(repoWorkspace, repoWorkspace.getFileSize());
file =
StarlarkFile.parseWithPrelude(
- ParserInput.create(bytes, repoWorkspace.asFragment()),
- file.getStatements(),
- env.getListener());
- if (file.containsErrors()) {
- throw new WorkspaceASTFunctionException(
- new BuildFileContainsErrorsException(
- LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER, "Failed to parse WORKSPACE file"),
- Transience.PERSISTENT);
+ ParserInput.create(bytes, repoWorkspace.asFragment()), file.getStatements());
+ if (!file.ok()) {
+ Event.replayEventsOn(env.getListener(), file.errors());
+ throw resolvedValueError("Failed to parse WORKSPACE file");
}
}
file =
@@ -119,14 +111,10 @@
ParserInput.create(
resolvedFile.isPresent() ? "" : ruleClassProvider.getDefaultWorkspaceSuffix(),
PathFragment.create("/DEFAULT.WORKSPACE.SUFFIX")),
- file.getStatements(),
- env.getListener());
- if (file.containsErrors()) {
- throw new WorkspaceASTFunctionException(
- new BuildFileContainsErrorsException(
- LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
- "Failed to parse default WORKSPACE file suffix"),
- Transience.PERSISTENT);
+ file.getStatements());
+ if (!file.ok()) {
+ Event.replayEventsOn(env.getListener(), file.errors());
+ throw resolvedValueError("Failed to parse default WORKSPACE file suffix");
}
return new WorkspaceASTValue(splitAST(file));
} catch (IOException ex) {
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
index 8ba96a8..31ae677 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
@@ -18,8 +18,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.events.EventKind;
-import com.google.devtools.build.lib.events.EventSensor;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos;
import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Breakpoint;
@@ -32,6 +30,7 @@
import com.google.devtools.build.lib.syntax.ParserInput;
import com.google.devtools.build.lib.syntax.Runtime;
import com.google.devtools.build.lib.syntax.StarlarkThread;
+import com.google.devtools.build.lib.syntax.SyntaxError;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;
import java.util.HashMap;
@@ -280,7 +279,7 @@
try {
Object result = doEvaluate(thread, statement);
return DebuggerSerialization.getValueProto(objectMap, "Evaluation result", result);
- } catch (EvalException | InterruptedException e) {
+ } catch (SyntaxError | EvalException | InterruptedException e) {
throw new DebugRequestException(e.getMessage());
}
}
@@ -294,7 +293,7 @@
* running.
*/
private Object doEvaluate(StarlarkThread thread, String content)
- throws EvalException, InterruptedException {
+ throws SyntaxError, EvalException, InterruptedException {
try {
servicingEvalRequest.set(true);
@@ -309,12 +308,10 @@
ParserInput input = ParserInput.create(content, PathFragment.create("<debug eval>"));
// Try parsing as an expression.
- EventSensor sensor = new EventSensor(EventKind.ERRORS);
- Expression.parse(input, sensor); // discard result
- if (!sensor.wasTriggered()) {
- // It's a valid expression; evaluate (and parse again).
- return thread.debugEval(input);
- } else {
+ try {
+ Expression expr = Expression.parse(input);
+ return thread.debugEval(expr);
+ } catch (SyntaxError unused) {
// Assume it is a file and execute it.
thread.debugExec(input);
return Runtime.NONE;
@@ -404,7 +401,7 @@
}
try {
return EvalUtils.toBoolean(doEvaluate(thread, condition));
- } catch (EvalException | InterruptedException e) {
+ } catch (SyntaxError | EvalException | InterruptedException e) {
throw new ConditionalBreakpointException(e.getMessage());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
index 63d5dd2..8c66c98 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
@@ -17,13 +17,11 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.util.SpellChecker;
import java.util.ArrayList;
import java.util.Collection;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
-import java.util.Set;
import java.util.concurrent.atomic.AtomicReference;
import javax.annotation.Nullable;
@@ -488,7 +486,10 @@
// Legacy behavior, to be removed.
Object result = thread.lookup(name);
if (result == null) {
- throw createInvalidIdentifierException(id, thread.getVariableNames());
+ String error =
+ ValidationEnvironment.createInvalidIdentifierException(
+ id.getName(), thread.getVariableNames());
+ throw new EvalException(id.getLocation(), error);
}
return result;
}
@@ -510,15 +511,15 @@
if (result == null) {
// Since Scope was set, we know that the variable is defined in the scope.
// However, the assignment was not yet executed.
- EvalException e = getSpecialException(id);
- throw e != null
- ? e
- : new EvalException(
- id.getLocation(),
- id.getScope().getQualifier()
- + " variable '"
- + name
- + "' is referenced before assignment.");
+ String error = ValidationEnvironment.getErrorForObsoleteThreadLocalVars(id.getName());
+ if (error == null) {
+ error =
+ id.getScope().getQualifier()
+ + " variable '"
+ + name
+ + "' is referenced before assignment.";
+ }
+ throw new EvalException(id.getLocation(), error);
}
return result;
}
@@ -599,40 +600,6 @@
throw new IllegalArgumentException("unexpected expression: " + expr.kind());
}
- /** Exception to provide a better error message for using PACKAGE_NAME or REPOSITORY_NAME. */
- private static EvalException getSpecialException(Identifier id) {
- if (id.getName().equals("PACKAGE_NAME")) {
- return new EvalException(
- id.getLocation(),
- "The value 'PACKAGE_NAME' has been removed in favor of 'package_name()', "
- + "please use the latter ("
- + "https://docs.bazel.build/versions/master/skylark/lib/native.html#package_name). ");
- }
- if (id.getName().equals("REPOSITORY_NAME")) {
- return new EvalException(
- id.getLocation(),
- "The value 'REPOSITORY_NAME' has been removed in favor of 'repository_name()', please"
- + " use the latter ("
- + "https://docs.bazel.build/versions/master/skylark/lib/native.html#repository_name).");
- }
- return null;
- }
-
- static EvalException createInvalidIdentifierException(Identifier id, Set<String> symbols) {
- if (id.getName().equals("$error$")) {
- return new EvalException(id.getLocation(), "contains syntax error(s)", true);
- }
-
- EvalException e = getSpecialException(id);
- if (e != null) {
- return e;
- }
-
- String suggestion = SpellChecker.didYouMean(id.getName(), symbols);
- return new EvalException(
- id.getLocation(), "name '" + id.getName() + "' is not defined" + suggestion);
- }
-
private static Object evalComprehension(StarlarkThread thread, Comprehension comp)
throws EvalException, InterruptedException {
final SkylarkDict<Object, Object> dict = comp.isDict() ? SkylarkDict.of(thread) : null;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java
index 0223684..1c741b6 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java
@@ -36,7 +36,6 @@
@Nullable private Location location;
private final String message;
- private final boolean dueToIncompleteAST;
private static final Joiner LINE_JOINER = Joiner.on("\n").skipNulls();
private static final Joiner FIELD_JOINER = Joiner.on(": ").skipNulls();
@@ -48,19 +47,6 @@
public EvalException(Location location, String message) {
this.location = location;
this.message = Preconditions.checkNotNull(message);
- this.dueToIncompleteAST = false;
- }
-
- /**
- * @param location the location where evaluation/execution failed.
- * @param message the error message.
- * @param dueToIncompleteAST if the error is caused by a previous error, such as parsing.
- */
- EvalException(Location location, String message, boolean dueToIncompleteAST) {
- super(null, null, /*enableSuppression=*/ true, /*writableStackTrace=*/ true);
- this.location = location;
- this.message = Preconditions.checkNotNull(message);
- this.dueToIncompleteAST = dueToIncompleteAST;
}
/**
@@ -83,7 +69,6 @@
LoggingUtil.logToRemote(Level.SEVERE, details, cause);
throw new IllegalArgumentException(details);
}
- this.dueToIncompleteAST = false;
}
public EvalException(Location location, Throwable cause) {
@@ -94,9 +79,9 @@
* Returns the error message with location info if exists.
*/
public String print() { // TODO(bazel-team): do we also need a toString() method?
- return LINE_JOINER.join("\n", FIELD_JOINER.join(getLocation(), message),
- (dueToIncompleteAST ? "due to incomplete AST" : ""),
- getCauseMessage(message));
+ // TODO(adonovan): figure out what this means and simplify it.
+ return LINE_JOINER.join(
+ "\n", FIELD_JOINER.join(getLocation(), message), "", getCauseMessage(message));
}
/**
@@ -140,13 +125,6 @@
}
/**
- * Returns a boolean that tells whether this exception was due to an incomplete AST
- */
- public boolean isDueToIncompleteAST() {
- return dueToIncompleteAST;
- }
-
- /**
* Ensures that this EvalException has proper location information.
* Does nothing if the exception already had a location, or if no location is provided.
* @return this EvalException, in fluent style.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 8c99b36..76296a1 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -1108,4 +1108,18 @@
"type '%s' has no operator [](%s)", getDataTypeName(object), getDataTypeName(key)));
}
}
+
+ /** Executes a Starlark file in a given StarlarkThread. */
+ public static void exec(StarlarkFile file, StarlarkThread thread)
+ throws EvalException, InterruptedException {
+ for (Statement stmt : file.getStatements()) {
+ execToplevelStatement(stmt, thread);
+ }
+ }
+
+ /** Executes a statement in a given StarlarkThread. */
+ public static void execToplevelStatement(Statement stmt, StarlarkThread thread)
+ throws EvalException, InterruptedException {
+ Eval.execToplevelStatement(thread, stmt);
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Expression.java b/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
index 06bc54c..3ff3996 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.syntax;
-import com.google.devtools.build.lib.events.EventHandler;
import java.io.IOException;
/**
@@ -66,14 +65,8 @@
public abstract Kind kind();
/** Parses an expression. */
- // TODO(adonovan): remove dependency from syntax -> EventHandler.
- // A call to Expression.parse either succeeds or fails; there is no useful middle ground, so an
- // exception is appropriate. The exception would contain the list of errors.
- // By contrast, a call to StarlarkFile.parse should return both a partial AST and a list of
- // errors,
- // and generally it is useful to keep both around, so if we put the errors in the root of the AST,
- // then client can deal with them however they like, e.g. by sending them to the event handler.
- public static Expression parse(ParserInput input, EventHandler eventHandler) {
- return Parser.parseExpression(input, eventHandler);
+ public static Expression parse(ParserInput input) throws SyntaxError {
+ return Parser.parseExpression(input);
}
+
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FlagGuardedValue.java b/src/main/java/com/google/devtools/build/lib/syntax/FlagGuardedValue.java
index d79e901..62bbb39 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FlagGuardedValue.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FlagGuardedValue.java
@@ -15,7 +15,6 @@
package com.google.devtools.build.lib.syntax;
import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.syntax.StarlarkSemantics.FlagIdentifier;
/**
@@ -65,31 +64,26 @@
}
/**
- * Returns an {@link EvalException} with error appropriate to throw when one attempts to access
- * this guard's protected object when it should be inaccessible in the given semantics.
+ * Returns an error describing an attempt to access this guard's protected object when it should
+ * be inaccessible in the given semantics.
*
* @throws IllegalArgumentException if {@link #isObjectAccessibleUsingSemantics} is true given the
* semantics
*/
- public EvalException getEvalExceptionFromAttemptingAccess(
- Location location, StarlarkSemantics semantics, String symbolDescription) {
+ String getErrorFromAttemptingAccess(StarlarkSemantics semantics, String name) {
Preconditions.checkArgument(!isObjectAccessibleUsingSemantics(semantics),
"getEvalExceptionFromAttemptingAccess should only be called if the underlying "
+ "object is inaccessible given the semantics");
- if (flagType == FlagType.EXPERIMENTAL) {
- return new EvalException(
- location,
- symbolDescription
- + " is experimental and thus unavailable with the current flags. It may be "
- + "enabled by setting --" + flagIdentifier.getFlagName());
- } else {
- return new EvalException(
- location,
- symbolDescription
- + " is deprecated and will be removed soon. It may be temporarily re-enabled by "
- + "setting --" + flagIdentifier.getFlagName() + "=false");
-
- }
+ return flagType == FlagType.EXPERIMENTAL
+ ? name
+ + " is experimental and thus unavailable with the current flags. It may be enabled by"
+ + " setting --"
+ + flagIdentifier.getFlagName()
+ : name
+ + " is deprecated and will be removed soon. It may be temporarily re-enabled by"
+ + " setting --"
+ + flagIdentifier.getFlagName()
+ + "=false";
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index 65c924c..2344873 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -32,8 +32,7 @@
// ValidationEnvironment.
@Nullable private ValidationEnvironment.Scope scope;
- // TODO(adonovan): lock down, after removing last use in skyframe serialization.
- public Identifier(String name) {
+ Identifier(String name) {
this.name = name;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java b/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java
index fc35657..0c9fa87 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java
@@ -18,10 +18,8 @@
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
-import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.HashMap;
@@ -57,8 +55,6 @@
.put('|', TokenKind.PIPE_EQUALS)
.build();
- private final EventHandler eventHandler;
-
// Input buffer and position
private final char[] buffer;
private int pos;
@@ -97,7 +93,9 @@
// the stream. Whitespace is handled differently when this is nonzero.
private int openParenStackDepth = 0;
- private boolean containsErrors;
+ // List of errors appended to by Lexer and Parser.
+ private final List<Event> errors;
+
/**
* True after a NEWLINE token.
* In other words, we are outside an expression and we have to check the indentation.
@@ -114,15 +112,13 @@
*/
private final List<Event> stringEscapeEvents = new ArrayList<>();
- /**
- * Constructs a lexer which tokenizes the contents of the specified InputBuffer. Any errors during
- * lexing are reported on "handler".
- */
- public Lexer(ParserInput input, EventHandler eventHandler, LineNumberTable lineNumberTable) {
+ /** Constructs a lexer which tokenizes the parser input. Errors are appended to {@code errors}. */
+ Lexer(ParserInput input, List<Event> errors) {
+ LineNumberTable lnt = LineNumberTable.create(input.getContent(), input.getPath());
this.buffer = input.getContent();
this.pos = 0;
- this.eventHandler = eventHandler;
- this.locationInfo = new LocationInfo(input.getPath(), lineNumberTable);
+ this.errors = errors;
+ this.locationInfo = new LocationInfo(input.getPath(), lnt);
this.checkIndentation = true;
this.comments = new ArrayList<>();
this.dents = 0;
@@ -131,10 +127,6 @@
indentStack.push(0);
}
- public Lexer(ParserInput input, EventHandler eventHandler) {
- this(input, eventHandler, LineNumberTable.create(input.getContent(), input.getPath()));
- }
-
List<Comment> getComments() {
return comments;
}
@@ -147,24 +139,15 @@
* Returns the filename from which the lexer's input came. Returns an empty value if the input
* came from a string.
*/
- public PathFragment getFilename() {
+ PathFragment getFilename() {
return locationInfo.filename != null ? locationInfo.filename : PathFragment.EMPTY_FRAGMENT;
}
/**
- * Returns true if there were errors during scanning of this input file or
- * string. The Lexer may attempt to recover from errors, but clients should
- * not rely on the results of scanning if this flag is set.
- */
- public boolean containsErrors() {
- return containsErrors;
- }
-
- /**
* Returns the next token, or EOF if it is the end of the file. It is an error to call nextToken()
* after EOF has been returned.
*/
- public Token nextToken() {
+ Token nextToken() {
boolean afterNewline = token.kind == TokenKind.NEWLINE;
token.kind = null;
tokenize();
@@ -190,8 +173,7 @@
}
private void error(String message, int start, int end) {
- this.containsErrors = true;
- eventHandler.handle(Event.error(createLocation(start, end), message));
+ errors.add(Event.error(createLocation(start, end), message));
}
Location createLocation(int start, int end) {
@@ -953,17 +935,6 @@
}
/**
- * Returns the string at the current line, minus the new line.
- *
- * @param line the line from which to retrieve the String, 1-based
- * @return the text of the line
- */
- public String stringAtLine(int line) {
- Pair<Integer, Integer> offsets = locationInfo.lineNumberTable.getOffsetsForLine(line);
- return bufferSlice(offsets.first, offsets.second);
- }
-
- /**
* Returns parts of the source buffer based on offsets
*
* @param start the beginning offset for the slice
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
index 0b4c767..9eeb6ec 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
@@ -21,7 +21,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
@@ -37,19 +36,12 @@
* Recursive descent parser for LL(2) BUILD language. Loosely based on Python 2 grammar. See
* https://docs.python.org/2/reference/grammar.html
*/
-// TODO(adonovan): break syntax->events dependency and simplify error handling in the API. The
-// result of parsing is a complete, partial, or even empty, file plus a list of errors. For
-// StarlarkFile.parse, we should materialize the error list within the StarlarkFile and remove all
-// mention of event handlers; let the client decide whether to throw or report errors. For
-// Expression.parse, throwing an exception is appropriate: expressions are typically so short that
-// only one error is wanted, so the result can be all-or-nothing.
+// TODO(adonovan): break syntax->event.Event dependency.
@VisibleForTesting
final class Parser {
- /**
- * Combines the parser result into a single value object.
- */
- public static final class ParseResult {
+ /** Combines the parser result into a single value object. */
+ static final class ParseResult {
/** The statements (rules, basically) from the parsed file. */
final List<Statement> statements;
@@ -59,23 +51,23 @@
/** Represents every statement in the file. */
final Location location;
- /** Whether the file contained any errors. */
- final boolean containsErrors;
-
+ // Errors encountered during scanning or parsing.
+ // These lists are ultimately owned by StarlarkFile.
+ final List<Event> errors;
final List<Event> stringEscapeEvents;
ParseResult(
List<Statement> statements,
List<Comment> comments,
Location location,
- boolean containsErrors,
+ List<Event> errors,
List<Event> stringEscapeEvents) {
// No need to copy here; when the object is created, the parser instance is just about to go
// out of scope and be garbage collected.
this.statements = Preconditions.checkNotNull(statements);
this.comments = Preconditions.checkNotNull(comments);
this.location = location;
- this.containsErrors = containsErrors;
+ this.errors = errors;
this.stringEscapeEvents = stringEscapeEvents;
}
}
@@ -118,7 +110,7 @@
private static final boolean DEBUGGING = false;
private final Lexer lexer;
- private final EventHandler eventHandler;
+ private final List<Event> errors;
// TODO(adonovan): opt: compute this by subtraction.
private static final Map<TokenKind, TokenKind> augmentedAssignmentMethods =
@@ -167,9 +159,9 @@
// Intern string literals, as some files contain many literals for the same string.
private final Map<String, String> stringInterner = new HashMap<>();
- private Parser(Lexer lexer, EventHandler eventHandler) {
+ private Parser(Lexer lexer, List<Event> errors) {
this.lexer = lexer;
- this.eventHandler = eventHandler;
+ this.errors = errors;
nextToken();
}
@@ -189,16 +181,16 @@
}
// Main entry point for parsing a file.
- static ParseResult parseFile(ParserInput input, EventHandler eventHandler) {
- Lexer lexer = new Lexer(input, eventHandler);
- Parser parser = new Parser(lexer, eventHandler);
+ static ParseResult parseFile(ParserInput input) {
+ List<Event> errors = new ArrayList<>();
+ Lexer lexer = new Lexer(input, errors);
+ Parser parser = new Parser(lexer, errors);
List<Statement> statements;
try (SilentCloseable c =
Profiler.instance()
.profile(ProfilerTask.STARLARK_PARSER, input.getPath().getPathString())) {
statements = parser.parseFileInput();
}
- boolean errors = parser.errorsCount > 0 || lexer.containsErrors();
return new ParseResult(
statements,
lexer.getComments(),
@@ -207,30 +199,6 @@
lexer.getStringEscapeEvents());
}
- /** Parses a sequence of statements, possibly followed by newline tokens. */
- static List<Statement> parseStatements(ParserInput input, EventHandler eventHandler) {
- Lexer lexer = new Lexer(input, eventHandler);
- Parser parser = new Parser(lexer, eventHandler);
- List<Statement> result = new ArrayList<>();
- parser.parseStatement(result);
- while (parser.token.kind == TokenKind.NEWLINE) {
- parser.nextToken();
- }
- parser.expect(TokenKind.EOF);
- return result;
- }
-
- /**
- * Convenience wrapper for {@link #parseStatements} where exactly one statement is expected.
- *
- * @throws IllegalArgumentException if the number of parsed statements was not exactly one
- */
- @VisibleForTesting
- static Statement parseStatement(ParserInput input, EventHandler eventHandler) {
- List<Statement> stmts = parseStatements(input, eventHandler);
- return Iterables.getOnlyElement(stmts);
- }
-
// stmt ::= simple_stmt
// | def_stmt
// | for_stmt
@@ -251,15 +219,18 @@
}
/** Parses an expression, possibly followed by newline tokens. */
- @VisibleForTesting
- static Expression parseExpression(ParserInput input, EventHandler eventHandler) {
- Lexer lexer = new Lexer(input, eventHandler);
- Parser parser = new Parser(lexer, eventHandler);
+ static Expression parseExpression(ParserInput input) throws SyntaxError {
+ List<Event> errors = new ArrayList<>();
+ Lexer lexer = new Lexer(input, errors);
+ Parser parser = new Parser(lexer, errors);
Expression result = parser.parseExpression();
while (parser.token.kind == TokenKind.NEWLINE) {
parser.nextToken();
}
parser.expect(TokenKind.EOF);
+ if (!errors.isEmpty()) {
+ throw new SyntaxError(errors);
+ }
return result;
}
@@ -292,7 +263,7 @@
errorsCount++;
// Limit the number of reported errors to avoid spamming output.
if (errorsCount <= 5) {
- eventHandler.handle(Event.error(location, message));
+ errors.add(Event.error(location, message));
}
}
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 f8f429d..f383b36 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
@@ -20,38 +20,37 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.syntax.Parser.ParseResult;
import java.io.IOException;
+import java.util.Collections;
import java.util.List;
import javax.annotation.Nullable;
-/** Syntax tree for a Starlark file, such as a Bazel BUILD or .bzl file. */
-public class StarlarkFile extends Node {
+/**
+ * Syntax tree for a Starlark file, such as a Bazel BUILD or .bzl file.
+ *
+ * <p>Call {@link #parse} to parse a file. Parser errors are recorded in the syntax tree (see {@link
+ * #errors}), which may be incomplete.
+ */
+public final class StarlarkFile extends Node {
private final ImmutableList<Statement> statements;
-
private final ImmutableList<Comment> comments;
-
- /**
- * Whether any errors were encountered during scanning or parsing.
- */
- private final boolean containsErrors;
-
+ final List<Event> errors; // appended to by ValidationEnvironment
private final List<Event> stringEscapeEvents;
-
@Nullable private final String contentHashCode;
private StarlarkFile(
ImmutableList<Statement> statements,
- boolean containsErrors,
+ List<Event> errors,
String contentHashCode,
Location location,
ImmutableList<Comment> comments,
List<Event> stringEscapeEvents) {
this.statements = statements;
- this.containsErrors = containsErrors;
- this.contentHashCode = contentHashCode;
this.comments = comments;
- this.setLocation(location);
+ this.errors = errors;
this.stringEscapeEvents = stringEscapeEvents;
+ this.contentHashCode = contentHashCode;
+ this.setLocation(location);
}
private static StarlarkFile create(
@@ -76,7 +75,7 @@
ImmutableList<Statement> statements = statementsbuilder.build();
return new StarlarkFile(
statements,
- result.containsErrors,
+ result.errors,
contentHashCode,
result.location,
ImmutableList.copyOf(result.comments),
@@ -91,7 +90,7 @@
ImmutableList<Statement> statements = this.statements.subList(firstStatement, lastStatement);
return new StarlarkFile(
statements,
- containsErrors,
+ errors,
null,
this.statements.get(firstStatement).getLocation(),
ImmutableList.of(),
@@ -99,96 +98,40 @@
}
/**
- * Returns true if any errors were encountered during scanning or parsing. If
- * set, clients should not rely on the correctness of the AST for builds or
- * BUILD-file editing.
+ * Returns an unmodifiable view of the list of scanner, parser, and (perhaps) resolver errors
+ * accumulated in this Starlark file.
*/
- public boolean containsErrors() {
- return containsErrors;
+ public List<Event> errors() {
+ return Collections.unmodifiableList(errors);
+ }
+
+ /** Returns errors().isEmpty(). */
+ public boolean ok() {
+ return errors.isEmpty();
}
/**
- * Returns an (immutable, ordered) list of statements in this BUILD file.
+ * Appends string escaping errors to {@code errors}. The Lexer diverts such errors into a separate
+ * bucket as they should be selectively reported depending on a StarlarkSemantics, to which the
+ * lexer/parser does not have access. This function is called by ValidationEnvironment, which has
+ * access to a StarlarkSemantics and can thus decide whether to respect or ignore these events.
+ *
+ * <p>Naturally this function should be called at most once.
*/
+ void addStringEscapeEvents() {
+ errors.addAll(stringEscapeEvents);
+ }
+
+ /** Returns an (immutable, ordered) list of statements in this BUILD file. */
public ImmutableList<Statement> getStatements() {
return statements;
}
- /**
- * Returns an (immutable, ordered) list of comments in this BUILD file.
- */
+ /** Returns an (immutable, ordered) list of comments in this BUILD file. */
public ImmutableList<Comment> getComments() {
return comments;
}
- /** Returns true if there was no error event. */
- public boolean replayLexerEvents(StarlarkThread thread, EventHandler eventHandler) {
- if (thread.getSemantics().incompatibleRestrictStringEscapes()
- && !stringEscapeEvents.isEmpty()) {
- Event.replayEventsOn(eventHandler, stringEscapeEvents);
- return false;
- }
- return true;
- }
-
- /**
- * Executes this Starlark file in the given StarlarkThread.
- *
- * <p>Execution stops at the first execution error, which is reported to the event handler. (Other
- * kinds of errors, such as problems within calls to built-in functions like rule or attr, cause
- * events to be reported but do not cause Starlark execution to fail.
- *
- * <p>This method does not affect the value of {@link #containsErrors()}, which refers only to
- * lexer/parser errors.
- *
- * @return true if no error occurred during execution.
- */
- // TODO(adonovan): move to EvalUtils.
- public boolean exec(StarlarkThread thread, EventHandler eventHandler)
- throws InterruptedException {
- for (Statement stmt : statements) {
- if (!execTopLevelStatement(stmt, thread, eventHandler)) {
- return false;
- }
- }
- return true;
- }
-
- /**
- * Executes a top-level statement of this Starlark file in the given StarlarkThread.
- *
- * <p>If there was an execution error, it is reported to the event handler, and the function
- * returns false. (Other kinds of errors, such as problems within calls to built-in functions like
- * rule or attr, cause events to be reported but do not cause Starlark execution to fail.
- *
- * <p>This method does not affect the value of {@link #containsErrors()}, which refers only to
- * lexer/parser errors.
- *
- * @return true if no error occurred during execution.
- */
- public boolean execTopLevelStatement(
- Statement stmt, StarlarkThread thread, EventHandler eventHandler)
- throws InterruptedException {
- try {
- Eval.execToplevelStatement(thread, stmt);
- return true;
- } catch (EvalException e) {
- // Do not report errors caused by a previous parsing error, as it has already been
- // reported.
- if (e.isDueToIncompleteAST()) {
- return false;
- }
- // When the exception is raised from another file, report first the location in the
- // BUILD file (as it is the most probable cause for the error).
- Location exnLoc = e.getLocation();
- Location nodeLoc = stmt.getLocation();
- eventHandler.handle(Event.error(
- (exnLoc == null || !nodeLoc.getPath().equals(exnLoc.getPath())) ? nodeLoc : exnLoc,
- e.getMessage()));
- return false;
- }
- }
-
@Override
public void prettyPrint(Appendable buffer, int indentLevel) throws IOException {
// Only statements are printed, not comments.
@@ -209,31 +152,30 @@
/**
* Parse the specified file, returning its syntax tree with the preludeStatements inserted at the
- * front of its statement list. All errors during scanning or parsing will be reported to the
- * event handler.
+ * front of its statement list.
*/
public static StarlarkFile parseWithPrelude(
- ParserInput input, List<Statement> preludeStatements, EventHandler eventHandler) {
- Parser.ParseResult result = Parser.parseFile(input, eventHandler);
+ ParserInput input, List<Statement> preludeStatements) {
+ Parser.ParseResult result = Parser.parseFile(input);
return create(
preludeStatements, result, /* contentHashCode= */ null, /*allowImportInternal=*/ false);
}
/**
* Parse the specified build file, returning its AST. All load statements parsed that way will be
- * exempt from visibility restrictions. All errors during scanning or parsing will be reported to
- * the event handler.
+ * exempt from visibility restrictions.
*/
+ // TODO(adonovan): make LoadStatement.allowInternal publicly settable, and delete this.
public static StarlarkFile parseVirtualBuildFile(
- ParserInput input, List<Statement> preludeStatements, EventHandler eventHandler) {
- Parser.ParseResult result = Parser.parseFile(input, eventHandler);
+ ParserInput input, List<Statement> preludeStatements) {
+ Parser.ParseResult result = Parser.parseFile(input);
return create(
preludeStatements, result, /* contentHashCode= */ null, /*allowImportInternal=*/ true);
}
- public static StarlarkFile parseWithDigest(
- ParserInput input, byte[] digest, EventHandler eventHandler) throws IOException {
- Parser.ParseResult result = Parser.parseFile(input, eventHandler);
+ // TODO(adonovan): make the digest publicly settable, and delete this.
+ public static StarlarkFile parseWithDigest(ParserInput input, byte[] digest) throws IOException {
+ Parser.ParseResult result = Parser.parseFile(input);
return create(
/* preludeStatements= */ ImmutableList.of(),
result,
@@ -241,59 +183,46 @@
/* allowImportInternal= */ false);
}
- public static StarlarkFile parse(ParserInput input, EventHandler eventHandler) {
- Parser.ParseResult result = Parser.parseFile(input, eventHandler);
+ /**
+ * Parse a Starlark file.
+ *
+ * <p>A syntax tree is always returned, even in case of error. Errors are recorded in the tree.
+ * Example usage:
+ *
+ * <pre>
+ * StarlarkFile file = StarlarkFile.parse(input);
+ * if (!file.ok()) {
+ * Event.replayEventsOn(handler, file.errors());
+ * ...
+ * }
+ * </pre>
+ */
+ public static StarlarkFile parse(ParserInput input) {
+ Parser.ParseResult result = Parser.parseFile(input);
return create(
- /* preludeStatements= */ ImmutableList.<Statement>of(),
+ /* preludeStatements= */ ImmutableList.of(),
result,
/* contentHashCode= */ null,
/* allowImportInternal=*/ false);
}
- /**
- * Parse the specified file but avoid the validation of the imports, returning its AST. All errors
- * during scanning or parsing will be reported to the event handler.
- */
- // TODO(adonovan): redundant; delete.
- public static StarlarkFile parseWithoutImports(ParserInput input, EventHandler eventHandler) {
- ParseResult result = Parser.parseFile(input, eventHandler);
- return new StarlarkFile(
- ImmutableList.copyOf(result.statements),
- result.containsErrors,
- /* contentHashCode= */ null,
- result.location,
- ImmutableList.copyOf(result.comments),
- result.stringEscapeEvents);
+ // TODO(adonovan): legacy function for copybara compatibility; delete ASAP.
+ public static StarlarkFile parseWithoutImports(ParserInput input, EventHandler handler) {
+ StarlarkFile file = parse(input);
+ Event.replayEventsOn(handler, file.errors());
+ return file;
}
- /**
- * Run static checks on the syntax tree.
- *
- * @return a new syntax tree (or the same), with the containsErrors flag updated.
- */
- // TODO(adonovan): eliminate. Most callers need validation because they intend to execute the
- // file, and should be made to use higher-level operations in EvalUtils.
- // rest should skip this step. Called from EvaluationTestCase, ParserTest, ASTFileLookupFunction.
- public StarlarkFile validate(
- StarlarkThread thread, boolean isBuildFile, EventHandler eventHandler) {
+ // TODO(adonovan): legacy function for copybara compatibility; delete ASAP.
+ public boolean exec(StarlarkThread thread, EventHandler eventHandler)
+ throws InterruptedException {
try {
- ValidationEnvironment.validateFile(this, thread, isBuildFile);
- return this;
- } catch (EvalException e) {
- if (!e.isDueToIncompleteAST()) {
- eventHandler.handle(Event.error(e.getLocation(), e.getMessage()));
- }
+ EvalUtils.exec(this, thread);
+ return true;
+ } catch (EvalException ex) {
+ eventHandler.handle(Event.error(ex.getLocation(), ex.getMessage()));
+ return false;
}
- if (containsErrors) {
- return this; // already marked as errant
- }
- return new StarlarkFile(
- statements,
- /*containsErrors=*/ true,
- contentHashCode,
- getLocation(),
- comments,
- stringEscapeEvents);
}
/**
@@ -318,27 +247,31 @@
/**
* Parses, resolves and evaluates the input and returns the value of the last statement if it's an
- * Expression or else null. In case of error (either during validation or evaluation), it throws
- * an EvalException. The return value is as for eval(StarlarkThread).
+ * Expression or else null. In case of scan/parse/resolver error, it throws a SyntaxError
+ * containing one or more specific errors. If evaluation fails, it throws an EvalException or
+ * InterruptedException. The return value is as for eval(StarlarkThread).
*/
// Note: uses Starlark (not BUILD) validation semantics.
// TODO(adonovan): move to EvalUtils; see other eval function.
@Nullable
public static Object eval(ParserInput input, StarlarkThread thread)
- throws EvalException, InterruptedException {
- StarlarkFile ast = parseAndValidateSkylark(input, thread);
- return ast.eval(thread);
+ throws SyntaxError, EvalException, InterruptedException {
+ StarlarkFile file = parseAndValidateSkylark(input, thread);
+ if (!file.ok()) {
+ throw new SyntaxError(file.errors());
+ }
+ return file.eval(thread);
}
/**
- * Parses and validates the input and returns the syntax tree. In case of error during validation,
- * it throws an EvalException. Uses Starlark (not BUILD) validation semantics.
+ * Parses and validates the input and returns the syntax tree. It uses Starlark (not BUILD)
+ * validation semantics.
+ *
+ * <p>The thread is primarily used for its GlobalFrame. Scan/parse/validate errors are recorded in
+ * the StarlarkFile. It is the caller's responsibility to inspect them.
*/
- // TODO(adonovan): move to EvalUtils; see above.
- public static StarlarkFile parseAndValidateSkylark(ParserInput input, StarlarkThread thread)
- throws EvalException {
- StarlarkFile file = parse(input, thread.getEventHandler());
- file.replayLexerEvents(thread, thread.getEventHandler());
+ public static StarlarkFile parseAndValidateSkylark(ParserInput input, StarlarkThread thread) {
+ StarlarkFile file = parse(input);
ValidationEnvironment.validateFile(file, thread, /*isBuildFile=*/ false);
return file;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java
index 037943b..b9d3a1d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java
@@ -38,7 +38,6 @@
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
-import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Objects;
@@ -1179,44 +1178,25 @@
return vars;
}
- private static final class EvalEventHandler implements EventHandler {
- List<Event> messages = new ArrayList<>();
-
- @Override
- public void handle(Event event) {
- if (event.getKind() == EventKind.ERROR) {
- messages.add(event);
- }
- }
- }
-
- /** Evaluates a Skylark statement in this thread. (Debugger API) */
+ /** Evaluates a Skylark statement in this thread. (Debugger API) This operation mutates expr. */
// TODO(adonovan): push this up into the debugger once the eval API is finalized.
- public Object debugEval(ParserInput input) throws EvalException, InterruptedException {
- EvalEventHandler handler = new EvalEventHandler();
- Expression expr = Expression.parse(input, handler);
- if (!handler.messages.isEmpty()) {
- Event ev = handler.messages.get(0);
- throw new EvalException(ev.getLocation(), ev.getMessage());
- }
+ public Object debugEval(Expression expr) throws EvalException, InterruptedException {
return Eval.eval(this, expr);
}
/** Executes a Skylark file (sequence of statements) in this thread. (Debugger API) */
// TODO(adonovan): push this up into the debugger once the exec API is finalized.
- public void debugExec(ParserInput input) throws EvalException, InterruptedException {
- EvalEventHandler handler = new EvalEventHandler();
- StarlarkFile file = StarlarkFile.parse(input, handler);
- if (!handler.messages.isEmpty()) {
- Event ev = handler.messages.get(0);
- throw new EvalException(ev.getLocation(), ev.getMessage());
+ public void debugExec(ParserInput input) throws SyntaxError, EvalException, InterruptedException {
+ StarlarkFile file = StarlarkFile.parse(input);
+ ValidationEnvironment.validateFile(file, this, /*isBuildFile=*/ false);
+ if (!file.ok()) {
+ throw new SyntaxError(file.errors());
}
for (Statement stmt : file.getStatements()) {
if (stmt instanceof LoadStatement) {
- throw new EvalException(null, "cannot execute load statements in debugger");
+ throw new EvalException(stmt.getLocation(), "cannot execute load statements in debugger");
}
}
- ValidationEnvironment.validateFile(file, this, /*isBuildFile=*/ false);
Eval.execStatements(this, file.getStatements());
}
@@ -1441,6 +1421,7 @@
/** An exception thrown by {@link #FAIL_FAST_HANDLER}. */
// TODO(bazel-team): Possibly extend RuntimeException instead of IllegalArgumentException.
+ // TODO(adonovan): move to EventCollectionApparatus.
public static class FailFastException extends IllegalArgumentException {
public FailFastException(String s) {
super(s);
@@ -1455,6 +1436,7 @@
* assertions) need to be able to distinguish between organically occurring exceptions and
* exceptions thrown by this handler.
*/
+ // TODO(adonovan): move to EventCollectionApparatus.
public static final EventHandler FAIL_FAST_HANDLER =
new EventHandler() {
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxError.java b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxError.java
new file mode 100644
index 0000000..112fcce
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxError.java
@@ -0,0 +1,67 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.syntax;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.events.Event;
+import java.util.List;
+
+/**
+ * An exception that indicates a static error associated with the syntax, such as scanner or parse
+ * error, a structural problem, or a failure of identifier resolution. The exception records one or
+ * more errors, each with a syntax location.
+ *
+ * <p>SyntaxError is thrown by operations such as {@link Expression#parse}, which are "all or
+ * nothing". By contrast, {@link StarlarkFile#parse} does not throw an exception; instead, it
+ * records the accumulated scanner, parser, and optionally validation errors within the syntax tree,
+ * so that clients may obtain partial information from a damaged file.
+ *
+ * <p>Clients that fail abruptly when encountering parse errors are encouraged to use SyntaxError,
+ * as in this example:
+ *
+ * <pre>
+ * StarlarkFile file = StarlarkFile.parse(input);
+ * if (!file.ok()) {
+ * throw new SyntaxError(file.errors());
+ * }
+ * </pre>
+ */
+public final class SyntaxError extends Exception {
+
+ private final ImmutableList<Event> errors;
+
+ /** Construct a SyntaxError from a non-empty list of errors. */
+ public SyntaxError(List<Event> errors) {
+ if (errors.isEmpty()) {
+ throw new IllegalArgumentException("no errors");
+ }
+ this.errors = ImmutableList.copyOf(errors);
+ }
+
+ /** Returns an immutable non-empty list of errors. */
+ public ImmutableList<Event> errors() {
+ return errors;
+ }
+
+ @Override
+ public String getMessage() {
+ String first = errors.get(0).getMessage();
+ if (errors.size() > 1) {
+ return String.format("%s (+ %d more)", first, errors.size() - 1);
+ } else {
+ return first;
+ }
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
index 31e15c8..426454f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -16,8 +16,8 @@
import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.util.SpellChecker;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
@@ -32,6 +32,14 @@
* <p>When a variable is defined, it is visible in the entire block. For example, a global variable
* is visible in the entire file; a variable in a function is visible in the entire function block
* (even on the lines before its first assignment).
+ *
+ * <p>Validation is a mutation of the syntax tree, as it attaches scope information to Identifier
+ * nodes. (In the future, it will attach additional information to functions to support lexical
+ * scope, and even compilation of the trees to bytecode.) Validation errors are reported in the
+ * analogous manner to scan/parse errors: for a StarlarkFile, they are appended to {@code
+ * StarlarkFile.errors}; for an expression they will be [TODO(adonovan): implement] reported by an
+ * SyntaxError exception. It is legal to validate a file that already contains scan/parse errors,
+ * though it may lead to secondary validation errors.
*/
// TODO(adonovan): make this class private. Call it through the EvalUtils facade.
public final class ValidationEnvironment extends NodeVisitor {
@@ -66,31 +74,20 @@
}
}
- /**
- * We use an unchecked exception around EvalException because the NodeVisitor doesn't let visit
- * methods throw checked exceptions. We might change that later.
- */
- private static class ValidationException extends RuntimeException {
- EvalException exception;
-
- ValidationException(EvalException e) {
- exception = e;
- }
-
- ValidationException(Location location, String message) {
- exception = new EvalException(location, message);
- }
- }
-
+ private final List<Event> errors;
private final StarlarkThread thread;
private Block block;
private int loopCount;
/** In BUILD files, we have a slightly different behavior for legacy reasons. */
+ // TODO(adonovan): eliminate isBuildFile. It is necessary because the prelude is implemented
+ // by inserting shared Statements, which must not be mutated, into each StarlarkFile.
+ // Instead, we should implement the prelude by executing it like a .bzl module
+ // and putting its members in the initial environment of the StarlarkFile.
private final boolean isBuildFile;
- /** Create a ValidationEnvironment for a given global StarlarkThread (containing builtins). */
- private ValidationEnvironment(StarlarkThread thread, boolean isBuildFile) {
+ private ValidationEnvironment(List<Event> errors, StarlarkThread thread, boolean isBuildFile) {
Preconditions.checkArgument(thread.isGlobal());
+ this.errors = errors;
this.thread = thread;
this.isBuildFile = isBuildFile;
block = new Block(Scope.Universe, null);
@@ -98,6 +95,10 @@
block.variables.addAll(builtinVariables);
}
+ void addError(Location loc, String message) {
+ errors.add(Event.error(loc, message));
+ }
+
/**
* First pass: add all definitions to the current block. This is done because symbols are
* sometimes used before their definition point (e.g. a functions are not necessarily declared in
@@ -143,7 +144,7 @@
Set<String> names = new HashSet<>();
for (LoadStatement.Binding b : load.getBindings()) {
if (!names.add(b.getLocalName().getName())) {
- throw new ValidationException(
+ addError(
b.getLocalName().getLocation(),
String.format(
"load statement defines '%s' more than once", b.getLocalName().getName()));
@@ -180,7 +181,7 @@
assign(elem);
}
} else {
- throw new ValidationException(lhs.getLocation(), "cannot assign to '" + lhs + "'");
+ addError(lhs.getLocation(), "cannot assign to '" + lhs + "'");
}
}
@@ -191,12 +192,12 @@
// The identifier might not exist because it was restricted (hidden) by the current semantics.
// If this is the case, output a more helpful error message than 'not found'.
FlagGuardedValue result = thread.getRestrictedBindings().get(node.getName());
- if (result != null) {
- throw new ValidationException(
- result.getEvalExceptionFromAttemptingAccess(
- node.getLocation(), thread.getSemantics(), node.getName()));
- }
- throw new ValidationException(Eval.createInvalidIdentifierException(node, getAllSymbols()));
+ addError(
+ node.getLocation(),
+ result != null
+ ? result.getErrorFromAttemptingAccess(thread.getSemantics(), node.getName())
+ : createInvalidIdentifierException(node.getName(), getAllSymbols()));
+ return;
}
// TODO(laurentlb): In BUILD files, calling setScope will throw an exception. This happens
// because some AST nodes are shared across multipe ASTs (due to the prelude file).
@@ -205,11 +206,39 @@
}
}
+ // This is exposed to Eval until validation becomes a precondition for evaluation.
+ static String createInvalidIdentifierException(String name, Set<String> candidates) {
+ if (name.equals("$error$")) {
+ return "contains syntax error(s)";
+ }
+
+ String error = getErrorForObsoleteThreadLocalVars(name);
+ if (error != null) {
+ return error;
+ }
+
+ String suggestion = SpellChecker.didYouMean(name, candidates);
+ return "name '" + name + "' is not defined" + suggestion;
+ }
+
+ static String getErrorForObsoleteThreadLocalVars(String name) {
+ if (name.equals("PACKAGE_NAME")) {
+ return "The value 'PACKAGE_NAME' has been removed in favor of 'package_name()', "
+ + "please use the latter ("
+ + "https://docs.bazel.build/versions/master/skylark/lib/native.html#package_name). ";
+ }
+ if (name.equals("REPOSITORY_NAME")) {
+ return "The value 'REPOSITORY_NAME' has been removed in favor of 'repository_name()', please"
+ + " use the latter ("
+ + "https://docs.bazel.build/versions/master/skylark/lib/native.html#repository_name).";
+ }
+ return null;
+ }
+
@Override
public void visit(ReturnStatement node) {
if (block.scope != Scope.Local) {
- throw new ValidationException(
- node.getLocation(), "return statements must be inside a function");
+ addError(node.getLocation(), "return statements must be inside a function");
}
super.visit(node);
}
@@ -217,7 +246,7 @@
@Override
public void visit(ForStatement node) {
if (block.scope != Scope.Local) {
- throw new ValidationException(
+ addError(
node.getLocation(),
"for loops are not allowed at the top level. You may move it inside a function "
+ "or use a comprehension, [f(x) for x in sequence]");
@@ -233,7 +262,7 @@
@Override
public void visit(LoadStatement node) {
if (block.scope == Scope.Local) {
- throw new ValidationException(node.getLocation(), "load statement not at top level");
+ addError(node.getLocation(), "load statement not at top level");
}
super.visit(node);
}
@@ -241,9 +270,8 @@
@Override
public void visit(FlowStatement node) {
if (node.getKind() != TokenKind.PASS && loopCount <= 0) {
- throw new ValidationException(
- node.getLocation(),
- node.getKind().toString() + " statement must be inside a for loop");
+ addError(
+ node.getLocation(), node.getKind().toString() + " statement must be inside a for loop");
}
super.visit(node);
}
@@ -281,7 +309,7 @@
@Override
public void visit(DefStatement node) {
if (block.scope == Scope.Local) {
- throw new ValidationException(
+ addError(
node.getLocation(),
"nested functions are not allowed. Move the function to the top level.");
}
@@ -304,7 +332,7 @@
@Override
public void visit(IfStatement node) {
if (block.scope != Scope.Local) {
- throw new ValidationException(
+ addError(
node.getLocation(),
"if statements are not allowed at the top level. You may move it inside a function "
+ "or use an if expression (x if condition else y).");
@@ -321,7 +349,7 @@
@Override
public void visit(AugmentedAssignmentStatement node) {
if (node.getLHS() instanceof ListExpression) {
- throw new ValidationException(
+ addError(
node.getLocation(), "cannot perform augmented assignment on a list or tuple expression");
}
// Other bad cases are handled in assign.
@@ -338,7 +366,7 @@
// TODO(adonovan): make error message more precise: "x reassigned at top level"
// and emit a secondary error "x previously declared here". This requires an
// upcoming changes to report events not exceptions.
- throw new ValidationException(
+ addError(
location,
String.format(
"Variable %s is read only (read more at %s)",
@@ -367,8 +395,8 @@
return all;
}
- /** Throws ValidationException if a load() appears after another kind of statement. */
- private static void checkLoadAfterStatement(List<Statement> statements) {
+ // Report an error if a load statement appears after another kind of statement.
+ private void checkLoadAfterStatement(List<Statement> statements) {
Location firstStatement = null;
for (Statement statement : statements) {
@@ -382,7 +410,7 @@
if (firstStatement == null) {
continue;
}
- throw new ValidationException(
+ addError(
statement.getLocation(),
"load() statements must be called before any other statement. "
+ "First non-load() statement appears at "
@@ -414,32 +442,20 @@
closeBlock();
}
- // Public entry point, throwing variant.
- // TODO(adonovan): combine with variant below.
- public static void validateFile(StarlarkFile file, StarlarkThread thread, boolean isBuildFile)
- throws EvalException {
- try {
- ValidationEnvironment venv = new ValidationEnvironment(thread, isBuildFile);
- venv.validateToplevelStatements(file.getStatements());
- // Check that no closeBlock was forgotten.
- Preconditions.checkState(venv.block.parent == null);
- } catch (ValidationException e) {
- throw e.exception;
+ /**
+ * Performs static checks, including name resolution on {@code file}, which is mutated. The {@code
+ * StarlarkThread} provides the global names and the StarlarkSemantics. Errors are appended to
+ * {@link StarlarkFile#errors}.
+ */
+ // TODO(adonovan): replace thread by Set<String> and StarlarkSemantics.
+ public static void validateFile(StarlarkFile file, StarlarkThread thread, boolean isBuildFile) {
+ ValidationEnvironment venv = new ValidationEnvironment(file.errors, thread, isBuildFile);
+ if (thread.getSemantics().incompatibleRestrictStringEscapes()) {
+ file.addStringEscapeEvents();
}
- }
-
- // Public entry point, error handling variant.
- public static boolean validateFile(
- StarlarkFile file, StarlarkThread thread, boolean isBuildFile, EventHandler eventHandler) {
- try {
- validateFile(file, thread, isBuildFile);
- return true;
- } catch (EvalException e) {
- if (!e.isDueToIncompleteAST()) {
- eventHandler.handle(Event.error(e.getLocation(), e.getMessage()));
- }
- return false;
- }
+ venv.validateToplevelStatements(file.getStatements());
+ // Check that no closeBlock was forgotten.
+ Preconditions.checkState(venv.block.parent == null);
}
/** Open a new lexical block that will contain the future declarations. */
diff --git a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
index a960812..18c4ce6 100644
--- a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
+++ b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
@@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.StarlarkSemanticsOptions;
import com.google.devtools.build.lib.skylarkbuildapi.TopLevelBootstrap;
@@ -429,7 +430,8 @@
pending.add(path);
ParserInput parserInputSource = getInputSource(path.toString());
- StarlarkFile file = StarlarkFile.parse(parserInputSource, eventHandler);
+ StarlarkFile file = StarlarkFile.parse(parserInputSource);
+ Event.replayEventsOn(eventHandler, file.errors());
moduleDocMap.put(label, getModuleDoc(file));
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
index eb6cbe5..c052eb9 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
@@ -103,7 +103,7 @@
"runfiles");
ExtendedEventHandler listener = Mockito.mock(ExtendedEventHandler.class);
ParserInput input = ParserInput.fromLines("test()");
- FuncallExpression ast = (FuncallExpression) Expression.parse(input, listener);
+ FuncallExpression ast = (FuncallExpression) Expression.parse(input);
Rule rule =
WorkspaceFactoryHelper.createAndAddRepositoryRule(
packageBuilder, buildRuleClass(attributes), null, kwargs, ast);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 806442f..dc91321 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -1188,10 +1188,7 @@
" pattern,",
"])",
"other_variable = glob(include = ['a'], exclude = ['b'])",
- "third_variable = glob(['c'], exclude_directories = 0)"),
- event -> {
- throw new IllegalArgumentException(event.getMessage());
- }));
+ "third_variable = glob(['c'], exclude_directories = 0)")));
assertThat(globPatternExtractor.getExcludeDirectoriesPatterns())
.containsExactly("ab", "a", "**/*");
assertThat(globPatternExtractor.getIncludeDirectoriesPatterns()).containsExactly("c");
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
index 0c81e9b..5272098 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
@@ -134,11 +134,12 @@
/** Instantiates a {@link SkylarkInfo} with fields a=1, b=2, c=3 (and nothing else). */
private static SkylarkInfo instantiateWithA1B2C3(SkylarkProvider provider) throws Exception{
// Code under test begins with the entry point in BaseFunction.
- Object result = provider.call(
- ImmutableList.of(),
- ImmutableMap.of("a", 1, "b", 2, "c", 3),
- /*ast=*/ null,
- /*env=*/ null);
+ Object result =
+ provider.call(
+ ImmutableList.of(),
+ ImmutableMap.of("a", 1, "b", 2, "c", 3),
+ /*ast=*/ null,
+ /*thread=*/ null);
assertThat(result).isInstanceOf(SkylarkInfo.class);
return (SkylarkInfo) result;
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
index 9ce4643..e1b2bdb 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
@@ -138,7 +138,9 @@
public StarlarkFile ast(Path buildFile) throws IOException {
byte[] bytes = FileSystemUtils.readWithKnownFileSize(buildFile, buildFile.getFileSize());
ParserInput input = ParserInput.create(bytes, buildFile.asFragment());
- return StarlarkFile.parse(input, eventHandler);
+ StarlarkFile file = StarlarkFile.parse(input);
+ Event.replayEventsOn(eventHandler, file.errors());
+ return file;
}
/** Evaluates the {@code buildFileAST} into a {@link Package}. */
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
index 4ca6764..4ab2bd7 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
@@ -343,7 +343,6 @@
Pair<Set<Label>, Boolean> result = parseListKeepGoing("foo/...");
assertThat(result.first).containsExactlyElementsIn(rulesBeneathFoo);
assertContainsEvent("syntax error at 'build'");
- assertContainsEvent("name 'invalid' is not defined");
reporter.addHandler(failFastHandler);
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 7baff22..002297c 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -59,6 +59,7 @@
import com.google.devtools.build.lib.syntax.StarlarkFile;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
+import com.google.devtools.build.lib.syntax.SyntaxError;
import com.google.devtools.build.lib.testutil.MoreAsserts;
import com.google.devtools.build.lib.util.FileTypeSet;
import java.util.Collection;
@@ -705,6 +706,9 @@
protected void evalAndExport(String... lines) throws Exception {
ParserInput input = ParserInput.fromLines(lines);
StarlarkFile file = StarlarkFile.parseAndValidateSkylark(input, ev.getStarlarkThread());
+ if (!file.errors().isEmpty()) {
+ throw new SyntaxError(file.errors());
+ }
SkylarkImportLookupFunction.execAndExport(
file, FAKE_LABEL, ev.getEventHandler(), ev.getStarlarkThread());
}
@@ -1255,14 +1259,6 @@
}
@Test
- public void testStructMembersAreImmutable() throws Exception {
- checkErrorContains(
- "cannot assign to 's.x'",
- "s = struct(x = 'a')",
- "s.x = 'b'\n");
- }
-
- @Test
public void testStructDictMembersAreMutable() throws Exception {
eval(
"s = struct(x = {'a' : 1})",
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java b/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java
index a614466..4a4e846 100644
--- a/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventKind;
import com.google.devtools.build.lib.events.util.EventCollectionApparatus;
import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos;
@@ -763,11 +764,13 @@
return StarlarkThread.builder(mutability).useDefaultSemantics().build();
}
- private StarlarkFile parseBuildFile(String path, String... lines) throws IOException {
- Path file = scratch.file(path, lines);
- byte[] bytes = FileSystemUtils.readWithKnownFileSize(file, file.getFileSize());
- ParserInput inputSource = ParserInput.create(bytes, file.asFragment());
- return StarlarkFile.parse(inputSource, events.reporter());
+ private StarlarkFile parseBuildFile(String filename, String... lines) throws IOException {
+ Path path = scratch.file(filename, lines);
+ byte[] bytes = FileSystemUtils.readWithKnownFileSize(path, path.getFileSize());
+ ParserInput input = ParserInput.create(bytes, path.asFragment());
+ StarlarkFile file = StarlarkFile.parse(input);
+ Event.replayEventsOn(events.reporter(), file.errors());
+ return file;
}
private StarlarkFile parseSkylarkFile(String path, String... lines) throws IOException {
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java
index 12ca90a..18e5fbe 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java
@@ -18,6 +18,7 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
import java.io.IOException;
import org.junit.Test;
@@ -66,8 +67,12 @@
* string.
*/
private void assertExprPrettyMatches(String source, String expected) {
- Expression node = parseExpression(source);
- assertPrettyMatches(node, expected);
+ try {
+ Expression node = parseExpression(source);
+ assertPrettyMatches(node, expected);
+ } catch (SyntaxError ex) {
+ Event.replayEventsOn(getEventHandler(), ex.errors());
+ }
}
/**
@@ -75,8 +80,12 @@
* given string.
*/
private void assertExprTostringMatches(String source, String expected) {
- Expression node = parseExpression(source);
- assertThat(node.toString()).isEqualTo(expected);
+ try {
+ Expression node = parseExpression(source);
+ assertThat(node.toString()).isEqualTo(expected);
+ } catch (SyntaxError ex) {
+ Event.replayEventsOn(getEventHandler(), ex.errors());
+ }
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index 41b0ff0..00592ae 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -32,9 +32,9 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/**
- * Test of evaluation behavior. (Implicitly uses lexer + parser.)
- */
+/** Test of evaluation behavior. (Implicitly uses lexer + parser.) */
+// TODO(adonovan): separate tests of parser, resolver, Starlark core evaluator,
+// and BUILD and .bzl features.
@RunWith(JUnit4.class)
public class EvaluationTest extends EvaluationTestCase {
@@ -107,11 +107,6 @@
.testStatement("1 if True else 2", 1)
.testStatement("1 if False else 2", 2)
.testStatement("1 + 2 if 3 + 4 else 5 + 6", 3);
-
- setFailFast(false);
- parseExpression("1 if 2");
- assertContainsError(
- "missing else clause in conditional expression or semicolon before if");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/LValueBoundNamesTest.java b/src/test/java/com/google/devtools/build/lib/syntax/LValueBoundNamesTest.java
index 0b651fc..df33118 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/LValueBoundNamesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/LValueBoundNamesTest.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.syntax;
import com.google.common.truth.Truth;
+import com.google.devtools.build.lib.events.Event;
import java.util.Arrays;
import java.util.Set;
import java.util.stream.Collectors;
@@ -52,7 +53,10 @@
private static void assertBoundNames(String assignment, String... expectedBoundNames) {
ParserInput input = ParserInput.fromLines(assignment);
- StarlarkFile file = StarlarkFile.parse(input, StarlarkThread.FAIL_FAST_HANDLER);
+ StarlarkFile file = StarlarkFile.parse(input);
+ for (Event error : file.errors()) {
+ throw new AssertionError(error);
+ }
Expression lhs = ((AssignmentStatement) file.getStatements().get(0)).getLHS();
Set<String> boundNames =
Identifier.boundIdentifiers(lhs).stream()
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java b/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java
index 74bba9b..412aa59 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java
@@ -15,15 +15,12 @@
import static com.google.common.truth.Truth.assertThat;
-import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventHandler;
-import com.google.devtools.build.lib.events.EventKind;
import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
+import java.util.List;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -33,6 +30,10 @@
*/
@RunWith(JUnit4.class)
public class LexerTest {
+
+ // TODO(adonovan): make these these tests less unnecessarily stateful.
+
+ private final List<Event> errors = new ArrayList<>();
private String lastError;
private Location lastErrorLocation;
@@ -43,19 +44,10 @@
private Lexer createLexer(String input) {
PathFragment somePath = PathFragment.create("/some/path.txt");
ParserInput inputSource = ParserInput.create(input, somePath);
- Reporter reporter = new Reporter(new EventBus());
- reporter.addHandler(new EventHandler() {
- @Override
- public void handle(Event event) {
- if (EventKind.ERRORS.contains(event.getKind())) {
- lastErrorLocation = event.getLocation();
- lastError = lastErrorLocation.getPath() + ":"
- + event.getLocation().getStartLineAndColumn().getLine() + ": " + event.getMessage();
- }
- }
- });
-
- return new Lexer(inputSource, reporter);
+ errors.clear();
+ lastErrorLocation = null;
+ lastError = null;
+ return new Lexer(inputSource, errors);
}
private ArrayList<Token> allTokens(Lexer lexer) {
@@ -65,6 +57,17 @@
tok = lexer.nextToken();
result.add(tok.copy());
} while (tok.kind != TokenKind.EOF);
+
+ for (Event error : errors) {
+ lastErrorLocation = error.getLocation();
+ lastError =
+ error.getLocation().getPath()
+ + ":"
+ + error.getLocation().getStartLineAndColumn().getLine()
+ + ": "
+ + error.getMessage();
+ }
+
return result;
}
@@ -482,16 +485,16 @@
public void testContainsErrors() throws Exception {
Lexer lexerSuccess = createLexer("foo");
allTokens(lexerSuccess); // ensure the file has been completely scanned
- assertThat(lexerSuccess.containsErrors()).isFalse();
+ assertThat(errors).isEmpty();
Lexer lexerFail = createLexer("f$o");
allTokens(lexerFail);
- assertThat(lexerFail.containsErrors()).isTrue();
+ assertThat(errors).isNotEmpty();
String s = "'unterminated";
lexerFail = createLexer(s);
allTokens(lexerFail);
- assertThat(lexerFail.containsErrors()).isTrue();
+ assertThat(errors).isNotEmpty();
assertThat(lastErrorLocation.getStartOffset()).isEqualTo(0);
assertThat(lastErrorLocation.getEndOffset()).isEqualTo(s.length());
assertThat(values(tokens(s))).isEqualTo("STRING(unterminated) NEWLINE EOF");
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/NodeVisitorTest.java b/src/test/java/com/google/devtools/build/lib/syntax/NodeVisitorTest.java
index 6be69bf..93b03a8 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/NodeVisitorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/NodeVisitorTest.java
@@ -15,7 +15,6 @@
import static com.google.common.truth.Truth.assertThat;
-import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import org.junit.Test;
@@ -26,13 +25,17 @@
@RunWith(JUnit4.class)
public final class NodeVisitorTest {
- private StarlarkFile parse(String... lines) throws IOException {
+ private static StarlarkFile parse(String... lines) throws SyntaxError {
ParserInput input = ParserInput.fromLines(lines);
- return StarlarkFile.parse(input, StarlarkThread.FAIL_FAST_HANDLER);
+ StarlarkFile file = StarlarkFile.parse(input);
+ if (!file.ok()) {
+ throw new SyntaxError(file.errors());
+ }
+ return file;
}
@Test
- public void everyIdentifierAndParameterIsVisitedInOrder() throws IOException {
+ public void everyIdentifierAndParameterIsVisitedInOrder() throws Exception {
final List<String> idents = new ArrayList<>();
final List<String> params = new ArrayList<>();
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
index 45c8655..04d41da 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
@@ -47,20 +47,31 @@
events.setFailFast(failFast);
}
- private void clearEvents() {
- events.clear();
+ // Joins the lines, parse, and returns an expression.
+ private static Expression parseExpression(String... lines) throws SyntaxError {
+ ParserInput input = ParserInput.fromLines(lines);
+ return Expression.parse(input);
}
- // Joins the lines, parse, and returns an expression.
- private Expression parseExpression(String... lines) {
- ParserInput input = ParserInput.fromLines(lines);
- return Expression.parse(input, events.reporter());
+ // Parses the expression, asserts that parsing fails,
+ // and returns the first error message.
+ private static String parseExpressionError(String src) {
+ ParserInput input = ParserInput.fromLines(src);
+ try {
+ Expression.parse(input);
+ throw new AssertionError("parseExpression(%s) succeeded unexpectedly: " + src);
+ } catch (SyntaxError ex) {
+ return ex.errors().get(0).getMessage();
+ }
}
// Joins the lines, parses, and returns a file.
+ // Errors are reported to this.events.
private StarlarkFile parseFile(String... lines) {
ParserInput input = ParserInput.fromLines(lines);
- return StarlarkFile.parse(input, events.reporter());
+ StarlarkFile file = StarlarkFile.parse(input);
+ Event.replayEventsOn(events.reporter(), file.errors());
+ return file;
}
// Joins the lines, parses, and returns the sole statement.
@@ -152,23 +163,14 @@
@Test
public void testNonAssociativeOperators() throws Exception {
- events.setFailFast(false);
-
- parseExpression("0 < 2 < 4");
- assertContainsError("Operator '<' is not associative with operator '<'");
- clearEvents();
-
- parseExpression("0 == 2 < 4");
- assertContainsError("Operator '==' is not associative with operator '<'");
- clearEvents();
-
- parseExpression("1 in [1, 2] == True");
- assertContainsError("Operator 'in' is not associative with operator '=='");
- clearEvents();
-
- parseExpression("1 >= 2 <= 3");
- assertContainsError("Operator '>=' is not associative with operator '<='");
- clearEvents();
+ assertThat(parseExpressionError("0 < 2 < 4"))
+ .contains("Operator '<' is not associative with operator '<'");
+ assertThat(parseExpressionError("0 == 2 < 4"))
+ .contains("Operator '==' is not associative with operator '<'");
+ assertThat(parseExpressionError("1 in [1, 2] == True"))
+ .contains("Operator 'in' is not associative with operator '=='");
+ assertThat(parseExpressionError("1 >= 2 <= 3"))
+ .contains("Operator '>=' is not associative with operator '<='");
}
@Test
@@ -340,7 +342,7 @@
assertLocation(0, 14, slice.getLocation());
}
- private void evalSlice(String statement, Object... expectedArgs) {
+ private static void evalSlice(String statement, Object... expectedArgs) throws SyntaxError {
SliceExpression e = (SliceExpression) parseExpression(statement);
// There is no way to evaluate the expression here, so we rely on string comparison.
@@ -357,8 +359,10 @@
public void testErrorRecovery() throws Exception {
setFailFast(false);
- String expr = "f(1, [x for foo foo foo foo], 3)";
- FuncallExpression e = (FuncallExpression) parseExpression(expr);
+ // We call parseFile, not parseExpression, as the latter is all-or-nothing.
+ String src = "f(1, [x for foo foo foo foo], 3)";
+ FuncallExpression e =
+ (FuncallExpression) ((ExpressionStatement) parseStatement(src)).getExpression();
assertContainsError("syntax error at 'foo'");
@@ -378,7 +382,7 @@
assertThat(arg1val.getName()).isEqualTo("$error$");
assertLocation(5, 29, arg1val.getLocation());
- assertThat(expr.substring(5, 28)).isEqualTo("[x for foo foo foo foo]");
+ assertThat(src.substring(5, 28)).isEqualTo("[x for foo foo foo foo]");
assertThat(arg1val.getLocation().getEndLineAndColumn().getColumn()).isEqualTo(29);
IntegerLiteral arg2 = (IntegerLiteral) e.getArguments().get(2).getValue();
@@ -387,23 +391,19 @@
@Test
public void testDoesntGetStuck() throws Exception {
- setFailFast(false);
-
// Make sure the parser does not get stuck when trying
// to parse an expression containing a syntax error.
// This usually results in OutOfMemoryError because the
// parser keeps filling up the error log.
// We need to make sure that we will always advance
// in the token stream.
- parseExpression("f(1, ], 3)");
- parseExpression("f(1, ), 3)");
- parseExpression("[ ) for v in 3)");
-
- assertContainsError(""); // "" matches any, i.e., there were some events
+ parseExpressionError("f(1, ], 3)");
+ parseExpressionError("f(1, ), 3)");
+ parseExpressionError("[ ) for v in 3)");
}
@Test
- public void testSecondaryLocation() {
+ public void testSecondaryLocation() throws SyntaxError {
String expr = "f(1 % 2)";
FuncallExpression call = (FuncallExpression) parseExpression(expr);
Argument.Passed arg = call.getArguments().get(0);
@@ -411,7 +411,7 @@
}
@Test
- public void testPrimaryLocation() {
+ public void testPrimaryLocation() throws SyntaxError {
String expr = "f(1 + 2)";
FuncallExpression call = (FuncallExpression) parseExpression(expr);
Argument.Passed arg = call.getArguments().get(0);
@@ -427,32 +427,25 @@
@Test
public void testAssignKeyword() {
- setFailFast(false);
- parseExpression("with = 4");
- assertContainsError("keyword 'with' not supported");
- assertContainsError("syntax error at 'with': expected expression");
+ assertThat(parseExpressionError("with = 4")).contains("keyword 'with' not supported");
}
@Test
public void testBreak() {
- setFailFast(false);
- parseExpression("break");
- assertContainsError("syntax error at 'break': expected expression");
+ assertThat(parseExpressionError("break"))
+ .contains("syntax error at 'break': expected expression");
}
@Test
public void testTry() {
- setFailFast(false);
- parseExpression("try: 1 + 1");
- assertContainsError("'try' not supported, all exceptions are fatal");
- assertContainsError("syntax error at 'try': expected expression");
+ assertThat(parseExpressionError("try: 1 + 1"))
+ .contains("'try' not supported, all exceptions are fatal");
}
@Test
public void testDel() {
- setFailFast(false);
- parseExpression("del d['a']");
- assertContainsError("'del' not supported, use '.pop()' to delete");
+ assertThat(parseExpressionError("del d['a']"))
+ .contains("'del' not supported, use '.pop()' to delete");
}
@Test
@@ -474,10 +467,7 @@
@Test
public void testInvalidAssign() {
- setFailFast(false);
- parseExpression("1 + (b = c)");
- assertContainsError("syntax error");
- clearEvents();
+ assertThat(parseExpressionError("1 + (b = c)")).contains("syntax error");
}
@Test
@@ -641,7 +631,7 @@
assertThat(getText(stmtStr, stmt)).isEqualTo(stmtStr);
}
- private void assertExpressionLocationCorrect(String exprStr) {
+ private static void assertExpressionLocationCorrect(String exprStr) throws SyntaxError {
Expression expr = parseExpression(exprStr);
assertThat(getText(exprStr, expr)).isEqualTo(exprStr);
// Also try it with another token at the end (newline), which broke the location in the past.
@@ -714,16 +704,9 @@
@Test
public void testTupleWithTrailingComma() throws Exception {
- setFailFast(false);
-
// Unlike Python, we require parens here.
- parseExpression("0, 1, 2, 3,");
- assertContainsError("Trailing comma");
- clearEvents();
-
- parseExpression("1 + 2,");
- assertContainsError("Trailing comma");
- clearEvents();
+ assertThat(parseExpressionError("0, 1, 2, 3,")).contains("Trailing comma");
+ assertThat(parseExpressionError("1 + 2,")).contains("Trailing comma");
ListExpression tuple = (ListExpression) parseExpression("(0, 1, 2, 3,)");
assertThat(tuple.isTuple()).isTrue();
@@ -825,31 +808,12 @@
@Test
public void testListComprehensionSyntax() throws Exception {
- setFailFast(false);
-
- parseExpression("[x for");
- assertContainsError("syntax error at 'newline'");
- clearEvents();
-
- parseExpression("[x for x");
- assertContainsError("syntax error at 'newline'");
- clearEvents();
-
- parseExpression("[x for x in");
- assertContainsError("syntax error at 'newline'");
- clearEvents();
-
- parseExpression("[x for x in []");
- assertContainsError("syntax error at 'newline'");
- clearEvents();
-
- parseExpression("[x for x for y in ['a']]");
- assertContainsError("syntax error at 'for'");
- clearEvents();
-
- parseExpression("[x for x for y in 1, 2]");
- assertContainsError("syntax error at 'for'");
- clearEvents();
+ assertThat(parseExpressionError("[x for")).contains("syntax error at 'newline'");
+ assertThat(parseExpressionError("[x for x")).contains("syntax error at 'newline'");
+ assertThat(parseExpressionError("[x for x in")).contains("syntax error at 'newline'");
+ assertThat(parseExpressionError("[x for x in []")).contains("syntax error at 'newline'");
+ assertThat(parseExpressionError("[x for x for y in ['a']]")).contains("syntax error at 'for'");
+ assertThat(parseExpressionError("[x for x for y in 1, 2]")).contains("syntax error at 'for'");
}
@Test
@@ -912,18 +876,6 @@
}
@Test
- public void testParserContainsErrorsIfSyntaxException() throws Exception {
- setFailFast(false);
- parseExpression("'foo' %%");
- assertContainsError("syntax error at '%'");
- }
-
- @Test
- public void testParserDoesNotContainErrorsIfSuccess() throws Exception {
- parseExpression("'foo'");
- }
-
- @Test
public void testParserContainsErrors() throws Exception {
setFailFast(false);
parseFile("*");
@@ -1398,4 +1350,10 @@
collectAllStringsInStringLiteralsVisitor.visit(file);
assertThat(uniqueStringInstances).containsExactly("cat", "dog", "fish");
}
+
+ @Test
+ public void testConditionalExpressions() throws Exception {
+ assertThat(parseExpressionError("1 if 2"))
+ .contains("missing else clause in conditional expression or semicolon before if");
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index 19c021a..5907fd1 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -46,8 +46,10 @@
import org.junit.runners.JUnit4;
/** Tests of Starlark evaluation. */
+// This test uses 'extends' to make a copy of EvaluationTest whose
+// mode is overridden to SKYLARK, changing various environmental parameters.
@RunWith(JUnit4.class)
-public class SkylarkEvaluationTest extends EvaluationTest {
+public final class SkylarkEvaluationTest extends EvaluationTest {
@Before
public final void setup() throws Exception {
@@ -974,15 +976,25 @@
flowStatementAfterLoop("continue");
}
+ // TODO(adonovan): move this and all tests that use it to Validation tests.
+ private void assertValidationError(String expectedError, final String... lines) throws Exception {
+ SyntaxError error = assertThrows(SyntaxError.class, () -> eval(lines));
+ assertThat(error).hasMessageThat().contains(expectedError);
+ }
+
private void flowStatementInsideFunction(String statement) throws Exception {
- checkEvalErrorContains(statement + " statement must be inside a for loop",
+ assertValidationError(
+ statement + " statement must be inside a for loop",
+ //
"def foo():",
" " + statement,
"x = foo()");
}
- private void flowStatementAfterLoop(String statement) throws Exception {
- checkEvalErrorContains(statement + " statement must be inside a for loop",
+ private void flowStatementAfterLoop(String statement) throws Exception {
+ assertValidationError(
+ statement + " statement must be inside a for loop",
+ //
"def foo2():",
" for i in range(0, 3):",
" pass",
@@ -991,6 +1003,11 @@
}
@Test
+ public void testStructMembersAreImmutable() throws Exception {
+ assertValidationError("cannot assign to 's.x'", "s = struct(x = 'a')", "s.x = 'b'\n");
+ }
+
+ @Test
public void testNoneAssignment() throws Exception {
new SkylarkTest()
.setUp("def foo(x=None):", " x = 1", " x = None", " return 2", "s = foo()")
@@ -1578,17 +1595,21 @@
@Test
public void testInvalidAugmentedAssignment_ListExpression() throws Exception {
- new SkylarkTest().testIfErrorContains(
+ assertValidationError(
"cannot perform augmented assignment on a list or tuple expression",
+ //
"def f(a, b):",
" [a, b] += []",
"f(1, 2)");
}
+
@Test
public void testInvalidAugmentedAssignment_NotAnLValue() throws Exception {
- newTest().testIfErrorContains(
- "cannot assign to 'x + 1'", "x + 1 += 2");
+ assertValidationError(
+ "cannot assign to 'x + 1'",
+ //
+ "x + 1 += 2");
}
@Test
@@ -1841,12 +1862,13 @@
}
@Test
+ // TODO(adonovan): move to Validation tests.
public void testTypo() throws Exception {
- new SkylarkTest()
- .testIfErrorContains(
- "name 'my_variable' is not defined (did you mean 'myVariable'?)",
- "myVariable = 2",
- "x = my_variable + 1");
+ assertValidationError(
+ "name 'my_variable' is not defined (did you mean 'myVariable'?)",
+ //
+ "myVariable = 2",
+ "x = my_variable + 1");
}
@Test
@@ -2223,6 +2245,7 @@
}
@Test
+ // TODO(adonovan): move to Validation tests.
public void testExperimentalFlagGuardedValue() throws Exception {
// This test uses an arbitrary experimental flag to verify this functionality. If this
// experimental flag were to go away, this test may be updated to use any experimental flag.
@@ -2234,6 +2257,7 @@
"GlobalSymbol is experimental and thus unavailable with the current "
+ "flags. It may be enabled by setting --experimental_build_setting_api";
+
new SkylarkTest(ImmutableMap.of("GlobalSymbol", val), "--experimental_build_setting_api=true")
.setUp("var = GlobalSymbol")
.testLookup("var", "foo");
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFileTest.java b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFileTest.java
index 24a7fa0..a590be9 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFileTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFileTest.java
@@ -15,49 +15,44 @@
import static com.google.common.truth.Truth.assertThat;
+import com.google.common.base.Joiner;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
-import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
import com.google.devtools.build.lib.testutil.MoreAsserts;
-import com.google.devtools.build.lib.testutil.Scratch;
-import com.google.devtools.build.lib.vfs.FileSystemUtils;
-import com.google.devtools.build.lib.vfs.Path;
-import java.io.IOException;
+import com.google.devtools.build.lib.vfs.PathFragment;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
/** Unit tests for StarlarkFile. */
+// TODO(adonovan): move tests of parser into ParserTest
+// and tests of evaluator into Starlark scripts.
@RunWith(JUnit4.class)
-public class StarlarkFileTest extends EvaluationTestCase {
+public class StarlarkFileTest {
- private Scratch scratch = new Scratch();
-
- @Override
- public StarlarkThread newStarlarkThread() throws Exception {
- return newBuildStarlarkThread();
+ private static StarlarkThread newThread() {
+ return StarlarkThread.builder(Mutability.create("test")).useDefaultSemantics().build();
}
/**
- * Parses the contents of the specified string (using DUMMY_PATH as the fake filename) and returns
- * the AST. Resets the error handler beforehand.
+ * Parses the contents of the specified string (using 'foo.star' as the apparent filename) and
+ * returns the AST. Resets the error handler beforehand.
*/
- private StarlarkFile parseBuildFile(String... lines) throws IOException {
- Path file = scratch.file("/a/build/file/BUILD", lines);
- byte[] bytes = FileSystemUtils.readWithKnownFileSize(file, file.getFileSize());
- ParserInput input = ParserInput.create(bytes, file.asFragment());
- return StarlarkFile.parse(input, getEventHandler());
+ private static StarlarkFile parseFile(String... lines) {
+ String src = Joiner.on("\n").join(lines);
+ ParserInput input = ParserInput.create(src, PathFragment.create("foo.star"));
+ return StarlarkFile.parse(input);
}
@Test
- public void testParseBuildFileOK() throws Exception {
- StarlarkFile buildfile =
- parseBuildFile(
+ public void testExecuteBuildFileOK() throws Exception {
+ StarlarkFile file =
+ parseFile(
"# a file in the build language",
"",
"x = [1,2,'foo',4] + [1,2, \"%s%d\" % ('foo', 1)]");
-
- assertThat(buildfile.exec(thread, getEventHandler())).isTrue();
+ StarlarkThread thread = newThread();
+ EvalUtils.exec(file, thread);
// Test final environment is correctly modified:
//
@@ -68,74 +63,48 @@
}
@Test
- public void testEvalException() throws Exception {
- setFailFast(false);
- StarlarkFile buildfile = parseBuildFile("x = 1", "y = [2,3]", "", "z = x + y");
+ public void testExecException() throws Exception {
+ StarlarkFile file = parseFile("x = 1", "y = [2,3]", "", "z = x + y");
- assertThat(buildfile.exec(thread, getEventHandler())).isFalse();
- Event e = assertContainsError("unsupported operand type(s) for +: 'int' and 'list'");
- assertThat(e.getLocation().getStartLineAndColumn().getLine()).isEqualTo(4);
+ StarlarkThread thread = newThread();
+ try {
+ EvalUtils.exec(file, thread);
+ throw new AssertionError("execution succeeded unexpectedly");
+ } catch (EvalException ex) {
+ assertThat(ex.getMessage()).contains("unsupported operand type(s) for +: 'int' and 'list'");
+ assertThat(ex.getLocation().getStartLineAndColumn().getLine()).isEqualTo(4);
+ }
}
@Test
public void testParsesFineWithNewlines() throws Exception {
- StarlarkFile buildFileAST = parseBuildFile("foo()", "bar()", "something = baz()", "bar()");
- assertThat(buildFileAST.getStatements()).hasSize(4);
+ StarlarkFile file = parseFile("foo()", "bar()", "something = baz()", "bar()");
+ assertThat(file.getStatements()).hasSize(4);
}
@Test
public void testFailsIfNewlinesAreMissing() throws Exception {
- setFailFast(false);
+ StarlarkFile file = parseFile("foo() bar() something = baz() bar()");
- StarlarkFile buildFileAST = parseBuildFile("foo() bar() something = baz() bar()");
-
- Event event = assertContainsError("syntax error at \'bar\': expected newline");
- assertThat(event.getLocation().getPath().toString()).isEqualTo("/a/build/file/BUILD");
- assertThat(event.getLocation().getStartLineAndColumn().getLine()).isEqualTo(1);
- assertThat(buildFileAST.containsErrors()).isTrue();
+ Event event =
+ MoreAsserts.assertContainsEvent(file.errors(), "syntax error at \'bar\': expected newline");
+ assertThat(event.getLocation().toString()).isEqualTo("foo.star:1:7");
}
@Test
public void testImplicitStringConcatenationFails() throws Exception {
- setFailFast(false);
- StarlarkFile buildFileAST = parseBuildFile("a = 'foo' 'bar'");
- Event event = assertContainsError(
- "Implicit string concatenation is forbidden, use the + operator");
- assertThat(event.getLocation().getPath().toString()).isEqualTo("/a/build/file/BUILD");
- assertThat(event.getLocation().getStartLineAndColumn().getLine()).isEqualTo(1);
- assertThat(event.getLocation().getStartLineAndColumn().getColumn()).isEqualTo(10);
- assertThat(buildFileAST.containsErrors()).isTrue();
+ StarlarkFile file = parseFile("a = 'foo' 'bar'");
+ Event event =
+ MoreAsserts.assertContainsEvent(
+ file.errors(), "Implicit string concatenation is forbidden, use the + operator");
+ assertThat(event.getLocation().toString()).isEqualTo("foo.star:1:10");
}
@Test
public void testImplicitStringConcatenationAcrossLinesIsIllegal() throws Exception {
- setFailFast(false);
- StarlarkFile buildFileAST = parseBuildFile("a = 'foo'\n 'bar'");
+ StarlarkFile file = parseFile("a = 'foo'\n 'bar'");
- Event event = assertContainsError("indentation error");
- assertThat(event.getLocation().getPath().toString()).isEqualTo("/a/build/file/BUILD");
- assertThat(event.getLocation().getStartLineAndColumn().getLine()).isEqualTo(2);
- assertThat(event.getLocation().getStartLineAndColumn().getColumn()).isEqualTo(2);
- assertThat(buildFileAST.containsErrors()).isTrue();
- }
-
- @Test
- public void testWithSyntaxErrorsDoesNotPrintDollarError() throws Exception {
- setFailFast(false);
- StarlarkFile buildFile =
- parseBuildFile(
- "abi = '$(ABI)-glibc-' + glibc_version + '-' + $(TARGET_CPU) + '-linux'",
- "libs = [abi + opt_level + '/lib/libcc.a']",
- "shlibs = [abi + opt_level + '/lib/libcc.so']",
- "+* shlibs", // syntax error at '+'
- "cc_library(name = 'cc',",
- " srcs = libs,",
- " includes = [ abi + opt_level + '/include' ])");
- assertThat(buildFile.containsErrors()).isTrue();
- assertContainsError("syntax error at '*': expected expression");
- assertThat(buildFile.exec(thread, getEventHandler())).isFalse();
- MoreAsserts.assertDoesNotContainEvent(getEventCollector(), "$error$");
- // This message should not be printed anymore.
- MoreAsserts.assertDoesNotContainEvent(getEventCollector(), "contains syntax error(s)");
+ Event event = MoreAsserts.assertContainsEvent(file.errors(), "indentation error");
+ assertThat(event.getLocation().toString()).isEqualTo("foo.star:2:2");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadDebuggingTest.java b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadDebuggingTest.java
index 6d5aece..ffb9f37 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadDebuggingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadDebuggingTest.java
@@ -191,7 +191,7 @@
StarlarkThread thread = newStarlarkThread();
thread.update("a", 1);
- Object result = thread.debugEval(ParserInput.create("a", null));
+ Object result = thread.debugEval(Expression.parse(ParserInput.create("a", null)));
assertThat(result).isEqualTo(1);
}
@@ -202,7 +202,9 @@
thread.update("a", 1);
EvalException e =
- assertThrows(EvalException.class, () -> thread.debugEval(ParserInput.create("b", null)));
+ assertThrows(
+ EvalException.class,
+ () -> thread.debugEval(Expression.parse(ParserInput.create("b", null))));
assertThat(e).hasMessageThat().isEqualTo("name 'b' is not defined");
}
@@ -211,13 +213,9 @@
StarlarkThread thread = newStarlarkThread();
thread.update("a", "string");
- Object result =
- thread.debugEval(ParserInput.create("a.startswith('str')", PathFragment.EMPTY_FRAGMENT));
- assertThat(result).isEqualTo(Boolean.TRUE);
-
- thread.debugExec(ParserInput.create("a = 1", PathFragment.EMPTY_FRAGMENT));
-
- result = thread.debugEval(ParserInput.create("a", PathFragment.EMPTY_FRAGMENT));
- assertThat(result).isEqualTo(1);
+ assertThat(thread.debugEval(Expression.parse(ParserInput.fromLines("a.startswith('str')"))))
+ .isEqualTo(true);
+ thread.debugExec(ParserInput.fromLines("a = 1"));
+ assertThat(thread.debugEval(Expression.parse(ParserInput.fromLines("a")))).isEqualTo(1);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadTest.java b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadTest.java
index a3746ee..f12b59a 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadTest.java
@@ -63,7 +63,7 @@
@Test
public void testReference() throws Exception {
setFailFast(false);
- EvalException e = assertThrows(EvalException.class, () -> eval("foo"));
+ SyntaxError e = assertThrows(SyntaxError.class, () -> eval("foo"));
assertThat(e).hasMessageThat().isEqualTo("name 'foo' is not defined");
update("foo", "bar");
assertThat(eval("foo")).isEqualTo("bar");
@@ -72,8 +72,7 @@
// Test assign and reference through interpreter:
@Test
public void testAssignAndReference() throws Exception {
- setFailFast(false);
- EvalException e = assertThrows(EvalException.class, () -> eval("foo"));
+ SyntaxError e = assertThrows(SyntaxError.class, () -> eval("foo"));
assertThat(e).hasMessageThat().isEqualTo("name 'foo' is not defined");
eval("foo = 'bar'");
assertThat(eval("foo")).isEqualTo("bar");
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
index 35425cc..3836db8 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
@@ -37,6 +37,7 @@
import com.google.devtools.build.lib.syntax.StarlarkThread;
import com.google.devtools.build.lib.syntax.StarlarkThread.FailFastException;
import com.google.devtools.build.lib.syntax.Statement;
+import com.google.devtools.build.lib.syntax.SyntaxError;
import com.google.devtools.build.lib.syntax.ValidationEnvironment;
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.testutil.TestMode;
@@ -152,12 +153,17 @@
protected final StarlarkFile parseStarlarkFileWithoutValidation(String... lines) {
ParserInput input = ParserInput.fromLines(lines);
- return StarlarkFile.parse(input, getEventHandler());
+ StarlarkFile file = StarlarkFile.parse(input);
+ Event.replayEventsOn(getEventHandler(), file.errors());
+ return file;
}
private StarlarkFile parseStarlarkFile(String... lines) {
- StarlarkFile ast = parseStarlarkFileWithoutValidation(lines);
- return ast.validate(thread, /*isBuildFile=*/ false, getEventHandler());
+ ParserInput input = ParserInput.fromLines(lines);
+ StarlarkFile file = StarlarkFile.parse(input);
+ ValidationEnvironment.validateFile(file, thread, /*isBuildFile=*/ false);
+ Event.replayEventsOn(getEventHandler(), file.errors());
+ return file;
}
/** Parses and validates a file and returns its statements. */
@@ -174,8 +180,8 @@
}
/** Parses an expression. */
- protected final Expression parseExpression(String... lines) {
- return Expression.parse(ParserInput.fromLines(lines), getEventHandler());
+ protected final Expression parseExpression(String... lines) throws SyntaxError {
+ return Expression.parse(ParserInput.fromLines(lines));
}
public EvaluationTestCase update(String varname, Object value) throws Exception {
@@ -187,26 +193,34 @@
return thread.moduleLookup(varname);
}
+ // TODO(adonovan): this function does far too much:
+ // - two modes, BUILD vs Skylark
+ // - parse + validate + BUILD dialect checks + execute.
+ // Break the tests down into tests of just the scanner, parser, validator, build dialect checks,
+ // or execution, and assert that all passes except the one of interest succeed.
+ // All BUILD-dialect stuff belongs in bazel proper (lib.packages), not here.
public Object eval(String... lines) throws Exception {
ParserInput input = ParserInput.fromLines(lines);
- if (testMode == TestMode.SKYLARK) {
- // TODO(adonovan): inline this call and factor with 'else' case.
- return StarlarkFile.eval(input, thread);
- } else {
- StarlarkFile file = StarlarkFile.parse(input, thread.getEventHandler());
- if (ValidationEnvironment.validateFile(
- file, thread, /*isBuildFile=*/ true, thread.getEventHandler())) {
- PackageFactory.checkBuildSyntax(file, thread.getEventHandler());
+ StarlarkFile file = StarlarkFile.parse(input);
+ ValidationEnvironment.validateFile(file, thread, testMode == TestMode.BUILD);
+ if (testMode == TestMode.SKYLARK) { // .bzl and other dialects
+ if (!file.ok()) {
+ throw new SyntaxError(file.errors());
}
- return file.eval(thread);
+ } else {
+ // For BUILD mode, validation events are reported but don't (yet)
+ // prevent execution. We also apply BUILD dialect syntax checks.
+ Event.replayEventsOn(getEventHandler(), file.errors());
+ PackageFactory.checkBuildSyntax(file, getEventHandler());
}
+ return file.eval(thread);
}
public void checkEvalError(String msg, String... input) throws Exception {
try {
eval(input);
fail("Expected error '" + msg + "' but got no error");
- } catch (EvalException | FailFastException e) {
+ } catch (SyntaxError | EvalException | FailFastException e) {
assertThat(e).hasMessageThat().isEqualTo(msg);
}
}
@@ -215,7 +229,7 @@
try {
eval(input);
fail("Expected error containing '" + msg + "' but got no error");
- } catch (EvalException | FailFastException e) {
+ } catch (SyntaxError | EvalException | FailFastException e) {
assertThat(e).hasMessageThat().contains(msg);
}
}
@@ -223,7 +237,7 @@
public void checkEvalErrorDoesNotContain(String msg, String... input) throws Exception {
try {
eval(input);
- } catch (EvalException | FailFastException e) {
+ } catch (SyntaxError | EvalException | FailFastException e) {
assertThat(e).hasMessageThat().doesNotContain(msg);
}
}
