bazel packages: use EventHandler not EvalException in .bzl "export" o…

…peration This allows us to delete one of the deprecated EvalException(Location,...) constructors. Similar follow-up changes (events not exceptions) will be required to eliminate the remaining such constructor. As a bonus, this approach allows multiple errors to be reported at once. PiperOrigin-RevId: 350365352
ptarjan · Jan 6, 2021 · f15bc64 · f15bc64
1 parent 82552dd
commit f15bc64
Show file tree

Hide file tree

Showing 10 changed files with 72 additions and 66 deletions.
diff --git a/...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
@@ -48,6 +48,8 @@
 import com.google.devtools.build.lib.cmdline.LabelValidator;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate;
 import com.google.devtools.build.lib.packages.AttributeMap;
@@ -85,6 +87,7 @@
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkRuleFunctionsApi;
 import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.Pair;
+import com.google.errorprone.annotations.FormatMethod;
 import java.util.Collection;
 import java.util.Map;
 import java.util.concurrent.ExecutionException;
@@ -322,8 +325,7 @@ public StarlarkCallable rule(
     }
 
     if (executable || test) {
-      addAttribute(
-          builder,
+      builder.addAttribute(
           attr("$is_executable", BOOLEAN)
               .value(true)
               .nonconfigurable("Called from RunCommand.isExecutable, which takes a Target")
@@ -466,15 +468,6 @@ private static void checkAttributeName(String name) throws EvalException {
     return attributes.build();
   }
 
-  private static void addAttribute(RuleClass.Builder builder, Attribute attribute)
-      throws EvalException {
-    try {
-      builder.addAttribute(attribute);
-    } catch (IllegalStateException ex) {
-      throw new EvalException(ex);
-    }
-  }
-
   /**
    * Parses a sequence of label strings with a repo mapping.
    *
@@ -716,15 +709,17 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> kwarg
     }
 
     /** Export a RuleFunction from a Starlark file with a given name. */
-    public void export(Label starlarkLabel, String ruleClassName) throws EvalException {
+    @Override
+    public void export(EventHandler handler, Label starlarkLabel, String ruleClassName) {
       Preconditions.checkState(ruleClass == null && builder != null);
       this.starlarkLabel = starlarkLabel;
       if (type == RuleClassType.TEST != TargetUtils.isTestRuleName(ruleClassName)) {
-        throw new EvalException(
-            definitionLocation,
-            "Invalid rule class name '"
-                + ruleClassName
-                + "', test rule class names must end with '_test' and other rule classes must not");
+        errorf(
+            handler,
+            "Invalid rule class name '%s', test rule class names must end with '_test' and other"
+                + " rule classes must not",
+            ruleClassName);
+        return;
       }
       // Thus far, we only know if we have a rule transition. While iterating through attributes,
       // check if we have an attribute transition.
@@ -739,10 +734,11 @@ public void export(Label starlarkLabel, String ruleClassName) throws EvalExcepti
         hasStarlarkDefinedTransition |= attr.hasStarlarkDefinedTransition();
         if (attr.hasAnalysisTestTransition()) {
           if (!builder.isAnalysisTest()) {
-            throw new EvalException(
-                definitionLocation,
-                "Only rule definitions with analysis_test=True may have attributes with "
-                    + "analysis_test_transition transitions");
+            errorf(
+                handler,
+                "Only rule definitions with analysis_test=True may have attributes with"
+                    + " analysis_test_transition transitions");
+            continue;
           }
           builder.setHasAnalysisTestTransition();
         }
@@ -751,14 +747,12 @@ public void export(Label starlarkLabel, String ruleClassName) throws EvalExcepti
         if (name.equals(FunctionSplitTransitionAllowlist.ATTRIBUTE_NAME)
             || name.equals(FunctionSplitTransitionAllowlist.LEGACY_ATTRIBUTE_NAME)) {
           if (!BuildType.isLabelType(attr.getType())) {
-            throw new EvalException(
-                definitionLocation,
-                "_allowlist_function_transition attribute must be a label type");
+            errorf(handler, "_allowlist_function_transition attribute must be a label type");
+            continue;
           }
           if (attr.getDefaultValueUnchecked() == null) {
-            throw new EvalException(
-                definitionLocation,
-                "_allowlist_function_transition attribute must have a default value");
+            errorf(handler, "_allowlist_function_transition attribute must have a default value");
+            continue;
           }
           Label defaultLabel = (Label) attr.getDefaultValueUnchecked();
           // Check the label value for package and target name, to make sure this works properly
@@ -775,36 +769,44 @@ public void export(Label starlarkLabel, String ruleClassName) throws EvalExcepti
                   && defaultLabel
                       .getName()
                       .equals(FunctionSplitTransitionAllowlist.LEGACY_LABEL.getName()))) {
-            throw new EvalException(
-                definitionLocation,
-                "_allowlist_function_transition attribute ("
-                    + defaultLabel
-                    + ") does not have the expected value "
-                    + FunctionSplitTransitionAllowlist.LABEL);
+            errorf(
+                handler,
+                "_allowlist_function_transition attribute (%s) does not have the expected value %s",
+                defaultLabel,
+                FunctionSplitTransitionAllowlist.LABEL);
+            continue;
           }
           hasFunctionTransitionAllowlist = true;
           builder.setHasFunctionTransitionAllowlist();
         }
-        addAttribute(builder, attr);
+
+        try {
+          builder.addAttribute(attr);
+        } catch (IllegalStateException ex) {
+          // TODO(bazel-team): stop using unchecked exceptions in this way.
+          errorf(handler, "cannot add attribute: %s", ex.getMessage());
+        }
       }
       // TODO(b/121385274): remove when we stop allowlisting starlark transitions
       if (hasStarlarkDefinedTransition) {
         if (!hasFunctionTransitionAllowlist) {
-          throw new EvalException(
-              definitionLocation,
-              String.format(
-                  "Use of Starlark transition without allowlist attribute"
-                      + " '_allowlist_function_transition'. See Starlark transitions documentation"
-                      + " for details and usage: %s %s",
-                  builder.getRuleDefinitionEnvironmentLabel(), builder.getType()));
+          errorf(
+              handler,
+              "Use of Starlark transition without allowlist attribute"
+                  + " '_allowlist_function_transition'. See Starlark transitions documentation"
+                  + " for details and usage: %s %s",
+              builder.getRuleDefinitionEnvironmentLabel(),
+              builder.getType());
+          return;
         }
       } else {
         if (hasFunctionTransitionAllowlist) {
-          throw new EvalException(
-              definitionLocation,
-              String.format(
-                  "Unused function-based split transition allowlist: %s %s",
-                  builder.getRuleDefinitionEnvironmentLabel(), builder.getType()));
+          errorf(
+              handler,
+              "Unused function-based split transition allowlist: %s %s",
+              builder.getRuleDefinitionEnvironmentLabel(),
+              builder.getType());
+          return;
         }
       }
 
@@ -813,13 +815,18 @@ public void export(Label starlarkLabel, String ruleClassName) throws EvalExcepti
       } catch (IllegalArgumentException | IllegalStateException ex) {
         // TODO(adonovan): this catch statement is an abuse of exceptions. Be more specific.
         String msg = ex.getMessage();
-        throw new EvalException(definitionLocation, msg != null ? msg : ex.toString(), ex);
+        errorf(handler, "%s", msg != null ? msg : ex.toString());
       }
 
       this.builder = null;
       this.attributes = null;
     }
 
+    @FormatMethod
+    private void errorf(EventHandler handler, String format, Object... args) {
+      handler.handle(Event.error(definitionLocation, String.format(format, args)));
+    }
+
     public RuleClass getRuleClass() {
       Preconditions.checkState(ruleClass != null && builder == null);
       return ruleClass;

diff --git a/...ava/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java b/...ava/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java
@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.analysis.starlark.StarlarkAttrModule.Descriptor;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.AttributeValueSource;
 import com.google.devtools.build.lib.packages.BazelModuleContext;
 import com.google.devtools.build.lib.packages.BazelStarlarkContext;
@@ -141,7 +142,7 @@ public boolean isImmutable() {
     }
 
     @Override
-    public void export(Label extensionLabel, String exportedName) {
+    public void export(EventHandler handler, Label extensionLabel, String exportedName) {
       this.extensionLabel = extensionLabel;
       this.exportedName = exportedName;
     }

diff --git a/.../java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java b/.../java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java
@@ -443,8 +443,8 @@ private static void assertValidBuildToolsVersion(Rule rule, String buildToolsVer
       throw new EvalException(
           rule.getLocation(),
           String.format(
-              "Bazel does not recognize Android build tools version %s", buildToolsVersion),
-          e);
+              "Bazel does not recognize Android build tools version %s: %s",
+              buildToolsVersion, e.getMessage()));
     }
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java
@@ -20,6 +20,7 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
 import java.util.Arrays;
@@ -149,7 +150,7 @@ public ImmutableSet<String> getParamAttributes() {
   }
 
   @Override
-  public void export(Label extensionLabel, String name) {
+  public void export(EventHandler handler, Label extensionLabel, String name) {
     Preconditions.checkArgument(!isExported());
     this.aspectClass = new StarlarkAspectClass(extensionLabel, name);
   }

diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkExportable.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkExportable.java
@@ -15,7 +15,7 @@
 package com.google.devtools.build.lib.packages;
 
 import com.google.devtools.build.lib.cmdline.Label;
-import net.starlark.java.eval.EvalException;
+import com.google.devtools.build.lib.events.EventHandler;
 import net.starlark.java.eval.StarlarkValue;
 
 /**
@@ -30,8 +30,8 @@ public interface StarlarkExportable extends StarlarkValue {
   boolean isExported();
 
   /**
-   * Notify the value that it is exported from {@code extensionLabel}
-   * extension with name {@code exportedName}.
+   * Notify the value that it is exported from {@code extensionLabel} extension with name {@code
+   * exportedName}.
    */
-  void export(Label extensionLabel, String exportedName) throws EvalException;
+  void export(EventHandler handler, Label extensionLabel, String exportedName);
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkProvider.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkProvider.java
@@ -17,6 +17,7 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.util.Fingerprint;
 import java.util.Collection;
@@ -173,7 +174,7 @@ public String getErrorMessageForUnknownField(String name) {
   }
 
   @Override
-  public void export(Label extensionLabel, String exportedName) {
+  public void export(EventHandler handler, Label extensionLabel, String exportedName) {
     Preconditions.checkState(!isExported());
     this.key = new Key(extensionLabel, exportedName);
   }

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -1031,11 +1031,7 @@ public static void execAndExport(
           if (value instanceof StarlarkExportable) {
             StarlarkExportable exp = (StarlarkExportable) value;
             if (!exp.isExported()) {
-              try {
-                exp.export(label, name);
-              } catch (EvalException ex) {
-                handler.handle(Event.error(null, ex.getMessageWithStack()));
-              }
+              exp.export(handler, label, name);
             }
           }
         });

diff --git a/src/main/java/net/starlark/java/eval/EvalException.java b/src/main/java/net/starlark/java/eval/EvalException.java
@@ -74,7 +74,7 @@ private static String getCauseMessage(Throwable cause) {
     return msg != null ? msg : cause.toString();
   }
 
-  // TODO(adonovan): delete all constructors below. Stop using Location.
+  // TODO(adonovan): delete constructors below. Stop using Location.
 
   /**
    * Constructs an EvalException with a message and optional location (deprecated).
@@ -96,8 +96,7 @@ public EvalException(@Nullable Location location, String message) {
    * message, so callers should incorporate {@code cause.getMessage()} into {@code message} if
    * desired.
    */
-  // TODO(adonovan): eliminate.
-  public EvalException(@Nullable Location location, String message, @Nullable Throwable cause) {
+  private EvalException(@Nullable Location location, String message, @Nullable Throwable cause) {
     super(Preconditions.checkNotNull(message), cause);
     this.location = location;
   }

diff --git a/src/test/java/com/google/devtools/build/lib/packages/RequiredProvidersTest.java b/src/test/java/com/google/devtools/build/lib/packages/RequiredProvidersTest.java
@@ -45,7 +45,7 @@ private static final class P3 implements TransitiveInfoProvider {}
 
   static {
     try {
-      P_STARLARK.export(Label.create("foo/bar", "x.bzl"), "p_starlark");
+      P_STARLARK.export(ev -> {}, Label.create("foo/bar", "x.bzl"), "p_starlark");
     } catch (LabelSyntaxException e) {
       throw new AssertionError(e);
     }

diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
@@ -264,8 +264,9 @@ public void testRuleClassTooLongAttributeName() throws Exception {
     assertThat(ev.getEventCollector()).hasSize(1);
     Event event = ev.getEventCollector().iterator().next();
     assertThat(event.getKind()).isEqualTo(EventKind.ERROR);
+    assertThat(event.getLocation().toString()).isEqualTo(":2:9");
     assertThat(event.getMessage())
-        .matches(":2:9: Attribute r\\.x{150}'s name is too long \\(150 > 128\\)");
+        .matches("Attribute r\\.x{150}'s name is too long \\(150 > 128\\)");
   }
 
   @Test