Add @nonnull to generated options and warn if @nonnull is missing.

Progress towards #2774.
bumptech · Dec 31, 2017 · 62d7464 · 62d7464
1 parent ebff466
commit 62d7464
Show file tree

Hide file tree

Showing 37 changed files with 105 additions and 1 deletion.
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,9 +2,13 @@
 
 import com.bumptech.glide.annotation.GlideOption;
 import com.bumptech.glide.annotation.GlideType;
+import com.google.common.base.Function;
+import com.google.common.collect.FluentIterable;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Set;
 import javax.annotation.processing.ProcessingEnvironment;
+import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.ExecutableElement;
@@ -14,6 +18,7 @@
 import javax.lang.model.type.DeclaredType;
 import javax.lang.model.type.TypeKind;
 import javax.lang.model.type.TypeMirror;
+import javax.tools.Diagnostic.Kind;
 
 /**
  * Validates that classes annotated with {@link com.bumptech.glide.annotation.GlideExtension}
@@ -73,6 +78,7 @@ private void validateGlideOption(ExecutableElement executableElement) {
   }
 
   private void validateNewGlideOption(ExecutableElement executableElement) {
+    validateNewGlideOptionAnnotations(executableElement);
     validateGlideOptionParameters(executableElement);
     TypeMirror returnType = executableElement.getReturnType();
     if (!isRequestOptions(returnType)) {
@@ -84,6 +90,26 @@ private void validateNewGlideOption(ExecutableElement executableElement) {
     validateGlideOptionOverride(executableElement);
   }
 
+  private void validateNewGlideOptionAnnotations(ExecutableElement executableElement) {
+    Set<String> annotationNames =
+        FluentIterable.from(executableElement.getAnnotationMirrors())
+            .transform(new Function<AnnotationMirror, String>() {
+              @Override
+              public String apply(AnnotationMirror input) {
+                return input.getAnnotationType().asElement().toString();
+              }
+            })
+            .toSet();
+    if (!annotationNames.contains("android.support.annotation.NonNull")) {
+      processingEnvironment.getMessager().printMessage(
+          Kind.WARNING,
+          executableElement.getEnclosingElement() + "#" + executableElement.getSimpleName()
+              + " is missing the @NonNull annotation,"
+              + " please add it to ensure that your extension methods are always returning non-null"
+              + " values");
+    }
+  }
+
   private void validateDeprecatedGlideOption(ExecutableElement executableElement) {
     validateStaticVoid(executableElement, GlideOption.class);
     validateGlideOptionParameters(executableElement);

diff --git a/...ompiler/src/main/java/com/bumptech/glide/annotation/compiler/RequestOptionsGenerator.java b/...ompiler/src/main/java/com/bumptech/glide/annotation/compiler/RequestOptionsGenerator.java
@@ -78,6 +78,8 @@ final class RequestOptionsGenerator {
       REQUEST_OPTIONS_PACKAGE_NAME + "." + REQUEST_OPTIONS_SIMPLE_NAME;
   private static final ClassName CHECK_RESULT_CLASS_NAME =
       ClassName.get("android.support.annotation", "CheckResult");
+  private static final ClassName NON_NULL_CLASS_NAME =
+      ClassName.get("android.support.annotation", "NonNull");
 
   private final ProcessingEnvironment processingEnvironment;
   private final ClassName requestOptionsName;
@@ -298,6 +300,8 @@ private List<MethodAndStaticVar> generateMethodsForRequestOptionsExtensionNew(
     code = new StringBuilder(code.substring(0, code.length() - 2));
     code.append(")");
     builder.addStatement(code.toString(), args.toArray(new Object[0]));
+
+    builder.addAnnotation(AnnotationSpec.builder(NON_NULL_CLASS_NAME).build());
     builder.addAnnotation(AnnotationSpec.builder(CHECK_RESULT_CLASS_NAME).build());
 
     List<MethodAndStaticVar> result = new ArrayList<>();
@@ -370,6 +374,7 @@ private List<MethodAndStaticVar> generateMethodsForRequestOptionsExtensionDeprec
     builder.addStatement(code.toString(), args.toArray(new Object[0]));
 
     builder.addStatement("return this");
+    builder.addAnnotation(AnnotationSpec.builder(NON_NULL_CLASS_NAME).build());
     builder.addAnnotation(AnnotationSpec.builder(CHECK_RESULT_CLASS_NAME).build());
 
     List<MethodAndStaticVar> result = new ArrayList<>();

diff --git a/...rc/test/java/com/bumptech/glide/annotation/compiler/InvalidGlideOptionsExtensionTest.java b/...rc/test/java/com/bumptech/glide/annotation/compiler/InvalidGlideOptionsExtensionTest.java
@@ -244,12 +244,14 @@ public void compilation_withRequestOptionsReturnValue_succeeds() {
             JavaFileObjects.forSourceLines(
                 "Extension",
                 "package com.bumptech.glide.test;",
+                "import android.support.annotation.NonNull;",
                 "import com.bumptech.glide.annotation.GlideExtension;",
                 "import com.bumptech.glide.annotation.GlideOption;",
                 "import com.bumptech.glide.request.RequestOptions;",
                 "@GlideExtension",
                 "public class Extension {",
                 "  private Extension() {}",
+                "  @NonNull",
                 "  @GlideOption",
                 "  public static RequestOptions doSomething(RequestOptions options) {",
                 "    return options;",
@@ -268,16 +270,44 @@ public void compilation_withNonRequestOptionsReturnValue_fails() {
             JavaFileObjects.forSourceLines(
                 "Extension",
                 "package com.bumptech.glide.test;",
+                "import android.support.annotation.NonNull;",
                 "import com.bumptech.glide.annotation.GlideExtension;",
                 "import com.bumptech.glide.annotation.GlideOption;",
                 "import com.bumptech.glide.request.RequestOptions;",
                 "@GlideExtension",
                 "public class Extension {",
                 "  private Extension() {}",
+                "  @NonNull",
                 "  @GlideOption",
                 "  public static Object doSomething(RequestOptions options) {",
                 "    return options;",
                 "  }",
                 "}"));
   }
+
+  @Test
+  public void compilation_withMissingNonNullAnnotation_warns() {
+    Compilation compilation = javac()
+        .withProcessors(new GlideAnnotationProcessor())
+        .compile(
+            emptyAppModule(),
+            JavaFileObjects.forSourceLines(
+                "Extension",
+                "package com.bumptech.glide.test;",
+                "import com.bumptech.glide.annotation.GlideExtension;",
+                "import com.bumptech.glide.annotation.GlideOption;",
+                "import com.bumptech.glide.request.RequestOptions;",
+                "@GlideExtension",
+                "public class Extension {",
+                "  private Extension() {}",
+                "  @GlideOption",
+                "  public static RequestOptions doSomething(RequestOptions options) {",
+                "    return options;",
+                "  }",
+                "}"));
+    assertThat(compilation).succeeded();
+    assertThat(compilation).hadWarningCount(1);
+    assertThat(compilation).hadWarningContaining("@NonNull");
+    assertThat(compilation).hadWarningContaining("com.bumptech.glide.test.Extension#doSomething");
+  }
 }
diff --git a/...iler/test/src/test/resources/GlideExtensionOptionsTest/MemoizeStaticMethod/Extension.java b/...iler/test/src/test/resources/GlideExtensionOptionsTest/MemoizeStaticMethod/Extension.java
@@ -1,5 +1,6 @@
 package com.bumptech.glide.test;
 
+import android.support.annotation.NonNull;
 import com.bumptech.glide.annotation.GlideExtension;
 import com.bumptech.glide.annotation.GlideOption;
 import com.bumptech.glide.request.RequestOptions;
@@ -11,6 +12,7 @@ private Extension() {
     // Utility class.
   }
 
+  @NonNull
   @GlideOption(memoizeStaticMethod = true)
   public static RequestOptions test(RequestOptions requestOptions) {
     return requestOptions.centerCrop();

diff --git a/...r/test/src/test/resources/GlideExtensionOptionsTest/MemoizeStaticMethod/GlideOptions.java b/...r/test/src/test/resources/GlideExtensionOptionsTest/MemoizeStaticMethod/GlideOptions.java
@@ -596,6 +596,7 @@ public final GlideOptions autoClone() {
   /**
    * @see Extension#test(RequestOptions)
    */
+  @NonNull
   @CheckResult
   public GlideOptions test() {
     return (GlideOptions) Extension.test(this);

diff --git a/...r/test/src/test/resources/GlideExtensionOptionsTest/MemoizeStaticMethod/GlideRequest.java b/...r/test/src/test/resources/GlideExtensionOptionsTest/MemoizeStaticMethod/GlideRequest.java
@@ -651,6 +651,7 @@ public GlideRequest<TranscodeType> dontAnimate() {
   /**
    * @see GlideOptions#test()
    */
+  @NonNull
   @CheckResult
   public GlideRequest<TranscodeType> test() {
     if (getMutableOptions() instanceof GlideOptions) {

diff --git a/.../compiler/test/src/test/resources/GlideExtensionOptionsTest/OverrideExtend/Extension.java b/.../compiler/test/src/test/resources/GlideExtensionOptionsTest/OverrideExtend/Extension.java
@@ -1,5 +1,6 @@
 package com.bumptech.glide.test;
 
+import android.support.annotation.NonNull;
 import com.bumptech.glide.annotation.GlideExtension;
 import com.bumptech.glide.annotation.GlideOption;
 import com.bumptech.glide.request.RequestOptions;
@@ -11,6 +12,7 @@ private Extension() {
     // Utility class.
   }
 
+  @NonNull
   @GlideOption(override = GlideOption.OVERRIDE_EXTEND)
   public static RequestOptions centerCrop(RequestOptions requestOptions) {
     return requestOptions.centerCrop();

diff --git a/...mpiler/test/src/test/resources/GlideExtensionOptionsTest/OverrideExtend/GlideOptions.java b/...mpiler/test/src/test/resources/GlideExtensionOptionsTest/OverrideExtend/GlideOptions.java
@@ -589,6 +589,7 @@ public final GlideOptions autoClone() {
    * @see RequestOptions#centerCrop()
    */
   @Override
+  @NonNull
   @CheckResult
   public GlideOptions centerCrop() {
     return (GlideOptions) Extension.centerCrop(super.centerCrop());

diff --git a/...mpiler/test/src/test/resources/GlideExtensionOptionsTest/OverrideExtend/GlideRequest.java b/...mpiler/test/src/test/resources/GlideExtensionOptionsTest/OverrideExtend/GlideRequest.java
@@ -637,6 +637,7 @@ public GlideRequest<TranscodeType> dontAnimate() {
   /**
    * @see GlideOptions#centerCrop()
    */
+  @NonNull
   @CheckResult
   public GlideRequest<TranscodeType> centerCrop() {
     if (getMutableOptions() instanceof GlideOptions) {

diff --git a/...c/test/resources/GlideExtensionOptionsTest/OverrideExtendMultipleArguments/Extension.java b/...c/test/resources/GlideExtensionOptionsTest/OverrideExtendMultipleArguments/Extension.java
@@ -1,5 +1,6 @@
 package com.bumptech.glide.test;
 
+import android.support.annotation.NonNull;
 import com.bumptech.glide.annotation.GlideExtension;
 import com.bumptech.glide.annotation.GlideOption;
 import com.bumptech.glide.request.RequestOptions;
@@ -11,6 +12,7 @@ private Extension() {
     // Utility class.
   }
 
+  @NonNull
   @GlideOption(override = GlideOption.OVERRIDE_EXTEND)
   public static RequestOptions override(RequestOptions requestOptions, int width, int height) {
     return requestOptions

diff --git a/...est/resources/GlideExtensionOptionsTest/OverrideExtendMultipleArguments/GlideOptions.java b/...est/resources/GlideExtensionOptionsTest/OverrideExtendMultipleArguments/GlideOptions.java
@@ -580,6 +580,7 @@ public final GlideOptions autoClone() {
    * @see RequestOptions#override(int, int)
    */
   @Override
+  @NonNull
   @CheckResult
   public GlideOptions override(int width, int height) {
     return (GlideOptions) Extension.override(super.override(width, height), width, height);

diff --git a/...est/resources/GlideExtensionOptionsTest/OverrideExtendMultipleArguments/GlideRequest.java b/...est/resources/GlideExtensionOptionsTest/OverrideExtendMultipleArguments/GlideRequest.java
@@ -637,6 +637,7 @@ public GlideRequest<TranscodeType> dontAnimate() {
   /**
    * @see GlideOptions#override(int, int)
    */
+  @NonNull
   @CheckResult
   public GlideRequest<TranscodeType> override(int width, int height) {
     if (getMutableOptions() instanceof GlideOptions) {

diff --git a/...compiler/test/src/test/resources/GlideExtensionOptionsTest/OverrideReplace/Extension.java b/...compiler/test/src/test/resources/GlideExtensionOptionsTest/OverrideReplace/Extension.java
@@ -1,5 +1,6 @@
 package com.bumptech.glide.test;
 
+import android.support.annotation.NonNull;
 import com.bumptech.glide.annotation.GlideExtension;
 import com.bumptech.glide.annotation.GlideOption;
 import com.bumptech.glide.request.RequestOptions;
@@ -11,6 +12,7 @@ private Extension() {
     // Utility class.
   }
 
+  @NonNull
   @GlideOption(override = GlideOption.OVERRIDE_REPLACE)
   public static RequestOptions centerCrop(RequestOptions requestOptions) {
     return requestOptions.centerCrop();

diff --git a/...piler/test/src/test/resources/GlideExtensionOptionsTest/OverrideReplace/GlideOptions.java b/...piler/test/src/test/resources/GlideExtensionOptionsTest/OverrideReplace/GlideOptions.java
@@ -587,6 +587,7 @@ public final GlideOptions autoClone() {
   /**
    * @see Extension#centerCrop(RequestOptions)
    */
+  @NonNull
   @CheckResult
   public GlideOptions centerCrop() {
     return (GlideOptions) Extension.centerCrop(this);

diff --git a/...piler/test/src/test/resources/GlideExtensionOptionsTest/OverrideReplace/GlideRequest.java b/...piler/test/src/test/resources/GlideExtensionOptionsTest/OverrideReplace/GlideRequest.java
@@ -637,6 +637,7 @@ public GlideRequest<TranscodeType> dontAnimate() {
   /**
    * @see GlideOptions#centerCrop()
    */
+  @NonNull
   @CheckResult
   public GlideRequest<TranscodeType> centerCrop() {
     if (getMutableOptions() instanceof GlideOptions) {

diff --git a/...ompiler/test/src/test/resources/GlideExtensionOptionsTest/SkipStaticMethod/Extension.java b/...ompiler/test/src/test/resources/GlideExtensionOptionsTest/SkipStaticMethod/Extension.java
@@ -1,5 +1,6 @@
 package com.bumptech.glide.test;
 
+import android.support.annotation.NonNull;
 import com.bumptech.glide.annotation.GlideExtension;
 import com.bumptech.glide.annotation.GlideOption;
 import com.bumptech.glide.request.RequestOptions;
@@ -11,6 +12,7 @@ private Extension() {
     // Utility class.
   }
 
+  @NonNull
   @GlideOption(skipStaticMethod = true)
   public static RequestOptions test(RequestOptions requestOptions) {
     return requestOptions.centerCrop();

diff --git a/...iler/test/src/test/resources/GlideExtensionOptionsTest/SkipStaticMethod/GlideOptions.java b/...iler/test/src/test/resources/GlideExtensionOptionsTest/SkipStaticMethod/GlideOptions.java
@@ -594,6 +594,7 @@ public final GlideOptions autoClone() {
   /**
    * @see Extension#test(RequestOptions)
    */
+  @NonNull
   @CheckResult
   public GlideOptions test() {
     return (GlideOptions) Extension.test(this);

diff --git a/...iler/test/src/test/resources/GlideExtensionOptionsTest/SkipStaticMethod/GlideRequest.java b/...iler/test/src/test/resources/GlideExtensionOptionsTest/SkipStaticMethod/GlideRequest.java
@@ -651,6 +651,7 @@ public GlideRequest<TranscodeType> dontAnimate() {
   /**
    * @see GlideOptions#test()
    */
+  @NonNull
   @CheckResult
   public GlideRequest<TranscodeType> test() {
     if (getMutableOptions() instanceof GlideOptions) {

diff --git a/...ompiler/test/src/test/resources/GlideExtensionOptionsTest/StaticMethodName/Extension.java b/...ompiler/test/src/test/resources/GlideExtensionOptionsTest/StaticMethodName/Extension.java
@@ -1,5 +1,6 @@
 package com.bumptech.glide.test;
 
+import android.support.annotation.NonNull;
 import com.bumptech.glide.annotation.GlideExtension;
 import com.bumptech.glide.annotation.GlideOption;
 import com.bumptech.glide.request.RequestOptions;
@@ -11,6 +12,7 @@ private Extension() {
     // Utility class.
   }
 
+  @NonNull
   @GlideOption(staticMethodName = "testSomething")
   public static RequestOptions test(RequestOptions requestOptions) {
     return requestOptions.centerCrop();

diff --git a/...iler/test/src/test/resources/GlideExtensionOptionsTest/StaticMethodName/GlideOptions.java b/...iler/test/src/test/resources/GlideExtensionOptionsTest/StaticMethodName/GlideOptions.java
@@ -594,6 +594,7 @@ public final GlideOptions autoClone() {
   /**
    * @see Extension#test(RequestOptions)
    */
+  @NonNull
   @CheckResult
   public GlideOptions test() {
     return (GlideOptions) Extension.test(this);

diff --git a/...iler/test/src/test/resources/GlideExtensionOptionsTest/StaticMethodName/GlideRequest.java b/...iler/test/src/test/resources/GlideExtensionOptionsTest/StaticMethodName/GlideRequest.java
@@ -651,6 +651,7 @@ public GlideRequest<TranscodeType> dontAnimate() {
   /**
    * @see GlideOptions#test()
    */
+  @NonNull
   @CheckResult
   public GlideRequest<TranscodeType> test() {
     if (getMutableOptions() instanceof GlideOptions) {

diff --git a/...on/compiler/test/src/test/resources/GlideExtensionWithOptionTest/ExtensionWithOption.java b/...on/compiler/test/src/test/resources/GlideExtensionWithOptionTest/ExtensionWithOption.java
@@ -1,5 +1,6 @@
 package com.bumptech.glide.test;
 
+import android.support.annotation.NonNull;
 import com.bumptech.glide.annotation.GlideExtension;
 import com.bumptech.glide.annotation.GlideOption;
 import com.bumptech.glide.request.RequestOptions;
@@ -11,6 +12,7 @@ private ExtensionWithOption() {
     // Utility class.
   }
 
+  @NonNull
   @GlideOption
   public static RequestOptions squareThumb(RequestOptions requestOptions) {
     return requestOptions.centerCrop();

diff --git a/annotation/compiler/test/src/test/resources/GlideExtensionWithOptionTest/GlideOptions.java b/annotation/compiler/test/src/test/resources/GlideExtensionWithOptionTest/GlideOptions.java
@@ -594,6 +594,7 @@ public final GlideOptions autoClone() {
   /**
    * @see ExtensionWithOption#squareThumb(RequestOptions)
    */
+  @NonNull
   @CheckResult
   public GlideOptions squareThumb() {
     return (GlideOptions) ExtensionWithOption.squareThumb(this);

diff --git a/annotation/compiler/test/src/test/resources/GlideExtensionWithOptionTest/GlideRequest.java b/annotation/compiler/test/src/test/resources/GlideExtensionWithOptionTest/GlideRequest.java
@@ -651,6 +651,7 @@ public GlideRequest<TranscodeType> dontAnimate() {
   /**
    * @see GlideOptions#squareThumb()
    */
+  @NonNull
   @CheckResult
   public GlideRequest<TranscodeType> squareThumb() {
     if (getMutableOptions() instanceof GlideOptions) {

diff --git a/.../src/test/resources/LegacyGlideExtensionOptionsTest/MemoizeStaticMethod/GlideOptions.java b/.../src/test/resources/LegacyGlideExtensionOptionsTest/MemoizeStaticMethod/GlideOptions.java
@@ -596,6 +596,7 @@ public final GlideOptions autoClone() {
   /**
    * @see Extension#test(RequestOptions)
    */
+  @NonNull
   @CheckResult
   public GlideOptions test() {
     if (isAutoCloneEnabled()) {

diff --git a/.../src/test/resources/LegacyGlideExtensionOptionsTest/MemoizeStaticMethod/GlideRequest.java b/.../src/test/resources/LegacyGlideExtensionOptionsTest/MemoizeStaticMethod/GlideRequest.java
@@ -651,6 +651,7 @@ public GlideRequest<TranscodeType> dontAnimate() {
   /**
    * @see GlideOptions#test()
    */
+  @NonNull
   @CheckResult
   public GlideRequest<TranscodeType> test() {
     if (getMutableOptions() instanceof GlideOptions) {

diff --git a/.../test/src/test/resources/LegacyGlideExtensionOptionsTest/OverrideExtend/GlideOptions.java b/.../test/src/test/resources/LegacyGlideExtensionOptionsTest/OverrideExtend/GlideOptions.java
@@ -589,6 +589,7 @@ public final GlideOptions autoClone() {
    * @see RequestOptions#centerCrop()
    */
   @Override
+  @NonNull
   @CheckResult
   public GlideOptions centerCrop() {
     if (isAutoCloneEnabled()) {

diff --git a/.../test/src/test/resources/LegacyGlideExtensionOptionsTest/OverrideExtend/GlideRequest.java b/.../test/src/test/resources/LegacyGlideExtensionOptionsTest/OverrideExtend/GlideRequest.java
@@ -637,6 +637,7 @@ public GlideRequest<TranscodeType> dontAnimate() {
   /**
    * @see GlideOptions#centerCrop()
    */
+  @NonNull
   @CheckResult
   public GlideRequest<TranscodeType> centerCrop() {
     if (getMutableOptions() instanceof GlideOptions) {

diff --git a/...test/src/test/resources/LegacyGlideExtensionOptionsTest/OverrideReplace/GlideOptions.java b/...test/src/test/resources/LegacyGlideExtensionOptionsTest/OverrideReplace/GlideOptions.java
@@ -587,6 +587,7 @@ public final GlideOptions autoClone() {
   /**
    * @see Extension#centerCrop(RequestOptions)
    */
+  @NonNull
   @CheckResult
   public GlideOptions centerCrop() {
     if (isAutoCloneEnabled()) {