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);
     }
   }
 }