starlark: clean up Dict construction

Dict.copyOf used to call fromJava for each map value.
This was a hack to support Bazel's attr.string_list_dict type
(a Map<String, List<String>>) so that lists in dicts would be
recursively converted; but it has no place here.
Now, fromJava makes only a shallow copy, and Dict.copyOf
asserts checkValid on all keys and values.

Dict.Builder is a builder of Dicts, mutable or immutable.
This avoids a number of hash table copies.

Various places in the loading phase assume that Starlark.fromJava
is the appropriate conversion from the internal form of rule
attribute values to Starlark form. That is now incorrect.
Attribute.valueToStarlark is the correct conversion, and those
places now call it instead.

PiperOrigin-RevId: 340350758

src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java

@@ -576,12 +576,12 @@ public Dict<String, String> getExecutionInfoDict() {
     if (executionInfo == null) {
       return null;
     }
-    return Dict.copyOf(null, executionInfo);
+    return Dict.immutableCopyOf(executionInfo);
   }
 
   @Override
   public Dict<String, String> getEnv() {
-    return Dict.copyOf(null, env.getFixedEnv().toMap());
+    return Dict.immutableCopyOf(env.getFixedEnv().toMap());
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/analysis/ActionsProvider.java

@@ -22,7 +22,7 @@
 import com.google.devtools.build.lib.starlarkbuildapi.ActionsInfoProviderApi;
 import java.util.HashMap;
 import java.util.Map;
-import net.starlark.java.eval.Starlark;
+import net.starlark.java.eval.Dict;
 import net.starlark.java.syntax.Location;
 
 /**
@@ -50,7 +50,7 @@ public static StructImpl create(Iterable<ActionAnalysisMetadata> actions) {
       }
     }
     ImmutableMap<String, Object> fields =
-        ImmutableMap.<String, Object>of("by_file", Starlark.fromJava(map, /*mutability=*/ null));
+        ImmutableMap.<String, Object>of("by_file", Dict.immutableCopyOf(map));
     return StarlarkInfo.create(INSTANCE, fields, Location.BUILTIN);
   }
 }

src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java

@@ -23,9 +23,7 @@
 import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
 import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
 import com.google.devtools.build.lib.packages.Package;
-import java.util.LinkedHashMap;
 import java.util.List;
-import java.util.Map;
 import java.util.stream.Collectors;
 import net.starlark.java.eval.Dict;
 
@@ -120,13 +118,13 @@ public String lookupVariable(String name) throws ExpansionException {
   }
 
   public Dict<String, String> collectMakeVariables() throws ExpansionException {
-    Map<String, String> map = new LinkedHashMap<>();
+    Dict.Builder<String, String> map = Dict.builder();
     // Collect variables in the reverse order as in lookupMakeVariable
     // because each update is overwriting.
     for (MakeVariableSupplier supplier : allMakeVariableSuppliers.reverse()) {
       map.putAll(supplier.getAllMakeVariables());
     }
-    return Dict.<String, String>copyOf(null, map);
+    return map.buildImmutable();
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java

@@ -16,7 +16,6 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.util.concurrent.ListenableFuture;
@@ -216,10 +215,10 @@ public boolean makeExecutable() {
 
   @Override
   public Dict<String, String> getStarlarkSubstitutions() {
-    ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+    Dict.Builder<String, String> builder = Dict.builder();
     for (Substitution entry : substitutions) {
       builder.put(entry.getKey(), entry.getValue());
     }
-    return Dict.copyOf(null, builder.build());
+    return builder.buildImmutable();
   }
 }

src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/ConfiguredTargetsUtil.java

@@ -16,7 +16,6 @@
 import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMap;
 import com.google.devtools.build.lib.packages.Provider;
 import com.google.devtools.build.lib.packages.StarlarkProvider;
-import java.util.LinkedHashMap;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.Starlark;
 
@@ -34,7 +33,7 @@ private ConfiguredTargetsUtil() {}
    */
   public static Dict<String, Object> getProvidersDict(
       AbstractConfiguredTarget target, TransitiveInfoProviderMap providers) {
-    LinkedHashMap<String, Object> res = new LinkedHashMap<>();
+    Dict.Builder<String, Object> res = Dict.builder();
     for (int i = 0; i < providers.getProviderCount(); i++) {
       // The key may be of many types, but we need a string for the intended use.
       Object key = providers.getProviderKeyAt(i);
@@ -61,6 +60,6 @@ public static Dict<String, Object> getProvidersDict(
         // This is OK. If this is not a valid StarlarkValue, we just leave it out of the map.
       }
     }
-    return Dict.copyOf(null, res);
+    return res.buildImmutable();
   }
 }

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

@@ -38,7 +38,6 @@
 import java.util.Map;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Printer;
-import net.starlark.java.eval.Starlark;
 
 /**
  * This class implements {@link TransitionFactory} to provide a starlark-defined transition that
@@ -94,7 +93,7 @@ class FunctionSplitTransition extends StarlarkTransition implements SplitTransit
       LinkedHashMap<String, Object> attributes = new LinkedHashMap<>();
       for (String attribute : attributeMap.getAttributeNames()) {
         Object val = attributeMap.get(attribute, attributeMap.getAttributeType(attribute));
-        attributes.put(Attribute.getStarlarkName(attribute), Starlark.fromJava(val, null));
+        attributes.put(Attribute.getStarlarkName(attribute), Attribute.valueToStarlark(val));
       }
       attrObject = StructProvider.STRUCT.create(attributes, ERROR_MESSAGE_FOR_NO_ATTR);
     }

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

@@ -30,6 +30,7 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Printer;
 import net.starlark.java.eval.Starlark;
@@ -164,7 +165,7 @@ public void addAttribute(Attribute a, Object val) {
       // Starlark as a Map<String, Label>; this special case preserves that behavior temporarily.
       if (type.getLabelClass() != LabelClass.DEPENDENCY || type == BuildType.LABEL_DICT_UNARY) {
         // Attribute values should be type safe
-        attrBuilder.put(skyname, Starlark.fromJava(val, null));
+        attrBuilder.put(skyname, Attribute.valueToStarlark(val));
         return;
       }
       if (a.isExecutable()) {
@@ -213,14 +214,14 @@ public void addAttribute(Attribute a, Object val) {
         List<?> allPrereq = context.getRuleContext().getPrerequisites(a.getName());
         attrBuilder.put(skyname, StarlarkList.immutableCopyOf(allPrereq));
       } else if (type == BuildType.LABEL_KEYED_STRING_DICT) {
-        ImmutableMap.Builder<TransitiveInfoCollection, String> builder = ImmutableMap.builder();
+        Dict.Builder<TransitiveInfoCollection, String> builder = Dict.builder();
         Map<Label, String> original = BuildType.LABEL_KEYED_STRING_DICT.cast(val);
         List<? extends TransitiveInfoCollection> allPrereq =
             context.getRuleContext().getPrerequisites(a.getName());
         for (TransitiveInfoCollection prereq : allPrereq) {
           builder.put(prereq, original.get(AliasProvider.getDependencyLabel(prereq)));
         }
-        attrBuilder.put(skyname, Starlark.fromJava(builder.build(), null));
+        attrBuilder.put(skyname, builder.buildImmutable());
       } else if (type == BuildType.LABEL_DICT_UNARY) {
         Map<Label, TransitiveInfoCollection> prereqsByLabel = new LinkedHashMap<>();
         for (TransitiveInfoCollection target :

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

@@ -462,7 +462,7 @@ private static StructImpl buildSplitAttributeInfo(
         }
       }
 
-      splitAttrInfos.put(attr.getPublicName(), Dict.copyOf(null, splitPrereqsMap));
+      splitAttrInfos.put(attr.getPublicName(), Dict.immutableCopyOf(splitPrereqsMap));
     }
 
     return StructProvider.STRUCT.create(

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

@@ -35,7 +35,6 @@
 import java.util.Map;
 import java.util.Objects;
 import net.starlark.java.eval.EvalException;
-import net.starlark.java.eval.Starlark;
 
 /**
  * This class implements {@link TransitionFactory} to provide a starlark-defined transition that
@@ -90,7 +89,7 @@ class FunctionPatchTransition extends StarlarkTransition implements PatchTransit
           continue;
         }
         attributes.put(
-            Attribute.getStarlarkName(attribute.getPublicName()), Starlark.fromJava(val, null));
+            Attribute.getStarlarkName(attribute.getPublicName()), Attribute.valueToStarlark(val));
       }
       attrObject =
           StructProvider.STRUCT.create(

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java

@@ -1020,15 +1020,15 @@ private StructImpl calculateDownloadResult(Optional<Checksum> checksum, Path dow
           Transience.PERSISTENT);
     }
 
-    ImmutableMap.Builder<String, Object> out = new ImmutableMap.Builder<>();
+    Dict.Builder<String, Object> out = Dict.builder();
     out.put("success", true);
     out.put("integrity", finalChecksum.toSubresourceIntegrity());
 
     // For compatibility with older Bazel versions that don't support non-SHA256 checksums.
     if (finalChecksum.getKeyType() == KeyType.SHA256) {
       out.put("sha256", finalChecksum.toString());
     }
-    return StructProvider.STRUCT.createWithBuiltinLocation(Dict.copyOf(null, out.build()));
+    return StructProvider.STRUCT.createWithBuiltinLocation(out.buildImmutable());
   }
 
   private static ImmutableList<String> checkAllUrls(Iterable<?> urlList) throws EvalException {

src/main/java/com/google/devtools/build/lib/packages/Attribute.java

@@ -57,6 +57,7 @@
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 import net.starlark.java.eval.ClassObject;
+import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkValue;
@@ -2377,4 +2378,43 @@ public <TYPE> Attribute.Builder<TYPE> cloneBuilder(Type<TYPE> tp) {
   public Attribute.Builder<?> cloneBuilder() {
     return cloneBuilder(this.type);
   }
+
+  /**
+   * Converts a rule attribute value from internal form to Starlark form. Internal form may use any
+   * subtype of {@link List} or {@link Map} for {@code list} and {@code dict} attributes, whereas
+   * Starlark uses only immutable {@link StarlarkList} and {@link Dict}.
+   *
+   * <p>The conversion is similar to {@link Starlark#fromJava} for all types except {@code
+   * attr.string_list_dict} ({@code Map<String, List<String>>}), for which fromJava does not
+   * recursively convert elements. (Doing so is expensive.)
+   *
+   * <p>It is tempting to require that attributes are stored internally in Starlark form. However, a
+   * number of obstacles would need to be overcome:
+   *
+   * <ol>
+   *   <li>Some obscure attribute types such as TRISTATE and DISTRIBUTION are not currently legal
+   *       Starlark values.
+   *   <li>ImmutableList is significantly more compact than StarlarkList for small lists (n &lt; 2).
+   *       StarlarkList would need multiple representations and a builder to achieve parity.
+   *   <li>The types used by the Type mechanism would need changing; this has extensive
+   *       ramifications.
+   * </ol>
+   */
+  public static Object valueToStarlark(Object x) {
+    // Is x a non-empty string_list_dict?
+    if (x instanceof Map) {
+      Map<?, ?> map = (Map) x;
+      if (!map.isEmpty() && map.values().iterator().next() instanceof List) {
+        // Recursively convert subelements.
+        Dict.Builder<Object, Object> dict = Dict.builder();
+        for (Map.Entry<?, ?> e : map.entrySet()) {
+          dict.put((String) e.getKey(), Starlark.fromJava(e.getValue(), null));
+        }
+        return dict.buildImmutable();
+      }
+    }
+
+    // For all other attribute values, shallow conversion is safe.
+    return Starlark.fromJava(x, null);
+  }
 }

src/main/java/com/google/devtools/build/lib/packages/ImplicitOutputsFunction.java

@@ -96,8 +96,7 @@ public ImmutableMap<String, String> calculateOutputs(
         // since we don't yet have a build configuration.
         if (!map.isConfigurable(attrName)) {
           Object value = map.get(attrName, attrType);
-          attrValues.put(
-              Attribute.getStarlarkName(attrName), Starlark.fromJava(value, /*mutability=*/ null));
+          attrValues.put(Attribute.getStarlarkName(attrName), Attribute.valueToStarlark(value));
         }
       }
       ClassObject attrs =

src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java

@@ -39,7 +39,6 @@
 import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
-import java.util.TreeMap;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
@@ -371,7 +370,7 @@ private static Object starlarkifyValue(Mutability mu, Object val, Package pkg)
       return Tuple.copyOf(l);
     }
     if (val instanceof Map) {
-      Map<Object, Object> m = new TreeMap<>();
+      Dict.Builder<Object, Object> m = Dict.builder();
       for (Map.Entry<?, ?> e : ((Map<?, ?>) val).entrySet()) {
         Object key = starlarkifyValue(mu, e.getKey(), pkg);
         Object mapVal = starlarkifyValue(mu, e.getValue(), pkg);
@@ -382,7 +381,7 @@ private static Object starlarkifyValue(Mutability mu, Object val, Package pkg)
 
         m.put(key, mapVal);
       }
-      return Starlark.fromJava(m, mu);
+      return m.build(mu);
     }
     if (val.getClass().isAnonymousClass()) {
       // Computed defaults. They will be represented as

src/main/java/com/google/devtools/build/lib/packages/Type.java

@@ -79,6 +79,11 @@
  * since we seem to be doing fine so far without it), and not overly complicate BUILD files or rule
  * implementation functions.
  */
+// TODO(adonovan): update documentation here and elsewhere to use the term
+// "rule attribute values" or "valid attribute types" where appropriate,
+// and not "value in the build language", which is a much broader set of
+// possible Starlark values. Also link to the canonical set of valid attribute
+// types, both Starlark and native.
 public abstract class Type<T> {
 
   protected Type() {}

src/main/java/com/google/devtools/build/lib/query2/cquery/StarlarkOutputFormatterCallback.java

@@ -88,9 +88,16 @@ public Object buildOptions(ConfiguredTarget target) {
             Object optionValue = field.get(options);
 
             try {
+              // fromJava is not a deep validity check.
+              // It is not guaranteed to catch all errors,
+              // nor does it specify how it reports the errors it does find.
+              // Passing arbitrary Java values into the Starlark interpreter
+              // is not safe.
+              // TODO(cparsons,twigg): fix it: convert value by explicit cases.
               result.put(optionKey, Starlark.fromJava(optionValue, null));
-            } catch (IllegalArgumentException exception) {
+            } catch (IllegalArgumentException | NullPointerException ex) {
               // optionValue is not a valid Starlark value, so skip this option.
+              // (e.g. tristate; a map with null values)
             }
           } catch (IllegalAccessException e) {
             throw new IllegalStateException(e);

src/main/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkData.java

@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.rules.android;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -343,7 +342,7 @@ public Dict<Provider, NativeInfo> processLocalTestData(
                       resourceConfigurationFilters, String.class, "resource_configuration_filters"),
                   Sequence.cast(densities, String.class, "densities")));
 
-      ImmutableMap.Builder<Provider, NativeInfo> builder = ImmutableMap.builder();
+      Dict.Builder<Provider, NativeInfo> builder = Dict.builder();
       builder.putAll(getNativeInfosFrom(resourceApk, ctx.getLabel()));
       builder.put(
           AndroidBinaryDataInfo.PROVIDER,
@@ -353,7 +352,7 @@ public Dict<Provider, NativeInfo> processLocalTestData(
               resourceApk.toResourceInfo(ctx.getLabel()),
               resourceApk.toAssetsInfo(ctx.getLabel()),
               resourceApk.toManifestInfo().get()));
-      return Dict.copyOf((Mutability) null, builder.build());
+      return builder.buildImmutable();
     } catch (RuleErrorException e) {
       throw handleRuleException(errorReporter, e);
     }
@@ -565,7 +564,7 @@ public AndroidBinaryDataInfo shrinkDataApk(
 
   public static Dict<Provider, NativeInfo> getNativeInfosFrom(
       ResourceApk resourceApk, Label label) {
-    ImmutableMap.Builder<Provider, NativeInfo> builder = ImmutableMap.builder();
+    Dict.Builder<Provider, NativeInfo> builder = Dict.builder();
 
     builder
         .put(AndroidResourcesInfo.PROVIDER, resourceApk.toResourceInfo(label))
@@ -578,7 +577,7 @@ public static Dict<Provider, NativeInfo> getNativeInfosFrom(
         getJavaInfoForRClassJar(
             resourceApk.getResourceJavaClassJar(), resourceApk.getResourceJavaSrcJar()));
 
-    return Dict.copyOf((Mutability) null, builder.build());
+    return builder.buildImmutable();
   }
 
   private static JavaInfo getJavaInfoForRClassJar(Artifact rClassJar, Artifact rClassSrcJar) {