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

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();
       }
     }
 

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

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<Map<String, Object>> result
             = new ImmutableList.Builder<Map<String, Object>>();
         for (Object entry : (List) resolved) {
           if (!(entry instanceof Map)) {
-            throw new ResolvedFileFunctionException(
-                new BuildFileContainsErrorsException(
-                    LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
-                    "Symbol 'resolved' in resolved file "
-                        + key.getPath()
-                        + " contains a non-map entry"));
+            throw resolvedValueError(
+                "Symbol 'resolved' in resolved file "
+                    + key.getPath()
+                    + " contains a non-map entry");
           }
           ImmutableMap.Builder<String, Object> entryBuilder
               = new ImmutableMap.Builder<String, Object>();
           for (Map.Entry<Object, Object> keyValue : ((Map<Object, Object>) entry).entrySet()) {
             Object attribute = keyValue.getKey();
             if (!(attribute instanceof String)) {
-              throw new ResolvedFileFunctionException(
-                  new BuildFileContainsErrorsException(
-                      LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
-                      "Symbol 'resolved' in resolved file "
-                          + key.getPath()
-                          + " contains a non-string key in one of its entries"));
+              throw resolvedValueError(
+                  "Symbol 'resolved' in resolved file "
+                      + key.getPath()
+                      + " contains a non-string key in one of its entries");
             }
             entryBuilder.put((String) attribute, keyValue.getValue());
           }
@@ -133,6 +125,11 @@ 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) {

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

@@ -18,13 +18,15 @@
 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;
 import com.google.devtools.build.lib.syntax.ParserInput;
 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),

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<Statement> statements = ast.getStatements();
-    for (Statement statement : statements) {
-      if (!ast.execTopLevelStatement(statement, extensionThread, eventHandler)) {
+    for (Statement stmt : file.getStatements()) {
+      try {
+        EvalUtils.execToplevelStatement(stmt, thread);
+      } catch (EvalException ex) {
+        handler.handle(Event.error(ex.getLocation(), ex.getMessage()));
         break;
       }
-      possiblyExport(statement, extensionLabel, eventHandler, extensionThread);
+      possiblyExport(stmt, extensionLabel, handler, thread);
     }
   }
 

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,50 +84,37 @@ 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 =
           StarlarkFile.parseWithPrelude(
               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) {