From f0890f0d659f6f2afb94d60d5d762c8bcbbbd122 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 1 Oct 2019 07:28:48 -0700 Subject: [PATCH] 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 --- .../build/lib/packages/PackageFactory.java | 47 ++-- .../build/lib/packages/WorkspaceFactory.java | 23 +- .../repository/ResolvedFileFunction.java | 63 +++-- .../lib/skyframe/ASTFileLookupFunction.java | 7 +- .../skyframe/SkylarkImportLookupFunction.java | 20 +- .../lib/skyframe/WorkspaceASTFunction.java | 40 +-- .../skylarkdebug/server/ThreadHandler.java | 19 +- .../devtools/build/lib/syntax/Eval.java | 59 +--- .../build/lib/syntax/EvalException.java | 28 +- .../devtools/build/lib/syntax/EvalUtils.java | 14 + .../devtools/build/lib/syntax/Expression.java | 13 +- .../build/lib/syntax/FlagGuardedValue.java | 32 +-- .../devtools/build/lib/syntax/Identifier.java | 3 +- .../devtools/build/lib/syntax/Lexer.java | 51 +--- .../devtools/build/lib/syntax/Parser.java | 75 ++---- .../build/lib/syntax/StarlarkFile.java | 251 +++++++----------- .../build/lib/syntax/StarlarkThread.java | 38 +-- .../build/lib/syntax/SyntaxError.java | 67 +++++ .../lib/syntax/ValidationEnvironment.java | 148 ++++++----- .../devtools/build/skydoc/SkydocMain.java | 4 +- .../skylark/SkylarkRepositoryContextTest.java | 2 +- .../lib/packages/PackageFactoryTest.java | 5 +- .../lib/packages/SkylarkProviderTest.java | 11 +- .../util/PackageFactoryApparatus.java | 4 +- .../pkgcache/TargetPatternEvaluatorTest.java | 1 - .../SkylarkRuleClassFunctionsTest.java | 12 +- .../server/SkylarkDebugServerTest.java | 13 +- .../build/lib/syntax/ASTPrettyPrintTest.java | 17 +- .../build/lib/syntax/EvaluationTest.java | 11 +- .../lib/syntax/LValueBoundNamesTest.java | 6 +- .../devtools/build/lib/syntax/LexerTest.java | 43 +-- .../build/lib/syntax/NodeVisitorTest.java | 11 +- .../devtools/build/lib/syntax/ParserTest.java | 162 +++++------ .../lib/syntax/SkylarkEvaluationTest.java | 48 +++- .../build/lib/syntax/StarlarkFileTest.java | 117 +++----- .../syntax/StarlarkThreadDebuggingTest.java | 18 +- .../build/lib/syntax/StarlarkThreadTest.java | 5 +- .../lib/syntax/util/EvaluationTestCase.java | 48 ++-- 38 files changed, 701 insertions(+), 835 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/syntax/SyntaxError.java 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 b4ea17ab184f9c..2be21acc4d05ab 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 @@ public static StarlarkFile parseBuildFile( 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 @@ public Package.Builder evaluateBuildFile( pkgBuilder.setContainsErrors(); } - if (file.containsErrors()) { - pkgBuilder.setContainsErrors(); - } - pkgBuilder.setThirdPartyLicenceExistencePolicy( ruleClassProvider.getThirdPartyLicenseExistencePolicy()); @@ -1799,19 +1795,36 @@ public Package.Builder evaluateBuildFile( } } - 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 dc59b49eb502fd..99c7d154fed356 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 @@ void parseForTesting( 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 @@ private void execute( } } - 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 ec711e88aa52e2..86d738ab2241a6 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 @@ public SkyValue compute(SkyKey skyKey, Environment env) 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 @@ public SkyValue compute(SkyKey skyKey, Environment env) .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> result = new ImmutableList.Builder>(); 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 entryBuilder = new ImmutableMap.Builder(); for (Map.Entry keyValue : ((Map) 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 @@ public SkyValue compute(SkyKey skyKey, Environment env) } } + 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 5cb106c715d923..a21e2e1f04de93 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 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionExcept /*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 b467f95c143bac..caace6667d62ab 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 @@ private SkylarkImportLookupValue computeInternal( throw new SkylarkImportFailedException(astLookupValue.getErrorMsg()); } StarlarkFile file = astLookupValue.getAST(); - if (file.containsErrors()) { + if (!file.ok()) { throw SkylarkImportFailedException.skylarkErrors(filePath); } @@ -615,19 +616,18 @@ private Extension createExtension( } } + // 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 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 e33ade14f4a789..96d3c4c3a8297f 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 @@ public SkyValue compute(SkyKey skyKey, Environment env) 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 @@ public SkyValue compute(SkyKey skyKey, Environment env) 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 8ba96a8c431a8c..31ae67712cd501 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 @@ SkylarkDebuggingProtos.Value evaluate(long threadId, String statement) 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 @@ SkylarkDebuggingProtos.Value evaluate(long threadId, String statement) * running. */ private Object doEvaluate(StarlarkThread thread, String content) - throws EvalException, InterruptedException { + throws SyntaxError, EvalException, InterruptedException { try { servicingEvalRequest.set(true); @@ -309,12 +308,10 @@ private Object doEvaluate(StarlarkThread thread, String content) ParserInput input = ParserInput.create(content, PathFragment.create("")); // 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 @@ private boolean hasBreakpointMatchedAtLocation(StarlarkThread thread, Location l } 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 63d5dd25beea2e..8c66c9804e2ceb 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 @@ private static Object doEval(StarlarkThread thread, Expression expr) // 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 @@ private static Object doEval(StarlarkThread thread, Expression expr) 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 @@ private static Object doEval(StarlarkThread thread, Expression expr) 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 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 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 0223684be3fb24..1c741b68b933c2 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 @@ public class EvalException extends Exception { @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 class EvalException extends Exception { 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 @@ public EvalException(Location location, String message, Throwable cause) { LoggingUtil.logToRemote(Level.SEVERE, details, cause); throw new IllegalArgumentException(details); } - this.dueToIncompleteAST = false; } public EvalException(Location location, Throwable cause) { @@ -94,9 +79,9 @@ public EvalException(Location location, Throwable cause) { * 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)); } /** @@ -139,13 +124,6 @@ public Location getLocation() { return location; } - /** - * 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. 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 8c99b36f53b426..76296a1391173d 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 @@ public static Object index(Object object, Object key, StarlarkThread thread, Loc "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 06bc54c16707d0..3ff3996a94fbff 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 final void prettyPrint(Appendable buffer, int indentLevel) throws IOExcep 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 d79e901ac4f1dc..62bbb39031a97d 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 @@ public static FlagGuardedValue onlyWhenIncompatibleFlagIsFalse( } /** - * 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 65c924c2482c30..2344873a33fb8f 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 @@ public final class Identifier extends Expression { // 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 fc35657e440a38..0c9fa87ecbe09a 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 @@ final class Lexer { .put('|', TokenKind.PIPE_EQUALS) .build(); - private final EventHandler eventHandler; - // Input buffer and position private final char[] buffer; private int pos; @@ -97,7 +93,9 @@ private static class LocationInfo { // 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 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 static class LocationInfo { */ private final List 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 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 @@ public Lexer(ParserInput input, EventHandler eventHandler, LineNumberTable lineN indentStack.push(0); } - public Lexer(ParserInput input, EventHandler eventHandler) { - this(input, eventHandler, LineNumberTable.create(input.getContent(), input.getPath())); - } - List getComments() { return comments; } @@ -147,24 +139,15 @@ List getStringEscapeEvents() { * 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) { } 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) { @@ -952,17 +934,6 @@ private void tokenize() { setToken(TokenKind.EOF, pos, pos); } - /** - * 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 offsets = locationInfo.lineNumberTable.getOffsetsForLine(line); - return bufferSlice(offsets.first, offsets.second); - } - /** * Returns parts of the source buffer based on offsets * 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 0b4c7671a5a520..9eeb6ecb7f3e9e 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 statements; @@ -59,23 +51,23 @@ public static final class ParseResult { /** 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 errors; final List stringEscapeEvents; ParseResult( List statements, List comments, Location location, - boolean containsErrors, + List errors, List 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 @@ public static final class ParseResult { private static final boolean DEBUGGING = false; private final Lexer lexer; - private final EventHandler eventHandler; + private final List errors; // TODO(adonovan): opt: compute this by subtraction. private static final Map augmentedAssignmentMethods = @@ -167,9 +159,9 @@ public static final class ParseResult { // Intern string literals, as some files contain many literals for the same string. private final Map stringInterner = new HashMap<>(); - private Parser(Lexer lexer, EventHandler eventHandler) { + private Parser(Lexer lexer, List errors) { this.lexer = lexer; - this.eventHandler = eventHandler; + this.errors = errors; nextToken(); } @@ -189,16 +181,16 @@ private static Location locationFromStatements(Lexer lexer, List stat } // 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 errors = new ArrayList<>(); + Lexer lexer = new Lexer(input, errors); + Parser parser = new Parser(lexer, errors); List 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 @@ static ParseResult parseFile(ParserInput input, EventHandler eventHandler) { lexer.getStringEscapeEvents()); } - /** Parses a sequence of statements, possibly followed by newline tokens. */ - static List parseStatements(ParserInput input, EventHandler eventHandler) { - Lexer lexer = new Lexer(input, eventHandler); - Parser parser = new Parser(lexer, eventHandler); - List 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 stmts = parseStatements(input, eventHandler); - return Iterables.getOnlyElement(stmts); - } - // stmt ::= simple_stmt // | def_stmt // | for_stmt @@ -251,15 +219,18 @@ private void parseStatement(List list) { } /** 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 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 @@ private void reportError(Location location, String message) { 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 f8f429d454563e..f383b369af33ac 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. + * + *

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 statements; - private final ImmutableList comments; - - /** - * Whether any errors were encountered during scanning or parsing. - */ - private final boolean containsErrors; - + final List errors; // appended to by ValidationEnvironment private final List stringEscapeEvents; - @Nullable private final String contentHashCode; private StarlarkFile( ImmutableList statements, - boolean containsErrors, + List errors, String contentHashCode, Location location, ImmutableList comments, List 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 @@ private static StarlarkFile create( ImmutableList statements = statementsbuilder.build(); return new StarlarkFile( statements, - result.containsErrors, + result.errors, contentHashCode, result.location, ImmutableList.copyOf(result.comments), @@ -91,7 +90,7 @@ public StarlarkFile subTree(int firstStatement, int lastStatement) { ImmutableList statements = this.statements.subList(firstStatement, lastStatement); return new StarlarkFile( statements, - containsErrors, + errors, null, this.statements.get(firstStatement).getLocation(), ImmutableList.of(), @@ -99,94 +98,38 @@ public StarlarkFile subTree(int firstStatement, int lastStatement) { } /** - * 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 errors() { + return Collections.unmodifiableList(errors); } - /** - * Returns an (immutable, ordered) list of statements in this BUILD file. - */ - public ImmutableList getStatements() { - return statements; + /** Returns errors().isEmpty(). */ + public boolean ok() { + return errors.isEmpty(); } /** - * Returns an (immutable, ordered) list of comments 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. + * + *

Naturally this function should be called at most once. */ - public ImmutableList getComments() { - return comments; + void addStringEscapeEvents() { + errors.addAll(stringEscapeEvents); } - /** 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. - * - *

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. - * - *

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; + /** Returns an (immutable, ordered) list of statements in this BUILD file. */ + public ImmutableList getStatements() { + return statements; } - /** - * Executes a top-level statement of this Starlark file in the given StarlarkThread. - * - *

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. - * - *

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; - } + /** Returns an (immutable, ordered) list of comments in this BUILD file. */ + public ImmutableList getComments() { + return comments; } @Override @@ -209,31 +152,30 @@ public void accept(NodeVisitor visitor) { /** * 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 preludeStatements, EventHandler eventHandler) { - Parser.ParseResult result = Parser.parseFile(input, eventHandler); + ParserInput input, List 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 preludeStatements, EventHandler eventHandler) { - Parser.ParseResult result = Parser.parseFile(input, eventHandler); + ParserInput input, List 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 @@ public static StarlarkFile parseWithDigest( /* allowImportInternal= */ false); } - public static StarlarkFile parse(ParserInput input, EventHandler eventHandler) { - Parser.ParseResult result = Parser.parseFile(input, eventHandler); + /** + * Parse a Starlark file. + * + *

A syntax tree is always returned, even in case of error. Errors are recorded in the tree. + * Example usage: + * + * 

+   * StarlarkFile file = StarlarkFile.parse(input);
+   * if (!file.ok()) {
+   *    Event.replayEventsOn(handler, file.errors());
+   *    ...
+   * }
+   *
+ */ + public static StarlarkFile parse(ParserInput input) { + Parser.ParseResult result = Parser.parseFile(input); return create( - /* preludeStatements= */ ImmutableList.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())); - } - } - if (containsErrors) { - return this; // already marked as errant + EvalUtils.exec(this, thread); + return true; + } catch (EvalException ex) { + eventHandler.handle(Event.error(ex.getLocation(), ex.getMessage())); + return false; } - return new StarlarkFile( - statements, - /*containsErrors=*/ true, - contentHashCode, - getLocation(), - comments, - stringEscapeEvents); } /** @@ -318,27 +247,31 @@ public Object eval(StarlarkThread thread) throws EvalException, InterruptedExcep /** * 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. + * + *

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 037943b91d6074..b9d3a1dd8120a2 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 @@ public Set getVariableNames() { return vars; } - private static final class EvalEventHandler implements EventHandler { - List 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 @@ private static StarlarkThread.GlobalFrame createDefaultGlobals() { /** 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 @@ public FailFastException(String s) { * 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 00000000000000..112fcce8c6caba --- /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. + * + *

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. + * + *

Clients that fail abruptly when encountering parse errors are encouraged to use SyntaxError, + * as in this example: + * + * 

+ * StarlarkFile file = StarlarkFile.parse(input);
+ * if (!file.ok()) {
+ *     throw new SyntaxError(file.errors());
+ * }
+ *
+ */ +public final class SyntaxError extends Exception { + + private final ImmutableList errors; + + /** Construct a SyntaxError from a non-empty list of errors. */ + public SyntaxError(List errors) { + if (errors.isEmpty()) { + throw new IllegalArgumentException("no errors"); + } + this.errors = ImmutableList.copyOf(errors); + } + + /** Returns an immutable non-empty list of errors. */ + public ImmutableList 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 31e15c81fcf682..426454f1661fb0 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 @@ *

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). + * + *

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 @@ private static class Block { } } - /** - * 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 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 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 @@ private ValidationEnvironment(StarlarkThread thread, boolean isBuildFile) { 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 @@ private void collectDefinitions(Statement stmt) { Set 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 @@ private void assign(Expression lhs) { assign(elem); } } else { - throw new ValidationException(lhs.getLocation(), "cannot assign to '" + lhs + "'"); + addError(lhs.getLocation(), "cannot assign to '" + lhs + "'"); } } @@ -191,12 +192,12 @@ public void visit(Identifier node) { // 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 @@ public void visit(Identifier node) { } } + // This is exposed to Eval until validation becomes a precondition for evaluation. + static String createInvalidIdentifierException(String name, Set 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 @@ public void visit(ReturnStatement node) { @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 @@ public void visit(ForStatement node) { @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 @@ public void visit(LoadStatement node) { @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 @@ public void visit(Comprehension node) { @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 @@ public void visit(DefStatement node) { @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 @@ public void visit(AssignmentStatement node) { @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 @@ private void declare(String varname, Location location) { // 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 @@ private Set getAllSymbols() { return all; } - /** Throws ValidationException if a load() appears after another kind of statement. */ - private static void checkLoadAfterStatement(List statements) { + // Report an error if a load statement appears after another kind of statement. + private void checkLoadAfterStatement(List statements) { Location firstStatement = null; for (Statement statement : statements) { @@ -382,7 +410,7 @@ private static void checkLoadAfterStatement(List statements) { 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 @@ private void validateToplevelStatements(List statements) { 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; - } - } - - // 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; + /** + * 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 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(); } + 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 a9608120c999d0..18c4ce66f64436 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 @@ private StarlarkThread recursiveEval( 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 eb6cbe57337bca..c052eb95788bac 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 @@ protected void setUpContextForRule( "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 806442f8621da8..dc913219701adb 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 @@ public void testGlobPatternExtractor() { " 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 0c81e9bda936c2..5272098fb3f6b2 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 @@ public void providerEquals() throws Exception { /** 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 9ce4643ad78f0b..e1b2bdb4bf40bf 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 Package createPackage( 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 4ca676406ba3ea..4ab2bd7ff3db3e 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 @@ public void testKeepGoingOnAllRulesBeneath() throws Exception { Pair, 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 7baff2219bc3b0..002297c032c645 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 @@ public void testRuleAddAttribute() throws Exception { 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()); } @@ -1254,14 +1258,6 @@ public void testStructsInDicts() throws Exception { "{struct(a = []): 'foo'}"); } - @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( 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 a6144667b2dbd4..4a4e84665b2e0b 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 @@ private static StarlarkThread newStarlarkThread() { 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 12ca90a69e60b5..18e5fbee46a8d0 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 @@ private void assertTostringMatches(Node node, String expected) { * 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 @@ private void assertExprPrettyMatches(String source, String expected) { * 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 41b0ff0f5f2780..00592ae6d27e7d 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 @@ public void testConditionalExpressions() throws Exception { .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 0b651fca81a995..df33118e7fc525 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 @@ public void complexListAssignment2() { 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 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 74bba9ba72b052..412aa59125db2f 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 errors = new ArrayList<>(); private String lastError; private Location lastErrorLocation; @@ -43,19 +44,10 @@ public class LexerTest { 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 allTokens(Lexer lexer) { @@ -65,6 +57,17 @@ private ArrayList allTokens(Lexer lexer) { 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 testLineNumbers() throws Exception { 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 6be69bf96c56be..93b03a87c684cb 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 idents = new ArrayList<>(); final List 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 45c8655d514fb2..04d41da3aee0d6 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 @@ private void setFailFast(boolean failFast) { events.setFailFast(failFast); } - private void clearEvents() { - events.clear(); - } - // Joins the lines, parse, and returns an expression. - private Expression parseExpression(String... lines) { + private static Expression parseExpression(String... lines) throws SyntaxError { ParserInput input = ParserInput.fromLines(lines); - return Expression.parse(input, events.reporter()); + return Expression.parse(input); + } + + // 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 @@ public void testPrecedence5() throws Exception { @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 @@ public void testSlice() throws Exception { 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 @@ private void evalSlice(String statement, Object... expectedArgs) { 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 @@ public void testErrorRecovery() throws Exception { 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 @@ public void testErrorRecovery() throws Exception { @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 @@ public void testSecondaryLocation() { } @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 @@ public void testAssignLocation() { @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 @@ public void testAssign() { @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 @@ private void assertStatementLocationCorrect(String stmtStr) { 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 @@ public void testTupleWithoutParens() throws Exception { @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 @@ public void testListExpressions9() throws Exception { @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 @@ -911,18 +875,6 @@ public void testParserRecovery() throws Exception { assertThat(statements).hasSize(3); } - @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); @@ -1398,4 +1350,10 @@ public void visit(StringLiteral stringLiteral) { 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 19c021a7c4f0f1..5907fd1a94fbde 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 @@ public void testForLoopContinueError() throws Exception { 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", @@ -990,6 +1002,11 @@ private void flowStatementAfterLoop(String statement) throws Exception { "y = foo2()"); } + @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() @@ -1578,17 +1595,21 @@ public void testAugmentedAssignmentHasNoSideEffects() throws Exception { @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 @@ public void testFunctionCallRecursion() throws Exception { } @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 @@ public void testAnalysisFailureInfo() throws Exception { } @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 @@ public void testExperimentalFlagGuardedValue() throws Exception { "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 24a7fa0bd41254..a590be976ab802 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 @@ public void testParseBuildFileOK() throws Exception { } @Test - public void testEvalException() throws Exception { - setFailFast(false); - StarlarkFile buildfile = parseBuildFile("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); + public void testExecException() throws Exception { + StarlarkFile file = parseFile("x = 1", "y = [2,3]", "", "z = x + y"); + + 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 buildFileAST = parseBuildFile("foo() bar() something = baz() bar()"); + StarlarkFile file = parseFile("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 6d5aece1d20f5a..ffb9f37be9dcee 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 @@ public void testEvaluateVariableInScope() throws Exception { 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 @@ public void testEvaluateVariableNotInScopeFails() throws Exception { 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 @@ public void testEvaluateExpressionOnVariableInScope() throws Exception { 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 a3746ee0abc741..f12b59ad524de6 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 @@ public void testAssign() throws Exception { @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 @@ public void testReference() throws Exception { // 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 35425cc97dc84f..3836db8d56fe21 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 @@ public StarlarkThread getStarlarkThread() { 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 @@ protected final Statement parseStatement(String... lines) { } /** 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 @@ public Object lookup(String varname) throws Exception { 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 @@ public void checkEvalErrorContains(String msg, String... input) throws Exception 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 checkEvalErrorContains(String msg, String... input) throws Exception 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); } }