Make @SkylarkModule detection (through superclasses/superinterfaces) …

…well-defined.

Moving forward, if a class not already annotated with @SkylarkModule has SkylarkModule supertypes A and B, then it must be the case that A is a type of B, or B is a type of A. The skylark type of a given class is dictated by the *most specific* superclass annotated with @SkylarkModule.

RELNOTES: None.
PiperOrigin-RevId: 197946898

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/StructApi.java

@@ -80,7 +80,7 @@ public interface StructApi extends SkylarkValue {
    * Callable Provider for new struct objects.
    */
   @SkylarkModule(name = "Provider", documented = false, doc = "")
-  public interface StructProviderApi {
+  public interface StructProviderApi extends ProviderApi {
 
     @SkylarkCallable(
         name = "struct",

src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkInterfaceUtils.java

@@ -33,27 +33,65 @@ private static final class ClassWithAnnotation<T extends Annotation> {
     }
   }
 
+  /**
+   * Returns the more specific class of two classes. Class x is more specific than class y
+   * if x is assignable to y. For example, of Integer.class and Object.class, Integer.class is more
+   * specific.
+   *
+   * <p>If either class is null, returns the other class.</p>
+   *
+   * <p>If the classes are identical, returns the class.</p>
+   *
+   * @throws IllegalArgumentException if neither class is assignable to the other
+   */
+  private static <T extends Annotation> ClassWithAnnotation<T> moreSpecificClass(
+      ClassWithAnnotation<T> x, ClassWithAnnotation<T> y) {
+    if (x == null) {
+      return y;
+    } else if (y == null) {
+      return x;
+    }
+    Class<?> xClass = x.klass;
+    Class<?> yClass = y.klass;
+    if (xClass.isAssignableFrom(yClass)) {
+      return y;
+    } else if (yClass.isAssignableFrom(xClass)) {
+      return x;
+    } else {
+      throw new IllegalArgumentException(String.format(
+          "Expected one of %s and %s to be assignable to each other",
+          xClass, yClass));
+    }
+  }
+
+  /**
+   * Searches a class or interface's class hierarchy for the given class annotation.
+   *
+   * <p>If the given class annotation appears multiple times within the class hierachy, this
+   * chooses the annotation on the most-specified class in the hierarchy.</p>
+   *
+   * @return a {@link ClassWithAnnotation} containing the best-fit annotation and the class
+   *     it was declared on
+   */
   @Nullable
   private static <T extends Annotation> ClassWithAnnotation<T> searchForClassAnnotation(
       Class<?> classObj,
       Class<T> annotationClass) {
     if (classObj.isAnnotationPresent(annotationClass)) {
       return new ClassWithAnnotation<T>(classObj, classObj.getAnnotation(annotationClass));
     }
+    ClassWithAnnotation<T> bestCandidate = null;
+
     Class<?> superclass = classObj.getSuperclass();
     if (superclass != null) {
       ClassWithAnnotation<T> result = searchForClassAnnotation(superclass, annotationClass);
-      if (result != null) {
-        return result;
-      }
+      bestCandidate = moreSpecificClass(result, bestCandidate);
     }
     for (Class<?> interfaceObj : classObj.getInterfaces()) {
       ClassWithAnnotation<T> result = searchForClassAnnotation(interfaceObj, annotationClass);
-      if (result != null) {
-        return result;
-      }
+      bestCandidate = moreSpecificClass(result, bestCandidate);
     }
-    return null;
+    return bestCandidate;
   }
 
   /**

src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java

@@ -477,6 +477,7 @@ static interface MockInterface {
     public Boolean isEmptyInterface(String str);
   }
 
+  @SkylarkModule(name = "MockSubClass", doc = "")
   static final class MockSubClass extends Mock implements MockInterface {
     @Override
     public Boolean isEmpty(String str) {

src/test/java/com/google/devtools/build/lib/syntax/SkylarkInterfaceUtilsTest.java

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.syntax;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
@@ -81,6 +82,65 @@ public static class MockClassD extends MockClassC {
     public void foo() {}
   }
 
+  /**
+   * A mock class that implements two unrelated module interfaces. This is invalid as the skylark
+   * type of such an object is ambiguous.
+   */
+  public static class ImplementsTwoUnrelatedInterfaceModules
+      implements MockInterfaceB1, MockInterfaceB2 {
+    @Override
+    public void foo() {}
+    @Override
+    public void bar() {}
+    @Override
+    public void baz() {}
+    @Override
+    public void qux() {}
+  }
+
+  /** ClassAModule test class */
+  @SkylarkModule(name = "ClassAModule", doc = "ClassAModule")
+  public static class ClassAModule {}
+
+  /** ExtendsClassA test class */
+  public static class ExtendsClassA extends ClassAModule {}
+
+  /** InterfaceBModule test interface */
+  @SkylarkModule(name = "InterfaceBModule", doc = "InterfaceBModule")
+  public static interface InterfaceBModule {}
+
+  /** ExtendsInterfaceB test interface */
+  public static interface ExtendsInterfaceB extends InterfaceBModule {}
+
+  /**
+   * A mock class which has two transitive superclasses ({@link ClassAModule} and
+   * {@link InterfaceBModule})) which are unrelated modules. This is invalid as the skylark type
+   * of such an object is ambiguous.
+   *
+   * In other words:
+   *   AmbiguousClass -> ClassAModule
+   *   AmbiguousClass -> InterfaceBModule
+   *   ... but ClassAModule and InterfaceBModule have no relation.
+   */
+  public static class AmbiguousClass extends ExtendsClassA implements ExtendsInterfaceB {}
+
+  /** SubclassOfBoth test interface */
+  @SkylarkModule(name = "SubclassOfBoth", doc = "SubclassOfBoth")
+  public static class SubclassOfBoth extends ExtendsClassA implements ExtendsInterfaceB {}
+
+  /**
+   * A mock class similar to {@link AmbiugousClass} in that it has two separate superclass-paths
+   * to skylark modules, but is resolvable.
+   *
+   * Concretely:
+   *   UnambiguousClass -> SubclassOfBoth
+   *   UnambiguousClass -> InterfaceBModule
+   *   SubclassOfBoth -> InterfaceBModule
+   *
+   * ... so UnambiguousClass is of type SubclassOfBoth.
+   */
+  public static class UnambiguousClass extends SubclassOfBoth implements ExtendsInterfaceB {}
+
   /** MockClassZ */
   public static class MockClassZ {
   }
@@ -130,6 +190,27 @@ public void testGetSkylarkModuleNotFound() throws Exception {
     assertThat(cls).isNull();
   }
 
+  @Test
+  public void testGetSkylarkModuleAmbiguous() throws Exception {
+    assertThrows(IllegalArgumentException.class,
+        () -> SkylarkInterfaceUtils.getSkylarkModule(ImplementsTwoUnrelatedInterfaceModules.class));
+  }
+
+  @Test
+  public void testGetSkylarkModuleTransitivelyAmbiguous() throws Exception {
+    assertThrows(IllegalArgumentException.class,
+        () -> SkylarkInterfaceUtils.getSkylarkModule(AmbiguousClass.class));
+  }
+
+  @Test
+  public void testGetSkylarkModuleUnambiguousComplex() throws Exception {
+    assertThat(SkylarkInterfaceUtils.getSkylarkModule(SubclassOfBoth.class))
+        .isEqualTo(SubclassOfBoth.class.getAnnotation(SkylarkModule.class));
+
+    assertThat(SkylarkInterfaceUtils.getSkylarkModule(UnambiguousClass.class))
+        .isEqualTo(SubclassOfBoth.class.getAnnotation(SkylarkModule.class));
+  }
+
   @Test
   public void testGetSkylarkCallableBasic() throws Exception {
     // Normal case. Ensure two-arg form is consistent with one-arg form.