Clarify invariants for @SkylarkModule and @SkylarkCallable

Also refactor away SkylarkModule.Resolver.

RELNOTES: None
PiperOrigin-RevId: 197955164

3 files changed, 56 insertions, 27 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkLateBoundDefault.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkLateBoundDefault.java
index 139f8656cb..d98c32aa69 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkLateBoundDefault.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkLateBoundDefault.java
@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.analysis.skylark;
 
 import com.google.common.base.Preconditions;
-import com.google.common.base.Strings;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
@@ -246,13 +245,13 @@ public class SkylarkLateBoundDefault<FragmentT> extends AbstractLabelLateBoundDe
       SkylarkLateBoundDefault resolver =
           fieldCache.get(cacheKey).get(fragmentFieldName);
       if (resolver == null) {
-        String fragmentName = SkylarkModule.Resolver.resolveName(fragmentClass);
-        if (Strings.isNullOrEmpty(fragmentName)) {
+        SkylarkModule moduleAnnotation = SkylarkInterfaceUtils.getSkylarkModule(fragmentClass);
+        if (moduleAnnotation == null) {
           throw new AssertionError("fragment class must have a valid skylark name");
         }
         throw new InvalidConfigurationFieldException(
             String.format("invalid configuration field name '%s' on fragment '%s'",
-                fragmentFieldName, fragmentName));
+                fragmentFieldName, moduleAnnotation.name()));
       }
       return resolver;
     } catch (ExecutionException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
index df3387a26a..4856601b59 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
@@ -19,10 +19,25 @@ import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
 /**
- * A marker interface for Java methods which can be called from Skylark.
+ * Annotates a Java method that can be called from Skylark.
  *
- * <p>Methods annotated with this annotation are expected to meet certain requirements which are
- * enforced by an annotation processor:
+ * <p>This annotation is only allowed to appear on methods of classes that are directly annotated
+ * with {@link SkylarkModule} or {@link SkylarkGlobalLibrary}. Since subtypes can't add new
+ * Skylark-accessible methods unless they have their own {@code @SkylarkModule} annotation, this
+ * implies that you can always determine the complete set of Skylark entry points for a given {@link
+ * SkylarkValue} type by looking at the ancestor class or interface from which it inherits its
+ * {@code @SkylarkModule}.
+ *
+ * <p>If a method is annotated with {@code @SkylarkCallable}, it is not allowed to have any
+ * overloads or hide any static or default methods. Overriding is allowed, but the {@code
+ * @SkylarkCallable} annotation itself must not be repeated on the override. This ensures that given
+ * a method, we can always determine its corresponding {@code @SkylarkCallable} annotation, if it
+ * has one, by scanning all methods of the same name in its class hierarchy, without worrying about
+ * complications like overloading or generics. The lookup functionality is implemented by {@link
+ * SkylarkInterfaceUtils#getSkylarkCallable}.
+ *
+ * <p>Methods having this annotation are required to satisfy the following (enforced by an
+ * annotation processor):
  *
  * <ul>
  *   <li>The method must be public.
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkModule.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkModule.java
index d6ff2351f6..6a6c2ac21d 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkModule.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkModule.java
@@ -18,41 +18,56 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
-import javax.annotation.Nullable;
-
 /**
- * An annotation to mark Skylark modules or Skylark accessible Java data types.
- * A Skylark modules always corresponds to exactly one Java class.
+ * This annotation is used on classes and interfaces that represent Skylark data types.
+ *
+ * <p>Conceptually, every {@code @SkylarkModule} annotation corresponds to a user-distinguishable
+ * Skylark type. The annotation holds metadata associated with that type, in particular its name and
+ * documentation. The annotation also implicitly demarcates the Skylark API of the type. It does not
+ * matter whether the annotation is used on a class or an interface.
+ *
+ * <p>Annotations are "inherited" and "overridden", in the sense that a child class or interface
+ * takes on the Skylark type of its ancestor by default, unless it has a direct annotation of its
+ * own. If there are multiple ancestors that have an annotation, then to avoid ambiguity we require
+ * that one of them is a subtype of the rest; that is the one whose annotation gets inherited. This
+ * ensures that every class implements at most one Skylark type, and not an ad hoc hybrid of
+ * multiple types. (In mathematical terms, the most-derived annotation for class or interface C is
+ * the minimum element in the partial order of all annotations defined on C and its ancestors, where
+ * the order relationship is X < Y if X annotates a subtype of what Y annotates.) The lookup logic
+ * for retrieving a class's {@code @SkylarkModule} is implemented by {@link
+ * SkylarkInterfaceUtils#getSkylarkModule}.
+ *
+ * <p>Inheriting an annotation is useful when the class is an implementation detail, such as a
+ * concrete implementation of an abstract interface. Overriding an annotation is useful when the
+ * class should have its own distinct user-visible API or documentation. For example, {@link
+ * SkylarkList} is an abstract type implemented by both {@link SkylarkList.MutableList} and {@link
+ * SkylarkList.Tuple}, all three of which are annotated. Annotating the list and tuple types allows
+ * them to define different methods, while annotating {@link SkylarkList} allows them to be
+ * identified as a single type for the purpose of type checking, documentation, and error messages.
+ *
+ * <p>All {@code @SkylarkModule}-annotated types should implement {@link SkylarkValue}. Conversely,
+ * all non-abstract implementations of {@link SkylarkValue} should have or inherit a {@code
+ * @SkylarkModule} annotation.
  */
 @Target({ElementType.TYPE})
 @Retention(RetentionPolicy.RUNTIME)
 public @interface SkylarkModule {
 
+  /** A type name that may be used in stringification and error messages. */
   String name();
 
+  /** A title for the documentation page generated for this type. */
   String title() default "";
 
   String doc();
 
   boolean documented() default true;
 
+  /**
+   * If true, this type is a singleton top-level type whose main purpose is to act as a namespace
+   * for other values.
+   */
   boolean namespace() default false;
 
   SkylarkModuleCategory category() default SkylarkModuleCategory.TOP_LEVEL_TYPE;
-
-  /** Helper method to quickly get the SkylarkModule name of a class (if present). */
-  public static final class Resolver {
-    /**
-     * Returns the Skylark name of the given class or null, if the SkylarkModule annotation is not
-     * present.
-     */
-    @Nullable
-    public static String resolveName(Class<?> clazz) {
-      SkylarkModule annotation = clazz.getAnnotation(SkylarkModule.class);
-      return (annotation == null) ? null : annotation.name();
-    }
-
-    /** Utility method only. */
-    private Resolver() {}
-  }
 }