Add @nonnull and warning to extension type methods.

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

Hide file tree

Showing 6 changed files with 109 additions and 62 deletions.
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
@@ -91,23 +91,7 @@ private void validateNewGlideOption(ExecutableElement 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");
-    }
+    validateAnnotatedNonNull(executableElement);
   }
 
   private void validateDeprecatedGlideOption(ExecutableElement executableElement) {
@@ -197,6 +181,7 @@ private void validateGlideType(ExecutableElement executableElement) {
 
   private void validateNewGlideType(ExecutableElement executableElement) {
     TypeMirror returnType = executableElement.getReturnType();
+    validateNewGlideTypeAnnotations(executableElement);
     if (!isRequestBuilder(returnType) || !typeMatchesExpected(returnType, executableElement)) {
       String expectedClassName = getGlideTypeValue(executableElement);
       throw new IllegalArgumentException("@GlideType methods should return a RequestBuilder<"
@@ -253,6 +238,30 @@ private static void validateGlideTypeParameters(ExecutableElement executableElem
     }
   }
 
+  private void validateNewGlideTypeAnnotations(ExecutableElement executableElement) {
+    validateAnnotatedNonNull(executableElement);
+  }
+
+  private void validateAnnotatedNonNull(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 static void validateStatic(ExecutableElement executableElement, Class<?> clazz) {
     if (!executableElement.getModifiers().contains(Modifier.STATIC)) {
       throw new IllegalArgumentException("@" + clazz.getSimpleName() + " methods must be static");

diff --git a/...ompiler/src/main/java/com/bumptech/glide/annotation/compiler/RequestManagerGenerator.java b/...ompiler/src/main/java/com/bumptech/glide/annotation/compiler/RequestManagerGenerator.java
@@ -60,6 +60,8 @@ final class RequestManagerGenerator {
       "com.bumptech.glide.manager.RequestManagerTreeNode";
   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 static final ClassName CONTEXT_CLASS_NAME =
       ClassName.get("android.content", "Context");
 
@@ -270,6 +272,7 @@ private MethodSpec generateAdditionalRequestManagerMethodLegacy(
         .addModifiers(Modifier.PUBLIC)
         .returns(parameterizedTypeName)
         .addJavadoc(processorUtil.generateSeeMethodJavadoc(extensionMethod))
+        .addAnnotation(AnnotationSpec.builder(NON_NULL_CLASS_NAME).build())
         .addStatement(
             "$T requestBuilder = this.as($T.class)", parameterizedTypeName, returnTypeClassName)
         .addStatement("$T.$N(requestBuilder)",
@@ -290,6 +293,7 @@ private MethodSpec generateAdditionalRequestManagerMethodNew(
         .addModifiers(Modifier.PUBLIC)
         .returns(parameterizedTypeName)
         .addJavadoc(processorUtil.generateSeeMethodJavadoc(extensionMethod))
+        .addAnnotation(AnnotationSpec.builder(NON_NULL_CLASS_NAME).build())
         .addStatement(
             "return ($T) $T.$N(this.as($T.class))",
             parameterizedTypeName,

diff --git a/...t/src/test/java/com/bumptech/glide/annotation/compiler/InvalidGlideTypeExtensionTest.java b/...t/src/test/java/com/bumptech/glide/annotation/compiler/InvalidGlideTypeExtensionTest.java
@@ -170,20 +170,22 @@ public void compilation_withAnnotatedStaticMethod_overridingExistingType_fails()
 
   @Test
   public void compilation_withAnnotatedStaticMethod_returningRequestBuilder_succeeds() {
-     Compilation compilation =
+    Compilation compilation =
         javac()
             .withProcessors(new GlideAnnotationProcessor())
             .compile(
                 emptyAppModule(),
                 JavaFileObjects.forSourceLines(
                     "Extension",
                     "package com.bumptech.glide.test;",
+                    "import android.support.annotation.NonNull;",
                     "import com.bumptech.glide.RequestBuilder;",
                     "import com.bumptech.glide.annotation.GlideExtension;",
                     "import com.bumptech.glide.annotation.GlideType;",
                     "@GlideExtension",
                     "public class Extension {",
                     "  private Extension() {}",
+                    "  @NonNull",
                     "  @GlideType(Number.class)",
                     "  public static RequestBuilder<Number> asNumber(",
                     "      RequestBuilder<Number> builder) {",
@@ -233,21 +235,21 @@ public void compilation_withAnnotatedStaticMethod_returningBuilderWithIncorrectT
         .compile(
             emptyAppModule(),
             JavaFileObjects.forSourceLines(
-                 "Extension",
-                 "package com.bumptech.glide.test;",
-                 "import com.bumptech.glide.RequestBuilder;",
-                 "import com.bumptech.glide.annotation.GlideExtension;",
-                 "import com.bumptech.glide.annotation.GlideType;",
-                 "@GlideExtension",
-                 "public class Extension {",
-                 "  private Extension() {}",
-                 "  @GlideType(Number.class)",
-                 "  public static RequestBuilder<Object> asNumber(",
-                 "      RequestBuilder<Object> builder) {",
-                 "    return builder;",
-                 "  }",
-                 "}"));
-   }
+                "Extension",
+                "package com.bumptech.glide.test;",
+                "import com.bumptech.glide.RequestBuilder;",
+                "import com.bumptech.glide.annotation.GlideExtension;",
+                "import com.bumptech.glide.annotation.GlideType;",
+                "@GlideExtension",
+                "public class Extension {",
+                "  private Extension() {}",
+                "  @GlideType(Number.class)",
+                "  public static RequestBuilder<Object> asNumber(",
+                "      RequestBuilder<Object> builder) {",
+                "    return builder;",
+                "  }",
+                "}"));
+  }
 
   @Test
   public void compilation_withAnnotatedStaticMethod_returningBuilder_andMultipleParams_fails() {
@@ -261,20 +263,20 @@ public void compilation_withAnnotatedStaticMethod_returningBuilder_andMultiplePa
         .compile(
             emptyAppModule(),
             JavaFileObjects.forSourceLines(
-                 "Extension",
-                 "package com.bumptech.glide.test;",
-                 "import com.bumptech.glide.RequestBuilder;",
-                 "import com.bumptech.glide.annotation.GlideExtension;",
-                 "import com.bumptech.glide.annotation.GlideType;",
-                 "@GlideExtension",
-                 "public class Extension {",
-                 "  private Extension() {}",
-                 "  @GlideType(Number.class)",
-                 "  public static RequestBuilder<Number> asNumber(",
-                 "      RequestBuilder<Number> builder, Object arg1) {",
-                 "    return builder;",
-                 "  }",
-                 "}"));
+                "Extension",
+                "package com.bumptech.glide.test;",
+                "import com.bumptech.glide.RequestBuilder;",
+                "import com.bumptech.glide.annotation.GlideExtension;",
+                "import com.bumptech.glide.annotation.GlideType;",
+                "@GlideExtension",
+                "public class Extension {",
+                "  private Extension() {}",
+                "  @GlideType(Number.class)",
+                "  public static RequestBuilder<Number> asNumber(",
+                "      RequestBuilder<Number> builder, Object arg1) {",
+                "    return builder;",
+                "  }",
+                "}"));
   }
 
   @Test
@@ -287,19 +289,47 @@ public void compilation_withAnnotatedStaticMethod_returningBuilder_nonBuilderPar
         .compile(
             emptyAppModule(),
             JavaFileObjects.forSourceLines(
-                 "Extension",
-                 "package com.bumptech.glide.test;",
-                 "import com.bumptech.glide.RequestBuilder;",
-                 "import com.bumptech.glide.annotation.GlideExtension;",
-                 "import com.bumptech.glide.annotation.GlideType;",
-                 "@GlideExtension",
-                 "public class Extension {",
-                 "  private Extension() {}",
-                 "  @GlideType(Number.class)",
-                 "  public static RequestBuilder<Number> asNumber(",
-                 "      Object arg) {",
-                 "    return null;",
-                 "  }",
-                 "}"));
-   }
+                "Extension",
+                "package com.bumptech.glide.test;",
+                "import com.bumptech.glide.RequestBuilder;",
+                "import com.bumptech.glide.annotation.GlideExtension;",
+                "import com.bumptech.glide.annotation.GlideType;",
+                "@GlideExtension",
+                "public class Extension {",
+                "  private Extension() {}",
+                "  @GlideType(Number.class)",
+                "  public static RequestBuilder<Number> asNumber(",
+                "      Object arg) {",
+                "    return null;",
+                "  }",
+                "}"));
+  }
+
+  @Test
+  public void compilation_withAnnotatedStaticMethod_returningRequestBuilder_missingNonNull_warns() {
+    Compilation compilation =
+        javac()
+            .withProcessors(new GlideAnnotationProcessor())
+            .compile(
+                emptyAppModule(),
+                JavaFileObjects.forSourceLines(
+                    "Extension",
+                    "package com.bumptech.glide.test;",
+                    "import com.bumptech.glide.RequestBuilder;",
+                    "import com.bumptech.glide.annotation.GlideExtension;",
+                    "import com.bumptech.glide.annotation.GlideType;",
+                    "@GlideExtension",
+                    "public class Extension {",
+                    "  private Extension() {}",
+                    "  @GlideType(Number.class)",
+                    "  public static RequestBuilder<Number> asNumber(",
+                    "      RequestBuilder<Number> builder) {",
+                    "    return builder;",
+                    "  }",
+                    "}"));
+    assertThat(compilation).succeeded();
+    assertThat(compilation).hadWarningCount(1);
+    assertThat(compilation).hadWarningContaining("@NonNull");
+    assertThat(compilation).hadWarningContaining("com.bumptech.glide.test.Extension#asNumber");
+  }
 }
diff --git a/...tation/compiler/test/src/test/resources/GlideExtensionWithTypeTest/ExtensionWithType.java b/...tation/compiler/test/src/test/resources/GlideExtensionWithTypeTest/ExtensionWithType.java
@@ -1,5 +1,6 @@
 package com.bumptech.glide.test;
 
+import android.support.annotation.NonNull;
 import com.bumptech.glide.RequestBuilder;
 import com.bumptech.glide.annotation.GlideExtension;
 import com.bumptech.glide.annotation.GlideType;
@@ -11,6 +12,7 @@ private ExtensionWithType() {
     // Utility class.
   }
 
+  @NonNull
   @GlideType(Number.class)
   public static RequestBuilder<Number> asNumber(RequestBuilder<Number> builder) {
     return builder;

diff --git a/annotation/compiler/test/src/test/resources/GlideExtensionWithTypeTest/GlideRequests.java b/annotation/compiler/test/src/test/resources/GlideExtensionWithTypeTest/GlideRequests.java
@@ -46,6 +46,7 @@ public <ResourceType> GlideRequest<ResourceType> as(Class<ResourceType> resource
   /**
    * @see ExtensionWithType#asNumber(RequestBuilder)
    */
+  @NonNull
   public GlideRequest<Number> asNumber() {
     return (GlideRequest<Number>) ExtensionWithType.asNumber(this.as(Number.class));
   }

diff --git a/...tion/compiler/test/src/test/resources/LegacyGlideExtensionWithTypeTest/GlideRequests.java b/...tion/compiler/test/src/test/resources/LegacyGlideExtensionWithTypeTest/GlideRequests.java
@@ -46,6 +46,7 @@ public <ResourceType> GlideRequest<ResourceType> as(Class<ResourceType> resource
   /**
    * @see ExtensionWithType#asInteger(RequestBuilder)
    */
+  @NonNull
   public GlideRequest<Number> asInteger() {
     GlideRequest<Number> requestBuilder = this.as(Number.class);
     ExtensionWithType.asInteger(requestBuilder);
-Original file line number
+Diff line change
@@ Expand Up @@
       /**
        * @see ExtensionWithType#asNumber(RequestBuilder)
        */
+      @NonNull
       public GlideRequest<Number> asNumber() {
         return (GlideRequest<Number>) ExtensionWithType.asNumber(this.as(Number.class));
       }
@@ Expand Down @@