diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java index a7783ad449947b..bedb2465138c39 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java @@ -576,12 +576,12 @@ public Dict getExecutionInfoDict() { if (executionInfo == null) { return null; } - return Dict.copyOf(null, executionInfo); + return Dict.immutableCopyOf(executionInfo); } @Override public Dict getEnv() { - return Dict.copyOf(null, env.getFixedEnv().toMap()); + return Dict.immutableCopyOf(env.getFixedEnv().toMap()); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ActionsProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ActionsProvider.java index 242d3545236138..f03d1181f1ac98 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ActionsProvider.java +++ b/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 actions) { } } ImmutableMap fields = - ImmutableMap.of("by_file", Starlark.fromJava(map, /*mutability=*/ null)); + ImmutableMap.of("by_file", Dict.immutableCopyOf(map)); return StarlarkInfo.create(INSTANCE, fields, Location.BUILTIN); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java index 3b19eb80d8fa60..bd6162ebfd96af 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java +++ b/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 collectMakeVariables() throws ExpansionException { - Map map = new LinkedHashMap<>(); + Dict.Builder 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.copyOf(null, map); + return map.buildImmutable(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java index b92079b425f781..ca2538503415bf 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionAction.java +++ b/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 getStarlarkSubstitutions() { - ImmutableMap.Builder builder = ImmutableMap.builder(); + Dict.Builder builder = Dict.builder(); for (Substitution entry : substitutions) { builder.put(entry.getKey(), entry.getValue()); } - return Dict.copyOf(null, builder.build()); + return builder.buildImmutable(); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/ConfiguredTargetsUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/ConfiguredTargetsUtil.java index 5c3ddfa0cae688..c5525656625e22 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/ConfiguredTargetsUtil.java +++ b/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 getProvidersDict( AbstractConfiguredTarget target, TransitiveInfoProviderMap providers) { - LinkedHashMap res = new LinkedHashMap<>(); + Dict.Builder 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 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(); } } 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 fc30badc887a83..b086acab378580 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 @@ -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 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); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributesCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributesCollection.java index ab0dbfa24b3415..84e376a57c0a70 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributesCollection.java +++ b/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; 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 builder = ImmutableMap.builder(); + Dict.Builder builder = Dict.builder(); Map original = BuildType.LABEL_KEYED_STRING_DICT.cast(val); List 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 prereqsByLabel = new LinkedHashMap<>(); for (TransitiveInfoCollection target : diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java index 079ab045552a43..0eb4d3e380cdab 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java +++ b/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( 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 4bf58c6a804c86..a0f4de28082593 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 @@ -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( diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index cbe91ba3a35800..81e55678537809 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -1020,7 +1020,7 @@ private StructImpl calculateDownloadResult(Optional checksum, Path dow Transience.PERSISTENT); } - ImmutableMap.Builder out = new ImmutableMap.Builder<>(); + Dict.Builder out = Dict.builder(); out.put("success", true); out.put("integrity", finalChecksum.toSubresourceIntegrity()); @@ -1028,7 +1028,7 @@ private StructImpl calculateDownloadResult(Optional checksum, Path dow 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 checkAllUrls(Iterable urlList) throws EvalException { diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index 3bc4a89bf9b2be..ae039c1c353dab 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/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 Attribute.Builder cloneBuilder(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}. + * + *

The conversion is similar to {@link Starlark#fromJava} for all types except {@code + * attr.string_list_dict} ({@code Map>}), for which fromJava does not + * recursively convert elements. (Doing so is expensive.) + * + *

It is tempting to require that attributes are stored internally in Starlark form. However, a + * number of obstacles would need to be overcome: + * + *

    + *
  1. Some obscure attribute types such as TRISTATE and DISTRIBUTION are not currently legal + * Starlark values. + *
  2. ImmutableList is significantly more compact than StarlarkList for small lists (n < 2). + * StarlarkList would need multiple representations and a builder to achieve parity. + *
  3. The types used by the Type mechanism would need changing; this has extensive + * ramifications. + *
+ */ + 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 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); + } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/ImplicitOutputsFunction.java b/src/main/java/com/google/devtools/build/lib/packages/ImplicitOutputsFunction.java index 5fa594a9b68362..b345cc48a5bced 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/ImplicitOutputsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/packages/ImplicitOutputsFunction.java @@ -96,8 +96,7 @@ public ImmutableMap 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 = diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java index f2f5deeb344199..20e385acb42a66 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java +++ b/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 m = new TreeMap<>(); + Dict.Builder 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 diff --git a/src/main/java/com/google/devtools/build/lib/packages/Type.java b/src/main/java/com/google/devtools/build/lib/packages/Type.java index 04d161863fd914..4f66ce3a4c01bb 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Type.java +++ b/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 { protected Type() {} diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/StarlarkOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/StarlarkOutputFormatterCallback.java index 7ed48e069e6ab8..c18c97593f5fda 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/cquery/StarlarkOutputFormatterCallback.java +++ b/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); diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkData.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkData.java index 77ced863dc03e1..bdda8ce14f9151 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidStarlarkData.java +++ b/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 processLocalTestData( resourceConfigurationFilters, String.class, "resource_configuration_filters"), Sequence.cast(densities, String.class, "densities"))); - ImmutableMap.Builder builder = ImmutableMap.builder(); + Dict.Builder builder = Dict.builder(); builder.putAll(getNativeInfosFrom(resourceApk, ctx.getLabel())); builder.put( AndroidBinaryDataInfo.PROVIDER, @@ -353,7 +352,7 @@ public Dict 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 getNativeInfosFrom( ResourceApk resourceApk, Label label) { - ImmutableMap.Builder builder = ImmutableMap.builder(); + Dict.Builder builder = Dict.builder(); builder .put(AndroidResourcesInfo.PROVIDER, resourceApk.toResourceInfo(label)) @@ -578,7 +577,7 @@ public static Dict getNativeInfosFrom( getJavaInfoForRClassJar( resourceApk.getResourceJavaClassJar(), resourceApk.getResourceJavaSrcJar())); - return Dict.copyOf((Mutability) null, builder.build()); + return builder.buildImmutable(); } private static JavaInfo getJavaInfoForRClassJar(Artifact rClassJar, Artifact rClassSrcJar) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfigInfo.java b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfigInfo.java index b440c9fb2dba96..08d9de28b55e53 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfigInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/apple/XcodeConfigInfo.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.rules.apple; import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; @@ -22,7 +21,6 @@ import com.google.devtools.build.lib.packages.NativeInfo; import com.google.devtools.build.lib.rules.apple.ApplePlatform.PlatformType; import com.google.devtools.build.lib.starlarkbuildapi.apple.XcodeConfigInfoApi; -import java.util.Map; import javax.annotation.Nullable; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; @@ -49,7 +47,7 @@ public class XcodeConfigInfo extends NativeInfo private final DottedVersion macosMinimumOsVersion; @Nullable private final DottedVersion xcodeVersion; @Nullable private final Availability availability; - @Nullable private final Map executionRequirements; + @Nullable private final Dict executionRequirements; // immutable public XcodeConfigInfo( DottedVersion iosSdkVersion, @@ -74,7 +72,7 @@ public XcodeConfigInfo( this.xcodeVersion = xcodeVersion; this.availability = availability; - ImmutableMap.Builder builder = ImmutableMap.builder(); + Dict.Builder builder = Dict.builder(); builder.put(ExecutionRequirements.REQUIRES_DARWIN, ""); switch (availability) { case LOCAL: @@ -87,7 +85,7 @@ public XcodeConfigInfo( break; } builder.put(ExecutionRequirements.REQUIREMENTS_SET, ""); - this.executionRequirements = builder.build(); + this.executionRequirements = builder.buildImmutable(); } /** Indicates the platform(s) on which an Xcode version is available. */ @@ -227,13 +225,13 @@ public String getAvailabilityString() { } /** Returns the execution requirements for actions that use this Xcode version. */ - public Map getExecutionRequirements() { + public Dict getExecutionRequirements() { return executionRequirements; } @Override public Dict getExecutionRequirementsDict() { - return Dict.copyOf(null, executionRequirements); + return executionRequirements; } public static XcodeConfigInfo fromRuleContext(RuleContext ruleContext) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java index 591d9ff5e24fce..8996be0faaf552 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java @@ -222,8 +222,7 @@ public Dict getEnvironmentVariable( String actionName, CcToolchainVariables variables) throws EvalException { - return Dict.copyOf( - null, + return Dict.immutableCopyOf( featureConfiguration .getFeatureConfiguration() .getEnvironmentVariables(actionName, variables)); diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleDebugOutputsInfo.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleDebugOutputsInfo.java index abeb755c59868c..ccda4f69128661 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleDebugOutputsInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleDebugOutputsInfo.java @@ -22,6 +22,7 @@ import com.google.devtools.build.lib.starlarkbuildapi.apple.AppleDebugOutputsApi; import java.util.HashMap; import java.util.Map; +import net.starlark.java.eval.Dict; /** * A provider that holds debug outputs of an Apple binary rule. @@ -66,7 +67,7 @@ public String toString() { public static final NativeProvider STARLARK_CONSTRUCTOR = new NativeProvider(AppleDebugOutputsInfo.class, STARLARK_NAME) {}; - private final ImmutableMap> outputsMap; + private final ImmutableMap> outputsMap; /** * Creates a new provider instance. @@ -83,16 +84,14 @@ public String toString() { *
  • output_type - an instance of {@link OutputType} * */ - private AppleDebugOutputsInfo(ImmutableMap> map) { + private AppleDebugOutputsInfo(ImmutableMap> map) { super(STARLARK_CONSTRUCTOR); this.outputsMap = map; } - /** - * Returns the multi-architecture dylib binary that apple_binary created. - */ + /** Returns the multi-architecture dylib binary that apple_binary created. */ @Override - public ImmutableMap> getOutputsMap() { + public ImmutableMap> getOutputsMap() { return outputsMap; } @@ -122,10 +121,10 @@ public Builder addOutput(String arch, OutputType outputType, Artifact artifact) } public AppleDebugOutputsInfo build() { - ImmutableMap.Builder> builder = ImmutableMap.builder(); + ImmutableMap.Builder> builder = ImmutableMap.builder(); for (Map.Entry> e : outputsByArch.entrySet()) { - builder.put(e.getKey(), ImmutableMap.copyOf(e.getValue())); + builder.put(e.getKey(), Dict.immutableCopyOf(e.getValue())); } return new AppleDebugOutputsInfo(builder.build()); diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleDebugOutputsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleDebugOutputsApi.java index 1889b5e0c28465..653c2bdd4f72fa 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleDebugOutputsApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/AppleDebugOutputsApi.java @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.starlarkbuildapi.core.StructApi; import net.starlark.java.annot.StarlarkBuiltin; import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.Dict; /** A provider that holds debug outputs of an apple_binary target. */ @StarlarkBuiltin( @@ -36,5 +37,5 @@ public interface AppleDebugOutputsApi extends StructApi { + " is any Apple architecture such as 'arm64' or 'armv7', 'output_type' is a string" + " descriptor such as 'bitcode_symbols' or 'dsym_binary', and the file is the file" + " matching that descriptor for that architecture.") - ImmutableMap> getOutputsMap(); + ImmutableMap> getOutputsMap(); } diff --git a/src/main/java/net/starlark/java/eval/Dict.java b/src/main/java/net/starlark/java/eval/Dict.java index 4b8a46a9cab79a..170f7ee58466b7 100644 --- a/src/main/java/net/starlark/java/eval/Dict.java +++ b/src/main/java/net/starlark/java/eval/Dict.java @@ -18,6 +18,8 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Iterator; @@ -389,16 +391,9 @@ public static Dict copyOf(@Nullable Mutability mu, Map dict = new Dict<>(mu); for (Map.Entry e : m.entrySet()) { - @SuppressWarnings("unchecked") - V v = (V) Starlark.fromJava(e.getValue(), mu); - dict.contents.put(e.getKey(), v); + dict.contents.put(Starlark.checkValid(e.getKey()), Starlark.checkValid(e.getValue())); } return dict; } @@ -408,6 +403,54 @@ public static Dict immutableCopyOf(Map m) return copyOf(null, m); } + /** Returns a new empty Dict.Builder. */ + public static Builder builder() { + return new Builder<>(); + } + + /** A reusable builder for Dicts. */ + public static final class Builder { + private final ArrayList items = new ArrayList<>(); // [k, v, ... k, v] + + /** Adds an entry (k, v) to the builder, overwriting any previous entry with the same key . */ + public Builder put(K k, V v) { + items.add(Starlark.checkValid(k)); + items.add(Starlark.checkValid(v)); + return this; + } + + /** Adds all the map's entries to the builder. */ + public Builder putAll(Map map) { + items.ensureCapacity(items.size() + 2 * map.size()); + for (Map.Entry e : map.entrySet()) { + put(e.getKey(), e.getValue()); + } + return this; + } + + /** Returns a new immutable Dict containing the entries added so far. */ + public Dict buildImmutable() { + return build(null); + } + + /** + * Returns a new Dict containing the entries added so far. The result has the specified + * mutability; null means immutable. + */ + public Dict build(@Nullable Mutability mu) { + int n = items.size() / 2; + LinkedHashMap map = Maps.newLinkedHashMapWithExpectedSize(n); + for (int i = 0; i < n; i++) { + @SuppressWarnings("unchecked") + K k = (K) items.get(2 * i); // safe + @SuppressWarnings("unchecked") + V v = (V) items.get(2 * i + 1); // safe + map.put(k, v); + } + return wrap(mu, map); + } + } + /** Puts the given entry into the dict, without calling {@link #checkMutable}. */ private Dict putUnsafe(K k, V v) { contents.put(k, v); diff --git a/src/main/java/net/starlark/java/eval/Starlark.java b/src/main/java/net/starlark/java/eval/Starlark.java index e874cc3e5c8f0e..a82b45386f0ad4 100644 --- a/src/main/java/net/starlark/java/eval/Starlark.java +++ b/src/main/java/net/starlark/java/eval/Starlark.java @@ -162,6 +162,9 @@ public static void checkHashable(Object x) throws EvalException { * null becomes {@link #NONE}. Any other non-Starlark value causes the function to throw * IllegalArgumentException. * + *

    Elements of Lists and Maps must be valid Starlark values; they are not recursively + * converted. (This avoids excessive unintended deep copying.) + * *

    This function is applied to the results of StarlarkMethod-annotated Java methods. */ public static Object fromJava(Object x, @Nullable Mutability mutability) { diff --git a/src/test/java/net/starlark/java/eval/StarlarkEvaluationTest.java b/src/test/java/net/starlark/java/eval/StarlarkEvaluationTest.java index 5120a5a6efd769..34acdb1fed5130 100644 --- a/src/test/java/net/starlark/java/eval/StarlarkEvaluationTest.java +++ b/src/test/java/net/starlark/java/eval/StarlarkEvaluationTest.java @@ -214,7 +214,7 @@ public String string() { @StarlarkMethod(name = "string_list_dict", documented = false) public Map> stringListDict() { - return ImmutableMap.of("a", ImmutableList.of("b", "c")); + return ImmutableMap.of("a", StarlarkList.immutableOf("b", "c")); } @StarlarkMethod( diff --git a/src/test/java/net/starlark/java/eval/StarlarkMutableTest.java b/src/test/java/net/starlark/java/eval/StarlarkMutableTest.java index 451f3e2a508992..6e8e9bf11c1e7b 100644 --- a/src/test/java/net/starlark/java/eval/StarlarkMutableTest.java +++ b/src/test/java/net/starlark/java/eval/StarlarkMutableTest.java @@ -14,6 +14,7 @@ package net.starlark.java.eval; +import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; @@ -67,7 +68,11 @@ public void testListViewsCheckMutability() throws Exception { @Test public void testDictViewsCheckMutability() throws Exception { Mutability mutability = Mutability.create("test"); - Dict dict = Dict.copyOf(mutability, ImmutableMap.of(1, 2, 3, 4)); + Dict dict = + Dict.copyOf( + mutability, + ImmutableMap.of( + StarlarkInt.of(1), StarlarkInt.of(2), StarlarkInt.of(3), StarlarkInt.of(4))); mutability.freeze(); { @@ -92,4 +97,45 @@ public void testDictViewsCheckMutability() throws Exception { () -> it.remove()); } } + + @Test + public void testDictBuilder() throws Exception { + // put + Dict dict1 = + Dict.builder() + .put("one", "1") + .put("two", "2.0") + .put("two", "2") // overrwrites previous entry + .put("three", "3") + .buildImmutable(); + assertThat(dict1.toString()).isEqualTo("{\"one\": \"1\", \"two\": \"2\", \"three\": \"3\"}"); + assertThrows(EvalException.class, dict1::clearEntries); // immutable + + // putAll + Dict dict2 = + Dict.builder() + .putAll(dict1) + .putAll(ImmutableMap.of("four", "4", "five", "5")) + .buildImmutable(); + assertThat(dict2.toString()) + .isEqualTo( + "{\"one\": \"1\", \"two\": \"2\", \"three\": \"3\", \"four\": \"4\", \"five\": \"5\"}"); + + // builder reuse and mutability + Dict.Builder builder = Dict.builder().putAll(dict1); + Mutability mu = Mutability.create("test"); + Dict dict3 = builder.build(mu); + dict3.putEntry("four", "4"); + assertThat(dict3.toString()) + .isEqualTo("{\"one\": \"1\", \"two\": \"2\", \"three\": \"3\", \"four\": \"4\"}"); + mu.close(); + assertThrows(EvalException.class, dict1::clearEntries); // frozen + builder.put("five", "5"); // keep building + Dict dict4 = builder.buildImmutable(); + assertThat(dict4.toString()) + .isEqualTo("{\"one\": \"1\", \"two\": \"2\", \"three\": \"3\", \"five\": \"5\"}"); + assertThat(dict3.toString()) + .isEqualTo( + "{\"one\": \"1\", \"two\": \"2\", \"three\": \"3\", \"four\": \"4\"}"); // unchanged + } }