bazel packages: minor StructProvider clean-ups
- Remove concept of "error format strings" from Provider interface.
Error messages are computed by a method. (Only the StarlarkInfo subclass
now talks about format strings).
- Eliminate StructProvider.create(Map, Location) by inlining 4 calls.
- Improve wording of StarlarkProvider unknown field error message.
- Add TODO notes on the challenge of moving the unknown field error
messages entirely into Providers.
This CL contains the uncomplicated parts a failed attempt to do those TODOs.
PiperOrigin-RevId: 341647758
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java
index a0e067c..a09da1f 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java
@@ -40,6 +40,7 @@
import com.google.devtools.build.lib.events.ExtendedEventHandler.FetchProgress;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.StarlarkInfo;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.packages.StructProvider;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
@@ -770,7 +771,8 @@
new IOException("thread interrupted"), Transience.TRANSIENT);
} catch (IOException e) {
if (allowFail) {
- return StructProvider.STRUCT.create(ImmutableMap.of("success", false), Location.BUILTIN);
+ return StarlarkInfo.create(
+ StructProvider.STRUCT, ImmutableMap.of("success", false), Location.BUILTIN);
} else {
throw new RepositoryFunctionException(e, Transience.TRANSIENT);
}
@@ -897,7 +899,8 @@
} catch (IOException e) {
env.getListener().post(w);
if (allowFail) {
- return StructProvider.STRUCT.create(ImmutableMap.of("success", false), Location.BUILTIN);
+ return StarlarkInfo.create(
+ StructProvider.STRUCT, ImmutableMap.of("success", false), Location.BUILTIN);
} else {
throw new RepositoryFunctionException(e, Transience.TRANSIENT);
}
@@ -1026,7 +1029,7 @@
if (finalChecksum.getKeyType() == KeyType.SHA256) {
out.put("sha256", finalChecksum.toString());
}
- return StructProvider.STRUCT.create(out.build(), Location.BUILTIN);
+ return StarlarkInfo.create(StructProvider.STRUCT, out.build(), Location.BUILTIN);
}
private static ImmutableList<String> checkAllUrls(Iterable<?> urlList) throws EvalException {
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 c009f85..61e1ada 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
@@ -27,16 +27,19 @@
// - Once to_{json,proto} are gone, StructApi can be deleted; structs should never again have
// methods.
// - StructImpl.location can be pushed down into subclasses that need it, much as we did for
-// StructImpl.provider in this CL.
-// - getErrorMessageFormatForUnknownField can become a method on provider.
-// It should compute a string from a parameter, not use higher-order formatting.
+// StructImpl.provider in CL 341102857.
// - StructImpl is then really just a collection of helper functions for subclasses
-// getValue(String, Class), repr, equals, hash. Move them, and merge it into Info interface.
+// getValue(String, Class), repr, equals, hash. Move them, and merge it into Info interface,
+// or rename it InfoStruct or StructuredInfo if we absolutely need inheritance.
// - Move StructProvider.STRUCT and make StructProvider private.
// The StructProvider.createStruct method could be a simple function like depset, select.
// StructProviderApi could be eliminated.
// - eliminate StarlarkInfo + StarlarkInfo.
-// - NativeInfo's two methods can (IIUC) be deleted immediately, and then NativeInfo itself.
+// - NativeInfo's get{FieldNames,Value} methods are not needed by the Starlark interpreter,
+// since all its fields are annotated. They exist for the hash/eq/str implementations
+// defined in StructImpl over all its subclasses, and for json.encode. More thought is
+// needed on how to bridge between annotated methods and user-defined Structures so that
+// they appear similar to clients like json.encode.
//
// Info (result of analysis)
// - StructImpl (structure with fields, to_{json,proto}). Implements Structure, StructApi.
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 106722f..c614100 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
@@ -47,13 +47,11 @@
String getPrintableName();
/**
- * Returns an error message format string for instances to use for their {@link
+ * Returns an error message for instances to use for their {@link
* net.starlark.java.eval.Structure#getErrorMessageForUnknownField(String)}.
- *
- * <p>The format string must contain one {@code '%s'} placeholder for the field name.
*/
- default String getErrorMessageFormatForUnknownField() {
- return String.format("'%s' value has no field or method '%%s'", getPrintableName());
+ default String getErrorMessageForUnknownField(String name) {
+ return String.format("'%s' value has no field or method '%s'", getPrintableName(), name);
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkInfo.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkInfo.java
index 94f88b6..55aae7f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkInfo.java
@@ -47,6 +47,8 @@
// TODO(adonovan): make the provider determine the error message
// (but: this has implications for struct+struct, the equivalence
// relation, and other observable behaviors).
+ // Perhaps it should be a property of the StarlarkInfo instance, but
+ // defined by a subclass?
@Nullable private final String unknownFieldError;
// TODO(adonovan): restrict type of provider to StarlarkProvider?
@@ -251,16 +253,12 @@
return ImmutableList.copyOf(keys);
}
- /**
- * Returns the custom (i.e. per-instance, as opposed to per-provider-type) error message string
- * format used by this provider instance, or null if not set.
- */
- @Nullable
+ /** Returns the per-instance error message, if specified, or the provider's message otherwise. */
@Override
- protected String getErrorMessageFormatForUnknownField() {
+ public String getErrorMessageForUnknownField(String name) {
return unknownFieldError != null
- ? unknownFieldError
- : super.getErrorMessageFormatForUnknownField();
+ ? String.format(unknownFieldError, name) + allAttributesSuffix()
+ : super.getErrorMessageForUnknownField(name);
}
@Override
@@ -315,7 +313,25 @@
// TODO(bazel-team): Make the special structs that need a custom error message use a different
// provider (subclassing BuiltinProvider) and a different StructImpl implementation. Then remove
// this functionality, thereby saving a string pointer field for the majority of providers that
- // don't need it.
+ // don't need it. However, this is tricky: if the error message is a property of the provider,
+ // then each flavor of struct must have a distinct provider of a unique class, and this would be
+ // observable to Starlark code. What would be their names: "struct", or something else? Should
+ // struct+struct fail when different flavors are mixed (as happens today when adding info
+ // instances of different providers)? Or should it return a new struct picking the provider of one
+ // operand arbitrarily (as it does today for custom error strings)? Or ignore providers and return
+ // a plain old struct, always? Or only if they differ? Or should we abolish struct+struct
+ // altogether? In other words, the advice in the @deprecated tag above is not compatible.
+ //
+ // brandjon notes: nearly all the uses of custom errors are for objects that properly should be
+ // Structures but not structs. They only leveraged the struct machinery for historical reasons and
+ // convenience.
+ // For instance, ctx.attr should have a custom error message, but should not support concatenation
+ // (it fails today but only because you can't produce two ctx.attr's that don't have common
+ // fields). It also should not support to_json().
+ // It's possible someone was crazy enough to take ctx.attr.to_json(), but we can probably break
+ // that case without causing too much trouble.
+ // If we migrate all these cases of non-providers away, whatever is left should be happy to use a
+ // default error message, and we can eliminate this extra detail.
@Deprecated
public static StarlarkInfo createWithCustomMessage(
Provider provider, Map<String, Object> values, String unknownFieldError) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkProvider.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkProvider.java
index baadd2a..f71778c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkProvider.java
@@ -45,9 +45,6 @@
*/
public final class StarlarkProvider implements StarlarkCallable, StarlarkExportable, Provider {
- /** Default value for {@link #errorMessageFormatForUnknownField}. */
- private static final String DEFAULT_ERROR_MESSAGE_FORMAT = "Object has no '%s' attribute.";
-
private final Location location;
// For schemaful providers, the sorted list of allowed field names.
@@ -58,9 +55,6 @@
/** Null iff this provider has not yet been exported. */
@Nullable private Key key;
- /** Error message format. Reassigned upon exporting. */
- private String errorMessageFormatForUnknownField;
-
/**
* Creates an unexported {@link StarlarkProvider} with no schema.
*
@@ -126,9 +120,6 @@
this.schema = schema;
this.location = location;
this.key = key; // possibly null
- this.errorMessageFormatForUnknownField =
- key == null ? DEFAULT_ERROR_MESSAGE_FORMAT
- : makeErrorMessageFormatForUnknownField(key.getExportedName());
}
@Override
@@ -174,19 +165,16 @@
}
@Override
- public String getErrorMessageFormatForUnknownField() {
- return errorMessageFormatForUnknownField;
+ public String getErrorMessageForUnknownField(String name) {
+ return String.format(
+ "'%s' value has no field or method '%s'",
+ isExported() ? key.getExportedName() : "struct", name);
}
@Override
public void export(Label extensionLabel, String exportedName) {
Preconditions.checkState(!isExported());
this.key = new Key(extensionLabel, exportedName);
- this.errorMessageFormatForUnknownField = makeErrorMessageFormatForUnknownField(exportedName);
- }
-
- private static String makeErrorMessageFormatForUnknownField(String exportedName) {
- return String.format("'%s' value has no field or method '%%s'", exportedName);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java b/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
index dbcb24f..cf30856 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
@@ -86,15 +86,16 @@
*
* <p>By default, it is the one specified by the provider.
*/
- protected String getErrorMessageFormatForUnknownField() {
- return getProvider().getErrorMessageFormatForUnknownField();
- }
-
@Override
public String getErrorMessageForUnknownField(String name) {
- String suffix = "Available attributes: "
+ return getProvider().getErrorMessageForUnknownField(name) + allAttributesSuffix();
+ }
+
+ final String allAttributesSuffix() {
+ // TODO(adonovan): when is it appropriate for the error to show all attributes,
+ // and when to show a single spelling suggestion (the default)?
+ return "\nAvailable attributes: "
+ Joiner.on(", ").join(Ordering.natural().sortedCopy(getFieldNames()));
- return String.format(getErrorMessageFormatForUnknownField(), name) + "\n" + suffix;
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java b/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java
index dcdd3a5..be24edd 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java
@@ -20,37 +20,33 @@
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkThread;
-import net.starlark.java.syntax.Location;
/**
* The provider for the built-in type {@code struct}.
*
* <p>Its singleton instance is {@link StructProvider#STRUCT}.
*/
-public final class StructProvider extends BuiltinProvider<StructImpl>
+public final class StructProvider extends BuiltinProvider<StarlarkInfo>
implements StructApi.StructProviderApi {
- /** "struct" function. */
+ /** Provider of "struct" instances. */
public static final StructProvider STRUCT = new StructProvider();
- StructProvider() {
- super("struct", StructImpl.class);
+ private StructProvider() {
+ super("struct", StarlarkInfo.class);
}
+ /** Implementation of {@code struct(**kwargs)} function exposed to Starlark. */
@Override
public StructImpl createStruct(Dict<String, Object> kwargs, StarlarkThread thread)
throws EvalException {
- return create(kwargs, thread.getCallerLocation());
- }
-
- public StructImpl create(Map<String, Object> fields, Location location) throws EvalException {
- if (fields.containsKey("to_json")) {
+ if (kwargs.containsKey("to_json")) {
throw Starlark.errorf("cannot override built-in struct function 'to_json'");
}
- if (fields.containsKey("to_proto")) {
+ if (kwargs.containsKey("to_proto")) {
throw Starlark.errorf("cannot override built-in struct function 'to_proto'");
}
- return StarlarkInfo.create(this, fields, location);
+ return StarlarkInfo.create(this, kwargs, thread.getCallerLocation());
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java
index 3414ca4..bb3ed11 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java
@@ -1345,8 +1345,8 @@
}
@Override
- public String getErrorMessageFormatForUnknownField() {
- return "ObjcProvider field '%s' could not be instantiated";
+ public String getErrorMessageForUnknownField(String name) {
+ return String.format("ObjcProvider field '%s' could not be instantiated", name);
}
}
}
