From cdf5a072c3b876f27bcbcd2cb1f0d216b24b1ec0 Mon Sep 17 00:00:00 2001 From: brandjon Date: Thu, 24 May 2018 14:34:40 -0700 Subject: [PATCH] Clarify invariants for @SkylarkModule and @SkylarkCallable Also refactor away SkylarkModule.Resolver. RELNOTES: None PiperOrigin-RevId: 197955164 --- .../skylark/SkylarkLateBoundDefault.java | 7 +-- .../lib/skylarkinterface/SkylarkCallable.java | 21 ++++++- .../lib/skylarkinterface/SkylarkModule.java | 55 ++++++++++++------- 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 139f8656cb7b4f..d98c32aa699064 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 static SkylarkLateBoundDefault forConfigurationFie 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 df3387a26a4656..4856601b59e6d4 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.Target; /** - * A marker interface for Java methods which can be called from Skylark. + * Annotates a Java method that can be called from Skylark. * - *

Methods annotated with this annotation are expected to meet certain requirements which are - * enforced by an annotation processor: + *

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}. + * + *

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}. + * + *

Methods having this annotation are required to satisfy the following (enforced by an + * annotation processor): * *

    *
  • 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 d6ff2351f670d5..6a6c2ac21da258 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.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. + * + *

    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. + * + *

    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}. + * + *

    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. + * + *

    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() {} - } }