Starlark transitions: support allowMultiple native options.

https://github.com/bazelbuild/bazel/blob/a7b1d3662e63a180f5d8d9b9188eac2aa486d02d/src/main/java/com/google/devtools/common/options/Option.java#L135-L146
documents that `allowMultiple` options can have converters that either return
entries or lists of entries.

Immediate motivation is to support the Starlark exec transition with
`--action_env` and `--host_action_env`. Without this we get failures in
`//third_party/bazel/src/test/shell/integration:action_env_test`, which sets those
flags to non-trivial values.

PiperOrigin-RevId: 581996512
Change-Id: I68a20aa31580de904d2a9703e3b57370aeb6bcbf

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -2008,6 +2008,7 @@ java_library(
     srcs = ["config/StarlarkDefinedConfigTransition.java"],
     deps = [
         ":config/core_options",
+        ":config/optioninfo",
         ":config/transitions/configuration_transition",
         ":config/transitions/patch_transition",
         "//src/main/java/com/google/devtools/build/lib/cmdline",

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

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.analysis.config;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition.PATCH_TRANSITION_KEY;
 
 import com.github.benmanes.caffeine.cache.Cache;
@@ -45,6 +46,9 @@
 import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigurationTransitionApi;
 import com.google.devtools.build.lib.util.RegexFilter;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.common.options.Converter;
+import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionDefinition;
 import com.google.devtools.common.options.TriState;
 import com.google.errorprone.annotations.FormatMethod;
 import java.util.HashMap;
@@ -243,6 +247,9 @@ public final Cache<RuleTransitionData, PatchTransition> getRuleTransitionCache()
    * a result of applying this transition.
    *
    * @param previousSettings a map representing the previous build settings
+   * @param attributeMapper a map of attributes
+   * @param optionInfoMap info about each option's {@link Option} type
+   * @param handler handler for messages
    * @return a map of changed build setting maps; each element of the map represents a different
    *     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
@@ -253,7 +260,10 @@ public final Cache<RuleTransitionData, PatchTransition> getRuleTransitionCache()
    */
   @Nullable
   public abstract ImmutableMap<String, Map<String, Object>> evaluate(
-      Map<String, Object> previousSettings, StructImpl attributeMap, EventHandler eventHandler)
+      Map<String, Object> previousSettings,
+      StructImpl attributeMapper,
+      ImmutableMap<String, OptionInfo> optionInfoMap,
+      EventHandler handler)
       throws InterruptedException;
 
   public static StarlarkDefinedConfigTransition newRegularTransition(
@@ -305,6 +315,7 @@ public boolean isForAnalysisTesting() {
     public ImmutableMap<String, Map<String, Object>> evaluate(
         Map<String, Object> previousSettings,
         StructImpl attributeMapper,
+        ImmutableMap<String, OptionInfo> optionInfoMap,
         EventHandler eventHandler) {
       return ImmutableMap.of(PATCH_TRANSITION_KEY, changedSettings);
     }
@@ -377,21 +388,34 @@ private static final class UnreadableInputSettingException extends Exception {
      * <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 settings map os settings to copy over * {@code optionInfoMap} info about each option's
+     *     {@link Option} type
+     * @param optionInfoMap info about each option's {@link Option} type
      * @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 static Dict<String, Object> createBuildSettingsDict(
-        Map<String, Object> settings, Mutability entryMu) throws UnreadableInputSettingException {
+        Map<String, Object> settings,
+        ImmutableMap<String, OptionInfo> optionInfoMap,
+        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) {
-          builder.put(entry.getKey(), getTransitionSafeString(entry.getKey(), entry.getValue()));
+          // Starlark#frromJava doesn't know how to read this value. Try again with a special
+          // allowlist of types we know how to make Starlark-compatible. This is not complete. If a
+          // value a) isn't Starlark-convertible and b) not special-cased here, Bazel emits a "can't
+          // process this setting" error.
+          builder.put(
+              entry.getKey(),
+              Starlark.fromJava(
+                  getTransitionSafeString(entry.getKey(), entry.getValue(), optionInfoMap),
+                  entryMu));
         }
       }
 
@@ -421,11 +445,21 @@ private static Dict<String, Object> createBuildSettingsDict(
 
     /**
      * Converts a Java-native flag value to a Starlark-readable string, or throws an exception if
-     * the flag's type can't be cleanly represented in Starlark.
+     * the flag's type can't be represented in Starlark.
+     *
+     * <p>This only kicks in for values {@link Starlark#fromJava} failed to directly convert. That
+     * implies they need extra processing, which is what happens here.
+     *
+     * <p>This is incomplete. It only handles types we explicitly know are Starlark-convertible or
+     * that handle {@link Converter#starlarkConvertible()}. Other flags emit a "can't process this
+     * setting" error.
      */
-    private static String getTransitionSafeString(String name, Object value)
+    private static Object getTransitionSafeString(
+        String name, Object value, ImmutableMap<String, OptionInfo> optionInfoMap)
         throws UnreadableInputSettingException {
       if (value instanceof RegexFilter) {
+        // RegExFilter doesn't serialize to the same value it originally had on the command line.
+        // Call toOriginalString, to do that properly.
         return Verify.verifyNotNull(((RegexFilter) value).toOriginalString());
       }
       if (value instanceof PathFragment
@@ -435,9 +469,42 @@ private static String getTransitionSafeString(String name, Object value)
           || value instanceof OutputPathsMode
           || value instanceof IncludeConfigFragmentsEnum
           || SAFE_NATIVE_FLAG_TYPES.contains(value.getClass().getSimpleName())) {
+        // Starlark#fromJava doesn't understand these Bazel-specific Java types. But their
+        // toString() methods serialize cleanly.
         return value.toString();
       }
-      throw new UnreadableInputSettingException(name, value.getClass());
+      // See if the option's converter knows how to produce to Starlark values.
+      OptionDefinition optionDef =
+          optionInfoMap.get(name.substring(COMMAND_LINE_OPTION_PREFIX.length())).getDefinition();
+      if (!optionDef.getConverter().starlarkConvertible()) {
+        throw new UnreadableInputSettingException(name, value.getClass());
+      }
+      if (optionDef.allowsMultiple()) {
+        // allowMultiple() options are complicated (see the definition of allowMultiple() in
+        // Option.java). They must be typed as a List<T>. Their converters can return either T or
+        // List<T>. In the latter case, the typed value is a concatenation of all the converted
+        // lists.
+        //
+        // Option metadata doesn't include enough information to know which version of the converter
+        // it uses. Also note we can't encode this information in the converter because different
+        // options may use the same converter with or without allowMultiple.
+        //
+        // For lack of direct support, this code infers the right logic.
+        var asList = ((List<?>) value);
+        // If this is an empty list, Starlark#fromJava should have handled it.
+        Verify.verify(!asList.isEmpty());
+        // The converter matches the option with generics. So we don't actually know how their types
+        // compare at runtime. We know allowMultiple options must be typed List<T>. We assume if the
+        // converter doesn't return a list, it returns a single T. Else it returns a List<T>. This
+        // works as long as the option isn't a List<List<?>>. This verification check confirms that.
+        Verify.verify(!(asList.get(0) instanceof List));
+        return asList.stream()
+            .map(o -> optionDef.getConverter().reverseForStarlark(o))
+            .collect(toImmutableList());
+      } else {
+        // This isn't allowMultiple, so the converter is a straightforward reversal.
+        return optionDef.getConverter().reverseForStarlark(value);
+      }
     }
 
     /**
@@ -454,7 +521,9 @@ private static String getTransitionSafeString(String name, Object value)
      * splits (i.e. accessing later via {@code ctx.split_attrs}).
      *
      * @param previousSettings a map representing the previous build settings
-     * @param attributeMapper a map of attributes
+     * @param attrObject the attributes of the rule to which this transition is attached
+     * @param optionInfoMap info about each option's {@link Option} type
+     * @param handler handler for messages
      * @return a map of the changed settings. An empty map is shorthand for the transition not
      *     changing any settings ({@code return {} } is simpler than assigning every output setting
      *     to itself). A null return means an error occurred and results are unusable.
@@ -463,7 +532,10 @@ private static String getTransitionSafeString(String name, Object value)
     @Nullable
     @Override
     public ImmutableMap<String, Map<String, Object>> evaluate(
-        Map<String, Object> previousSettings, StructImpl attributeMapper, EventHandler handler)
+        Map<String, Object> previousSettings,
+        StructImpl attrObject,
+        ImmutableMap<String, OptionInfo> optionInfoMap,
+        EventHandler handler)
         throws InterruptedException {
       // Call the Starlark function.
       Object result;
@@ -478,7 +550,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);
+        Dict<String, Object> previousSettingsDict =
+            createBuildSettingsDict(previousSettings, optionInfoMap, mu);
 
         // Create a new {@link BazelStarlarkContext} for the new thread. We need to
         // create a new context every time because {@link BazelStarlarkContext}s
@@ -487,7 +560,7 @@ public ImmutableMap<String, Map<String, Object>> evaluate(
 
         result =
             Starlark.fastcall(
-                thread, impl, new Object[] {previousSettingsDict, attributeMapper}, new Object[0]);
+                thread, impl, new Object[] {previousSettingsDict, attrObject}, 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

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

@@ -97,7 +97,7 @@ static ImmutableMap<String, BuildOptions> applyAndValidate(
       BuildOptions baselineToOptions = maybeGetExecDefaults(fromOptions, starlarkTransition);
 
       ImmutableMap<String, Map<String, Object>> transitions =
-          starlarkTransition.evaluate(settings, attrObject, handler);
+          starlarkTransition.evaluate(settings, attrObject, optionInfoMap, handler);
       if (transitions == null) {
         return null; // errors reported to handler
       } else if (transitions.isEmpty()) {
@@ -377,7 +377,7 @@ private static BuildOptions applyTransition(
           Field field = def.getField();
           // TODO(b/153867317): check for crashing options types in this logic.
           Object convertedValue;
-          if (def.getType() == List.class && optionValue instanceof List && !def.allowsMultiple()) {
+          if (def.getType() == List.class && optionValue instanceof List) {
             // This is possible with Starlark code like "{ //command_line_option:foo: ["a", "b"] }".
             // In that case def.getType() == List.class while optionValue.type == StarlarkList.
             // Unfortunately we can't check the *element* types because OptionDefinition won't tell
@@ -387,13 +387,10 @@ private static BuildOptions applyTransition(
             // generically safe way to do this. We convert its elements with .toString() with a ","
             // separator, which happens to work for most implementations. But that's not universally
             // guaranteed.
-            // TODO(b/153867317): support allowMultiple options too. This is subtle: see the
-            // description of allowMultiple in Option.java. allowMultiple converts have the choice
-            // of returning either a scalar or list.
             List<?> optionValueAsList = (List<?>) optionValue;
             if (optionValueAsList.isEmpty()) {
               convertedValue = ImmutableList.of();
-            } else {
+            } else if (!def.allowsMultiple()) {
               convertedValue =
                   def.getConverter()
                       .convert(
@@ -405,6 +402,21 @@ private static BuildOptions applyTransition(
                                           : element.toString())
                               .collect(joining(",")),
                           starlarkTransition.getPackageContext());
+            } else {
+              var valueBuilder = ImmutableList.builder();
+              // We can't use streams because def.getConverter().convert may throw an
+              // OptionsParsingException.
+              for (Object e : optionValueAsList) {
+                Object converted =
+                    def.getConverter()
+                        .convert(e.toString(), starlarkTransition.getPackageContext());
+                if (converted instanceof List) {
+                  valueBuilder.addAll((List<?>) converted);
+                } else {
+                  valueBuilder.add(converted);
+                }
+              }
+              convertedValue = valueBuilder.build();
             }
           } else if (def.getType() == List.class && optionValue == null) {
             throw ValidationException.format(

src/main/java/com/google/devtools/common/options/Converter.java

@@ -34,6 +34,33 @@ public interface Converter<T> {
    */
   String getTypeDescription();
 
+  /**
+   * Can this converter reverse-convert to a Starlark-readable value?
+   *
+   * <p>If so, {@link #reverseForStarlark} implements the reverse conversion. If not, {@link
+   * #reverseForStarlark} throws an {@link UnsupportedOperationException}.
+   */
+  default boolean starlarkConvertible() {
+    return false;
+  }
+
+  /**
+   * If {@link #starlarkConvertible()} is true, this reverses a converted value back to a
+   * Starlark-readable form.
+   *
+   * <p>If {@link #starlarkConvertible()} is true, throws an {@link UnsupportedOperationException}.
+   *
+   * @param converted If the option this value represents isn't {@link Option#allowMultiple}, an
+   *     object of the option's Java type. Else an entry in the option's {@link java.util.List}.
+   *     Always of type T. Referenced as an {@link Object} because calling code can call any
+   *     converter.
+   * @return A {@link String} version of the input. Calling {@link #convert} on this value should
+   *     faithfully reproduce {@code converted}.
+   */
+  default String reverseForStarlark(Object converted) {
+    throw new UnsupportedOperationException("This converter doesn't support Starlark reversal.");
+  }
+
   /** A converter that never reads its context parameter. */
   abstract class Contextless<T> implements Converter<T> {
 

src/main/java/com/google/devtools/common/options/Converters.java

@@ -683,6 +683,20 @@ public Map.Entry<String, String> convert(String input) throws OptionsParsingExce
       return Maps.immutableEntry(name, value);
     }
 
+    @Override
+    public boolean starlarkConvertible() {
+      return true;
+    }
+
+    @Override
+    public String reverseForStarlark(Object converted) {
+      @SuppressWarnings("unchecked")
+      Map.Entry<String, String> typedValue = (Map.Entry<String, String>) converted;
+      return typedValue.getValue() == null
+          ? typedValue.getKey()
+          : String.format("%s=%s", typedValue.getKey(), typedValue.getValue());
+    }
+
     @Override
     public String getTypeDescription() {
       return "a 'name=value' assignment with an optional value part";