From f15bc640eece703d7e98ff10fb160d8fe6829b1a Mon Sep 17 00:00:00 2001 From: adonovan Date: Wed, 6 Jan 2021 09:13:24 -0800 Subject: [PATCH] bazel packages: use EventHandler not EvalException in .bzl "export" operation 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 --- .../starlark/StarlarkRuleClassFunctions.java | 101 ++++++++++-------- .../starlark/StarlarkRepositoryModule.java | 3 +- .../android/AndroidSdkRepositoryFunction.java | 4 +- .../lib/packages/StarlarkDefinedAspect.java | 3 +- .../lib/packages/StarlarkExportable.java | 8 +- .../build/lib/packages/StarlarkProvider.java | 3 +- .../build/lib/skyframe/BzlLoadFunction.java | 6 +- .../net/starlark/java/eval/EvalException.java | 5 +- .../lib/packages/RequiredProvidersTest.java | 2 +- .../StarlarkRuleClassFunctionsTest.java | 3 +- 10 files changed, 72 insertions(+), 66 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index fe5bd9d0833777..3a5ab4355c2ce3 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/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 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/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java index 8f9210560165a7..0e2baf450c09f4 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java +++ b/src/main/java/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/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java index a3b67c2bd699e0..661fff2837053d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java +++ b/src/main/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 index 6c2efaba5c5fa2..3991894e154c05 100644 --- 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 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 index 0898726641ba4c..6c3baa0d23f774 100644 --- 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 index 7f0d0c735a2a61..520054bc527b7d 100644 --- 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 index e5f0016bf387d9..8e4f65b0824269 100644 --- 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 index f25255e72e6ff1..468c7289674b72 100644 --- 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 index 45d4db6476d298..267873a415a948 100644 --- 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 index 96a3b80b0e0526..49e0d0e14d4659 100644 --- 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