Refactor getParentWithSkylarkModule() into SkylarkInterfaceUtils

--
MOS_MIGRATED_REVID=135956016

5 files changed, 66 insertions, 64 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
index e9fef91231..9f6783487c 100644
--- a/src/main/java/com/google/devtools/build/lib/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -353,6 +353,7 @@ java_library(
         "skylarkinterface/*.java",
     ]),
     deps = [
+        "//src/main/java/com/google/devtools/build/lib:util",
         "//third_party:jsr305",
     ],
 )
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java
index 348a9ccef4..bee73efaf8 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.skylarkinterface;
 
+import com.google.devtools.build.lib.util.Pair;
 import java.lang.reflect.Method;
 import javax.annotation.Nullable;
 
@@ -22,33 +23,51 @@ import javax.annotation.Nullable;
  */
 public class SkylarkInterfaceUtils {
 
-  /**
-   * Returns the {@link SkylarkModule} annotation for the given class, if it exists, and
-   * null otherwise. The first annotation found will be returned, starting with {@code classObj}
-   * and following its base classes and interfaces recursively.
-   */
   @Nullable
-  public static SkylarkModule getSkylarkModule(Class<?> classObj) {
+  private static Pair<Class<?>, SkylarkModule> searchForSkylarkModule(Class<?> classObj) {
     if (classObj.isAnnotationPresent(SkylarkModule.class)) {
-      return classObj.getAnnotation(SkylarkModule.class);
+      return new Pair<Class<?>, SkylarkModule>(
+          classObj, classObj.getAnnotation(SkylarkModule.class));
     }
     Class<?> superclass = classObj.getSuperclass();
     if (superclass != null) {
-      SkylarkModule annotation = getSkylarkModule(superclass);
-      if (annotation != null) {
-        return annotation;
+      Pair<Class<?>, SkylarkModule> result = searchForSkylarkModule(superclass);
+      if (result != null) {
+        return result;
       }
     }
     for (Class<?> interfaceObj : classObj.getInterfaces()) {
-      SkylarkModule annotation = getSkylarkModule(interfaceObj);
-      if (annotation != null) {
-        return annotation;
+      Pair<Class<?>, SkylarkModule> result = searchForSkylarkModule(interfaceObj);
+      if (result != null) {
+        return result;
       }
     }
     return null;
   }
 
   /**
+   * Returns the {@link SkylarkModule} annotation for the given class, if it exists, and
+   * null otherwise. The first annotation found will be returned, starting with {@code classObj}
+   * and following its base classes and interfaces recursively.
+   */
+  @Nullable
+  public static SkylarkModule getSkylarkModule(Class<?> classObj) {
+    Pair<Class<?>, SkylarkModule> result = searchForSkylarkModule(classObj);
+    return result == null ? null : result.second;
+  }
+
+  /**
+   * Searches {@code classObj}'s class hierarchy and returns the first superclass or interface that
+   * is annotated with {@link SkylarkModule} (including possibly {@code classObj} itself), or null
+   * if none is found.
+   */
+  @Nullable
+  public static Class<?> getParentWithSkylarkModule(Class<?> classObj) {
+    Pair<Class<?>, SkylarkModule> result = searchForSkylarkModule(classObj);
+    return result == null ? null : result.first;
+  }
+
+  /**
    * Returns the {@link SkylarkCallable} annotation for the given method, if it exists, and
    * null otherwise. The first annotation of an overridden version of the method that is found
    * will be returned, starting with {@code classObj} and following its base classes and
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 9609544e19..29667813ad 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.syntax;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
@@ -155,31 +154,6 @@ public final class EvalUtils {
         || c.equals(PathFragment.class); // other known class
   }
 
-  /**
-   * Returns a transitive superclass or interface implemented by c which is annotated
-   * with SkylarkModule. Returns null if no such class or interface exists.
-   */
-  @VisibleForTesting
-  static Class<?> getParentWithSkylarkModule(Class<?> c) {
-    if (c == null) {
-      return null;
-    }
-    if (c.isAnnotationPresent(SkylarkModule.class)) {
-      return c;
-    }
-    Class<?> parent = getParentWithSkylarkModule(c.getSuperclass());
-    if (parent != null) {
-      return parent;
-    }
-    for (Class<?> ifparent : c.getInterfaces()) {
-      ifparent = getParentWithSkylarkModule(ifparent);
-      if (ifparent != null) {
-        return ifparent;
-      }
-    }
-    return null;
-  }
-
   // TODO(bazel-team): move the following few type-related functions to SkylarkType
   /**
    * Return the Skylark-type of {@code c}
@@ -205,10 +179,7 @@ public final class EvalUtils {
     }
     // TODO(bazel-team): also unify all implementations of ClassObject,
     // that we used to all print the same as "struct"?
-    //
-    // Check if one of the superclasses or implemented interfaces has the SkylarkModule
-    // annotation. If yes return that class.
-    Class<?> parent = getParentWithSkylarkModule(c);
+    Class<?> parent = SkylarkInterfaceUtils.getParentWithSkylarkModule(c);
     if (parent != null) {
       return parent;
     }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkInterfaceUtilsTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkInterfaceUtilsTest.java
index 23fc0b39d3..12aaf0ba58 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkInterfaceUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkInterfaceUtilsTest.java
@@ -85,35 +85,49 @@ public class SkylarkInterfaceUtilsTest {
   public static class MockClassZ {
   }
 
+  // The tests for getSkylarkModule() double as tests for getParentWithSkylarkModule(),
+  // since they share an implementation.
+
   @Test
   public void testGetSkylarkModuleBasic() throws Exception {
     // Normal case.
     SkylarkModule ann = SkylarkInterfaceUtils.getSkylarkModule(MockClassA.class);
+    Class<?> cls = SkylarkInterfaceUtils.getParentWithSkylarkModule(MockClassA.class);
     assertThat(ann).isNotNull();
     assertThat(ann.doc()).isEqualTo("MockClassA");
+    assertThat(cls).isNotNull();
+    assertThat(cls).isEqualTo(MockClassA.class);
   }
 
   @Test
   public void testGetSkylarkModuleSubclass() throws Exception {
     // Subclass's annotation is used.
     SkylarkModule ann = SkylarkInterfaceUtils.getSkylarkModule(MockClassC.class);
+    Class<?> cls = SkylarkInterfaceUtils.getParentWithSkylarkModule(MockClassC.class);
     assertThat(ann).isNotNull();
     assertThat(ann.doc()).isEqualTo("MockClassC");
+    assertThat(cls).isNotNull();
+    assertThat(cls).isEqualTo(MockClassC.class);
   }
 
   @Test
   public void testGetSkylarkModuleSubclassNoSubannotation() throws Exception {
     // Falls back on superclass's annotation.
     SkylarkModule ann = SkylarkInterfaceUtils.getSkylarkModule(MockClassD.class);
+    Class<?> cls = SkylarkInterfaceUtils.getParentWithSkylarkModule(MockClassD.class);
     assertThat(ann).isNotNull();
     assertThat(ann.doc()).isEqualTo("MockClassC");
+    assertThat(cls).isNotNull();
+    assertThat(cls).isEqualTo(MockClassC.class);
   }
 
   @Test
   public void testGetSkylarkModuleNotFound() throws Exception {
     // Doesn't exist.
     SkylarkModule ann = SkylarkInterfaceUtils.getSkylarkModule(MockClassZ.class);
+    Class<?> cls = SkylarkInterfaceUtils.getParentWithSkylarkModule(MockClassZ.class);
     assertThat(ann).isNull();
+    assertThat(cls).isNull();
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
index 661b8636a2..7acaaa7723 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java
@@ -265,37 +265,33 @@ public class ValidationTest extends EvaluationTestCase {
   }
 
   @Test
-  public void testParentWithSkylarkModule() throws Exception {
+  public void testGetSkylarkType() throws Exception {
     Class<?> emptyTupleClass = Tuple.empty().getClass();
     Class<?> tupleClass = Tuple.of(1, "a", "b").getClass();
-    Class<?> mutableListClass = new MutableList(Tuple.of(1, 2, 3), env).getClass();
+    Class<?> mutableListClass = new MutableList<>(Tuple.of(1, 2, 3), env).getClass();
 
-    assertThat(mutableListClass).isEqualTo(MutableList.class);
+    assertThat(EvalUtils.getSkylarkType(mutableListClass)).isEqualTo(MutableList.class);
     assertThat(MutableList.class.isAnnotationPresent(SkylarkModule.class)).isTrue();
-    assertThat(EvalUtils.getParentWithSkylarkModule(MutableList.class))
-        .isEqualTo(MutableList.class);
-    assertThat(EvalUtils.getParentWithSkylarkModule(emptyTupleClass)).isEqualTo(Tuple.class);
-    assertThat(EvalUtils.getParentWithSkylarkModule(tupleClass)).isEqualTo(Tuple.class);
-    // TODO(bazel-team): make a tuple not a list anymore.
-    assertThat(EvalUtils.getParentWithSkylarkModule(tupleClass)).isEqualTo(Tuple.class);
+    assertThat(EvalUtils.getSkylarkType(emptyTupleClass)).isEqualTo(Tuple.class);
+    assertThat(EvalUtils.getSkylarkType(tupleClass)).isEqualTo(Tuple.class);
 
-    // TODO(bazel-team): fix that?
-    assertThat(ClassObject.class.isAnnotationPresent(SkylarkModule.class)).isFalse();
-    assertThat(SkylarkClassObject.class.isAnnotationPresent(SkylarkModule.class))
-        .isTrue();
-    assertThat(
-            EvalUtils.getParentWithSkylarkModule(SkylarkClassObject.class)
-                == SkylarkClassObject.class)
-        .isTrue();
-    assertThat(EvalUtils.getParentWithSkylarkModule(ClassObject.class)).isNull();
+    assertThat(EvalUtils.getSkylarkType(SkylarkClassObject.class))
+        .isEqualTo(SkylarkClassObject.class);
+    try {
+      EvalUtils.getSkylarkType(ClassObject.class);
+      throw new Exception("Should have raised IllegalArgumentException exception");
+    } catch (IllegalArgumentException e) {
+      assertThat(e.getMessage()).contains(
+          "interface com.google.devtools.build.lib.syntax.ClassObject is not allowed "
+          + "as a Skylark value");
+    }
   }
 
   @Test
   public void testSkylarkTypeEquivalence() throws Exception {
-    // All subclasses of SkylarkList are made equivalent
     Class<?> emptyTupleClass = Tuple.empty().getClass();
     Class<?> tupleClass = Tuple.of(1, "a", "b").getClass();
-    Class<?> mutableListClass = new MutableList(Tuple.of(1, 2, 3), env).getClass();
+    Class<?> mutableListClass = new MutableList<>(Tuple.of(1, 2, 3), env).getClass();
 
     assertThat(SkylarkType.of(mutableListClass)).isEqualTo(SkylarkType.LIST);
     assertThat(SkylarkType.of(emptyTupleClass)).isEqualTo(SkylarkType.TUPLE);
@@ -315,7 +311,8 @@ public class ValidationTest extends EvaluationTestCase {
     // TODO(bazel-team): move to some other place to remove dependency of syntax tests on Artifact?
     assertThat(SkylarkType.of(Artifact.SpecialArtifact.class))
         .isEqualTo(SkylarkType.of(Artifact.class));
-    assertThat(SkylarkType.of(RuleConfiguredTarget.class)).isNotEqualTo(SkylarkType.of(SkylarkClassObject.class));
+    assertThat(SkylarkType.of(RuleConfiguredTarget.class))
+        .isNotEqualTo(SkylarkType.of(SkylarkClassObject.class));
   }
 
   @Test