Refactor SkylarkProvider constructors and add tests

A new constructor is exposed for building an already-exported SkylarkProvider. The existing constructor no longer takes a name argument (since it was almost entirely ignored). The contract around the name arg for BaseFunction has been refined: it is null if the subclass provides its own naming mechanism.

RELNOTES: None
PiperOrigin-RevId: 179804491

4 files changed, 170 insertions, 57 deletions

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 1e7d5fe5dc..1879227a38 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
@@ -336,10 +336,7 @@ public class SkylarkRuleClassFunctions {
                 "Expected list of strings or dictionary of string -> string for 'fields'");
             fieldNames = dict.keySet();
           }
-          return new SkylarkProvider(
-              "<no name>", // name is set on export.
-              fieldNames,
-              location);
+          return SkylarkProvider.createUnexportedSchemaful(fieldNames, location);
         }
       };
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Provider.java b/src/main/java/com/google/devtools/build/lib/packages/Provider.java
index b497b76131..2616a209e0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Provider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Provider.java
@@ -31,7 +31,9 @@ import javax.annotation.Nullable;
  * {@link SkylarkProvider}.
  *
  * <p>{@link Provider} serves both as "type identifier" for declared provider instances and as a
- * function that can be called to construct a provider.
+ * function that can be called to construct a provider. To the Skylark user, there are "providers"
+ * and "provider instances"; the former is a Java instance of this class, and the latter is a Java
+ * instance of {@link Info}.
  *
  * <p>Prefer to use {@link Key} as a serializable identifier of {@link Provider}. In particular,
  * {@link Key} should be used in all data structures exposed to Skyframe.
@@ -62,8 +64,19 @@ import javax.annotation.Nullable;
 @Immutable
 public abstract class Provider extends BaseFunction {
 
+  /**
+   * Constructs a provider.
+   *
+   * @param name provider name; should be null iff the subclass overrides {@link #getName}
+   * @param signature the signature for calling this provider as a Skylark function (to construct an
+   *     instance of the provider)
+   * @param location the location of this provider's Skylark definition. Use {@link
+   *     Location#BUILTIN} if it is a native provider.
+   */
   protected Provider(
-      String name, FunctionSignature.WithValues<Object, SkylarkType> signature, Location location) {
+      @Nullable String name,
+      FunctionSignature.WithValues<Object, SkylarkType> signature,
+      Location location) {
     super(name, signature, location);
   }
 
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 cdbf73edd1..1a94c9cf2b 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
@@ -28,66 +28,138 @@ import java.util.Objects;
 import javax.annotation.Nullable;
 
 /**
- * Declared provider defined in Skylark.
+ * A provider defined in Skylark rather than in native code.
  *
- * <p>This is a result of calling {@code provider()} function from Skylark ({@link
+ * <p>This is a result of calling the {@code provider()} function from Skylark ({@link
  * com.google.devtools.build.lib.analysis.skylark.SkylarkRuleClassFunctions#provider}).
+ *
+ * <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}.
+ *
+ * <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 pre-exported provider directly. Exported providers use only their key for {@link #equals} and
+ * {@link #hashCode}.
  */
 public class SkylarkProvider extends Provider implements SkylarkExportable {
 
-  private static final FunctionSignature.WithValues<Object, SkylarkType> SIGNATURE =
+  private static final FunctionSignature.WithValues<Object, SkylarkType> SCHEMALESS_SIGNATURE =
       FunctionSignature.WithValues.create(FunctionSignature.KWARGS);
 
+  private static final String DEFAULT_ERROR_MESSAGE_FORMAT = "Object has no '%s' attribute.";
+
   /**
-   * A map from provider fields to a continuous range of integers. This allows provider instances to
-   * store their values in an array rather than a map. {@code layout} will be null if the provider
-   * fields aren't known up front.
+   * 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.
    */
   @Nullable
   private final ImmutableMap<String, Integer> layout;
 
-  @Nullable private SkylarkKey key;
-  @Nullable private String errorMessageFormatForInstances;
+  /** Null iff this provider has not yet been exported. */
+  @Nullable
+  private SkylarkKey key;
+
+  /** Error message format. Reassigned upon exporting. */
+  private String errorMessageFormatForInstances;
+
+  /**
+   * Creates an unexported {@link SkylarkProvider} with no schema.
+   *
+   * <p>The resulting object needs to be exported later (via {@link #export}).
+   *
+   * @param location the location of the Skylark definition for this provider (tests may use {@link
+   *     Location#BUILTIN})
+   */
+  public static SkylarkProvider createUnexportedSchemaless(Location location) {
+    return new SkylarkProvider(/*key=*/ null, /*fields=*/ null, location);
+  }
+
+  /**
+   * Creates an unexported {@link SkylarkProvider} with a schema.
+   *
+   * <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 location the location of the Skylark definition for this provider (tests may use {@link
+   *     Location#BUILTIN})
+   */
+  public static SkylarkProvider createUnexportedSchemaful(
+      Iterable<String> fields, Location location) {
+    return new SkylarkProvider(/*key=*/ null, fields, location);
+  }
 
-  private static final String DEFAULT_ERROR_MESSAFE = "Object has no '%s' attribute.";
+  /**
+   * Creates an exported {@link SkylarkProvider} with no schema.
+   *
+   * @param key the key that identifies this provider
+   * @param location the location of the Skylark definition for this provider (tests may use {@link
+   *     Location#BUILTIN})
+   */
+  public static SkylarkProvider createExportedSchemaless(SkylarkKey key, Location location) {
+    return new SkylarkProvider(key, /*fields=*/ null, location);
+  }
 
   /**
-   * Creates a Skylark-defined Declared Provider ({@link Info} constructor).
+   * Creates an exported {@link SkylarkProvider} with no schema.
    *
-   * <p>Needs to be exported later.
+   * @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 location the location of the Skylark definition for this provider (tests may use {@link
+   *     Location#BUILTIN})
    */
-  public SkylarkProvider(String name,
-      @Nullable Iterable<String> fields,
-      Location location) {
-    this(name, buildSignature(fields), location);
+  public static SkylarkProvider createExportedSchemaful(
+      SkylarkKey key, Iterable<String> fields, Location location) {
+    return new SkylarkProvider(key, fields, location);
   }
 
+  /**
+   * Constructs the provider.
+   *
+   * <p>If {@code key} is null, the provider is unexported. If {@code fields} is null, the provider
+   * is schemaless.
+   */
   private SkylarkProvider(
-      String name,
-      FunctionSignature.WithValues<Object, SkylarkType> signature, Location location) {
-    super(name, signature, location);
-    if (signature.getSignature().getShape().hasKwArg()) {
-      layout = null;
+      @Nullable SkylarkKey key, @Nullable Iterable<String> fields, Location location) {
+    // 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.key = key;  // possibly null
+    this.errorMessageFormatForInstances =
+        key == null ? DEFAULT_ERROR_MESSAGE_FORMAT
+            : makeErrorMessageFormatForInstances(key.getExportedName());
+  }
+
+
+  private static FunctionSignature.WithValues<Object, SkylarkType> buildSignature(
+      @Nullable Iterable<String> fields) {
+    if (fields == null) {
+      return SCHEMALESS_SIGNATURE;
+    }
+    return FunctionSignature.WithValues.create(
+        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 : signature.getSignature().getNames()) {
+      for (String field : fields) {
         layoutBuilder.put(field, i++);
       }
-      layout = layoutBuilder.build();
+      return layoutBuilder.build();
     }
-    this.errorMessageFormatForInstances = DEFAULT_ERROR_MESSAFE;
-  }
-
-  private static FunctionSignature.WithValues<Object, SkylarkType> buildSignature(
-      @Nullable  Iterable<String> fields) {
-    if (fields == null) {
-      return SIGNATURE;
-    }
-    return
-        FunctionSignature.WithValues.create(
-        FunctionSignature.namedOnly(0, ImmutableList.copyOf(fields).toArray(new String[0]))
-    );
   }
 
   @Override
@@ -123,6 +195,19 @@ public class SkylarkProvider extends Provider implements SkylarkExportable {
     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).
+   */
+  public @Nullable ImmutableList<String> getFields() {
+    if (layout == null) {
+      return null;
+    }
+    return ImmutableList.copyOf(layout.keySet());
+  }
+
   @Override
   public String getErrorMessageFormatForInstances() {
     return errorMessageFormatForInstances;
@@ -132,8 +217,11 @@ public class SkylarkProvider extends Provider implements SkylarkExportable {
   public void export(Label extensionLabel, String exportedName) {
     Preconditions.checkState(!isExported());
     this.key = new SkylarkKey(extensionLabel, exportedName);
-    this.errorMessageFormatForInstances =
-        String.format("'%s' object has no attribute '%%s'", exportedName);
+    this.errorMessageFormatForInstances = makeErrorMessageFormatForInstances(exportedName);
+  }
+
+  private static String makeErrorMessageFormatForInstances(String exportedName) {
+    return String.format("'%s' object has no attribute '%%s'", exportedName);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index e77af8fd44..b5ceb0c039 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -60,14 +60,20 @@ import javax.annotation.Nullable;
 // Also, use better pure maps to minimize map O(n) re-creation events when processing keyword maps.
 public abstract class BaseFunction implements SkylarkValue {
 
-  // The name of the function
-  private final String name;
+  /**
+   * The name of the function.
+   *
+   * <p>For safe extensibility, this class only retrieves name via the accessor {@link #getName}.
+   * This field must be null iff {@link #getName} is overridden.
+   */
+  @Nullable private final String name;
 
   // A function signature, including defaults and types
   // never null after it is configured
   @Nullable protected FunctionSignature.WithValues<Object, SkylarkType> signature;
 
   // Location of the function definition, or null for builtin functions
+  // TODO(bazel-team): Or make non-nullable, and use Location.BUILTIN for builtin functions?
   @Nullable protected Location location;
 
   // Some functions are also Namespaces or other Skylark entities.
@@ -96,8 +102,13 @@ public abstract class BaseFunction implements SkylarkValue {
   // We trust the user not to modify the list behind our back.
 
 
-  /** Returns the name of this function. */
+  /**
+   * Returns the name of this function.
+   *
+   * <p>A subclass must override this function if a null name is given to this class's constructor.
+   */
   public String getName() {
+    Preconditions.checkNotNull(name);
     return name;
   }
 
@@ -119,20 +130,22 @@ public abstract class BaseFunction implements SkylarkValue {
   /**
    * Creates an unconfigured BaseFunction with the given name.
    *
-   * @param name the function name
+   * <p>The name must be null if called from a subclass constructor where the subclass overrides
+   * {@link #getName}; otherwise it must be non-null.
    */
-  public BaseFunction(String name) {
+  public BaseFunction(@Nullable String name) {
     this.name = name;
   }
 
   /**
    * Constructs a BaseFunction with a given name, signature and location.
    *
-   * @param name the function name
+   * @param name the function name; null iff this is a subclass overriding {@link #getName}
    * @param signature the signature with default values and types
    * @param location the location of function definition
    */
-  public BaseFunction(String name,
+  public BaseFunction(
+      @Nullable String name,
       @Nullable FunctionSignature.WithValues<Object, SkylarkType> signature,
       @Nullable Location location) {
     this(name);
@@ -143,10 +156,11 @@ public abstract class BaseFunction implements SkylarkValue {
   /**
    * Constructs a BaseFunction with a given name, signature.
    *
-   * @param name the function name
+   * @param name the function name; null iff this is a subclass overriding {@link #getName}
    * @param signature the signature, with default values and types
    */
-  public BaseFunction(String name,
+  public BaseFunction(
+      @Nullable String name,
       @Nullable FunctionSignature.WithValues<Object, SkylarkType> signature) {
     this(name, signature, null);
   }
@@ -154,20 +168,20 @@ public abstract class BaseFunction implements SkylarkValue {
   /**
    * Constructs a BaseFunction with a given name and signature without default values or types.
    *
-   * @param name the function name
+   * @param name the function name; null iff this is a subclass overriding {@link #getName}
    * @param signature the signature, without default values or types
    */
-  public BaseFunction(String name, FunctionSignature signature) {
+  public BaseFunction(@Nullable String name, FunctionSignature signature) {
     this(name, FunctionSignature.WithValues.create(signature), null);
   }
 
   /**
    * Constructs a BaseFunction with a given name and list of unconfigured defaults.
    *
-   * @param name the function name
+   * @param name the function name; null iff this is a subclass overriding {@link #getName}
    * @param defaultValues a list of default values for the optional arguments to be configured.
    */
-  public BaseFunction(String name, @Nullable Iterable<Object> defaultValues) {
+  public BaseFunction(@Nullable String name, @Nullable Iterable<Object> defaultValues) {
     this(name);
     this.unconfiguredDefaultValues = defaultValues;
   }
@@ -550,14 +564,15 @@ public abstract class BaseFunction implements SkylarkValue {
       BaseFunction that = (BaseFunction) other;
       // In theory, the location alone unambiguously identifies a given function. However, in
       // some test cases the location might not have a valid value, thus we also check the name.
-      return Objects.equals(this.name, that.name) && Objects.equals(this.location, that.location);
+      return Objects.equals(this.getName(), that.getName())
+          && Objects.equals(this.location, that.location);
     }
     return false;
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(name, location);
+    return Objects.hash(getName(), location);
   }
 
   @Nullable