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

2 files changed, 108 insertions, 71 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;
   }