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
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 1e7d5fe..1879227 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 @@
"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 b497b76..2616a20 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 @@
* {@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 @@
@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 cdbf73e..1a94c9c 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 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;
- private static final String DEFAULT_ERROR_MESSAFE = "Object has no '%s' attribute.";
+ /** Error message format. Reassigned upon exporting. */
+ private String errorMessageFormatForInstances;
/**
- * Creates a Skylark-defined Declared Provider ({@link Info} constructor).
+ * Creates an unexported {@link SkylarkProvider} with no schema.
*
- * <p>Needs to be exported later.
+ * <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 SkylarkProvider(String name,
- @Nullable Iterable<String> fields,
- Location location) {
- this(name, buildSignature(fields), location);
+ 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);
+ }
+
+ /**
+ * 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 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 location the location of the Skylark definition for this provider (tests may use {@link
+ * Location#BUILTIN})
+ */
+ 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 @@
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 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 e77af8f..b5ceb0c 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 @@
// 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 @@
// 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 @@
/**
* 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 @@
/**
* 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 @@
/**
* 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 @@
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
