Expose structField callable methods of skylark objects to dir() and str() calls

RELNOTES: None.
PiperOrigin-RevId: 184498836

7 files changed, 180 insertions, 33 deletions

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 50fcb156c3..33b8c95d13 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
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.packages;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
@@ -108,14 +109,17 @@ public abstract class Info implements ClassObject, SkylarkValue, Serializable {
   public abstract boolean hasField(String name);
 
   /**
-   * {@inheritDoc}
-   *
-   * <p>Overrides {@link ClassObject#getValue(String)}, but does not allow {@link EvalException} to
-   * be thrown.
+   * <p>Wraps {@link ClassObject#getValue(String)}, returning null in cases where
+   * {@link EvalException} would have been thrown.
    */
-  @Nullable
-  @Override
-  public abstract Object getValue(String name);
+  @VisibleForTesting
+  public Object getValueOrNull(String name) {
+    try {
+      return getValue(name);
+    } catch (EvalException e) {
+      return null;
+    }
+  }
 
   /**
    * Returns the result of {@link #getValue(String)}, cast as the given type, throwing {@link
@@ -172,7 +176,7 @@ public abstract class Info implements ClassObject, SkylarkValue, Serializable {
       return false;
     }
     for (String field : getFieldNames()) {
-      if (!this.getValue(field).equals(other.getValue(field))) {
+      if (!Objects.equal(this.getValueOrNull(field), other.getValueOrNull(field))) {
         return false;
       }
     }
@@ -187,7 +191,7 @@ public abstract class Info implements ClassObject, SkylarkValue, Serializable {
     objectsToHash.add(provider);
     for (String field : fields) {
       objectsToHash.add(field);
-      objectsToHash.add(getValue(field));
+      objectsToHash.add(getValueOrNull(field));
     }
     return Objects.hashCode(objectsToHash.toArray());
   }
@@ -208,7 +212,7 @@ public abstract class Info implements ClassObject, SkylarkValue, Serializable {
       first = false;
       printer.append(fieldName);
       printer.append(" = ");
-      printer.repr(getValue(fieldName));
+      printer.repr(getValueOrNull(fieldName));
     }
     printer.append(")");
   }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/NativeInfo.java b/src/main/java/com/google/devtools/build/lib/packages/NativeInfo.java
index e1035c86e9..58ea8d3ea0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NativeInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NativeInfo.java
@@ -15,7 +15,11 @@ package com.google.devtools.build.lib.packages;
 
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.FuncallExpression;
+import com.google.devtools.build.lib.syntax.FuncallExpression.MethodDescriptor;
 import java.util.Map;
 
 /** Base class for native implementations of {@link Info}. */
@@ -23,19 +27,35 @@ import java.util.Map;
 public class NativeInfo extends Info {
   protected final ImmutableMap<String, Object> values;
 
+  // Initialized lazily.
+  private ImmutableSet<String> fieldNames;
+
   @Override
-  public Object getValue(String name) {
-    return values.get(name);
+  public Object getValue(String name) throws EvalException {
+    if (values.containsKey(name)) {
+      return values.get(name);
+    } else if (hasField(name)) {
+      MethodDescriptor methodDescriptor = FuncallExpression.getStructField(this.getClass(), name);
+      return FuncallExpression.invokeStructField(methodDescriptor, name, this);
+    } else {
+      return null;
+    }
   }
 
   @Override
   public boolean hasField(String name) {
-    return values.containsKey(name);
+    return getFieldNames().contains(name);
   }
 
   @Override
   public ImmutableCollection<String> getFieldNames() {
-    return values.keySet();
+    if (fieldNames == null) {
+      fieldNames = ImmutableSet.<String>builder()
+          .addAll(values.keySet())
+          .addAll(FuncallExpression.getStructFieldNames(this.getClass()))
+          .build();
+    }
+    return fieldNames;
   }
 
   public NativeInfo(NativeProvider<?> provider) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index 833b20a7fb..241789d268 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -42,12 +42,14 @@ import java.lang.reflect.Modifier;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
 /**
@@ -134,6 +136,48 @@ public final class FuncallExpression extends Expression {
                 }
               });
 
+  private static final LoadingCache<Class<?>, Map<String, MethodDescriptor>> fieldCache =
+      CacheBuilder.newBuilder()
+          .initialCapacity(10)
+          .maximumSize(100)
+          .build(
+              new CacheLoader<Class<?>, Map<String, MethodDescriptor>>() {
+
+                @Override
+                public Map<String, MethodDescriptor> load(Class<?> key) throws Exception {
+                  ImmutableMap.Builder<String, MethodDescriptor> fieldMap = ImmutableMap.builder();
+                  HashSet<String> fieldNamesForCollisions = new HashSet<>();
+                  List<MethodDescriptor> fieldMethods =
+                      methodCache
+                          .get(key)
+                          .values()
+                          .stream()
+                          .flatMap(List::stream)
+                          .filter(
+                              methodDescriptor -> methodDescriptor.getAnnotation().structField())
+                          .collect(Collectors.toList());
+
+                  for (MethodDescriptor fieldMethod : fieldMethods) {
+                    SkylarkCallable callable = fieldMethod.getAnnotation();
+                    String name = callable.name();
+                    if (name.isEmpty()) {
+                      name =
+                          StringUtilities.toPythonStyleFunctionName(
+                              fieldMethod.getMethod().getName());
+                    }
+                    // TODO(b/72113542): Validate with annotation processor instead of at runtime.
+                    if (!fieldNamesForCollisions.add(name)) {
+                      throw new IllegalArgumentException(
+                          String.format(
+                              "Class %s has two structField methods named %s defined",
+                              key.getName(), name));
+                    }
+                    fieldMap.put(name, fieldMethod);
+                  }
+                  return fieldMap.build();
+                }
+              });
+
   /**
    * Returns a map of methods and corresponding SkylarkCallable annotations of the methods of the
    * classObj class reachable from Skylark.
@@ -260,15 +304,30 @@ public final class FuncallExpression extends Expression {
     return printer.toString();
   }
 
-  /**
-   * Returns the list of Skylark callable Methods of objClass with the given name and argument
-   * number.
-   */
+  /** Returns the Skylark callable Method of objClass with structField=true and the given name. */
+  public static MethodDescriptor getStructField(Class<?> objClass, String methodName) {
+    try {
+      return fieldCache.get(objClass).get(methodName);
+    } catch (ExecutionException e) {
+      throw new IllegalStateException("Method loading failed: " + e);
+    }
+  }
+
+  /** Returns the list of names of Skylark callable Methods of objClass with structField=true. */
+  public static Set<String> getStructFieldNames(Class<?> objClass) {
+    try {
+      return fieldCache.get(objClass).keySet();
+    } catch (ExecutionException e) {
+      throw new IllegalStateException("Method loading failed: " + e);
+    }
+  }
+
+  /** Returns the list of Skylark callable Methods of objClass with the given name. */
   public static List<MethodDescriptor> getMethods(Class<?> objClass, String methodName) {
     try {
       return methodCache.get(objClass).get(methodName);
     } catch (ExecutionException e) {
-      throw new IllegalStateException("method invocation failed: " + e);
+      throw new IllegalStateException("Method loading failed: " + e);
     }
   }
 
@@ -280,10 +339,25 @@ public final class FuncallExpression extends Expression {
     try {
       return methodCache.get(objClass).keySet();
     } catch (ExecutionException e) {
-      throw new IllegalStateException("method invocation failed: " + e);
+      throw new IllegalStateException("Method loading failed: " + e);
     }
   }
 
+  /**
+   * Invokes the given structField=true method and returns the result.
+   *
+   * @param methodDescriptor the descriptor of the method to invoke
+   * @param fieldName the name of the struct field
+   * @param obj the object on which to invoke the method
+   * @return the method return value
+   * @throws EvalException if there was an issue evaluating the method
+   */
+  public static Object invokeStructField(
+      MethodDescriptor methodDescriptor, String fieldName, Object obj) throws EvalException {
+    Preconditions.checkArgument(methodDescriptor.getAnnotation().structField());
+    return callMethod(methodDescriptor, fieldName, obj, new Object[0], Location.BUILTIN, null);
+  }
+
   static Object callMethod(MethodDescriptor methodDescriptor, String methodName, Object obj,
       Object[] args, Location loc, Environment env) throws EvalException {
     try {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformInfoTest.java b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformInfoTest.java
index fa0add7958..8980635b79 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformInfoTest.java
@@ -95,12 +95,6 @@ public class PlatformInfoTest extends BuildViewTestCase {
                 .setLabel(makeLabel("//platform/plat1"))
                 .addConstraint(value1)
                 .addConstraint(value2)
-                .build(),
-            PlatformInfo.builder()
-                .setLabel(makeLabel("//platform/plat1"))
-                .addConstraint(value1)
-                .addConstraint(value2)
-                .setRemoteExecutionProperties("key=val") // execution properties are ignored.
                 .build())
         .addEqualityGroup(
             // Different label.
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
index ef0b7468ab..a4cb31a093 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
@@ -139,7 +139,7 @@ public final class SkylarkProviderTest {
   }
 
   /** Asserts that a {@link SkylarkInfo} has fields a=1, b=2, c=3 (and nothing else). */
-  private static void assertHasExactlyValuesA1B2C3(SkylarkInfo info) {
+  private static void assertHasExactlyValuesA1B2C3(SkylarkInfo info) throws Exception {
     assertThat(info.getFieldNames()).containsExactly("a", "b", "c");
     assertThat(info.getValue("a")).isEqualTo(1);
     assertThat(info.getValue("b")).isEqualTo(2);
diff --git a/src/test/java/com/google/devtools/build/lib/rules/java/JavaInfoSkylarkApiTest.java b/src/test/java/com/google/devtools/build/lib/rules/java/JavaInfoSkylarkApiTest.java
index dc8b505788..c0e9ade55e 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/java/JavaInfoSkylarkApiTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/java/JavaInfoSkylarkApiTest.java
@@ -19,7 +19,6 @@ import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.prettyA
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.packages.Info;
 import com.google.devtools.build.lib.packages.SkylarkProvider.SkylarkKey;
 import com.google.devtools.build.lib.rules.java.JavaRuleOutputJarsProvider.OutputJar;
@@ -724,7 +723,7 @@ public class JavaInfoSkylarkApiTest extends BuildViewTestCase {
     }
   }
 
-  private JavaInfo fetchJavaInfo() throws LabelSyntaxException {
+  private JavaInfo fetchJavaInfo() throws Exception {
     ConfiguredTarget myRuleTarget = getConfiguredTarget("//foo:my_skylark_rule");
     Info info =
         myRuleTarget.get(new SkylarkKey(Label.parseAbsolute("//foo:extension.bzl"), "result"));
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index 6baa230f79..c234a92a57 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -76,6 +76,42 @@ public class SkylarkEvaluationTest extends EvaluationTest {
   };
 
   @SkylarkModule(name = "Mock", doc = "")
+  static class NativeInfoMock extends NativeInfo {
+
+    private static final NativeProvider<NativeInfoMock> CONSTRUCTOR =
+        new NativeProvider<NativeInfoMock>(NativeInfoMock.class, "native_info_mock") {};
+
+    public NativeInfoMock() {
+      super(CONSTRUCTOR);
+    }
+
+    @SkylarkCallable(name = "callable_string", doc = "", structField = false)
+    public String callableString() {
+      return "a";
+    }
+
+    @SkylarkCallable(name = "struct_field_string", doc = "", structField = true)
+    public String structFieldString() {
+      return "a";
+    }
+
+    @SkylarkCallable(name = "struct_field_callable", doc = "", structField = true)
+    public BuiltinFunction structFieldCallable() {
+      return foobar;
+    }
+
+    @SkylarkCallable(
+      name = "struct_field_none",
+      doc = "",
+      structField = true,
+      allowReturnNones = true
+    )
+    public String structFieldNone() {
+      return null;
+    }
+  }
+
+  @SkylarkModule(name = "Mock", doc = "")
   static class Mock {
     @SkylarkCallable(doc = "")
     public static Integer valueOf(String str) {
@@ -1193,7 +1229,7 @@ public class SkylarkEvaluationTest extends EvaluationTest {
         "  return e",
         "e = str(func())").testLookup("e", "[3, [1, 4]]");
   }
-  
+
   @Test
   public void testDictTupleAssignmentAsLValue() throws Exception {
     new SkylarkTest().setUp("def func():",
@@ -1355,6 +1391,26 @@ public class SkylarkEvaluationTest extends EvaluationTest {
   }
 
   @Test
+  public void testStrNativeInfo() throws Exception {
+    new SkylarkTest()
+        .update("mock", new NativeInfoMock())
+        .testEval(
+            "str(mock)",
+            "'struct(struct_field_callable = <built-in function foobar>, struct_field_none = None, "
+                + "struct_field_string = \"a\")'");
+  }
+
+  @Test
+  public void testDirNativeInfo() throws Exception {
+    new SkylarkTest()
+        .update("mock", new NativeInfoMock())
+        .testEval(
+            "dir(mock)",
+            "['callable_string', 'struct_field_callable', "
+                + "'struct_field_none', 'struct_field_string']");
+  }
+
+  @Test
   public void testPrint() throws Exception {
     // TODO(fwe): cannot be handled by current testing suite
     setFailFast(false);
@@ -1561,10 +1617,10 @@ public class SkylarkEvaluationTest extends EvaluationTest {
     new SkylarkTest()
         .update("val", new SkylarkClassObjectWithSkylarkCallables())
         .testIfExactError(
-            // TODO(bazel-team): This should probably list callable_only_field/method as well.
+            // TODO(bazel-team): This should probably list callable_only_method as well.
             "'struct_with_skylark_callables' object has no attribute 'nonexistent_field'\n"
-                + "Available attributes: collision_field, collision_method, values_only_field, "
-                + "values_only_method",
+                + "Available attributes: callable_only_field, collision_field, collision_method, "
+                + "values_only_field, values_only_method",
             "v = val.nonexistent_field");
   }