diff options
author | 2017-12-21 05:53:21 -0800 | |
---|---|---|
committer | 2017-12-21 05:55:13 -0800 | |
commit | a60c6a4b76d6bdf49c3307eb6cb81f9796aaa359 (patch) | |
tree | 6444a00c72b5c80a72d4faa39501f9782bfcbd45 /src/main/java/com/google/devtools | |
parent | bfb8e8f3ed1779d6e5960824b7c651df8a634bbd (diff) |
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
Diffstat (limited to 'src/main/java/com/google/devtools')
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 |