Add support for return values from extension methods.

The old style is still supported for now to avoid breaking backwards compatibility, but the new style is available, simpler to write and avoids @checkresult warnings.
bumptech · Nov 6, 2017 · e78f2ee · e78f2ee
1 parent f75087f
commit e78f2ee
Show file tree

Hide file tree

Showing 51 changed files with 9,719 additions and 149 deletions.
diff --git a/...ion/compiler/src/main/java/com/bumptech/glide/annotation/compiler/ExtensionProcessor.java b/...ion/compiler/src/main/java/com/bumptech/glide/annotation/compiler/ExtensionProcessor.java
@@ -5,6 +5,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
+import javax.annotation.processing.ProcessingEnvironment;
 import javax.annotation.processing.RoundEnvironment;
 import javax.lang.model.element.TypeElement;
 
@@ -15,17 +16,22 @@
 final class ExtensionProcessor {
   private final ProcessorUtil processorUtil;
   private final IndexerGenerator indexerGenerator;
+  private final GlideExtensionValidator extensionValidator;
 
-  ExtensionProcessor(ProcessorUtil processorUtil, IndexerGenerator indexerGenerator) {
+  ExtensionProcessor(
+      ProcessingEnvironment processingEnvironment,
+      ProcessorUtil processorUtil,
+      IndexerGenerator indexerGenerator) {
     this.processorUtil = processorUtil;
     this.indexerGenerator = indexerGenerator;
+    extensionValidator = new GlideExtensionValidator(processingEnvironment, processorUtil);
   }
 
   boolean processExtensions(Set<? extends TypeElement> set, RoundEnvironment env) {
     List<TypeElement> elements = processorUtil.getElementsFor(GlideExtension.class, env);
     processorUtil.debugLog("Processing types : " + elements);
     for (TypeElement typeElement : elements) {
-      GlideExtensionValidator.validateExtension(typeElement);
+      extensionValidator.validateExtension(typeElement);
       processorUtil.debugLog("Processing elements: " + typeElement.getEnclosedElements());
     }
 

diff --git a/...mpiler/src/main/java/com/bumptech/glide/annotation/compiler/GlideAnnotationProcessor.java b/...mpiler/src/main/java/com/bumptech/glide/annotation/compiler/GlideAnnotationProcessor.java
@@ -80,7 +80,8 @@ public synchronized void init(ProcessingEnvironment processingEnvironment) {
     IndexerGenerator indexerGenerator = new IndexerGenerator(processorUtil);
     libraryModuleProcessor = new LibraryModuleProcessor(processorUtil, indexerGenerator);
     appModuleProcessor = new AppModuleProcessor(processingEnvironment, processorUtil);
-    extensionProcessor = new ExtensionProcessor(processorUtil, indexerGenerator);
+    extensionProcessor =
+        new ExtensionProcessor(processingEnvironment, processorUtil, indexerGenerator);
   }
 
   @Override

diff --git a/...ompiler/src/main/java/com/bumptech/glide/annotation/compiler/GlideExtensionValidator.java b/...ompiler/src/main/java/com/bumptech/glide/annotation/compiler/GlideExtensionValidator.java
@@ -2,6 +2,9 @@
 
 import com.bumptech.glide.annotation.GlideOption;
 import com.bumptech.glide.annotation.GlideType;
+import java.util.ArrayList;
+import java.util.List;
+import javax.annotation.processing.ProcessingEnvironment;
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.ExecutableElement;
@@ -21,9 +24,16 @@
  */
 final class GlideExtensionValidator {
 
-  private GlideExtensionValidator() { }
+  private final ProcessingEnvironment processingEnvironment;
+  private final ProcessorUtil processorUtil;
 
-  static void validateExtension(TypeElement typeElement) {
+  GlideExtensionValidator(
+      ProcessingEnvironment processingEnvironment, ProcessorUtil processorUtil) {
+    this.processingEnvironment = processingEnvironment;
+    this.processorUtil = processorUtil;
+  }
+
+  void validateExtension(TypeElement typeElement) {
     if (!typeElement.getModifiers().contains(Modifier.PUBLIC)) {
       throw new IllegalArgumentException("RequestOptionsExtensions must be public");
     }
@@ -43,29 +53,112 @@ static void validateExtension(TypeElement typeElement) {
       if (element.getKind() == ElementKind.METHOD) {
         ExecutableElement executableElement = (ExecutableElement) element;
         if (executableElement.getAnnotation(GlideOption.class) != null) {
-          validateExtendsRequestOptions(executableElement);
+          validateGlideOption(executableElement);
         } else if (executableElement.getAnnotation(GlideType.class) != null) {
           validateExtendsRequestManager(executableElement);
         }
       }
     }
   }
 
-  private static void validateExtendsRequestOptions(ExecutableElement executableElement) {
+  private void validateGlideOption(ExecutableElement executableElement) {
+    if (isVoid(executableElement)) {
+      validateDeprecatedGlideOption(executableElement);
+    } else {
+      validateNewGlideOption(executableElement);
+    }
+  }
+
+  private void validateNewGlideOption(ExecutableElement executableElement) {
+    validateGlideOptionParameters(executableElement);
+    TypeMirror returnType = executableElement.getReturnType();
+    if (!isRequestOptions(returnType)) {
+      throw new IllegalArgumentException("@GlideOption methods should return a RequestOptions"
+          + " object, but given: " + returnType + ". If you're using old style @GlideOption"
+          + " methods, your method may have a void return type, but doing so is deprecated and"
+          + " support will be removed in a future version");
+    }
+    validateGlideOptionOverride(executableElement);
+  }
+
+  private void validateDeprecatedGlideOption(ExecutableElement executableElement) {
     validateStaticVoid(executableElement, GlideOption.class);
+    validateGlideOptionParameters(executableElement);
+    validateGlideOptionOverride(executableElement);
+  }
+
+  private static void validateGlideOptionParameters(ExecutableElement executableElement) {
     if (executableElement.getParameters().isEmpty()) {
       throw new IllegalArgumentException("@GlideOption methods must take a "
           + "RequestOptions object as their first parameter, but given none");
     }
     VariableElement first = executableElement.getParameters().get(0);
     TypeMirror expected = first.asType();
-    if (!expected.toString().equals(
-        "com.bumptech.glide.request.RequestOptions")) {
+    if (!isRequestOptions(expected)) {
       throw new IllegalArgumentException("@GlideOption methods must take a"
           + " RequestOptions object as their first parameter, but given: " + expected);
     }
   }
 
+  private static boolean isRequestOptions(TypeMirror typeMirror) {
+    return typeMirror.toString().equals("com.bumptech.glide.request.RequestOptions");
+  }
+
+  private void validateGlideOptionOverride(ExecutableElement element) {
+    int overrideType = processorUtil.getOverrideType(element);
+    boolean isOverridingRequestOptionsMethod = isMethodInRequestOptions(element);
+    if (isOverridingRequestOptionsMethod && overrideType == GlideOption.OVERRIDE_NONE) {
+      throw new IllegalArgumentException("Accidentally attempting to override a method in"
+          + " RequestOptions. Add an 'override' value in the @GlideOption annotation"
+          + " if this is intentional. Offending method: "
+          + element.getEnclosingElement() + "#" + element);
+    } else if (!isOverridingRequestOptionsMethod && overrideType != GlideOption.OVERRIDE_NONE) {
+      throw new IllegalArgumentException("Requested to override an existing method in"
+          + " RequestOptions, but no such method was found. Offending method: "
+          + element.getEnclosingElement() + "#" + element);
+    }
+  }
+
+  private boolean isMethodInRequestOptions(ExecutableElement toFind) {
+    // toFind is a method in a GlideExtension whose first argument is a BaseRequestOptions<?> type.
+    // Since we're comparing against methods in BaseRequestOptions itself, we need to drop that
+    // first type.
+    TypeElement requestOptionsType =
+        processingEnvironment
+            .getElementUtils()
+            .getTypeElement(RequestOptionsGenerator.REQUEST_OPTIONS_QUALIFIED_NAME);
+    List<String> toFindParameterNames = getComparableParameterNames(toFind, true /*skipFirst*/);
+    String toFindSimpleName = toFind.getSimpleName().toString();
+    for (Element element : requestOptionsType.getEnclosedElements()) {
+      if (element.getKind() != ElementKind.METHOD) {
+        continue;
+      }
+      ExecutableElement inBase = (ExecutableElement) element;
+      if (toFindSimpleName.equals(inBase.getSimpleName().toString())) {
+        List<String> parameterNamesInBase =
+            getComparableParameterNames(inBase, false /*skipFirst*/);
+        if (parameterNamesInBase.equals(toFindParameterNames)) {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
+  private static List<String> getComparableParameterNames(
+      ExecutableElement element, boolean skipFirst) {
+    List<? extends VariableElement> parameters = element.getParameters();
+    if (skipFirst) {
+      parameters = parameters.subList(1, parameters.size());
+    }
+    List<String> result = new ArrayList<>(parameters.size());
+    for (VariableElement parameter : parameters) {
+      result.add(parameter.asType().toString());
+    }
+    return result;
+  }
+
+
   private static void validateExtendsRequestManager(ExecutableElement executableElement) {
     validateStaticVoid(executableElement, GlideType.class);
     if (executableElement.getParameters().size() != 1) {
@@ -82,13 +175,25 @@ private static void validateExtendsRequestManager(ExecutableElement executableEl
     }
   }
 
-  private static void validateStaticVoid(ExecutableElement executableElement, Class<?> clazz) {
+  private static void validateStatic(ExecutableElement executableElement, Class<?> clazz) {
     if (!executableElement.getModifiers().contains(Modifier.STATIC)) {
       throw new IllegalArgumentException("@" + clazz.getSimpleName() + " methods must be static");
     }
+  }
+
+  private static boolean isVoid(ExecutableElement executableElement) {
     TypeMirror returnType = executableElement.getReturnType();
-    if (returnType.getKind() != TypeKind.VOID) {
+    return returnType.getKind() == TypeKind.VOID;
+  }
+
+  private static void validateVoid(ExecutableElement executableElement, Class<?> clazz) {
+    if (!isVoid(executableElement)) {
       throw new IllegalArgumentException("@" + clazz.getSimpleName() + " methods must return void");
     }
   }
+
+  private static void validateStaticVoid(ExecutableElement executableElement, Class<?> clazz) {
+    validateStatic(executableElement, clazz);
+    validateVoid(executableElement, clazz);
+  }
 }
diff --git a/annotation/compiler/src/main/java/com/bumptech/glide/annotation/compiler/ProcessorUtil.java b/annotation/compiler/src/main/java/com/bumptech/glide/annotation/compiler/ProcessorUtil.java
@@ -3,6 +3,7 @@
 import static com.bumptech.glide.annotation.compiler.GlideAnnotationProcessor.DEBUG;
 
 import com.bumptech.glide.annotation.GlideExtension;
+import com.bumptech.glide.annotation.GlideOption;
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.base.Predicate;
@@ -93,6 +94,12 @@ boolean isExtension(TypeElement element) {
     return element.getAnnotation(GlideExtension.class) != null;
   }
 
+  int getOverrideType(ExecutableElement element) {
+    GlideOption glideOption =
+        element.getAnnotation(GlideOption.class);
+    return glideOption.override();
+  }
+
   void writeIndexer(TypeSpec indexer) {
     writeClass(COMPILER_PACKAGE_NAME, indexer);
   }
@@ -211,7 +218,6 @@ private CodeBlock generateSeeMethodJavadocInternal(
     return CodeBlock.of(javadocString, javadocArgs.toArray(new Object[0]));
   }
 
-
    /**
    * Returns a safe String to use in a Javadoc that will function in a link.
    *
@@ -239,8 +245,8 @@ void infoLog(String toLog) {
     processingEnv.getMessager().printMessage(Diagnostic.Kind.NOTE, "[" + round + "] " + toLog);
   }
 
-  void error(String toLog) {
-    processingEnv.getMessager().printMessage(Diagnostic.Kind.ERROR, toLog);
+  void warnLog(String toLog) {
+    processingEnv.getMessager().printMessage(Diagnostic.Kind.WARNING, toLog);
   }
 
   static CodeBlock generateCastingSuperCall(TypeName toReturn, ExecutableElement method) {