2 files changed, 126 insertions, 6 deletions
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 c301061dc3..2c7a2c888f 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
@@ -370,7 +370,10 @@ public final class FuncallExpression extends Expression {
     }
     if (matchingMethod == null) {
       String errorMessage;
-      if (argumentListConversionResult == null || argumentListConversionResult.getError() == null) {
+      if (ClassObject.class.isAssignableFrom(objClass)) {
+        errorMessage = String.format("struct has no method '%s'", methodName);
+      } else if (argumentListConversionResult == null
+          || argumentListConversionResult.getError() == null) {
         errorMessage =
             String.format(
                 "type '%s' has no method %s",
@@ -606,6 +609,8 @@ public final class FuncallExpression extends Expression {
     Object value = positionals.get(0);
     ImmutableList<Object> positionalArgs = positionals.subList(1, positionals.size());
     BaseFunction function = Runtime.getFunction(EvalUtils.getSkylarkType(value.getClass()), method);
+    Object fieldValue =
+        (value instanceof ClassObject) ? ((ClassObject) value).getValue(method) : null;
     if (function != null) {
       if (!isNamespace(value.getClass())) {
         // Use self as an implicit parameter in front.
@@ -613,11 +618,7 @@ public final class FuncallExpression extends Expression {
       }
       return function.call(
           positionalArgs, ImmutableMap.<String, Object>copyOf(keyWordArgs), call, env);
-    } else if (value instanceof ClassObject) {
-      Object fieldValue = ((ClassObject) value).getValue(method);
-      if (fieldValue == null) {
-        throw new EvalException(location, String.format("struct has no method '%s'", method));
-      }
+    } else if (fieldValue != null) {
       if (!(fieldValue instanceof BaseFunction)) {
         throw new EvalException(
             location, String.format("struct field '%s' is not a function", method));
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 a72f5e5a91..32932b6a73 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
@@ -26,6 +26,8 @@ import com.google.devtools.build.lib.analysis.FileConfiguredTarget;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.packages.NativeClassObjectConstructor;
+import com.google.devtools.build.lib.packages.SkylarkClassObject;
 import com.google.devtools.build.lib.skylarkinterface.Param;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
@@ -1288,4 +1290,121 @@ public class SkylarkEvaluationTest extends EvaluationTest {
     new SkylarkTest().setUp("def foo(a, b, c): return a+b if c else a-b\n").testStatement(
         "foo(23, 5, 0)", 18);
   }
+
+  @SkylarkModule(name = "SkylarkClassObjectWithSkylarkCallables", doc = "")
+  static final class SkylarkClassObjectWithSkylarkCallables extends SkylarkClassObject {
+    private static final NativeClassObjectConstructor CONSTRUCTOR =
+        new NativeClassObjectConstructor("struct_with_skylark_callables") {};
+
+    SkylarkClassObjectWithSkylarkCallables() {
+      super(
+          CONSTRUCTOR,
+          ImmutableMap.of(
+              "values_only_field",
+              "fromValues",
+              "values_only_method",
+              new BuiltinFunction("values_only_method", FunctionSignature.of()) {
+                public String invoke() {
+                  return "fromValues";
+                }
+              },
+              "collision_field",
+              "fromValues",
+              "collision_method",
+              new BuiltinFunction("collision_method", FunctionSignature.of()) {
+                public String invoke() {
+                  return "fromValues";
+                }
+              }));
+    }
+
+    @SkylarkCallable(name = "callable_only_field", doc = "", structField = true)
+    public String getCallableOnlyField() {
+      return "fromSkylarkCallable";
+    }
+
+    @SkylarkCallable(name = "callable_only_method", doc = "", structField = false)
+    public String getCallableOnlyMethod() {
+      return "fromSkylarkCallable";
+    }
+
+    @SkylarkCallable(name = "collision_field", doc = "", structField = true)
+    public String getCollisionField() {
+      return "fromSkylarkCallable";
+    }
+
+    @SkylarkCallable(name = "collision_method", doc = "", structField = false)
+    public String getCollisionMethod() {
+      return "fromSkylarkCallable";
+    }
+  }
+
+  @Test
+  public void testStructFieldDefinedOnlyInValues() throws Exception {
+    new SkylarkTest()
+        .update("val", new SkylarkClassObjectWithSkylarkCallables())
+        .setUp("v = val.values_only_field")
+        .testLookup("v", "fromValues");
+  }
+
+  @Test
+  public void testStructMethodDefinedOnlyInValues() throws Exception {
+    new SkylarkTest()
+        .update("val", new SkylarkClassObjectWithSkylarkCallables())
+        .setUp("v = val.values_only_method()")
+        .testLookup("v", "fromValues");
+  }
+
+  @Test
+  public void testStructFieldDefinedOnlyInSkylarkCallable() throws Exception {
+    new SkylarkTest()
+        .update("val", new SkylarkClassObjectWithSkylarkCallables())
+        .setUp("v = val.callable_only_field")
+        .testLookup("v", "fromSkylarkCallable");
+  }
+
+  @Test
+  public void testStructMethodDefinedOnlyInSkylarkCallable() throws Exception {
+    new SkylarkTest()
+        .update("val", new SkylarkClassObjectWithSkylarkCallables())
+        .setUp("v = val.callable_only_method()")
+        .testLookup("v", "fromSkylarkCallable");
+  }
+
+  @Test
+  public void testStructFieldDefinedInValuesAndSkylarkCallable() throws Exception {
+    new SkylarkTest()
+        .update("val", new SkylarkClassObjectWithSkylarkCallables())
+        .setUp("v = val.collision_field")
+        .testLookup("v", "fromValues");
+  }
+
+  @Test
+  public void testStructMethodDefinedInValuesAndSkylarkCallable() throws Exception {
+    new SkylarkTest()
+        .update("val", new SkylarkClassObjectWithSkylarkCallables())
+        .setUp("v = val.collision_method()")
+        .testLookup("v", "fromValues");
+  }
+
+  @Test
+  public void testStructFieldNotDefined() throws Exception {
+    new SkylarkTest()
+        .update("val", new SkylarkClassObjectWithSkylarkCallables())
+        .testIfExactError(
+            // TODO(bazel-team): This should probably list callable_only_field/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",
+            "v = val.nonexistent_field");
+  }
+
+  @Test
+  public void testStructMethodNotDefined() throws Exception {
+    new SkylarkTest()
+        .update("val", new SkylarkClassObjectWithSkylarkCallables())
+        .testIfExactError(
+            // TODO(bazel-team): This should probably match the error above better.
+            "struct has no method 'nonexistent_method'", "v = val.nonexistent_method()");
+  }
 }