Encapsulate SkylarkInfo layouts in a class

This avoids invalid layouts (non-sequential map values), and provides better separation between a layout's representation as a map and its view as a list.

Also removed a factory method that's unnecessary, now that the plan is not to closely tie SkylarkInfo to SkylarkProvider.

RELNOTES: None
PiperOrigin-RevId: 181807071

3 files changed, 193 insertions, 120 deletions

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 7a6a458ecd..5445f80f64 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
@@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 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.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.syntax.Concatable;
 import com.google.devtools.build.lib.syntax.EvalException;
@@ -70,6 +71,15 @@ public abstract class SkylarkInfo extends Info implements Concatable {
    */
   protected abstract Iterable<Object> getValues();
 
+  /** 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;
+  }
+
   /**
    * Creates a schemaless (map-based) provider instance with the given provider type and field
    * values.
@@ -109,49 +119,96 @@ public abstract class SkylarkInfo extends Info implements Concatable {
   }
 
   /**
-   * Creates a schemaful (array-based) provider instance with the given provider type and values,
-   * where the layout is specified by the provider type.
-   *
-   * <p>This factory method requires a {@link SkylarkProvider} in order to retrieve the layout. To
-   * obtain a schemaful provider instance for another kind of provider type, use {@link
-   * #createSchemafulWithCustomLayout} instead.
+   * Creates a schemaful (array-based) provider instance with the given provider type, layout, and
+   * values.
    *
-   * <p>{@code provider} must be schemaful. The order of {@code values} must correspond to {@code
-   * provider}'s layout.
+   * <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(
-      SkylarkProvider provider, Object[] values, @Nullable Location loc) {
-    Preconditions.checkArgument(provider.getLayout() != null, "provider cannot be schemaless");
-    return new CompactSkylarkInfo(provider, provider.getLayout(), values, loc);
-  }
-
-  /**
-   * Creates a schemaful (array-based) provider instance with the given provider type and values,
-   * and with a custom layout.
-   *
-   * <p>The order of the values must correspond to the given layout. Any layout specified by the
-   * provider (i.e., if it is a schemaful {@link SkylarkProvider}) is ignored.
-   *
-   * <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 createSchemafulWithCustomLayout(
       Provider provider,
-      ImmutableMap<String, Integer> layout,
+      Layout layout,
       Object[] values,
       @Nullable Location loc) {
     return new CompactSkylarkInfo(provider, layout, values, loc);
   }
 
-  /** Returns the layout for this provider if it is schemaful, null otherwise. */
-  public abstract ImmutableMap<String, Integer> getLayout();
+  /**
+   * 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
+  public static final class Layout {
 
-  /** Returns true if this provider is schemaful (array-based), false otherwise. */
-  public boolean isCompact() {
-    return getLayout() != null;
+    /**
+     * 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) {
+      ImmutableMap.Builder<String, Integer> layoutBuilder = ImmutableMap.builder();
+      int i = 0;
+      for (String field : fields) {
+        layoutBuilder.put(field, i++);
+      }
+      this.map = 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 whether or not a field is mentioned in the layout. */
+    public boolean hasField(String field) {
+      return map.containsKey(field);
+    }
+
+    /**
+     * 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. */
@@ -204,7 +261,7 @@ public abstract class SkylarkInfo extends Info implements Concatable {
     }
 
     @Override
-    public ImmutableMap<String, Integer> getLayout() {
+    public Layout getLayout() {
       return null;
     }
   }
@@ -212,13 +269,13 @@ public abstract class SkylarkInfo extends Info implements Concatable {
   /** A {@link SkylarkInfo} implementation that stores its values in array to save space. */
   private static final class CompactSkylarkInfo extends SkylarkInfo implements Concatable {
 
-    private final ImmutableMap<String, Integer> layout;
+    private final Layout layout;
     /** Treated as immutable. */
     private final Object[] values;
 
     CompactSkylarkInfo(
         Provider provider,
-        ImmutableMap<String, Integer> layout,
+        Layout layout,
         Object[] values,
         @Nullable Location loc) {
       super(provider, loc);
@@ -231,7 +288,7 @@ public abstract class SkylarkInfo extends Info implements Concatable {
 
     @Override
     public Object getValue(String name) {
-      Integer index = layout.get(name);
+      Integer index = layout.getFieldIndex(name);
       if (index == null) {
         return null;
       }
@@ -240,7 +297,7 @@ public abstract class SkylarkInfo extends Info implements Concatable {
 
     @Override
     public boolean hasField(String name) {
-      Integer index = layout.get(name);
+      Integer index = layout.getFieldIndex(name);
       return index != null && values[index] != null;
     }
 
@@ -261,7 +318,7 @@ public abstract class SkylarkInfo extends Info implements Concatable {
     }
 
     @Override
-    public ImmutableMap<String, Integer> getLayout() {
+    public Layout getLayout() {
       return layout;
     }
   }
@@ -299,15 +356,15 @@ public abstract class SkylarkInfo extends Info implements Concatable {
       if (leftInfo instanceof CompactSkylarkInfo && rightInfo instanceof CompactSkylarkInfo) {
         CompactSkylarkInfo compactLeft = (CompactSkylarkInfo) leftInfo;
         CompactSkylarkInfo compactRight = (CompactSkylarkInfo) rightInfo;
-        ImmutableMap<String, Integer> layout = compactLeft.layout;
-        if (layout == compactRight.layout) {
+        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 createSchemafulWithCustomLayout(provider, layout, newValues, loc);
+          return createSchemaful(provider, layout, newValues, loc);
         }
       }
       // Fall back on making a map-based instance.
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 932aef52a8..2bcbbbdcfb 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,11 +14,12 @@
 
 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.common.collect.ImmutableMap;
 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.skylarkinterface.SkylarkPrinter;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.FunctionSignature;
@@ -36,7 +37,7 @@ import javax.annotation.Nullable;
  * <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.CompactSkylarkInfo}.
+ * efficient since they do not use maps; see {@link SkylarkInfo}.
  *
  * <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
@@ -52,14 +53,11 @@ public class SkylarkProvider extends Provider implements SkylarkExportable {
   private static final String DEFAULT_ERROR_MESSAGE_FORMAT = "Object has no '%s' attribute.";
 
   /**
-   * A map from provider fields to a contiguous range of integers, as used in {@link
-   * SkylarkInfo.CompactSkylarkInfo}. The map entries are ordered by integer value.
-   *
-   * <p>This allows provider instances to store their values in an array rather than a map. {@code
-   * layout} will be null if the provider is schemaless, i.e., its fields aren't known up-front.
+   * For schemaful providers, a layout describing the allowed fields and their order in an
+   * array-based representation. For schemaless providers, null.
    */
   @Nullable
-  private final ImmutableMap<String, Integer> layout;
+  private final Layout layout;
 
   /** Null iff this provider has not yet been exported. */
   @Nullable
@@ -131,7 +129,7 @@ public class SkylarkProvider extends Provider implements SkylarkExportable {
     // We override getName() in order to use the name that is assigned when export() is called.
     // Hence BaseFunction's constructor gets a null name.
     super(/*name=*/ null, buildSignature(fields), location);
-    this.layout = buildLayout(fields);
+    this.layout = fields == null ? null : new Layout(fields);
     this.key = key;  // possibly null
     this.errorMessageFormatForUnknownField =
         key == null ? DEFAULT_ERROR_MESSAGE_FORMAT
@@ -148,21 +146,6 @@ public class SkylarkProvider extends Provider implements SkylarkExportable {
         FunctionSignature.namedOnly(0, ImmutableList.copyOf(fields).toArray(new String[0])));
   }
 
-  @Nullable
-  private static ImmutableMap<String, Integer> buildLayout(
-      @Nullable Iterable<String> fields) {
-    if (fields == null) {
-      return null;
-    } else {
-      ImmutableMap.Builder<String, Integer> layoutBuilder = ImmutableMap.builder();
-      int i = 0;
-      for (String field : fields) {
-        layoutBuilder.put(field, i++);
-      }
-      return layoutBuilder.build();
-    }
-  }
-
   @Override
   protected Info createInstanceFromSkylark(Object[] args, Location loc) throws EvalException {
     if (layout == null) {
@@ -171,7 +154,7 @@ public class SkylarkProvider extends Provider implements SkylarkExportable {
       return SkylarkInfo.createSchemaless(this, kwargs, loc);
     } else {
       // Note: This depends on the layout map using the same ordering as args.
-      return SkylarkInfo.createSchemaful(this, args, loc);
+      return SkylarkInfo.createSchemaful(this, layout, args, loc);
     }
   }
 
@@ -207,16 +190,13 @@ public class SkylarkProvider extends Provider implements SkylarkExportable {
     if (layout == null) {
       return null;
     }
-    return ImmutableList.copyOf(layout.keySet());
+    return ImmutableList.copyOf(layout.getFields());
   }
 
-  /**
-   * Returns the layout, or null if the provider is schemaless.
-   *
-   * <p>This is used only by SkylarkInfo.
-   */
+  /** Returns the layout, or null if the provider is schemaless. */
+  @VisibleForTesting
   @Nullable
-  ImmutableMap<String, Integer> getLayout() {
+  Layout getLayout() {
     return layout;
   }
 
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 9b131098b8..2dceca75f6 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
@@ -11,9 +11,11 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
+
 package com.google.devtools.build.lib.packages;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.expectThrows;
 
 import com.google.common.collect.ImmutableList;
@@ -21,7 +23,9 @@ 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 java.util.Map;
 import javax.annotation.Nullable;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -31,45 +35,74 @@ import org.junit.runners.JUnit4;
 @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 nullLocationDefaultsToBuiltin() throws Exception {
-    SkylarkProvider provider = makeUnexportedProviderWithFieldsF1F2();
-    SkylarkInfo info = SkylarkInfo.createSchemaless(provider, ImmutableMap.of(), null);
-    assertThat(info.getCreationLoc()).isEqualTo(Location.BUILTIN);
+  public void layoutAccessors() {
+    Layout layout = new Layout(ImmutableList.of("x", "y", "z"));
+    assertThat(layout.size()).isEqualTo(3);
+    assertThat(layout.hasField("x")).isTrue();
+    assertThat(layout.hasField("q")).isFalse();
+    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 customLayoutTakesPrecedenceOverProviderLayout() throws Exception {
-    SkylarkProvider provider = makeUnexportedProviderWithFieldsF1F2();
-    ImmutableMap<String, Integer> invertedLayout = ImmutableMap.of("f1", 1, "f2", 0);
-    SkylarkInfo info = SkylarkInfo.createSchemafulWithCustomLayout(
-        provider, invertedLayout, new Object[]{5, 4}, Location.BUILTIN);
-    assertThat(info.getValue("f1")).isEqualTo(4);
-    assertThat(info.getValue("f2")).isEqualTo(5);
+  public void layoutDisallowsDuplicates() {
+    assertThrows(
+        IllegalArgumentException.class,
+        () -> new Layout(ImmutableList.of("x", "y", "x")));
   }
 
   @Test
-  public void schemafulConstructionRequiresSchema() throws Exception {
-    SkylarkProvider provider = SkylarkProvider.createUnexportedSchemaless(Location.BUILTIN);
-    IllegalArgumentException expected = expectThrows(
-        IllegalArgumentException.class,
-        () -> SkylarkInfo.createSchemaful(provider, new Object[]{4, 5}, Location.BUILTIN));
-    assertThat(expected).hasMessageThat().contains("provider cannot be schemaless");
+  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);
+    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 = makeUnexportedProviderWithFieldsF1F2();
+    SkylarkProvider provider = makeProvider();
     IllegalArgumentException expected = expectThrows(
         IllegalArgumentException.class,
-        () -> SkylarkInfo.createSchemaful(provider, new Object[]{4}, Location.BUILTIN));
+        () -> 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 = makeUnexportedProviderWithFieldsF1F2();
+    SkylarkProvider provider = makeProvider();
     SkylarkInfo mapInfo = makeSchemalessInfoWithF1F2Values(provider, 5, null);
     SkylarkInfo compactInfo = makeSchemafulInfoWithF1F2Values(provider, 5, null);
     assertThat(mapInfo.isImmutable()).isFalse();
@@ -78,8 +111,7 @@ public class SkylarkInfoTest {
 
   @Test
   public void instancesOfExportedProvidersMayBeImmutable() throws Exception {
-    SkylarkProvider provider = makeUnexportedProviderWithFieldsF1F2();
-    provider.export(Label.create("package", "target"), "provider");
+    SkylarkProvider provider = makeExportedProvider();
     SkylarkInfo mapInfo = makeSchemalessInfoWithF1F2Values(provider, 5, null);
     SkylarkInfo compactInfo = makeSchemafulInfoWithF1F2Values(provider, 5, null);
     assertThat(mapInfo.isImmutable()).isTrue();
@@ -88,8 +120,7 @@ public class SkylarkInfoTest {
 
   @Test
   public void mutableIfContentsAreMutable() throws Exception {
-    SkylarkProvider provider = makeUnexportedProviderWithFieldsF1F2();
-    provider.export(Label.create("package", "target"), "provider");
+    SkylarkProvider provider = makeExportedProvider();
     SkylarkInfo mapInfo = makeSchemalessInfoWithF1F2Values(provider, 5, new Object());
     SkylarkInfo compactInfo = makeSchemafulInfoWithF1F2Values(provider, 5, new Object());
     assertThat(mapInfo.isImmutable()).isFalse();
@@ -98,8 +129,8 @@ public class SkylarkInfoTest {
 
   @Test
   public void equality_DifferentProviders() throws Exception {
-    SkylarkProvider provider1 = makeUnexportedProviderWithFieldsF1F2();
-    SkylarkProvider provider2 = makeUnexportedProviderWithFieldsF1F2();
+    SkylarkProvider provider1 = makeProvider();
+    SkylarkProvider provider2 = makeProvider();
     new EqualsTester()
         .addEqualityGroup(
             makeSchemalessInfoWithF1F2Values(provider1, 4, 5),
@@ -114,7 +145,7 @@ public class SkylarkInfoTest {
 
   @Test
   public void equality_DifferentValues() throws Exception {
-    SkylarkProvider provider = makeUnexportedProviderWithFieldsF1F2();
+    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()
@@ -135,8 +166,8 @@ public class SkylarkInfoTest {
 
   @Test
   public void concatWithDifferentProvidersFails() throws Exception {
-    SkylarkProvider provider1 = makeUnexportedProviderWithFieldsF1F2();
-    SkylarkProvider provider2 = makeUnexportedProviderWithFieldsF1F2();
+    SkylarkProvider provider1 = makeProvider();
+    SkylarkProvider provider2 = makeProvider();
     SkylarkInfo info1 = makeSchemalessInfoWithF1F2Values(provider1, 4, 5);
     SkylarkInfo info2 = makeSchemalessInfoWithF1F2Values(provider2, 4, 5);
     EvalException expected =
@@ -149,7 +180,7 @@ public class SkylarkInfoTest {
 
   @Test
   public void concatWithOverlappingFieldsFails() throws Exception {
-    SkylarkProvider provider1 = makeUnexportedProviderWithFieldsF1F2();
+    SkylarkProvider provider1 = makeProvider();
     SkylarkInfo info1 = makeSchemalessInfoWithF1F2Values(provider1, 4, 5);
     SkylarkInfo info2 = makeSchemalessInfoWithF1F2Values(provider1, 4, null);
     EvalException expected =
@@ -162,10 +193,10 @@ public class SkylarkInfoTest {
 
   @Test
   public void compactConcatReturnsCompact() throws Exception {
-    SkylarkProvider provider = makeUnexportedProviderWithFieldsF1F2();
-    SkylarkInfo p1 = makeSchemafulInfoWithF1F2Values(provider, 4, null);
-    SkylarkInfo p2 = makeSchemafulInfoWithF1F2Values(provider, null, 5);
-    SkylarkInfo result = (SkylarkInfo) p1.getConcatter().concat(p1, p2, Location.BUILTIN);
+    SkylarkProvider provider = makeProvider();
+    SkylarkInfo info1 = makeSchemafulInfoWithF1F2Values(provider, 4, null);
+    SkylarkInfo info2 = makeSchemafulInfoWithF1F2Values(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);
@@ -174,34 +205,38 @@ public class SkylarkInfoTest {
 
   @Test
   public void compactConcatWithDifferentLayoutsReturnsMap() throws Exception {
-    SkylarkProvider provider = makeUnexportedProviderWithFieldsF1F2();
+    SkylarkProvider provider = makeProvider();
     SkylarkInfo info1 = makeSchemafulInfoWithF1F2Values(provider, 4, null);
     SkylarkInfo info2 = makeInvertedSchemafulInfoWithF1F2Values(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 = makeUnexportedProviderWithFieldsF1F2();
-    SkylarkInfo p1 = makeSchemalessInfoWithF1F2Values(provider, 4, null);
-    SkylarkInfo p2 = makeSchemafulInfoWithF1F2Values(provider, null, 5);
-    SkylarkInfo result = (SkylarkInfo) p1.getConcatter().concat(p1, p2, Location.BUILTIN);
+    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);
   }
 
-  /**
-   * Creates an unexported schemaful {@link SkylarkProvider} with two fields, f1 and f2. The
-   * location is builtin.
-   */
-  private static SkylarkProvider makeUnexportedProviderWithFieldsF1F2() {
-    return SkylarkProvider.createUnexportedSchemaful(
-        ImmutableList.of("f1", "f2"), Location.BUILTIN);
+  /** Creates an unexported schemaless provider type with builtin location. */
+  private static SkylarkProvider makeProvider() {
+    return SkylarkProvider.createUnexportedSchemaless(Location.BUILTIN);
+  }
+
+  /** Creates an exported schemaless provider type with builtin location. */
+  private static SkylarkProvider makeExportedProvider() {
+    SkylarkProvider.SkylarkKey key = new SkylarkProvider.SkylarkKey(
+        Label.parseAbsoluteUnchecked("//package:target"), "provider");
+    return SkylarkProvider.createExportedSchemaless(key, Location.BUILTIN);
   }
 
   /**
@@ -226,7 +261,8 @@ public class SkylarkInfoTest {
    */
   private static SkylarkInfo makeSchemafulInfoWithF1F2Values(
       SkylarkProvider provider, @Nullable Object v1, @Nullable Object v2) {
-    return SkylarkInfo.createSchemaful(provider, new Object[]{v1, v2}, Location.BUILTIN);
+    return SkylarkInfo.createSchemaful(
+        provider, layoutF1F2, new Object[]{v1, v2}, Location.BUILTIN);
   }
 
   /**
@@ -235,7 +271,7 @@ public class SkylarkInfoTest {
    */
   private static SkylarkInfo makeInvertedSchemafulInfoWithF1F2Values(
       SkylarkProvider provider, @Nullable Object v1, @Nullable Object v2) {
-    return SkylarkInfo.createSchemafulWithCustomLayout(
-        provider, ImmutableMap.of("f1", 1, "f2", 0), new Object[]{v2, v1}, Location.BUILTIN);
+    return SkylarkInfo.createSchemaful(
+        provider, invertedLayoutF2F1, new Object[]{v2, v1}, Location.BUILTIN);
   }
 }