Info-related cleanups

- Reorder Info methods for consistency with ClassObject
- "StructConstructor" -> "StructProvider"
- Added javadoc

RELNOTES: None
PiperOrigin-RevId: 181469643

4 files changed, 81 insertions, 52 deletions

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 b398073f70..a40bacb549 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
@@ -268,7 +268,7 @@ public final class SkylarkRuleConfiguredTargetUtil {
       SkylarkRuleContext context, RuleConfiguredTargetBuilder builder, Object target, Location loc)
       throws EvalException {
 
-    Info oldStyleProviders = NativeProvider.STRUCT.create(loc);
+    Info oldStyleProviders = NativeProvider.STRUCT.createEmpty(loc);
     ArrayList<Info> declaredProviders = new ArrayList<>();
 
     if (target instanceof Info) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Info.java b/src/main/java/com/google/devtools/build/lib/packages/Info.java
index b885d65581..b3ed563639 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Info.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Info.java
@@ -99,6 +99,8 @@ public abstract class Info implements ClassObject, SkylarkValue, Serializable {
   /**
    * Preprocesses a map of field values to convert the field names and field values to
    * Skylark-acceptable names and types.
+   *
+   * <p>This preserves the order of the map entries.
    */
   protected static ImmutableMap<String, Object> copyValues(Map<String, Object> values) {
     ImmutableMap.Builder<String, Object> builder = ImmutableMap.builder();
@@ -111,23 +113,6 @@ public abstract class Info implements ClassObject, SkylarkValue, Serializable {
   }
 
   /**
-   * Returns whether the given field name exists.
-   *
-   * <p>This conceptually extends the API for {@link ClassObject}.
-   */
-  public abstract boolean hasField(String name);
-
-  /** Returns a value and try to cast it into specified type */
-  public <T> T getValue(String key, Class<T> type) throws EvalException {
-    Object obj = getValue(key);
-    if (obj == null) {
-      return null;
-    }
-    SkylarkType.checkType(obj, type, key);
-    return type.cast(obj);
-  }
-
-  /**
    * Returns the Skylark location where this provider instance was created.
    *
    * <p>Builtin provider instances may return {@link Location#BUILTIN}.
@@ -141,25 +126,44 @@ public abstract class Info implements ClassObject, SkylarkValue, Serializable {
   }
 
   /**
-   * Returns the fields of this struct.
+   * Returns whether the given field name exists.
    *
-   * Overrides {@link ClassObject#getFieldNames()}, but does not allow {@link EvalException} to
-   * be thrown.
+   * <p>This conceptually extends the API for {@link ClassObject}.
    */
-  @Override
-  public abstract ImmutableCollection<String> getFieldNames();
+  public abstract boolean hasField(String name);
 
   /**
-   * Returns the value associated with the name field in this struct,
-   * or null if the field does not exist.
+   * {@inheritDoc}
    *
-   * Overrides {@link ClassObject#getValue(String)}, but does not allow {@link EvalException} to
+   * <p>Overrides {@link ClassObject#getValue(String)}, but does not allow {@link EvalException} to
    * be thrown.
    */
   @Nullable
   @Override
   public abstract Object getValue(String name);
 
+  /**
+   * Returns the result of {@link #getValue(String)}, cast as the given type, throwing {@link
+   * EvalException} if the cast fails.
+   */
+  public <T> T getValue(String key, Class<T> type) throws EvalException {
+    Object obj = getValue(key);
+    if (obj == null) {
+      return null;
+    }
+    SkylarkType.checkType(obj, type, key);
+    return type.cast(obj);
+  }
+
+  /**
+   * {@inheritDoc}
+   *
+   * <p>Overrides {@link ClassObject#getFieldNames()}, but does not allow {@link EvalException} to
+   * be thrown.
+   */
+  @Override
+  public abstract ImmutableCollection<String> getFieldNames();
+
   @Override
   public String getErrorMessageForUnknownField(String name) {
     String suffix = "Available attributes: "
diff --git a/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java b/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
index 0f78e4659c..22a4eaaf51 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
@@ -45,7 +45,7 @@ public abstract class NativeProvider<V extends Info> extends Provider {
   private final String errorMessageFormatForUnknownField;
 
   /** "struct" function. */
-  public static final StructConstructor STRUCT = new StructConstructor();
+  public static final StructProvider STRUCT = new StructProvider();
 
   private final Class<V> valueClass;
 
@@ -61,17 +61,17 @@ public abstract class NativeProvider<V extends Info> extends Provider {
    * Skylark code.
    */
   @Deprecated
-  public static interface WithLegacySkylarkName {
+  public interface WithLegacySkylarkName {
     String getSkylarkName();
   }
 
   /**
-   * A constructor for default {@code struct}s.
+   * The provider for the built-in type {@code struct}.
    *
-   * <p>Singleton, instance is {@link #STRUCT}.
+   * <p>Its singleton instance is {@link #STRUCT}.
    */
-  public static final class StructConstructor extends NativeProvider<Info> {
-    private StructConstructor() {
+  public static final class StructProvider extends NativeProvider<Info> {
+    private StructProvider() {
       super(Info.class, "struct");
     }
 
@@ -82,11 +82,19 @@ public abstract class NativeProvider<V extends Info> extends Provider {
       return SkylarkInfo.fromMap(this, kwargs, loc);
     }
 
-    public Info create(Map<String, Object> values, String message) {
-      return new SkylarkInfo.MapBackedSkylarkInfo(this, values, message);
+    /**
+     * Creates a struct with the he given field values and message format for unknown fields.
+     *
+     * <p>The custom message is useful for objects that have fields but aren't exactly used as
+     * providers, such as the {@code native} object, and the struct fields of {@code ctx} like
+     * {@code ctx.attr}.
+     * */
+    public Info create(Map<String, Object> values, String errorMessageFormatForUnknownField) {
+      return new SkylarkInfo.MapBackedSkylarkInfo(this, values, errorMessageFormatForUnknownField);
     }
 
-    public Info create(Location loc) {
+    /** Creates an empty struct with the given location. */
+    public Info createEmpty(Location loc) {
       return SkylarkInfo.fromMap(this, ImmutableMap.of(), loc);
     }
   }
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 cc5d3ea93d..15e5b5e2a9 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
@@ -11,6 +11,7 @@
 // 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 com.google.common.base.Joiner;
@@ -21,22 +22,28 @@ 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.events.Location;
-import com.google.devtools.build.lib.packages.NativeProvider.StructConstructor;
+import com.google.devtools.build.lib.packages.NativeProvider.StructProvider;
 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 java.util.Arrays;
 import java.util.Map;
 
-/** Implementation of {@link Info} created from Skylark. */
+/**
+ * A provider instance of either 1) a Skylark-defined provider ({@link SkylarkInfo}), or 2) the
+ * built-in "struct" type ({@link NativeProvider#STRUCT}).
+ *
+ * <p>There are two concrete subclasses corresponding to two different implementation strategies.
+ * One is map-based and schemaless, the other has a fixed layout and is more memory-efficient.
+ */
 public abstract class SkylarkInfo extends Info implements Concatable {
 
   public SkylarkInfo(Provider provider, Location loc) {
     super(provider, loc);
   }
 
-  public SkylarkInfo(StructConstructor provider, String message) {
-    super(provider, message);
+  public SkylarkInfo(StructProvider provider, String errorMessageFormatForUnknownField) {
+    super(provider, errorMessageFormatForUnknownField);
   }
 
   @Override
@@ -46,7 +53,7 @@ public abstract class SkylarkInfo extends Info implements Concatable {
 
   @Override
   public boolean isImmutable() {
-    // If the provider is not yet exported the hash code of the object is subject to change
+    // If the provider is not yet exported, the hash code of the object is subject to change.
     if (!getProvider().isExported()) {
       return false;
     }
@@ -58,15 +65,21 @@ public abstract class SkylarkInfo extends Info implements Concatable {
     return true;
   }
 
-  /** Return all the values stored in the object. */
+  /**
+   * 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.
+   */
   protected abstract Iterable<Object> getValues();
 
   /**
-   * {@link SkylarkInfo} implementation that stores its values in a map. This is mainly used for the
-   * Skylark {@code struct()} constructor.
+   * A {@link SkylarkInfo} implementation that stores its values in a map. This is used for structs
+   * and for schemaless Skylark-defined providers.
    */
   static final class MapBackedSkylarkInfo extends SkylarkInfo {
-    protected final ImmutableMap<String, Object> values;
+
+    private final ImmutableMap<String, Object> values;
 
     public MapBackedSkylarkInfo(Provider provider, Map<String, Object> kwargs, Location loc) {
       super(provider, loc);
@@ -74,19 +87,21 @@ public abstract class SkylarkInfo extends Info implements Concatable {
     }
 
     public MapBackedSkylarkInfo(
-        StructConstructor provider, Map<String, Object> values, String message) {
-      super(provider, message);
+        StructProvider provider,
+        Map<String, Object> values,
+        String errorMessageFormatForUnknownField) {
+      super(provider, errorMessageFormatForUnknownField);
       this.values = copyValues(values);
     }
 
     @Override
-    public Object getValue(String name) {
-      return values.get(name);
+    public boolean hasField(String name) {
+      return values.containsKey(name);
     }
 
     @Override
-    public boolean hasField(String name) {
-      return values.containsKey(name);
+    public Object getValue(String name) {
+      return values.get(name);
     }
 
     @Override
@@ -163,6 +178,7 @@ public abstract class SkylarkInfo extends Info implements Concatable {
 
     @Override
     public Concatable concat(Concatable left, Concatable right, Location loc) throws EvalException {
+      // Casts are safe because SkylarkInfoConcatter is only used by SkylarkInfo.
       SkylarkInfo lval = (SkylarkInfo) left;
       SkylarkInfo rval = (SkylarkInfo) right;
       Provider provider = lval.getProvider();
@@ -170,7 +186,7 @@ public abstract class SkylarkInfo extends Info implements Concatable {
         throw new EvalException(
             loc,
             String.format(
-                "Cannot concat %s with %s",
+                "Cannot use '+' operator on instances of different providers (%s and %s)",
                 provider.getPrintableName(), rval.getProvider().getPrintableName()));
       }
       SetView<String> commonFields =
@@ -179,7 +195,8 @@ public abstract class SkylarkInfo extends Info implements Concatable {
       if (!commonFields.isEmpty()) {
         throw new EvalException(
             loc,
-            "Cannot concat structs with common field(s): " + Joiner.on(",").join(commonFields));
+            "Cannot use '+' operator on provider instances with overlapping field(s): "
+                + Joiner.on(",").join(commonFields));
       }
       // Keep homogeneous compact concatenations compact.
       if (lval instanceof CompactSkylarkInfo