Refactor getParentWithSkylarkModule() into SkylarkInterfaceUtils

--
MOS_MIGRATED_REVID=135956016

2 files changed, 32 insertions, 21 deletions

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