Refactor Info class

This simplifies the location field (now non-nullable), removes a couple redundant loc args, and clarifies the type's description and some javadoc.

RELNOTES: None
PiperOrigin-RevId: 180210943

6 files changed, 69 insertions, 41 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
index fcf06d8fdb..c30a9ed394 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
@@ -205,7 +205,7 @@ public final class ConfiguredAspect {
       return this;
     }
 
-    public Builder addSkylarkDeclaredProvider(Info declaredProvider, Location loc)
+    public Builder addSkylarkDeclaredProvider(Info declaredProvider)
         throws EvalException {
       Provider constructor = declaredProvider.getProvider();
       if (!constructor.isExported()) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index 8f53a9b27b..93b2df3771 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -291,7 +291,7 @@ public final class RuleConfiguredTargetBuilder {
    *
    * <p>Use {@link #addNativeDeclaredProvider(Info)} in definitions of native rules.
    */
-  public RuleConfiguredTargetBuilder addSkylarkDeclaredProvider(Info provider, Location loc)
+  public RuleConfiguredTargetBuilder addSkylarkDeclaredProvider(Info provider)
       throws EvalException {
     Provider constructor = provider.getProvider();
     if (!constructor.isExported()) {
@@ -313,7 +313,7 @@ public final class RuleConfiguredTargetBuilder {
    * Adds "declared providers" defined in native code to the rule. Use this method for declared
    * providers in definitions of native rules.
    *
-   * <p>Use {@link #addSkylarkDeclaredProvider(Info, Location)} for Skylark rule implementations.
+   * <p>Use {@link #addSkylarkDeclaredProvider(Info)} for Skylark rule implementations.
    */
   public RuleConfiguredTargetBuilder addNativeDeclaredProviders(Iterable<Info> providers) {
     for (Info provider : providers) {
@@ -326,7 +326,7 @@ public final class RuleConfiguredTargetBuilder {
    * Adds a "declared provider" defined in native code to the rule. Use this method for declared
    * providers in definitions of native rules.
    *
-   * <p>Use {@link #addSkylarkDeclaredProvider(Info, Location)} for Skylark rule implementations.
+   * <p>Use {@link #addSkylarkDeclaredProvider(Info)} for Skylark rule implementations.
    */
   public RuleConfiguredTargetBuilder addNativeDeclaredProvider(Info provider) {
     Provider constructor = provider.getProvider();
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
index deefbcb972..7f3a3b15b2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
@@ -320,9 +320,7 @@ public final class SkylarkRuleConfiguredTargetUtil {
         parseDefaultProviderKeys(declaredProvider, context, builder);
         defaultProviderProvidedExplicitly = true;
       } else {
-        Location creationLoc = declaredProvider.getCreationLocOrNull();
-        builder.addSkylarkDeclaredProvider(
-            declaredProvider, creationLoc == null ? loc : creationLoc);
+        builder.addSkylarkDeclaredProvider(declaredProvider);
       }
     }
 
@@ -495,7 +493,7 @@ public final class SkylarkRuleConfiguredTargetUtil {
     if (ruleContext.getRule().getRuleClassObject().isSkylarkTestable()) {
       Info actions =
           ActionsProvider.create(ruleContext.getAnalysisEnvironment().getRegisteredActions());
-      builder.addSkylarkDeclaredProvider(actions, loc);
+      builder.addSkylarkDeclaredProvider(actions);
     }
   }
 
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 2c6d3169d2..550ef181ea 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
@@ -35,62 +35,91 @@ import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
 
-/** Represents information provided by a {@link Provider}. */
+/** An instance (in the Skylark sense, not Java) of a {@link Provider}. */
 @SkylarkModule(
   name = "struct",
   category = SkylarkModuleCategory.BUILTIN,
   doc =
-      "A special language element to support structs (i.e. simple value objects). "
-          + "See the global <a href=\"globals.html#struct\">struct</a> function "
-          + "for more details."
+      "A generic object with fields. See the global <a href=\"globals.html#struct\"><code>struct"
+          + "</code></a> function for more details."
+          + "<p>Structs fields cannot be reassigned once the struct is created. Two structs are "
+          + "equal if they have the same fields and if corresponding field values are equal."
 )
 public abstract class Info implements ClassObject, SkylarkValue, Serializable {
+
+  /** The {@link Provider} that describes the type of this instance. */
   private final Provider provider;
+
+  /**
+   * The Skylark location where this provider instance was created.
+   *
+   * <p>Built-in provider instances may use {@link Location#BUILTIN}.
+   */
   private final Location creationLoc;
-  private final String errorMessage;
 
-  /** Creates an empty struct with a given location. */
-  public Info(Provider provider, Location location) {
+  /**
+   * Formattable string with one {@code '%s'} placeholder for the missing field name.
+   */
+  private final String errorMessageFormatForUnknownField;
+
+  /**
+   * Creates an empty struct with a given location.
+   *
+   * <p>If {@code location} is null, it defaults to {@link Location#BUILTIN}.
+   */
+  public Info(Provider provider, @Nullable Location location) {
     this.provider = provider;
-    this.creationLoc = location;
-    this.errorMessage = provider.getErrorMessageFormatForInstances();
+    this.creationLoc = location == null ? Location.BUILTIN : location;
+    this.errorMessageFormatForUnknownField = provider.getErrorMessageFormatForInstances();
   }
 
-  /** Creates a built-in struct (i.e. without creation loc). */
+  /** Creates a built-in struct (i.e. without a Skylark creation location). */
   public Info(Provider provider) {
     this.provider = provider;
-    this.creationLoc = null;
-    this.errorMessage = provider.getErrorMessageFormatForInstances();
+    this.creationLoc = Location.BUILTIN;
+    this.errorMessageFormatForUnknownField = provider.getErrorMessageFormatForInstances();
   }
 
   /**
-   * Creates a built-in struct (i.e. without creation loc).
+   * Creates a built-in struct (i.e. without creation location) that uses a specific error message
+   * for missing fields.
    *
-   * <p>Allows to supply a specific error message. Only used in
-   * {@link com.google.devtools.build.lib.packages.NativeProvider.StructConstructor#create(Map,
-   * String)} If you need to override an error message, preferred way is to create a specific {@link
-   * NativeProvider}.
+   * <p>Only used in {@link
+   * com.google.devtools.build.lib.packages.NativeProvider.StructConstructor#create(Map, String)}.
+   * If you need to override an error message, the preferred way is to create a new {@link
+   * NativeProvider} subclass.
    */
-  Info(Provider provider, String errorMessage) {
+  Info(Provider provider, String errorMessageFormatForUnknownField) {
     this.provider = provider;
-    this.creationLoc = null;
-    this.errorMessage = Preconditions.checkNotNull(errorMessage);
+    this.creationLoc = Location.BUILTIN;
+    this.errorMessageFormatForUnknownField =
+        Preconditions.checkNotNull(errorMessageFormatForUnknownField);
   }
 
-  // Ensure that values are all acceptable to Skylark before to stuff them in a ClassObject
+  /**
+   * Preprocesses a map of field values to convert the field names and field values to
+   * Skylark-acceptable names and types.
+   */
   protected static ImmutableMap<String, Object> copyValues(Map<String, Object> values) {
     ImmutableMap.Builder<String, Object> builder = ImmutableMap.builder();
     for (Map.Entry<String, Object> e : values.entrySet()) {
       builder.put(
-          Attribute.getSkylarkName(e.getKey()), SkylarkType.convertToSkylark(e.getValue(), null));
+          Attribute.getSkylarkName(e.getKey()),
+          SkylarkType.convertToSkylark(e.getValue(), /*env=*/ null));
     }
     return builder.build();
   }
 
+  /**
+   * Returns whether the given field name exists.
+   *
+   * <p>The "key" nomenclature is historic and for consistency with {@link ClassObject}.
+   */
+  // TODO(bazel-team): Rename to hasField(), and likewise in ClassObject.
   public abstract boolean hasKey(String name);
 
   /** Returns a value and try to cast it into specified type */
-  public <TYPE> TYPE getValue(String key, Class<TYPE> type) throws EvalException {
+  public <T> T getValue(String key, Class<T> type) throws EvalException {
     Object obj = getValue(key);
     if (obj == null) {
       return null;
@@ -99,19 +128,19 @@ public abstract class Info implements ClassObject, SkylarkValue, Serializable {
     return type.cast(obj);
   }
 
+  /**
+   * Returns the Skylark location where this provider instance was created.
+   *
+   * <p>Builtin provider instances may return {@link Location#BUILTIN}.
+   */
   public Location getCreationLoc() {
-    return Preconditions.checkNotNull(creationLoc, "This struct was not created in a Skylark code");
+    return creationLoc;
   }
 
   public Provider getProvider() {
     return provider;
   }
 
-  @Nullable
-  public Location getCreationLocOrNull() {
-    return creationLoc;
-  }
-
   /**
    * Returns the fields of this struct.
    *
@@ -132,11 +161,12 @@ public abstract class Info implements ClassObject, SkylarkValue, Serializable {
   @Override
   public abstract Object getValue(String name);
 
+  // TODO(bazel-team): Rename to getErrorMessageForUnknownField.
   @Override
   public String errorMessage(String name) {
     String suffix =
         "Available attributes: " + Joiner.on(", ").join(Ordering.natural().sortedCopy(getKeys()));
-    return String.format(errorMessage, name) + "\n" + suffix;
+    return String.format(errorMessageFormatForUnknownField, name) + "\n" + suffix;
   }
 
   @Override
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 2616a209e0..028a03d446 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
@@ -95,8 +95,9 @@ public abstract class Provider extends BaseFunction {
   /**
    * Returns an error message format for instances.
    *
-   * <p>Must contain one '%s' placeholder for field name.
+   * <p>Must contain one {@code '%s'} placeholder for field name.
    */
+  // TODO(bazel-team): Rename to getErrorMessageFormatForUnknownField().
   public abstract String getErrorMessageFormatForInstances();
 
   public SkylarkProviderIdentifier id() {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index ca57cbc88e..24112a4fae 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -155,8 +155,7 @@ public class SkylarkAspectFactory implements ConfiguredAspectFactory {
                   + "a sequence of declared providers, instead got a %s at index %d",
               o.getClass(),
               i);
-      Location creationLoc = declaredProvider.getCreationLocOrNull();
-      builder.addSkylarkDeclaredProvider(declaredProvider, creationLoc != null ? creationLoc : loc);
+      builder.addSkylarkDeclaredProvider(declaredProvider);
       i++;
     }
   }