bazel analysis: don't use EvalException for non-Starlark errors
StarlarkDefinedConfigTransition.evaluate and FunctionTransitionUtils.applyAndValidate
now report errors through the event handler, instead of
throwing EvalException. They use the new ValidationException class
for internal plumbing of non-Starlark errors.
This is another step towards eliminating the EvalException(Location)
constructor.
Also:
- inline SDCT.evalFunction
- tweak error messages
PiperOrigin-RevId: 350775434
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
index 598d754..ca009b0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -1767,7 +1767,9 @@
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/config",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/syntax",
+ "//third_party:error_prone_annotations",
"//third_party:guava",
+ "//third_party:jsr305",
],
)
@@ -2094,7 +2096,10 @@
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/common/options",
"//src/main/java/net/starlark/java/eval",
+ "//src/main/java/net/starlark/java/syntax",
+ "//third_party:error_prone_annotations",
"//third_party:guava",
+ "//third_party:jsr305",
],
)
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java
index caf5a40..031f046 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java
@@ -23,9 +23,11 @@
import com.google.devtools.build.lib.packages.BazelStarlarkContext;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigurationTransitionApi;
+import com.google.errorprone.annotations.FormatMethod;
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import javax.annotation.Nullable;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Mutability;
@@ -109,13 +111,14 @@
* child configuration (split transitions will have multiple elements in this map with keys
* provided by the transition impl, patch transitions should have a single element keyed by
* {@code PATCH_TRANSITION_KEY}). Each build setting map is a map from build setting to target
- * setting value; all other build settings will remain unchanged
- * @throws EvalException if there is an error evaluating the transition
+ * setting value; all other build settings will remain unchanged. Returns null if errors were
+ * reported to the handler.
* @throws InterruptedException if evaluating the transition is interrupted
*/
+ @Nullable
public abstract ImmutableMap<String, Map<String, Object>> evaluate(
Map<String, Object> previousSettings, StructImpl attributeMap, EventHandler eventHandler)
- throws EvalException, InterruptedException;
+ throws InterruptedException;
public static StarlarkDefinedConfigTransition newRegularTransition(
StarlarkCallable impl,
@@ -218,18 +221,23 @@
* @param attributeMapper a map of attributes
*/
// TODO(bazel-team): integrate dict-of-dicts return type with ctx.split_attr
+ @Nullable
@Override
public ImmutableMap<String, Map<String, Object>> evaluate(
- Map<String, Object> previousSettings, StructImpl attributeMapper, EventHandler eventHandler)
- throws EvalException, InterruptedException {
+ Map<String, Object> previousSettings, StructImpl attributeMapper, EventHandler handler)
+ throws InterruptedException {
+ // Call the Starlark function.
Object result;
- try {
+ try (Mutability mu = Mutability.create("eval_transition_function")) {
+ StarlarkThread thread = new StarlarkThread(mu, semantics);
+ thread.setPrintHandler(Event.makeDebugPrintHandler(handler));
+ starlarkContext.storeInThread(thread);
result =
- evalFunction(impl, ImmutableList.of(previousSettings, attributeMapper), eventHandler);
- } catch (EvalException e) {
- // TODO(adonovan): this doesn't look right. Consider interposing a call to a delegating
- // wrapper just to establish a fake frame for impl, then remove the catch.
- throw new EvalException(impl.getLocation(), e.getMessageWithStack());
+ Starlark.fastcall(
+ thread, impl, new Object[] {previousSettings, attributeMapper}, new Object[0]);
+ } catch (EvalException ex) {
+ handler.handle(Event.error(null, ex.getMessageWithStack()));
+ return null;
}
if (result instanceof Dict) {
@@ -260,9 +268,12 @@
return ImmutableMap.of(
PATCH_TRANSITION_KEY,
Dict.cast(result, String.class, Object.class, "dictionary of options"));
- } catch (EvalException e) {
- throw new EvalException(impl.getLocation(), e.getMessage());
+ } catch (EvalException ex) {
+ // TODO(adonovan): explain "want dict<string, any> or dict<string, dict<string, any>>".
+ errorf(handler, "invalid result from transition function: %s", ex.getMessage());
+ return null;
}
+
} else if (result instanceof Sequence) {
ImmutableMap.Builder<String, Map<String, Object>> builder = ImmutableMap.builder();
try {
@@ -274,34 +285,32 @@
Integer.toString(i++),
Dict.cast(toOptions, String.class, Object.class, "dictionary of options"));
}
- } catch (EvalException e) {
- throw new EvalException(impl.getLocation(), e.getMessage());
+ } catch (EvalException ex) {
+ // TODO(adonovan): explain "want sequence of dict<string, any>".
+ errorf(handler, "invalid result from transition function: %s", ex.getMessage());
+ return null;
}
return builder.build();
+
} else {
- throw new EvalException(
- impl.getLocation(),
- "Transition function must return a dictionary or list of dictionaries.");
+ errorf(
+ handler,
+ "transition function returned %s, want dict or list of dicts",
+ Starlark.type(result));
+ return null;
}
}
+ @FormatMethod
+ private void errorf(EventHandler handler, String format, Object... args) {
+ handler.handle(Event.error(impl.getLocation(), String.format(format, args)));
+ }
+
@Override
public void repr(Printer printer) {
printer.append("<transition object>");
}
- /** Evaluate the input function with the given argument, and return the return value. */
- private Object evalFunction(
- StarlarkCallable function, ImmutableList<Object> args, EventHandler eventHandler)
- throws InterruptedException, EvalException {
- try (Mutability mu = Mutability.create("eval_transition_function")) {
- StarlarkThread thread = new StarlarkThread(mu, semantics);
- thread.setPrintHandler(Event.makeDebugPrintHandler(eventHandler));
- starlarkContext.storeInThread(thread);
- return Starlark.call(thread, function, args, /*kwargs=*/ ImmutableMap.of());
- }
- }
-
@Override
public boolean equals(Object object) {
if (object == this) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
index f68f9ac..75c24db 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
@@ -27,12 +27,14 @@
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
+import com.google.errorprone.annotations.FormatMethod;
import java.lang.reflect.Field;
import java.util.Collection;
import java.util.HashSet;
@@ -42,6 +44,7 @@
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
+import javax.annotation.Nullable;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Mutability;
@@ -68,34 +71,57 @@
* @param buildOptions the pre-transition build options
* @param starlarkTransition the transition to apply
* @param attrObject the attributes of the rule to which this transition is attached
- * @return the post-transition build options.
+ * @return the post-transition build options, or null if errors were reported to handler.
*/
+ @Nullable
static Map<String, BuildOptions> applyAndValidate(
BuildOptions buildOptions,
StarlarkDefinedConfigTransition starlarkTransition,
StructImpl attrObject,
- EventHandler eventHandler)
- throws EvalException, InterruptedException {
- checkForDenylistedOptions(starlarkTransition);
+ EventHandler handler)
+ throws InterruptedException {
+ try {
+ checkForDenylistedOptions(starlarkTransition);
- // TODO(waltl): consider building this once and use it across different split
- // transitions.
- Map<String, OptionInfo> optionInfoMap = buildOptionInfo(buildOptions);
- Dict<String, Object> settings = buildSettings(buildOptions, optionInfoMap, starlarkTransition);
+ // TODO(waltl): consider building this once and use it across different split
+ // transitions.
+ Map<String, OptionInfo> optionInfoMap = buildOptionInfo(buildOptions);
+ Dict<String, Object> settings =
+ buildSettings(buildOptions, optionInfoMap, starlarkTransition);
- ImmutableMap.Builder<String, BuildOptions> splitBuildOptions = ImmutableMap.builder();
+ ImmutableMap.Builder<String, BuildOptions> splitBuildOptions = ImmutableMap.builder();
- ImmutableMap<String, Map<String, Object>> transitions =
- starlarkTransition.evaluate(settings, attrObject, eventHandler);
- validateFunctionOutputsMatchesDeclaredOutputs(transitions.values(), starlarkTransition);
+ ImmutableMap<String, Map<String, Object>> transitions =
+ starlarkTransition.evaluate(settings, attrObject, handler);
+ if (transitions == null) {
+ return null; // errors reported to handler
+ }
+ validateFunctionOutputsMatchesDeclaredOutputs(transitions.values(), starlarkTransition);
- for (Map.Entry<String, Map<String, Object>> entry : transitions.entrySet()) {
- Map<String, Object> newValues = handleImplicitPlatformChange(entry.getValue());
- BuildOptions transitionedOptions =
- applyTransition(buildOptions, newValues, optionInfoMap, starlarkTransition);
- splitBuildOptions.put(entry.getKey(), transitionedOptions);
+ for (Map.Entry<String, Map<String, Object>> entry : transitions.entrySet()) {
+ Map<String, Object> newValues = handleImplicitPlatformChange(entry.getValue());
+ BuildOptions transitionedOptions =
+ applyTransition(buildOptions, newValues, optionInfoMap, starlarkTransition);
+ splitBuildOptions.put(entry.getKey(), transitionedOptions);
+ }
+ return splitBuildOptions.build();
+
+ } catch (ValidationException ex) {
+ handler.handle(
+ Event.error(starlarkTransition.getLocationForErrorReporting(), ex.getMessage()));
+ return null;
}
- return splitBuildOptions.build();
+ }
+
+ private static final class ValidationException extends Exception {
+ ValidationException(String message) {
+ super(message);
+ }
+
+ @FormatMethod
+ static ValidationException format(String format, Object... args) {
+ return new ValidationException(String.format(format, args));
+ }
}
/**
@@ -110,7 +136,7 @@
* <li>Result: the mapping accidentally overrides the transition
* </ol>
*
- * <p>Transitions can alo explicitly set --platforms to be clear what platform they set.
+ * <p>Transitions can also explicitly set --platforms to be clear what platform they set.
*
* <p>Platform mappings:
* https://docs.bazel.build/versions/master/platforms-intro.html#platform-mappings.
@@ -132,9 +158,9 @@
}
private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition transition)
- throws EvalException {
+ throws ValidationException {
if (transition.getOutputs().contains("//command_line_option:define")) {
- throw Starlark.errorf(
+ throw new ValidationException(
"Starlark transition on --define not supported - try using build settings"
+ " (https://docs.bazel.build/skylark/config.html#user-defined-build-settings).");
}
@@ -148,18 +174,19 @@
private static void validateFunctionOutputsMatchesDeclaredOutputs(
Collection<Map<String, Object>> transitions,
StarlarkDefinedConfigTransition starlarkTransition)
- throws EvalException {
+ throws ValidationException {
for (Map<String, Object> transition : transitions) {
LinkedHashSet<String> remainingOutputs =
Sets.newLinkedHashSet(starlarkTransition.getOutputs());
for (String outputKey : transition.keySet()) {
if (!remainingOutputs.remove(outputKey)) {
- throw Starlark.errorf("transition function returned undeclared output '%s'", outputKey);
+ throw ValidationException.format(
+ "transition function returned undeclared output '%s'", outputKey);
}
}
if (!remainingOutputs.isEmpty()) {
- throw Starlark.errorf(
+ throw ValidationException.format(
"transition outputs [%s] were not defined by transition function",
Joiner.on(", ").join(remainingOutputs));
}
@@ -195,14 +222,14 @@
* @throws RuntimeException If the field corresponding to an option value in buildOptions is
* inaccessible due to Java language access control, or if an option name is an invalid key to
* the Starlark dictionary
- * @throws EvalException if any of the specified transition inputs do not correspond to a valid
- * build setting
+ * @throws ValidationException if any of the specified transition inputs do not correspond to a
+ * valid build setting
*/
static Dict<String, Object> buildSettings(
BuildOptions buildOptions,
Map<String, OptionInfo> optionInfoMap,
StarlarkDefinedConfigTransition starlarkTransition)
- throws EvalException {
+ throws ValidationException {
LinkedHashSet<String> remainingInputs = Sets.newLinkedHashSet(starlarkTransition.getInputs());
try (Mutability mutability = Mutability.create("build_settings")) {
@@ -219,15 +246,16 @@
}
OptionInfo optionInfo = entry.getValue();
+ Field field = optionInfo.getDefinition().getField();
+ FragmentOptions options = buildOptions.get(optionInfo.getOptionClass());
try {
- Field field = optionInfo.getDefinition().getField();
- FragmentOptions options = buildOptions.get(optionInfo.getOptionClass());
Object optionValue = field.get(options);
-
dict.putEntry(optionKey, optionValue == null ? Starlark.NONE : optionValue);
} catch (IllegalAccessException e) {
// These exceptions should not happen, but if they do, throw a RuntimeException.
throw new RuntimeException(e);
+ } catch (EvalException ex) {
+ throw new IllegalStateException(ex); // can't happen
}
}
@@ -236,11 +264,15 @@
if (!remainingInputs.remove(starlarkOption.getKey().toString())) {
continue;
}
- dict.putEntry(starlarkOption.getKey().toString(), starlarkOption.getValue());
+ try {
+ dict.putEntry(starlarkOption.getKey().toString(), starlarkOption.getValue());
+ } catch (EvalException ex) {
+ throw new IllegalStateException(ex); // can't happen
+ }
}
if (!remainingInputs.isEmpty()) {
- throw Starlark.errorf(
+ throw ValidationException.format(
"transition inputs [%s] do not correspond to valid settings",
Joiner.on(", ").join(remainingInputs));
}
@@ -261,14 +293,14 @@
* @param starlarkTransition transition object that is being applied. Used for error reporting and
* checking for analysis testing
* @return the post-transition build options
- * @throws EvalException If a requested option field is inaccessible
+ * @throws ValidationException If a requested option field is inaccessible
*/
private static BuildOptions applyTransition(
BuildOptions buildOptionsToTransition,
Map<String, Object> newValues,
Map<String, OptionInfo> optionInfoMap,
StarlarkDefinedConfigTransition starlarkTransition)
- throws EvalException {
+ throws ValidationException {
BuildOptions buildOptions = buildOptionsToTransition.clone();
// The names and values of options that are different after this transition.
Set<String> convertedNewValues = new HashSet<>();
@@ -301,7 +333,7 @@
}
try {
if (!optionInfoMap.containsKey(optionName)) {
- throw Starlark.errorf(
+ throw ValidationException.format(
"transition output '%s' does not correspond to a valid setting", entry.getKey());
}
@@ -340,7 +372,7 @@
} else if (optionValue instanceof String) {
convertedValue = def.getConverter().convert((String) optionValue);
} else {
- throw Starlark.errorf("Invalid value type for option '%s'", optionName);
+ throw ValidationException.format("Invalid value type for option '%s'", optionName);
}
Object oldValue = field.get(options);
@@ -352,13 +384,13 @@
}
} catch (IllegalArgumentException e) {
- throw Starlark.errorf(
+ throw ValidationException.format(
"IllegalArgumentError for option '%s': %s", optionName, e.getMessage());
} catch (IllegalAccessException e) {
throw new RuntimeException(
"IllegalAccess for option " + optionName + ": " + e.getMessage());
} catch (OptionsParsingException e) {
- throw Starlark.errorf(
+ throw ValidationException.format(
"OptionsParsingError for option '%s': %s", optionName, e.getMessage());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java
index b086aca..d207a26 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributeTransitionProvider.java
@@ -25,7 +25,6 @@
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
-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.AttributeMap;
@@ -36,7 +35,6 @@
import com.google.devtools.build.lib.starlarkbuildapi.SplitTransitionProviderApi;
import java.util.LinkedHashMap;
import java.util.Map;
-import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Printer;
/**
@@ -109,16 +107,12 @@
// outputs. Rather than complicate BuildOptionsView with more access points to BuildOptions,
// we just use the original BuildOptions and trust the transition's enforcement logic.
BuildOptions buildOptions = buildOptionsView.underlying();
- try {
- return applyAndValidate(
- buildOptions, starlarkDefinedConfigTransition, attrObject, eventHandler);
- } catch (EvalException e) {
- eventHandler.handle(
- Event.error(
- starlarkDefinedConfigTransition.getLocationForErrorReporting(),
- e.getMessageWithStack()));
+ Map<String, BuildOptions> res =
+ applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject, eventHandler);
+ if (res == null) {
return ImmutableMap.of("error", buildOptions.clone());
}
+ return res;
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java
index a0f4de2..259ede2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleTransitionProvider.java
@@ -34,7 +34,6 @@
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Objects;
-import net.starlark.java.eval.EvalException;
/**
* This class implements {@link TransitionFactory} to provide a starlark-defined transition that
@@ -108,20 +107,13 @@
@Override
public BuildOptions patch(BuildOptionsView buildOptionsView, EventHandler eventHandler)
throws InterruptedException {
- Map<String, BuildOptions> result;
// Starlark transitions already have logic to enforce they only access declared inputs and
// outputs. Rather than complicate BuildOptionsView with more access points to BuildOptions,
// we just use the original BuildOptions and trust the transition's enforcement logic.
BuildOptions buildOptions = buildOptionsView.underlying();
- try {
- result =
- applyAndValidate(
- buildOptions, starlarkDefinedConfigTransition, attrObject, eventHandler);
- } catch (EvalException e) {
- eventHandler.handle(
- Event.error(
- starlarkDefinedConfigTransition.getLocationForErrorReporting(),
- e.getMessageWithStack()));
+ Map<String, BuildOptions> result =
+ applyAndValidate(buildOptions, starlarkDefinedConfigTransition, attrObject, eventHandler);
+ if (result == null) {
return buildOptions.clone();
}
if (result.size() != 1) {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
index bb66659..d4ddb73 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
@@ -133,7 +133,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test");
- assertContainsEvent("Transition function must return a dictionary or list of dictionaries.");
+ assertContainsEvent("transition function returned string, want dict or list of dicts");
}
@Test
