Stop Blaze crashes due to transition inputs of values Starlark can't …

…read

Have Starlark.java return a new UnreadableInStarlarkException that can be caught separately as-needed.

Defer construction of the Dict from FunctionTransitionUtil to StarlarkDefinedConfigTransition where the proper mutability is available. Then, can just convert them all and error gracefully if a requested input is not available.

NOTE: There is a slight performance 'adjustment'.
Performance loss: All of the values are converted regardless of if the Starlark implementation actually reads them (versus before where they are converted as they are read). However, the list has already been curated to those that the Starlark impl specifically asked for so this should be fine in practice.
Performance gain: The new implementation avoids converting again if the same entry is asked for twice.
PiperOrigin-RevId: 463515966
Change-Id: Iffeb06ac1f05e4b8c37392d11751d7ef8924521e

src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java

@@ -311,6 +311,45 @@ public Boolean isForAnalysisTesting() {
       return false;
     }
 
+    /** An exception for validating that a transition is properly constructed */
+    private static final class UnreadableInputSettingException extends Exception {
+      public UnreadableInputSettingException(String unreadableSetting, Class<?> unreadableClass) {
+        super(
+            String.format(
+                "Input build setting %s is of type %s, which is unreadable in Starlark."
+                    + " Please submit a feature request.",
+                unreadableSetting, unreadableClass));
+      }
+    }
+
+    /**
+     * Copy settings into Starlark-readable Dict.
+     *
+     * <p>The returned (outer) Dict will be immutable but all the underlying entries will have
+     * mutability given by the entryMu param.
+     *
+     * @param settings map os settings to copy over
+     * @param entryMu Mutability context to use when copying individual entries
+     * @throws UnreadableInputSettingException when entry in build setting is not convertable (using
+     *     {@link Starlark#fromJava})
+     */
+    private Dict<String, Object> createBuildSettingsDict(
+        Map<String, Object> settings, Mutability entryMu) throws UnreadableInputSettingException {
+
+      // Need to convert contained values into Starlark readable values.
+      Dict.Builder<String, Object> builder = Dict.builder();
+      for (Map.Entry<String, Object> entry : settings.entrySet()) {
+        try {
+          builder.put(entry.getKey(), Starlark.fromJava(entry.getValue(), entryMu));
+        } catch (Starlark.InvalidStarlarkValueException e) {
+          throw new UnreadableInputSettingException(entry.getKey(), e.getInvalidClass());
+        }
+      }
+
+      // Want the 'outer' build settings dictionary to be immutable
+      return builder.buildImmutable();
+    }
+
     /**
      * This method evaluates the implementation function of the transition.
      *
@@ -349,6 +388,8 @@ public ImmutableMap<String, Map<String, Object>> evaluate(
         //  are used as inputs to the configuration.
         SymbolGenerator<Object> dummySymbolGenerator = new SymbolGenerator<>(new Object());
 
+        Dict<String, Object> previousSettingsDict = createBuildSettingsDict(previousSettings, mu);
+
         // Create a new {@link BazelStarlarkContext} for the new thread. We need to
         // create a new context every time because {@link BazelStarlarkContext}s
         // should be confined to a single thread.
@@ -364,7 +405,16 @@ public ImmutableMap<String, Map<String, Object>> evaluate(
         starlarkContext.storeInThread(thread);
         result =
             Starlark.fastcall(
-                thread, impl, new Object[] {previousSettings, attributeMapper}, new Object[0]);
+                thread, impl, new Object[] {previousSettingsDict, attributeMapper}, new Object[0]);
+      } catch (UnreadableInputSettingException ex) {
+        // TODO(blaze-configurability-team): Ideally, the error would happen (and thus location)
+        //   at the transition() call during loading phase. Instead, error happens at the impl
+        //  function call during the analysis phase.
+        handler.handle(
+            Event.error(
+                impl.getLocation(),
+                String.format("before calling %s: %s", impl.getName(), ex.getMessage())));
+        return null;
       } catch (EvalException ex) {
         handler.handle(Event.error(null, ex.getMessageWithStack()));
         return null;

src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java

@@ -50,9 +50,6 @@
 import java.util.TreeSet;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
-import net.starlark.java.eval.Dict;
-import net.starlark.java.eval.EvalException;
-import net.starlark.java.eval.Mutability;
 import net.starlark.java.eval.NoneType;
 import net.starlark.java.eval.Starlark;
 
@@ -87,7 +84,7 @@ static ImmutableMap<String, BuildOptions> applyAndValidate(
       // TODO(waltl): Consider building this once and using it across different split transitions,
       // or reusing BuildOptionDetails.
       ImmutableMap<String, OptionInfo> optionInfoMap = OptionInfo.buildMapFrom(buildOptions);
-      Dict<String, Object> settings =
+      ImmutableMap<String, Object> settings =
           buildSettings(buildOptions, optionInfoMap, starlarkTransition);
 
       ImmutableMap.Builder<String, BuildOptions> splitBuildOptions = ImmutableMap.builder();
@@ -157,7 +154,9 @@ private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition tr
   }
 
   /**
-   * Enter the options in buildOptions into a Starlark dictionary, and return the dictionary.
+   * Return an ImmutableMap containing only BuildOptions explicitly registered as transition inputs.
+   *
+   * <p>nulls are converted to Starlark.NONE but no other conversions are done.
    *
    * @throws IllegalArgumentException If the method is unable to look up the value in buildOptions
    *     corresponding to an entry in optionInfoMap
@@ -167,7 +166,7 @@ private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition tr
    * @throws ValidationException if any of the specified transition inputs do not correspond to a
    *     valid build setting
    */
-  private static Dict<String, Object> buildSettings(
+  private static ImmutableMap<String, Object> buildSettings(
       BuildOptions buildOptions,
       Map<String, OptionInfo> optionInfoMap,
       StarlarkDefinedConfigTransition starlarkTransition)
@@ -177,57 +176,49 @@ private static Dict<String, Object> buildSettings(
     LinkedHashSet<String> remainingInputs =
         Sets.newLinkedHashSet(inputsCanonicalizedToGiven.keySet());
 
-    try (Mutability mutability = Mutability.create("build_settings")) {
-      Dict<String, Object> dict = Dict.of(mutability);
-
-      // Add native options
-      for (Map.Entry<String, OptionInfo> entry : optionInfoMap.entrySet()) {
-        String optionName = entry.getKey();
-        String optionKey = COMMAND_LINE_OPTION_PREFIX + optionName;
+    ImmutableMap.Builder<String, Object> optionsBuilder = ImmutableMap.builder();
 
-        if (!remainingInputs.remove(optionKey)) {
-          // This option was not present in inputs. Skip it.
-          continue;
-        }
-        OptionInfo optionInfo = entry.getValue();
+    // Add native options
+    for (Map.Entry<String, OptionInfo> entry : optionInfoMap.entrySet()) {
+      String optionName = entry.getKey();
+      String optionKey = COMMAND_LINE_OPTION_PREFIX + optionName;
 
-        Field field = optionInfo.getDefinition().getField();
-        FragmentOptions options = buildOptions.get(optionInfo.getOptionClass());
-        try {
-          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
-        }
+      if (!remainingInputs.remove(optionKey)) {
+        // This option was not present in inputs. Skip it.
+        continue;
       }
-
-      // Add Starlark options
-      for (Map.Entry<Label, Object> starlarkOption : buildOptions.getStarlarkOptions().entrySet()) {
-        String canonicalLabelForm = starlarkOption.getKey().getUnambiguousCanonicalForm();
-        if (!remainingInputs.remove(canonicalLabelForm)) {
-          continue;
-        }
-        // Convert the canonical form to the user requested form that they expect to see in this
-        // dict.
-        String userRequestedLabelForm = inputsCanonicalizedToGiven.get(canonicalLabelForm);
-        try {
-          dict.putEntry(userRequestedLabelForm, starlarkOption.getValue());
-        } catch (EvalException ex) {
-          throw new IllegalStateException(ex); // can't happen
-        }
+      OptionInfo optionInfo = entry.getValue();
+
+      Field field = optionInfo.getDefinition().getField();
+      FragmentOptions options = buildOptions.get(optionInfo.getOptionClass());
+      try {
+        Object optionValue = field.get(options);
+        // convert nulls here b/c ImmutableMap bans null values
+        optionsBuilder.put(optionKey, optionValue == null ? Starlark.NONE : optionValue);
+      } catch (IllegalAccessException e) {
+        // These exceptions should not happen, but if they do, throw a RuntimeException.
+        throw new IllegalStateException(e);
       }
+    }
 
-      if (!remainingInputs.isEmpty()) {
-        throw ValidationException.format(
-            "transition inputs [%s] do not correspond to valid settings",
-            Joiner.on(", ").join(remainingInputs));
+    // Add Starlark options
+    for (Map.Entry<Label, Object> starlarkOption : buildOptions.getStarlarkOptions().entrySet()) {
+      String canonicalLabelForm = starlarkOption.getKey().getUnambiguousCanonicalForm();
+      if (!remainingInputs.remove(canonicalLabelForm)) {
+        continue;
       }
+      // Convert the canonical form to the user requested form that they expect to see
+      String userRequestedLabelForm = inputsCanonicalizedToGiven.get(canonicalLabelForm);
+      optionsBuilder.put(userRequestedLabelForm, starlarkOption.getValue());
+    }
 
-      return dict;
+    if (!remainingInputs.isEmpty()) {
+      throw ValidationException.format(
+          "transition inputs [%s] do not correspond to valid settings",
+          Joiner.on(", ").join(remainingInputs));
     }
+
+    return optionsBuilder.buildOrThrow();
   }
 
   /**

src/main/java/net/starlark/java/eval/Starlark.java

@@ -86,6 +86,24 @@ public void repr(Printer printer) {
    */
   public static final ImmutableMap<String, Object> UNIVERSE = makeUniverse();
 
+  /**
+   * An {@code IllegalArgumentException} subclass for when a non-Starlark object is encountered in a
+   * context where a Starlark value ({@code String}, {@code Boolean}, or {@code StarlarkValue}) was
+   * expected.
+   */
+  public static final class InvalidStarlarkValueException extends IllegalArgumentException {
+    private final Class<?> invalidClass;
+
+    public Class<?> getInvalidClass() {
+      return invalidClass;
+    }
+
+    private InvalidStarlarkValueException(Class<?> invalidClass) {
+      super("invalid Starlark value: " + invalidClass);
+      this.invalidClass = invalidClass;
+    }
+  }
+
   private static ImmutableMap<String, Object> makeUniverse() {
     ImmutableMap.Builder<String, Object> env = ImmutableMap.builder();
     env //
@@ -105,11 +123,11 @@ public static boolean valid(Object x) {
 
   /**
    * Returns {@code x} if it is a {@link #valid} Starlark value, otherwise throws
-   * IllegalArgumentException.
+   * InvalidStarlarkValueException.
    */
   public static <T> T checkValid(T x) {
     if (!valid(x)) {
-      throw new IllegalArgumentException("invalid Starlark value: " + x.getClass());
+      throw new InvalidStarlarkValueException(x.getClass());
     }
     return x;
   }
@@ -139,7 +157,7 @@ public static boolean isImmutable(Object x) {
     } else if (x instanceof StarlarkValue) {
       return ((StarlarkValue) x).isImmutable();
     } else {
-      throw new IllegalArgumentException("invalid Starlark value: " + x.getClass());
+      throw new InvalidStarlarkValueException(x.getClass());
     }
   }
 
@@ -162,7 +180,7 @@ public static void checkHashable(Object x) throws EvalException {
    * An Integer, Long, or BigInteger is converted to a Starlark int, a double is converted to a
    * Starlark float, a Java List or Map is converted to a Starlark list or dict, respectively, and
    * null becomes {@link #NONE}. Any other non-Starlark value causes the function to throw
-   * IllegalArgumentException.
+   * InvalidStarlarkValueException.
    *
    * <p>Elements of Lists and Maps must be valid Starlark values; they are not recursively
    * converted. (This avoids excessive unintended deep copying.)
@@ -189,7 +207,7 @@ public static Object fromJava(Object x, @Nullable Mutability mutability) {
     } else if (x instanceof Map) {
       return Dict.copyOf(mutability, (Map<?, ?>) x);
     }
-    throw new IllegalArgumentException("cannot expose internal type to Starlark: " + x.getClass());
+    throw new InvalidStarlarkValueException(x.getClass());
   }
 
   /**
@@ -204,7 +222,7 @@ public static boolean truth(Object x) {
     } else if (x instanceof String) {
       return !((String) x).isEmpty();
     } else {
-      throw new IllegalArgumentException("invalid Starlark value: " + x.getClass());
+      throw new InvalidStarlarkValueException(x.getClass());
     }
   }
 

src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java

@@ -37,6 +37,7 @@
 import com.google.testing.junit.testparameterinjector.TestParameters;
 import java.util.List;
 import java.util.Map;
+import java.util.regex.Pattern;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
@@ -549,6 +550,50 @@ public void testTransitionOnBuildSetting_dontStoreDefault() throws Exception {
         .doesNotContainKey(Label.parseAbsoluteUnchecked("//test:cute-animal-fact"));
   }
 
+  @Test
+  public void testTransitionOnBuildSetting_readingUnreadableBuildSetting() throws Exception {
+    scratch.file(
+        "test/transitions.bzl",
+        "def _transition_impl(settings, attr):",
+        "  old_value = settings['//command_line_option:unreadable_by_starlark']",
+        "  fail('This line should be unreachable.')",
+        "my_transition = transition(implementation = _transition_impl,",
+        "  inputs = ['//command_line_option:unreadable_by_starlark'],",
+        "  outputs = ['//command_line_option:unreadable_by_starlark'],",
+        ")");
+    writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests();
+
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//test");
+    assertContainsEvent(
+        Pattern.compile(
+            "test/transitions.bzl:1:5: before calling _transition_impl: Input build setting"
+                + " //command_line_option:unreadable_by_starlark is of type class"
+                + " \\S*UnreadableStringBox, which is unreadable in Starlark. Please submit a"
+                + " feature request."));
+  }
+
+  @Test
+  public void testTransitionOnBuildSetting_writingUnreadableBuildSetting() throws Exception {
+    scratch.file(
+        "test/transitions.bzl",
+        "def _transition_impl(settings, attr):",
+        "  return {",
+        "    '//command_line_option:unreadable_by_starlark': 'post-transition',",
+        "  }",
+        "my_transition = transition(implementation = _transition_impl,",
+        "  inputs = [],",
+        "  outputs = ['//command_line_option:unreadable_by_starlark'],",
+        ")");
+    writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests();
+
+    useConfiguration("--unreadable_by_starlark=pre-transition");
+
+    BuildConfigurationValue configuration = getConfiguration(getConfiguredTarget("//test"));
+    assertThat(configuration.getOptions().get(DummyTestOptions.class).unreadableByStarlark)
+        .isEqualTo(DummyTestOptions.UnreadableStringBox.create("post-transition"));
+  }
+
   @Test
   public void testTransitionReadsBuildSetting_fromDefault() throws Exception {
     scratch.file(

src/test/java/com/google/devtools/build/lib/analysis/util/DummyTestFragment.java

@@ -13,16 +13,19 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis.util;
 
+import com.google.auto.value.AutoValue;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.CoreOptionConverters.EmptyToNullLabelConverter;
 import com.google.devtools.build.lib.analysis.config.Fragment;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.RequiresOptions;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.common.options.Converter;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
 import com.google.devtools.common.options.OptionMetadataTag;
+import com.google.devtools.common.options.OptionsParsingException;
 import java.util.List;
 
 /**
@@ -92,5 +95,36 @@ public static class DummyTestOptions extends FragmentOptions {
         effectTags = {OptionEffectTag.NO_OP},
         help = "A regular bool-typed option")
     public boolean bool;
+
+    @Option(
+        name = "unreadable_by_starlark",
+        defaultValue = "anything",
+        converter = UnreadableStringBoxConverter.class,
+        documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+        effectTags = {OptionEffectTag.NO_OP},
+        help = "This cannot be used as an input to a Starlark transition")
+    public UnreadableStringBox unreadableByStarlark;
+
+    @AutoValue
+    public abstract static class UnreadableStringBox {
+      public abstract String value();
+
+      public static UnreadableStringBox create(String value) {
+        return new AutoValue_DummyTestFragment_DummyTestOptions_UnreadableStringBox(value);
+      }
+    }
+
+    public static class UnreadableStringBoxConverter implements Converter<UnreadableStringBox> {
+      @Override
+      public UnreadableStringBox convert(String input, Object conversionContext)
+          throws OptionsParsingException {
+        return UnreadableStringBox.create(input);
+      }
+
+      @Override
+      public String getTypeDescription() {
+        return "a string that is not readable by Starlark";
+      }
+    }
   }
 }