bazel packages: simplify SkylarkInfo
Previously, SkylarkInfo had one of two forms:
- schemaful, which held an array of values and a reference to
a Layout, which mapped keys to small integers; and
- schemaless, which held an ImmutableSortedMap, which is
internally a 7-word structure holding separate lists of
keys and values, doubly indirect.
This change replaces both by a simpler representation:
a single table of keys followed by values, with the keys sorted.
Lookup involves binary search over the key portion.
The table is more compact than the schemaless implementation,
and despite having O(log)-time lookup, there are fewer cache
misses than in a hash table.
It may be more compact even than the schemaful implementation,
because it doesn't use space for fields that are not defined.
Also:
- remove unnecessary Attribute.getSkylarkValue key munging
done by the old copyValues function.
This required fixing one caller (WorkspaceFactory).
- remove unnecessary Starlark.fromJava value munging
done by the old copyValues function.
This requires fixing four callers.
- assert validity of values.
- delete Layout
- optimize struct + struct to make only one pass and one allocation.
- rename createSchema{ful,less} to create.
- delete createEmpty
RELNOTES: N/A
PiperOrigin-RevId: 284810601
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 0e32ea5..efd0741 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
@@ -21,6 +21,7 @@
import com.google.devtools.build.lib.packages.SkylarkInfo;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.skylarkbuildapi.ActionsInfoProviderApi;
+import com.google.devtools.build.lib.syntax.Starlark;
import java.util.HashMap;
import java.util.Map;
@@ -45,12 +46,11 @@
for (Artifact artifact : action.getOutputs()) {
// In the case that two actions generated the same artifact, the first wins. They
// ought to be equal anyway.
- if (!map.containsKey(artifact)) {
- map.put(artifact, action);
- }
+ map.putIfAbsent(artifact, action);
}
}
- ImmutableMap<String, Object> fields = ImmutableMap.<String, Object>of("by_file", map);
- return SkylarkInfo.createSchemaless(INSTANCE, fields, Location.BUILTIN);
+ ImmutableMap<String, Object> fields =
+ ImmutableMap.<String, Object>of("by_file", Starlark.fromJava(map, /*mutability=*/ null));
+ return SkylarkInfo.create(INSTANCE, fields, Location.BUILTIN);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java
index cb81e4e..c39db2a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java
@@ -201,7 +201,12 @@
}
filesBuilder.put(
skyname,
- context.getRuleContext().getPrerequisiteArtifacts(a.getName(), Mode.DONT_CHECK).list());
+ StarlarkList.copyOf(
+ /*mutability=*/ null,
+ context
+ .getRuleContext()
+ .getPrerequisiteArtifacts(a.getName(), Mode.DONT_CHECK)
+ .list()));
if (type == BuildType.LABEL && !a.getTransitionFactory().isSplit()) {
Object prereq = context.getRuleContext().getPrerequisite(a.getName(), Mode.DONT_CHECK);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
index cc7e2c5..da65072 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -95,6 +95,7 @@
import com.google.devtools.build.lib.syntax.StarlarkThread;
import com.google.devtools.build.lib.util.FileTypeSet;
import com.google.devtools.build.lib.util.Pair;
+import java.util.Collection;
import java.util.Map;
import java.util.concurrent.ExecutionException;
@@ -252,8 +253,8 @@
@Override
public Provider provider(String doc, Object fields, Location location) throws EvalException {
- Iterable<String> fieldNames = null;
- if (fields instanceof Sequence<?>) {
+ Collection<String> fieldNames = null;
+ if (fields instanceof Sequence) {
@SuppressWarnings("unchecked")
Sequence<String> list =
(Sequence<String>)
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
index 8247ec4..d738760 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
@@ -44,6 +44,7 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
+import com.google.devtools.build.lib.packages.SkylarkInfo;
import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.packages.StructProvider;
@@ -324,7 +325,8 @@
SkylarkRuleContext context, RuleConfiguredTargetBuilder builder, Object target, Location loc)
throws EvalException {
- StructImpl oldStyleProviders = StructProvider.STRUCT.createEmpty(loc);
+ StructImpl oldStyleProviders =
+ SkylarkInfo.create(StructProvider.STRUCT, ImmutableMap.of(), loc);
Map<Provider.Key, Info> declaredProviders = new LinkedHashMap<>();
if (target instanceof Info) {
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 9bd5b11..76332bb 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
@@ -1480,7 +1480,9 @@
if (!attr.hasComputedDefault()) {
Object value = rule.get(attrName, attr.getType());
if (!EvalUtils.isNullOrNone(value)) {
- attrValues.put(attr.getName(), value);
+ // Some attribute values are not valid Starlark values:
+ // visibility is an ImmutableList, for example.
+ attrValues.put(attr.getName(), Starlark.fromJava(value, /*mutability=*/ 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 fa90b26..b073015 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
@@ -99,7 +99,8 @@
// since we don't yet have a build configuration.
if (!map.isConfigurable(attrName)) {
Object value = map.get(attrName, attrType);
- attrValues.put(attrName, value == null ? Starlark.NONE : value);
+ attrValues.put(
+ Attribute.getSkylarkName(attrName), Starlark.fromJava(value, /*mutability=*/ null));
}
}
ClassObject attrs =
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
index 8690f0c..77ca87d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
@@ -14,416 +14,237 @@
package com.google.devtools.build.lib.packages;
-import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableCollection;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSortedMap;
-import com.google.common.collect.Sets;
-import com.google.common.collect.Sets.SetView;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.syntax.ClassObject;
import com.google.devtools.build.lib.syntax.Concatable;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
-import com.google.devtools.build.lib.syntax.SkylarkClassObject;
import com.google.devtools.build.lib.syntax.Starlark;
-import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import java.util.Arrays;
+import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
-/**
- * A standard implementation for provider instances.
- *
- * <p>Instances may be either schemaless or schemaful (corresponding to the two different concrete
- * implementing classes). Schemaless instances are map-based, while schemaful instances have a fixed
- * layout and array and are therefore more efficient.
- */
-public abstract class SkylarkInfo extends StructImpl implements Concatable, SkylarkClassObject {
+/** An Info (provider instance) for providers defined in Starlark. */
+public final class SkylarkInfo extends StructImpl implements Concatable, ClassObject {
- // Private because this should not be subclassed outside this file.
- private SkylarkInfo(Provider provider, @Nullable Location loc) {
+ // For a n-element info, the table contains n key strings, sorted,
+ // followed by the n corresponding legal Starlark values.
+ private final Object[] table;
+
+ // A format string with one %s placeholder for the missing field name.
+ // If null, uses the default format specified by the provider.
+ // TODO(adonovan): make the provider determine the error message
+ // (but: this has implications for struct+struct, the equivalence
+ // relation, and other observable behaviors).
+ @Nullable private final String unknownFieldError;
+
+ private SkylarkInfo(
+ Provider provider,
+ Object[] table,
+ @Nullable Location loc,
+ @Nullable String unknownFieldError) {
super(provider, loc);
+ this.table = table;
+ this.unknownFieldError = unknownFieldError;
}
- @Override
- public Concatter getConcatter() {
- return SkylarkInfoConcatter.INSTANCE;
- }
-
- @Override
- public boolean isImmutable() {
- // If the provider is not yet exported, the hash code of the object is subject to change.
- if (!getProvider().isExported()) {
- return false;
+ // Converts a map to a table of sorted keys followed by corresponding values.
+ private static Object[] toTable(Map<String, Object> values) {
+ int n = values.size();
+ Object[] table = new Object[n + n];
+ int i = 0;
+ for (String key : values.keySet()) {
+ table[i++] = key;
}
- // TODO(bazel-team): If we export at the end of a full module's evaluation, instead of at the
- // end of every top-level statement, then we can assume that exported implies frozen, and just
- // return true here without a traversal.
- for (Object item : getValues()) {
- if (item != null && !EvalUtils.isImmutable(item)) {
- return false;
- }
+ Arrays.sort(table, 0, n);
+ for (i = 0; i < n; i++) {
+ Object x = values.get(table[i]);
+ table[n + i] = Starlark.checkValid(x);
}
- return true;
+ return table;
}
/**
- * Returns all the field values stored in the object, in the canonical order.
- *
- * <p>{@code protected} because this is only used for {@link #isImmutable}. It saves us having to
- * get values one-by-one.
+ * Constructs a SkylarkInfo from a list of (possibly null but otherwise legal) Starlark values
+ * corresponding to a list of field names, which must be sorted. This exists solely for the
+ * SkylarkProvider constructor form {@code p(a=..., b=...)}.
*/
- protected abstract Iterable<Object> getValues();
+ static SkylarkInfo fromSortedFieldList(
+ Provider provider,
+ ImmutableList<String> sortedFieldNames,
+ Object[] values,
+ @Nullable Location loc) {
+ // Count non-null values.
+ int n = 0;
+ for (Object x : values) {
+ if (x != null) {
+ n++;
+ }
+ }
+
+ // Copy the non-null values and their keys.
+ Object[] table = new Object[n + n];
+ for (int i = 0, j = 0; i < values.length; i++) {
+ Object x = values[i];
+ if (x != null) {
+ table[j] = sortedFieldNames.get(i);
+ table[n + j] = Starlark.checkValid(x); // redundant defensive check
+ if (++j == n) {
+ break; // remaining values are all null
+ }
+ }
+ }
+
+ return new SkylarkInfo(provider, table, loc, /*unknownFieldError=*/ null);
+ }
+
+ @Override
+ public ImmutableCollection<String> getFieldNames() {
+ // TODO(adonovan): opt: can we avoid allocating three objects?
+ @SuppressWarnings("unchecked")
+ List<String> keys = (List<String>) (List<?>) Arrays.asList(table).subList(0, table.length / 2);
+ return ImmutableList.copyOf(keys);
+ }
/**
* Returns the custom (i.e. per-instance, as opposed to per-provider-type) error message string
* format used by this provider instance, or null if not set.
*/
@Nullable
- public abstract String getCustomErrorMessageFormatForUnknownField();
-
- /** Returns the layout for this provider if it is schemaful, null otherwise. */
- @Nullable
- public abstract Layout getLayout();
-
- /** Returns true if this provider is schemaful (array-based), false otherwise. */
- public boolean isCompact() {
- return getLayout() != null;
+ @Override
+ protected String getErrorMessageFormatForUnknownField() {
+ return unknownFieldError != null
+ ? unknownFieldError
+ : super.getErrorMessageFormatForUnknownField();
}
@Override
- public Object getValue(Location loc, StarlarkSemantics starlarkSemantics, String name)
- throws EvalException {
- // By default, a SkylarkInfo's field values are not affected by the Starlark semantics.
- return getValue(name);
+ public boolean isImmutable() {
+ // If the provider is not yet exported, the hash code of the object is subject to change.
+ // TODO(adonovan): implement isHashable?
+ if (!getProvider().isExported()) {
+ return false;
+ }
+ // TODO(bazel-team): If we export at the end of a full module's evaluation, instead of at the
+ // end of every top-level statement, then we can assume that exported implies frozen, and just
+ // return true here without a traversal.
+ for (int i = table.length / 2; i < table.length; i++) {
+ if (!EvalUtils.isImmutable(table[i])) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ @Override
+ public Object getValue(String name) {
+ int n = table.length / 2;
+ int i = Arrays.binarySearch(table, 0, n, name);
+ if (i < 0) {
+ return null;
+ }
+ return table[n + i];
}
/**
- * Creates a schemaless (map-based) provider instance with the given provider type and field
- * values.
+ * Creates a schemaless provider instance with the given provider type and field values.
*
* <p>{@code loc} is the creation location for this instance. Built-in provider instances may use
* {@link Location#BUILTIN}, which is the default if null.
*/
- public static SkylarkInfo createSchemaless(
+ public static SkylarkInfo create(
Provider provider, Map<String, Object> values, @Nullable Location loc) {
- return new MapBackedSkylarkInfo(
- provider, values, loc, /*errorMessageFormatForUnknownField=*/ null);
+ return new SkylarkInfo(provider, toTable(values), loc, /*unknownFieldError=*/ null);
}
/**
- * Creates a schemaless (map-based) provider instance with the given provider type, field values,
- * and unknown-field error message.
+ * Creates a schemaless provider instance with the given provider type, field values, and
+ * unknown-field error message.
*
- * <p>This is used to create structs for special purposes, such as {@code ctx.attr} and the
- * {@code native} module. The creation location will be {@link Location#BUILTIN}.
+ * <p>This is used to create structs for special purposes, such as {@code ctx.attr} and the {@code
+ * native} module. The creation location will be {@link Location#BUILTIN}.
*
- * <p>{@code errorMessageFormatForUnknownField} is a string format, as for {@link
+ * <p>{@code unknownFieldError} is a string format, as for {@link
* Provider#getErrorMessageFormatForUnknownField}.
*
- * <p>It is preferred to not use this method. Instead, create a new subclass of {@link
- * NativeProvider} with the desired error message format, and create a corresponding {@link
- * NativeInfo} subclass.
+ * @deprecated Do not use this method. Instead, create a new subclass of {@link NativeProvider}
+ * with the desired error message format, and create a corresponding {@link NativeInfo}
+ * subclass.
*/
// TODO(bazel-team): Make the special structs that need a custom error message use a different
// provider (subclassing NativeProvider) and a different StructImpl implementation. Then remove
// this functionality, thereby saving a string pointer field for the majority of providers that
// don't need it.
- public static SkylarkInfo createSchemalessWithCustomMessage(
- Provider provider, Map<String, Object> values, String errorMessageFormatForUnknownField) {
- Preconditions.checkNotNull(errorMessageFormatForUnknownField);
- return new MapBackedSkylarkInfo(
- provider, values, Location.BUILTIN, errorMessageFormatForUnknownField);
+ @Deprecated
+ public static SkylarkInfo createWithCustomMessage(
+ Provider provider, Map<String, Object> values, String unknownFieldError) {
+ Preconditions.checkNotNull(unknownFieldError);
+ return new SkylarkInfo(provider, toTable(values), Location.BUILTIN, unknownFieldError);
}
- /**
- * Creates a schemaful (array-based) provider instance with the given provider type, layout, and
- * values.
- *
- * <p>The order of the values must correspond to the given layout.
- *
- * <p>{@code loc} is the creation location for this instance. Built-in provider instances may use
- * {@link Location#BUILTIN}, which is the default if null.
- */
- public static SkylarkInfo createSchemaful(
- Provider provider,
- Layout layout,
- Object[] values,
- @Nullable Location loc) {
- return new CompactSkylarkInfo(provider, layout, values, loc);
+ @Override
+ public Concatter getConcatter() {
+ return SkylarkInfo::concat;
}
- /**
- * A specification of what fields a provider instance has, and how they are ordered in an
- * array-backed implementation.
- *
- * <p>The provider instance may only have fields that appear in its layout. Not all fields in the
- * layout need be present on the instance.
- */
- @Immutable
- @AutoCodec
- public static final class Layout {
-
- /**
- * A map from field names to a contiguous range of integers [0, n), ordered by integer value.
- */
- private final ImmutableMap<String, Integer> map;
-
- /**
- * Constructs a {@link Layout} from the given field names.
- *
- * <p>The order of the field names is preserved in the layout.
- *
- * @throws IllegalArgumentException if any field names are given more than once
- */
- public Layout(Iterable<String> fields) {
- this(makeMap(fields));
+ private static Concatable concat(Concatable left, Concatable right, Location loc)
+ throws EvalException {
+ // Casts are safe because this Concatter is only used by SkylarkInfo.
+ SkylarkInfo x = (SkylarkInfo) left;
+ SkylarkInfo y = (SkylarkInfo) right;
+ Provider xprov = x.getProvider();
+ Provider yprov = y.getProvider();
+ if (!xprov.equals(yprov)) {
+ throw new EvalException(
+ loc,
+ String.format(
+ "Cannot use '+' operator on instances of different providers (%s and %s)",
+ xprov.getPrintableName(), yprov.getPrintableName()));
}
- @AutoCodec.VisibleForSerialization
- @AutoCodec.Instantiator
- Layout(ImmutableMap<String, Integer> map) {
- this.map = map;
- }
-
- private static ImmutableMap<String, Integer> makeMap(Iterable<String> fields) {
- ImmutableMap.Builder<String, Integer> layoutBuilder = ImmutableMap.builder();
- int i = 0;
- for (String field : fields) {
- layoutBuilder.put(field, i++);
- }
- return layoutBuilder.build();
- }
-
- @Override
- public boolean equals(Object other) {
- if (!(other instanceof Layout)) {
- return false;
- }
- if (map == other) {
- return true;
- }
- return map.equals(((Layout) other).map);
- }
-
- @Override
- public int hashCode() {
- return map.hashCode();
- }
-
- /** Returns the number of fields in the layout. */
- public int size() {
- return map.size();
- }
-
- /**
- * Returns the index position associated with the given field, or null if the field is not
- * mentioned by the layout.
- */
- public Integer getFieldIndex(String field) {
- return map.get(field);
- }
-
- /** Returns the field names specified by this layout, in order. */
- public ImmutableCollection<String> getFields() {
- return map.keySet();
- }
-
- /** Returns the entry set of the underlying map, in order. */
- public ImmutableCollection<Map.Entry<String, Integer>> entrySet() {
- return map.entrySet();
- }
- }
-
- /** A {@link SkylarkInfo} implementation that stores its values in a map. */
- private static final class MapBackedSkylarkInfo extends SkylarkInfo {
- private final ImmutableSortedMap<String, Object> values;
-
- /**
- * Formattable string with one {@code '%s'} placeholder for the missing field name.
- *
- * <p>If null, uses the default format specified by the provider.
- */
- @Nullable
- private final String errorMessageFormatForUnknownField;
-
- MapBackedSkylarkInfo(
- Provider provider,
- Map<String, Object> values,
- @Nullable Location loc,
- @Nullable String errorMessageFormatForUnknownField) {
- super(provider, loc);
- // TODO(b/74396075): Phase out the unnecessary conversions done by this call to copyValues.
- this.values = copyValues(values);
- this.errorMessageFormatForUnknownField = errorMessageFormatForUnknownField;
- }
-
- /**
- * Preprocesses a map of field values to convert the field names and field values to
- * Skylark-acceptable names and types.
- *
- * <p>Entries are ordered by key.
- */
- private static ImmutableSortedMap<String, Object> copyValues(Map<String, Object> values) {
- ImmutableSortedMap.Builder<String, Object> builder = ImmutableSortedMap.naturalOrder();
- for (Map.Entry<String, Object> e : values.entrySet()) {
- builder.put(Attribute.getSkylarkName(e.getKey()), Starlark.fromJava(e.getValue(), null));
- }
- return builder.build();
- }
-
- @Override
- public Object getValue(String name) {
- return values.get(name);
- }
-
- @Override
- public ImmutableCollection<String> getFieldNames() {
- return values.keySet();
- }
-
- @Override
- protected Iterable<Object> getValues() {
- return values.values();
- }
-
- @Override
- protected String getErrorMessageFormatForUnknownField() {
- return errorMessageFormatForUnknownField != null
- ? errorMessageFormatForUnknownField : super.getErrorMessageFormatForUnknownField();
- }
-
- @Override
- public String getCustomErrorMessageFormatForUnknownField() {
- return errorMessageFormatForUnknownField;
- }
-
- @Override
- public Layout getLayout() {
- return null;
- }
- }
-
- /** A {@link SkylarkInfo} implementation that stores its values in array to save space. */
- private static final class CompactSkylarkInfo extends SkylarkInfo implements Concatable {
-
- private final Layout layout;
- /** Treated as immutable. */
- private final Object[] values;
-
- CompactSkylarkInfo(
- Provider provider,
- Layout layout,
- Object[] values,
- @Nullable Location loc) {
- super(provider, loc);
- this.layout = Preconditions.checkNotNull(layout);
- Preconditions.checkArgument(
- layout.size() == values.length,
- "Layout has length %s, but number of given values was %s", layout.size(), values.length);
- this.values = new Object[values.length];
- for (int i = 0; i < values.length; i++) {
- // TODO(b/74396075): Phase out this unnecessary conversion.
- // NB: fromJava treats null as None, but we need nulls to indicate a field is not present.
- if (values[i] != null) {
- this.values[i] = Starlark.fromJava(values[i], null);
- }
- }
- }
-
- @Override
- public Object getValue(String name) {
- Integer index = layout.getFieldIndex(name);
- if (index == null) {
- return null;
- }
- return values[index];
- }
-
- @Override
- public ImmutableCollection<String> getFieldNames() {
- ImmutableSet.Builder<String> result = ImmutableSet.builder();
- for (Map.Entry<String, Integer> entry : layout.entrySet()) {
- if (values[entry.getValue()] != null) {
- result.add(entry.getKey());
- }
- }
- return result.build();
- }
-
- @Override
- protected Iterable<Object> getValues() {
- return Arrays.asList(values);
- }
-
- @Override
- public String getCustomErrorMessageFormatForUnknownField() {
- return null;
- }
-
- @Override
- public Layout getLayout() {
- return layout;
- }
- }
-
- /** Concatter for concrete {@link SkylarkInfo} subclasses. */
- private static final class SkylarkInfoConcatter implements Concatable.Concatter {
- private static final SkylarkInfoConcatter INSTANCE = new SkylarkInfoConcatter();
-
- private SkylarkInfoConcatter() {}
-
- @Override
- public Concatable concat(Concatable left, Concatable right, Location loc) throws EvalException {
- // Casts are safe because SkylarkInfoConcatter is only used by SkylarkInfo.
- SkylarkInfo leftInfo = (SkylarkInfo) left;
- SkylarkInfo rightInfo = (SkylarkInfo) right;
- Provider provider = leftInfo.getProvider();
- if (!provider.equals(rightInfo.getProvider())) {
+ // ztable = merge(x.table, y.table)
+ int xsize = x.table.length / 2;
+ int ysize = y.table.length / 2;
+ int zsize = xsize + ysize;
+ Object[] ztable = new Object[zsize + zsize];
+ int xi = 0;
+ int yi = 0;
+ int zi = 0;
+ while (xi < xsize && yi < ysize) {
+ String xk = (String) x.table[xi];
+ String yk = (String) y.table[yi];
+ int cmp = xk.compareTo(yk);
+ if (cmp < 0) {
+ ztable[zi] = xk;
+ ztable[zi + zsize] = x.table[xi + xsize];
+ xi++;
+ } else if (cmp > 0) {
+ ztable[zi] = yk;
+ ztable[zi + zsize] = y.table[yi + ysize];
+ yi++;
+ } else {
throw new EvalException(
- loc,
- String.format(
- "Cannot use '+' operator on instances of different providers (%s and %s)",
- provider.getPrintableName(), rightInfo.getProvider().getPrintableName()));
+ loc, String.format("cannot add struct instances with common field '%s'", xk));
}
- SetView<String> commonFields =
- Sets.intersection(
- ImmutableSet.copyOf(leftInfo.getFieldNames()),
- ImmutableSet.copyOf(rightInfo.getFieldNames()));
- if (!commonFields.isEmpty()) {
- throw new EvalException(
- loc,
- "Cannot use '+' operator on provider instances with overlapping field(s): "
- + Joiner.on(",").join(commonFields));
- }
- // Keep homogeneous compact concatenations compact.
- if (leftInfo instanceof CompactSkylarkInfo && rightInfo instanceof CompactSkylarkInfo) {
- CompactSkylarkInfo compactLeft = (CompactSkylarkInfo) leftInfo;
- CompactSkylarkInfo compactRight = (CompactSkylarkInfo) rightInfo;
- Layout layout = compactLeft.layout;
- if (layout.equals(compactRight.layout)) {
- int nvals = layout.size();
- Object[] newValues = new Object[nvals];
- for (int i = 0; i < nvals; i++) {
- newValues[i] =
- (compactLeft.values[i] != null) ? compactLeft.values[i] : compactRight.values[i];
- }
- return createSchemaful(provider, layout, newValues, loc);
- }
- }
- // Fall back on making a map-based instance.
- ImmutableMap.Builder<String, Object> newValues = ImmutableMap.builder();
- for (String field : leftInfo.getFieldNames()) {
- newValues.put(field, leftInfo.getValue(field));
- }
- for (String field : rightInfo.getFieldNames()) {
- newValues.put(field, rightInfo.getValue(field));
- }
- return createSchemaless(provider, newValues.build(), loc);
+ zi++;
}
+ while (xi < xsize) {
+ ztable[zi] = x.table[xi];
+ ztable[zi + zsize] = x.table[xi + xsize];
+ xi++;
+ zi++;
+ }
+ while (yi < ysize) {
+ ztable[zi] = y.table[yi];
+ ztable[zi + zsize] = y.table[yi + ysize];
+ yi++;
+ zi++;
+ }
+
+ return new SkylarkInfo(xprov, ztable, loc, x.unknownFieldError);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java
index afa538a..99ed600 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java
@@ -14,12 +14,10 @@
package com.google.devtools.build.lib.packages;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.packages.SkylarkInfo.Layout;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.syntax.BaseFunction;
import com.google.devtools.build.lib.syntax.EvalException;
@@ -27,6 +25,7 @@
import com.google.devtools.build.lib.syntax.FunctionSignature;
import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.syntax.StarlarkThread;
+import java.util.Collection;
import java.util.Map;
import java.util.Objects;
import javax.annotation.Nullable;
@@ -39,8 +38,7 @@
*
* <p>{@code SkylarkProvider}s may be either schemaless or schemaful. Instances of schemaless
* providers can have any set of fields on them, whereas instances of schemaful providers may have
- * only the fields that are named in the schema. Schemaful provider instances are more space
- * efficient since they do not use maps; see {@link SkylarkInfo}.
+ * only the fields that are named in the schema.
*
* <p>Exporting a {@code SkylarkProvider} creates a key that is used to uniquely identify it.
* Usually a provider is exported by calling {@link #export}, but a test may wish to just create a
@@ -54,12 +52,11 @@
private final Location location;
- /**
- * For schemaful providers, a layout describing the allowed fields and their order in an
- * array-based representation. For schemaless providers, null.
- */
- @Nullable
- private final Layout layout;
+ /** For schemaful providers, the sorted list of allowed field names. */
+ // (The requirement for sortedness comes from SkylarkInfo.fromSortedFieldList,
+ // as it permits it to use the same list of names both to interpret the
+ // call arguments and to populate the SkylarkInfo.table without temporaries.)
+ @Nullable private final ImmutableList<String> fields;
/** Null iff this provider has not yet been exported. */
@Nullable
@@ -85,15 +82,15 @@
*
* <p>The resulting object needs to be exported later (via {@link #export}).
*
- * @param fields a list of allowed field names for instances of this provider, in some canonical
- * order
+ * @param fields the allowed field names for instances of this provider
* @param location the location of the Skylark definition for this provider (tests may use {@link
* Location#BUILTIN})
*/
+ // TODO(adonovan): in what sense is this "schemaful" if fields is null?
public static SkylarkProvider createUnexportedSchemaful(
- Iterable<String> fields, Location location) {
+ @Nullable Collection<String> fields, Location location) {
return new SkylarkProvider(
- /*key=*/ null, fields == null ? null : ImmutableList.copyOf(fields), location);
+ /*key=*/ null, fields == null ? null : ImmutableList.sortedCopyOf(fields), location);
}
/**
@@ -111,14 +108,15 @@
* Creates an exported {@link SkylarkProvider} with no schema.
*
* @param key the key that identifies this provider
- * @param fields a list of allowed field names for instances of this provider, in some canonical
- * order
+ * @param fields the allowed field names for instances of this provider
* @param location the location of the Skylark definition for this provider (tests may use {@link
* Location#BUILTIN})
*/
+ // TODO(adonovan): in what sense is this "schemaful" if fields is null?
public static SkylarkProvider createExportedSchemaful(
- SkylarkKey key, Iterable<String> fields, Location location) {
- return new SkylarkProvider(key, fields == null ? null : ImmutableList.copyOf(fields), location);
+ SkylarkKey key, @Nullable Collection<String> fields, Location location) {
+ return new SkylarkProvider(
+ key, fields == null ? null : ImmutableList.sortedCopyOf(fields), location);
}
/**
@@ -131,30 +129,32 @@
@Nullable SkylarkKey key, @Nullable ImmutableList<String> fields, Location location) {
super(buildSignature(fields), /*defaultValues=*/ null);
this.location = location;
- this.layout = fields == null ? null : new Layout(fields);
+ this.fields = fields;
this.key = key; // possibly null
this.errorMessageFormatForUnknownField =
key == null ? DEFAULT_ERROR_MESSAGE_FORMAT
: makeErrorMessageFormatForUnknownField(key.getExportedName());
}
- private static FunctionSignature buildSignature(@Nullable Iterable<String> fields) {
+ private static FunctionSignature buildSignature(@Nullable ImmutableList<String> fields) {
return fields == null
? FunctionSignature.KWARGS // schemaless
- : FunctionSignature.namedOnly(0, ImmutableList.copyOf(fields).toArray(new String[0]));
+ : FunctionSignature.namedOnly(0, fields.toArray(new String[0]));
}
@Override
protected Object call(Object[] args, @Nullable FuncallExpression ast, StarlarkThread thread)
throws EvalException, InterruptedException {
Location loc = ast != null ? ast.getLocation() : Location.BUILTIN;
- if (layout == null) {
+ if (fields == null) {
+ // provider(**kwargs)
@SuppressWarnings("unchecked")
Map<String, Object> kwargs = (Map<String, Object>) args[0];
- return SkylarkInfo.createSchemaless(this, kwargs, loc);
+ return SkylarkInfo.create(this, kwargs, loc);
} else {
- // Note: This depends on the layout map using the same ordering as args.
- return SkylarkInfo.createSchemaful(this, layout, args, loc);
+ // provider(a=..., b=..., ...)
+ // The order of args is determined by the signature: that is, fields, which is sorted.
+ return SkylarkInfo.fromSortedFieldList(this, fields, args, loc);
}
}
@@ -184,25 +184,10 @@
return getName();
}
- /**
- * Returns the list of fields used to define this provider, or null if the provider is schemaless.
- *
- * <p>Note: In the future, this method may be replaced by one that returns more detailed schema
- * information (if/when the allowed schemas for structs become more complex).
- */
+ /** Returns the list of fields allowed by this provider, or null if the provider is schemaless. */
@Nullable
public ImmutableList<String> getFields() {
- if (layout == null) {
- return null;
- }
- return ImmutableList.copyOf(layout.getFields());
- }
-
- /** Returns the layout, or null if the provider is schemaless. */
- @VisibleForTesting
- @Nullable
- Layout getLayout() {
- return layout;
+ return fields;
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java b/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java
index b5e1c91..998f632 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java
@@ -14,7 +14,6 @@
package com.google.devtools.build.lib.packages;
-import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skylarkbuildapi.core.StructApi;
import com.google.devtools.build.lib.syntax.Dict;
@@ -45,7 +44,7 @@
if (kwargsMap.containsKey("to_proto")) {
throw new EvalException(loc, "cannot override built-in struct function 'to_proto'");
}
- return SkylarkInfo.createSchemaless(this, kwargsMap, loc);
+ return SkylarkInfo.create(this, kwargsMap, loc);
}
/**
@@ -57,12 +56,6 @@
* */
public SkylarkInfo create(
Map<String, Object> values, String errorMessageFormatForUnknownField) {
- return SkylarkInfo.createSchemalessWithCustomMessage(
- this, values, errorMessageFormatForUnknownField);
- }
-
- /** Creates an empty struct with the given location. */
- public SkylarkInfo createEmpty(Location loc) {
- return SkylarkInfo.createSchemaless(this, ImmutableMap.of(), loc);
+ return SkylarkInfo.createWithCustomMessage(this, values, errorMessageFormatForUnknownField);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index 5506192..378623ec 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -357,13 +357,19 @@
ImmutableMap<String, Object> workspaceFunctions, String version) {
ImmutableMap.Builder<String, Object> env = new ImmutableMap.Builder<>();
Starlark.addMethods(env, new SkylarkNativeModule());
- for (Map.Entry<String, Object> function : workspaceFunctions.entrySet()) {
- // "workspace" is explicitly omitted from the native module, as it must only occur at the
- // top of a WORKSPACE file.
- // TODO(cparsons): Clean up separation between environments.
- if (!function.getKey().equals("workspace")) {
- env.put(function);
+ for (Map.Entry<String, Object> entry : workspaceFunctions.entrySet()) {
+ String name = entry.getKey();
+ if (name.startsWith("$")) {
+ // Skip "abstract" rules like "$go_rule".
+ continue;
}
+ // "workspace" is explicitly omitted from the native module,
+ // as it must only occur at the top of a WORKSPACE file.
+ // TODO(cparsons): Clean up separation between environments.
+ if (name.equals("workspace")) {
+ continue;
+ }
+ env.put(entry);
}
env.put("bazel_version", version);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/ApplePlatform.java b/src/main/java/com/google/devtools/build/lib/rules/apple/ApplePlatform.java
index d891fce..ce63586 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/apple/ApplePlatform.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/apple/ApplePlatform.java
@@ -184,7 +184,7 @@
for (ApplePlatform type : values()) {
fields.put(type.skylarkKey, type);
}
- return SkylarkInfo.createSchemaless(constructor, fields, Location.BUILTIN);
+ return SkylarkInfo.create(constructor, fields, Location.BUILTIN);
}
@Override
@@ -248,7 +248,7 @@
for (PlatformType type : values()) {
fields.put(type.skylarkKey, type);
}
- return SkylarkInfo.createSchemaless(constructor, fields, Location.BUILTIN);
+ return SkylarkInfo.create(constructor, fields, Location.BUILTIN);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleSkylarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleSkylarkCommon.java
index 9be7e79..b93aaad 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleSkylarkCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleSkylarkCommon.java
@@ -268,6 +268,6 @@
"binary_provider", output.getBinaryInfoProvider(),
"debug_outputs_provider", output.getDebugOutputsProvider(),
"output_groups", Dict.copyOf(thread.mutability(), outputGroups));
- return SkylarkInfo.createSchemaless(constructor, fields, Location.BUILTIN);
+ return SkylarkInfo.create(constructor, fields, Location.BUILTIN);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java b/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
index 94d8d79..f929813 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
@@ -19,7 +19,7 @@
/** An interface for Skylark values (such as structs) that have fields. */
// TODO(adonovan): rename "HasFields".
-public interface ClassObject {
+public interface ClassObject extends StarlarkValue {
/**
* Returns the value of the field with the given name, or null if the field does not exist.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkQueryable.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkQueryable.java
index fa9bdd5..66d0bba 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkQueryable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkQueryable.java
@@ -23,7 +23,7 @@
// TODO(adonovan): merge with SkylarkIndexable: no type supports 'x in y' without y[x],
// and 'x in y' can be defined in terms of y[x], at least as a default implementation.
// (Implementations of 'x in y' may choose to interpret failure of y[x] as false or a failure.)
-public interface SkylarkQueryable {
+public interface SkylarkQueryable extends StarlarkValue {
/** Returns whether the key is in the object. */
boolean containsKey(Object key, Location loc) throws EvalException;
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
index 511996e..2fa6234 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
@@ -17,15 +17,11 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
-import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.testing.EqualsTester;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.packages.SkylarkInfo.Layout;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.StarlarkValue;
-import java.util.Map;
import javax.annotation.Nullable;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -35,143 +31,58 @@
@RunWith(JUnit4.class)
public class SkylarkInfoTest {
- private static final Layout layoutF1F2 = new Layout(ImmutableList.of("f1", "f2"));
- private static final Layout invertedLayoutF2F1 = new Layout(ImmutableList.of("f2", "f1"));
-
- @Test
- public void layoutAccessors() {
- Layout layout = new Layout(ImmutableList.of("x", "y", "z"));
- assertThat(layout.size()).isEqualTo(3);
- assertThat(layout.getFieldIndex("a")).isNull();
- assertThat(layout.getFieldIndex("z")).isEqualTo(2);
- assertThat(layout.getFields()).containsExactly("x", "y", "z").inOrder();
- assertThat(
- layout.entrySet().stream()
- .map(Map.Entry::getKey)
- .collect(ImmutableList.toImmutableList()))
- .containsExactly("x", "y", "z").inOrder();
- }
-
- @Test
- public void layoutDisallowsDuplicates() {
- assertThrows(
- IllegalArgumentException.class,
- () -> new Layout(ImmutableList.of("x", "y", "x")));
- }
-
- @Test
- public void layoutEquality() {
- new EqualsTester()
- .addEqualityGroup(
- new Layout(ImmutableList.of("a", "b", "c")),
- new Layout(ImmutableList.of("a", "b", "c")))
- .addEqualityGroup(
- new Layout(ImmutableList.of("x", "y", "z")))
- .addEqualityGroup(
- new Layout(ImmutableList.of("c", "b", "a")))
- .testEquals();
- }
-
@Test
public void nullLocationDefaultsToBuiltin() throws Exception {
- SkylarkInfo info = SkylarkInfo.createSchemaless(makeProvider(), ImmutableMap.of(), null);
+ SkylarkInfo info = SkylarkInfo.create(makeProvider(), ImmutableMap.of(), null);
assertThat(info.getCreationLoc()).isEqualTo(Location.BUILTIN);
}
@Test
- public void givenLayoutTakesPrecedenceOverProviderLayout() throws Exception {
- SkylarkProvider provider =
- SkylarkProvider.createUnexportedSchemaful(ImmutableList.of("f1", "f2"), Location.BUILTIN);
- SkylarkInfo info =
- SkylarkInfo.createSchemaful(
- provider, invertedLayoutF2F1, new Object[]{5, 4}, Location.BUILTIN);
- assertThat(info.getLayout()).isEqualTo(invertedLayoutF2F1); // not the one in the provider
- }
-
- @Test
- public void schemafulValuesMustMatchLayoutArity() throws Exception {
- SkylarkProvider provider = makeProvider();
- IllegalArgumentException expected =
- assertThrows(
- IllegalArgumentException.class,
- () ->
- SkylarkInfo.createSchemaful(
- provider, layoutF1F2, new Object[] {4}, Location.BUILTIN));
- assertThat(expected).hasMessageThat()
- .contains("Layout has length 2, but number of given values was 1");
- }
-
- @Test
public void instancesOfUnexportedProvidersAreMutable() throws Exception {
SkylarkProvider provider = makeProvider();
- SkylarkInfo mapInfo = makeSchemalessInfoWithF1F2Values(provider, 5, null);
- SkylarkInfo compactInfo = makeSchemafulInfoWithF1F2Values(provider, 5, null);
- assertThat(mapInfo.isImmutable()).isFalse();
- assertThat(compactInfo.isImmutable()).isFalse();
+ SkylarkInfo info = makeInfoWithF1F2Values(provider, 5, null);
+ assertThat(info.isImmutable()).isFalse();
}
@Test
public void instancesOfExportedProvidersMayBeImmutable() throws Exception {
SkylarkProvider provider = makeExportedProvider();
- SkylarkInfo mapInfo = makeSchemalessInfoWithF1F2Values(provider, 5, null);
- SkylarkInfo compactInfo = makeSchemafulInfoWithF1F2Values(provider, 5, null);
- assertThat(mapInfo.isImmutable()).isTrue();
- assertThat(compactInfo.isImmutable()).isTrue();
+ SkylarkInfo info = makeInfoWithF1F2Values(provider, 5, null);
+ assertThat(info.isImmutable()).isTrue();
}
@Test
public void mutableIfContentsAreMutable() throws Exception {
SkylarkProvider provider = makeExportedProvider();
StarlarkValue v = new StarlarkValue() {};
- SkylarkInfo mapInfo = makeSchemalessInfoWithF1F2Values(provider, 5, v);
- SkylarkInfo compactInfo = makeSchemafulInfoWithF1F2Values(provider, 5, v);
- assertThat(mapInfo.isImmutable()).isFalse();
- assertThat(compactInfo.isImmutable()).isFalse();
+ SkylarkInfo info = makeInfoWithF1F2Values(provider, 5, v);
+ assertThat(info.isImmutable()).isFalse();
}
@Test
- public void equality_DifferentProviders() throws Exception {
+ public void equivalence() throws Exception {
SkylarkProvider provider1 = makeProvider();
SkylarkProvider provider2 = makeProvider();
- new EqualsTester()
- .addEqualityGroup(
- makeSchemalessInfoWithF1F2Values(provider1, 4, 5),
- makeSchemafulInfoWithF1F2Values(provider1, 4, 5),
- makeInvertedSchemafulInfoWithF1F2Values(provider1, 4, 5))
- .addEqualityGroup(
- makeSchemalessInfoWithF1F2Values(provider2, 4, 5),
- makeSchemafulInfoWithF1F2Values(provider2, 4, 5),
- makeInvertedSchemafulInfoWithF1F2Values(provider2, 4, 5))
- .testEquals();
- }
-
- @Test
- public void equality_DifferentValues() throws Exception {
- SkylarkProvider provider = makeProvider();
- // These comparisons include the case where the physical array is {4, 5} on both instances but
- // they compare different due to different layouts.
- new EqualsTester()
- .addEqualityGroup(
- makeSchemalessInfoWithF1F2Values(provider, 4, 5),
- makeSchemafulInfoWithF1F2Values(provider, 4, 5),
- makeInvertedSchemafulInfoWithF1F2Values(provider, 4, 5))
- .addEqualityGroup(
- makeSchemalessInfoWithF1F2Values(provider, 5, 4),
- makeSchemafulInfoWithF1F2Values(provider, 5, 4),
- makeInvertedSchemafulInfoWithF1F2Values(provider, 5, 4))
- .addEqualityGroup(
- makeSchemalessInfoWithF1F2Values(provider, 4, null),
- makeSchemafulInfoWithF1F2Values(provider, 4, null),
- makeInvertedSchemafulInfoWithF1F2Values(provider, 4, null))
- .testEquals();
+ // equal providers and fields
+ assertThat(makeInfoWithF1F2Values(provider1, 4, 5))
+ .isEqualTo(makeInfoWithF1F2Values(provider1, 4, 5));
+ // different providers => unequal
+ assertThat(makeInfoWithF1F2Values(provider1, 4, 5))
+ .isNotEqualTo(makeInfoWithF1F2Values(provider2, 4, 5));
+ // different fields => unequal
+ assertThat(makeInfoWithF1F2Values(provider1, 4, 5))
+ .isNotEqualTo(makeInfoWithF1F2Values(provider1, 4, 6));
+ // different sets of fields => unequal
+ assertThat(makeInfoWithF1F2Values(provider1, 4, 5))
+ .isNotEqualTo(makeInfoWithF1F2Values(provider1, 4, null));
}
@Test
public void concatWithDifferentProvidersFails() throws Exception {
SkylarkProvider provider1 = makeProvider();
SkylarkProvider provider2 = makeProvider();
- SkylarkInfo info1 = makeSchemalessInfoWithF1F2Values(provider1, 4, 5);
- SkylarkInfo info2 = makeSchemalessInfoWithF1F2Values(provider2, 4, 5);
+ SkylarkInfo info1 = makeInfoWithF1F2Values(provider1, 4, 5);
+ SkylarkInfo info2 = makeInfoWithF1F2Values(provider2, 4, 5);
EvalException expected =
assertThrows(
EvalException.class, () -> info1.getConcatter().concat(info1, info2, Location.BUILTIN));
@@ -182,46 +93,33 @@
@Test
public void concatWithOverlappingFieldsFails() throws Exception {
SkylarkProvider provider1 = makeProvider();
- SkylarkInfo info1 = makeSchemalessInfoWithF1F2Values(provider1, 4, 5);
- SkylarkInfo info2 = makeSchemalessInfoWithF1F2Values(provider1, 4, null);
+ SkylarkInfo info1 = makeInfoWithF1F2Values(provider1, 4, 5);
+ SkylarkInfo info2 = makeInfoWithF1F2Values(provider1, 4, null);
EvalException expected =
assertThrows(
EvalException.class, () -> info1.getConcatter().concat(info1, info2, Location.BUILTIN));
- assertThat(expected).hasMessageThat()
- .contains("Cannot use '+' operator on provider instances with overlapping field(s): f1");
+ assertThat(expected)
+ .hasMessageThat()
+ .contains("cannot add struct instances with common field 'f1'");
}
@Test
- public void compactConcatReturnsCompact() throws Exception {
+ public void concatWithSameFields() throws Exception {
SkylarkProvider provider = makeProvider();
- SkylarkInfo info1 = makeSchemafulInfoWithF1F2Values(provider, 4, null);
- SkylarkInfo info2 = makeSchemafulInfoWithF1F2Values(provider, null, 5);
+ SkylarkInfo info1 = makeInfoWithF1F2Values(provider, 4, null);
+ SkylarkInfo info2 = makeInfoWithF1F2Values(provider, null, 5);
SkylarkInfo result = (SkylarkInfo) info1.getConcatter().concat(info1, info2, Location.BUILTIN);
- assertThat(result.isCompact()).isTrue();
assertThat(result.getFieldNames()).containsExactly("f1", "f2");
assertThat(result.getValue("f1")).isEqualTo(4);
assertThat(result.getValue("f2")).isEqualTo(5);
}
@Test
- public void compactConcatWithDifferentLayoutsReturnsMap() throws Exception {
+ public void concatWithDifferentFields() throws Exception {
SkylarkProvider provider = makeProvider();
- SkylarkInfo info1 = makeSchemafulInfoWithF1F2Values(provider, 4, null);
- SkylarkInfo info2 = makeInvertedSchemafulInfoWithF1F2Values(provider, null, 5);
+ SkylarkInfo info1 = makeInfoWithF1F2Values(provider, 4, null);
+ SkylarkInfo info2 = makeInfoWithF1F2Values(provider, null, 5);
SkylarkInfo result = (SkylarkInfo) info1.getConcatter().concat(info1, info2, Location.BUILTIN);
- assertThat(result.isCompact()).isFalse();
- assertThat(result.getFieldNames()).containsExactly("f1", "f2");
- assertThat(result.getValue("f1")).isEqualTo(4);
- assertThat(result.getValue("f2")).isEqualTo(5);
- }
-
- @Test
- public void allOtherConcatReturnsMap() throws Exception {
- SkylarkProvider provider = makeProvider();
- SkylarkInfo info1 = makeSchemalessInfoWithF1F2Values(provider, 4, null);
- SkylarkInfo info2 = makeSchemafulInfoWithF1F2Values(provider, null, 5);
- SkylarkInfo result = (SkylarkInfo) info1.getConcatter().concat(info1, info2, Location.BUILTIN);
- assertThat(result.isCompact()).isFalse();
assertThat(result.getFieldNames()).containsExactly("f1", "f2");
assertThat(result.getValue("f1")).isEqualTo(4);
assertThat(result.getValue("f2")).isEqualTo(5);
@@ -240,10 +138,10 @@
}
/**
- * Creates a schemaless instance of a provider with the given values for fields f1 and f2. Either
- * field value may be null, in which case it is omitted.
+ * Creates an instance of a provider with the given values for fields f1 and f2. Either field
+ * value may be null, in which case it is omitted.
*/
- private static SkylarkInfo makeSchemalessInfoWithF1F2Values(
+ private static SkylarkInfo makeInfoWithF1F2Values(
SkylarkProvider provider, @Nullable Object v1, @Nullable Object v2) {
ImmutableMap.Builder<String, Object> values = ImmutableMap.builder();
if (v1 != null) {
@@ -252,26 +150,7 @@
if (v2 != null) {
values.put("f2", v2);
}
- return SkylarkInfo.createSchemaless(provider, values.build(), Location.BUILTIN);
+ return SkylarkInfo.create(provider, values.build(), Location.BUILTIN);
}
- /**
- * Creates a schemaful instance of a provider with the given values for fields f1 and f2. Either
- * field value may be null, in which case it is omitted.
- */
- private static SkylarkInfo makeSchemafulInfoWithF1F2Values(
- SkylarkProvider provider, @Nullable Object v1, @Nullable Object v2) {
- return SkylarkInfo.createSchemaful(
- provider, layoutF1F2, new Object[]{v1, v2}, Location.BUILTIN);
- }
-
- /**
- * Same as {@link #makeSchemafulInfoWithF1F2Values}, except the layout in the resulting
- * CompactSkylarkInfo is reversed.
- */
- private static SkylarkInfo makeInvertedSchemafulInfoWithF1F2Values(
- SkylarkProvider provider, @Nullable Object v1, @Nullable Object v2) {
- return SkylarkInfo.createSchemaful(
- provider, invertedLayoutF2F1, new Object[]{v2, v1}, Location.BUILTIN);
- }
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
index 842625e..b13f04e 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
@@ -67,7 +67,6 @@
public void schemalessProvider_Instantiation() throws Exception {
SkylarkProvider provider = SkylarkProvider.createUnexportedSchemaless(/*location=*/ null);
SkylarkInfo info = instantiateWithA1B2C3(provider);
- assertThat(info.isCompact()).isFalse();
assertHasExactlyValuesA1B2C3(info);
}
@@ -76,7 +75,6 @@
SkylarkProvider provider = SkylarkProvider.createUnexportedSchemaful(
ImmutableList.of("a", "b", "c"), /*location=*/ null);
SkylarkInfo info = instantiateWithA1B2C3(provider);
- assertThat(info.isCompact()).isTrue();
assertHasExactlyValuesA1B2C3(info);
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java
index 90a7885..d7f34ee 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java
@@ -24,6 +24,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.packages.SkylarkInfo;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.packages.StructProvider;
import com.google.devtools.build.lib.syntax.Depset;
@@ -110,11 +111,13 @@
assertThat(PyStructUtils.getTransitiveSources(info)).isSameInstanceAs(sources);
}
+ private static final StructImpl emptyInfo =
+ SkylarkInfo.create(StructProvider.STRUCT, ImmutableMap.of(), /* loc= */ null);
+
@Test
public void getTransitiveSources_Missing() {
- StructImpl info = StructProvider.STRUCT.createEmpty(null);
assertHasMissingFieldMessage(
- () -> PyStructUtils.getTransitiveSources(info), "transitive_sources");
+ () -> PyStructUtils.getTransitiveSources(emptyInfo), "transitive_sources");
}
@Test
@@ -144,8 +147,7 @@
@Test
public void getUsesSharedLibraries_Missing() throws Exception {
- StructImpl info = StructProvider.STRUCT.createEmpty(null);
- assertThat(PyStructUtils.getUsesSharedLibraries(info)).isFalse();
+ assertThat(PyStructUtils.getUsesSharedLibraries(emptyInfo)).isFalse();
}
@Test
@@ -165,8 +167,7 @@
@Test
public void getImports_Missing() throws Exception {
- StructImpl info = StructProvider.STRUCT.createEmpty(null);
- assertHasOrderAndContainsExactly(PyStructUtils.getImports(info), Order.COMPILE_ORDER);
+ assertHasOrderAndContainsExactly(PyStructUtils.getImports(emptyInfo), Order.COMPILE_ORDER);
}
@Test
@@ -183,8 +184,7 @@
@Test
public void getHasPy2OnlySources_Missing() throws Exception {
- StructImpl info = StructProvider.STRUCT.createEmpty(null);
- assertThat(PyStructUtils.getHasPy2OnlySources(info)).isFalse();
+ assertThat(PyStructUtils.getHasPy2OnlySources(emptyInfo)).isFalse();
}
@Test
@@ -202,8 +202,7 @@
@Test
public void getHasPy3OnlySources_Missing() throws Exception {
- StructImpl info = StructProvider.STRUCT.createEmpty(null);
- assertThat(PyStructUtils.getHasPy3OnlySources(info)).isFalse();
+ assertThat(PyStructUtils.getHasPy3OnlySources(emptyInfo)).isFalse();
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 5a71e61..82f353d 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -1180,7 +1180,7 @@
@Test
public void testStructConcatenationCommonFields() throws Exception {
checkEvalErrorContains(
- "Cannot use '+' operator on provider instances with overlapping field(s): a",
+ "cannot add struct instances with common field 'a'",
"x = struct(a = 1, b = 2)",
"y = struct(c = 1, a = 2)",
"z = x + y\n");
@@ -1385,7 +1385,7 @@
public void declaredProvidersWithOverlappingFieldsConcatError() throws Exception {
evalAndExport("data = provider(fields=['f1', 'f2'])");
checkEvalErrorContains(
- "Cannot use '+' operator on provider instances with overlapping field(s): f1",
+ "cannot add struct instances with common field 'f1'",
"d1 = data(f1 = 4)",
"d2 = data(f1 = 5)",
"d1 + d2");
