Skip to content

Commit

Permalink
Allow return values from @GlideType extension methods.
Browse files Browse the repository at this point in the history
  • Loading branch information
@sjudd
sjudd committed Nov 6, 2017
1 parent e78f2ee commit 7fccb32
Show file tree
Hide file tree
Showing 9 changed files with 1,012 additions and 24 deletions.
89 changes: 72 additions & 17 deletions ...ompiler/src/main/java/com/bumptech/glide/annotation/compiler/GlideExtensionValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import javax.lang.model.element.Modifier;
import javax.lang.model.element.TypeElement;
import javax.lang.model.element.VariableElement;
import javax.lang.model.type.DeclaredType;
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeMirror;

Expand Down Expand Up @@ -39,30 +40,32 @@ void validateExtension(TypeElement typeElement) {
}
for (Element element : typeElement.getEnclosedElements()) {
if (element.getKind() == ElementKind.CONSTRUCTOR) {
if (!element.getModifiers().contains(Modifier.PRIVATE)) {
throw new IllegalArgumentException("RequestOptionsExtensions must be public, with private"
+ " constructors and only static methods. Found a non-private constructor");
}
ExecutableElement executableElement = (ExecutableElement) element;
if (!executableElement.getParameters().isEmpty()) {
throw new IllegalArgumentException("RequestOptionsExtensions must be public, with private"
+ " constructors and only static methods. Found parameters in the constructor");
}
continue;
}
if (element.getKind() == ElementKind.METHOD) {
validateExtensionConstructor(element);
} else if (element.getKind() == ElementKind.METHOD) {
ExecutableElement executableElement = (ExecutableElement) element;
if (executableElement.getAnnotation(GlideOption.class) != null) {
validateGlideOption(executableElement);
} else if (executableElement.getAnnotation(GlideType.class) != null) {
validateExtendsRequestManager(executableElement);
validateGlideType(executableElement);
}
}
}
}

private static void validateExtensionConstructor(Element element) {
if (!element.getModifiers().contains(Modifier.PRIVATE)) {
throw new IllegalArgumentException("RequestOptionsExtensions must be public, with private"
+ " constructors and only static methods. Found a non-private constructor");
}
ExecutableElement executableElement = (ExecutableElement) element;
if (!executableElement.getParameters().isEmpty()) {
throw new IllegalArgumentException("RequestOptionsExtensions must be public, with private"
+ " constructors and only static methods. Found parameters in the constructor");
}
}

private void validateGlideOption(ExecutableElement executableElement) {
if (isVoid(executableElement)) {
if (returnsVoid(executableElement)) {
validateDeprecatedGlideOption(executableElement);
} else {
validateNewGlideOption(executableElement);
Expand Down Expand Up @@ -158,9 +161,61 @@ private static List<String> getComparableParameterNames(
return result;
}

private void validateGlideType(ExecutableElement executableElement) {
if (returnsVoid(executableElement)) {
validateDeprecatedGlideType(executableElement);
} else {
validateNewGlideType(executableElement);
}
}

private static void validateExtendsRequestManager(ExecutableElement executableElement) {
private void validateNewGlideType(ExecutableElement executableElement) {
TypeMirror returnType = executableElement.getReturnType();
if (!isRequestBuilder(returnType) || !typeMatchesExpected(returnType, executableElement)) {
String expectedClassName = getGlideTypeValue(executableElement);
throw new IllegalArgumentException("@GlideType methods should return a RequestBuilder<"
+ expectedClassName + "> object, but given: " + returnType + ". If you're"
+ " using old style @GlideType methods, your method may have a void return type, but"
+ " doing so is deprecated and support will be removed in a future version");
}
validateGlideTypeParameters(executableElement);
}

private String getGlideTypeValue(ExecutableElement executableElement) {
return
processorUtil
.findClassValuesFromAnnotationOnClassAsNames(
executableElement, GlideType.class).iterator().next();
}

private boolean typeMatchesExpected(
TypeMirror returnType, ExecutableElement executableElement) {
if (!(returnType instanceof DeclaredType)) {
return false;
}
List<? extends TypeMirror> typeArguments = ((DeclaredType) returnType).getTypeArguments();
if (typeArguments.size() != 1) {
return false;
}
TypeMirror argument = typeArguments.get(0);
String expected = getGlideTypeValue(executableElement);
if (!argument.toString().equals(expected)) {
return false;
}
return true;
}

private boolean isRequestBuilder(TypeMirror typeMirror) {
TypeMirror toCompare = processingEnvironment.getTypeUtils().erasure(typeMirror);
return toCompare.toString().equals("com.bumptech.glide.RequestBuilder");
}

private static void validateDeprecatedGlideType(ExecutableElement executableElement) {
validateStaticVoid(executableElement, GlideType.class);
validateGlideTypeParameters(executableElement);
}

private static void validateGlideTypeParameters(ExecutableElement executableElement) {
if (executableElement.getParameters().size() != 1) {
throw new IllegalArgumentException("@GlideType methods must take a"
+ " RequestBuilder object as their first and only parameter, but given multiple for: "
Expand All @@ -181,13 +236,13 @@ private static void validateStatic(ExecutableElement executableElement, Class<?>
}
}

private static boolean isVoid(ExecutableElement executableElement) {
private static boolean returnsVoid(ExecutableElement executableElement) {
TypeMirror returnType = executableElement.getReturnType();
return returnType.getKind() == TypeKind.VOID;
}

private static void validateVoid(ExecutableElement executableElement, Class<?> clazz) {
if (!isVoid(executableElement)) {
if (!returnsVoid(executableElement)) {
throw new IllegalArgumentException("@" + clazz.getSimpleName() + " methods must return void");
}
}
Expand Down
33 changes: 32 additions & 1 deletion ...ompiler/src/main/java/com/bumptech/glide/annotation/compiler/RequestManagerGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import javax.lang.model.element.Modifier;
import javax.lang.model.element.TypeElement;
import javax.lang.model.type.DeclaredType;
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeMirror;
import javax.lang.model.util.Elements;

Expand Down Expand Up @@ -251,7 +252,16 @@ public MethodSpec apply(ExecutableElement input) {

// Generates methods added to RequestManager via GlideExtensions.
private MethodSpec generateAdditionalRequestManagerMethod(ExecutableElement extensionMethod) {
String returnType = processorUtil.findClassValuesFromAnnotationOnClassAsNames(extensionMethod,
if (extensionMethod.getReturnType().getKind() == TypeKind.VOID) {
return generateAdditionalRequestManagerMethodLegacy(extensionMethod);
} else {
return generateAdditionalRequestManagerMethodNew(extensionMethod);
}
}

private MethodSpec generateAdditionalRequestManagerMethodLegacy(
ExecutableElement extensionMethod) {
String returnType = processorUtil.findClassValuesFromAnnotationOnClassAsNames(extensionMethod,
GlideType.class).iterator().next();
ClassName returnTypeClassName = ClassName.bestGuess(returnType);
ParameterizedTypeName parameterizedTypeName =
Expand All @@ -269,6 +279,27 @@ private MethodSpec generateAdditionalRequestManagerMethod(ExecutableElement exte
.build();
}

private MethodSpec generateAdditionalRequestManagerMethodNew(
ExecutableElement extensionMethod) {
String returnType = processorUtil.findClassValuesFromAnnotationOnClassAsNames(extensionMethod,
GlideType.class).iterator().next();
ClassName returnTypeClassName = ClassName.bestGuess(returnType);
ParameterizedTypeName parameterizedTypeName =
ParameterizedTypeName.get(generatedRequestBuilderClassName, returnTypeClassName);

return MethodSpec.methodBuilder(extensionMethod.getSimpleName().toString())
.addModifiers(Modifier.PUBLIC)
.returns(parameterizedTypeName)
.addJavadoc(processorUtil.generateSeeMethodJavadoc(extensionMethod))
.addStatement(
"return ($T) $T.$N(this.as($T.class))",
parameterizedTypeName,
extensionMethod.getEnclosingElement(),
extensionMethod.getSimpleName(),
returnTypeClassName)
.build();
}

/**
* The {@code RequestOptions} subclass should always be our
* generated subclass type to avoid inadvertent errors where a different subclass is applied that
Expand Down
135 changes: 135 additions & 0 deletions ...t/src/test/java/com/bumptech/glide/annotation/compiler/InvalidGlideTypeExtensionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,4 +164,139 @@ public void compilation_withAnnotatedStaticMethod_overridingExistingType_fails()
+ " com.bumptech.glide.test.GlideRequests");
compilation.generatedSourceFile(subpackage("GlideRequests"));
}

@Test
public void compilation_withAnnotatedStaticMethod_returningRequestBuilder_succeeds() {
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).succeededWithoutWarnings();
}

@Test
public void compilation_withAnnotatedStaticMethod_returningNonRequestBuilder_fails() {
expectedException.expectMessage(
"@GlideType methods should return a RequestBuilder<java.lang.Number> object, but given:"
+ " java.lang.Object. If you're using old style @GlideType methods, your method may"
+ " have a void return type, but doing so is deprecated and support will be removed"
+ " in a future version");
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 Object asNumber(",
" RequestBuilder<Number> builder) {",
" return new Object();",
" }",
"}"));
}

@Test
public void compilation_withAnnotatedStaticMethod_returningBuilderWithIncorrectType_fails() {
expectedException.expectMessage(
"@GlideType methods should return a RequestBuilder<java.lang.Number> object, but given:"
+ " com.bumptech.glide.RequestBuilder<java.lang.Object>. If you're using old style"
+ " @GlideType methods, your method may have a void return type, but doing so is"
+ " deprecated and support will be removed in a future version");
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<Object> asNumber(",
" RequestBuilder<Object> builder) {",
" return builder;",
" }",
"}"));
}

@Test
public void compilation_withAnnotatedStaticMethod_returningBuilder_andMultipleParams_fails() {
expectedException.expectMessage(
"@GlideType methods must take a RequestBuilder object as their first and only parameter,"
+ " but given multiple for:"
+ " com.bumptech.glide.test.Extension#asNumber("
+ "com.bumptech.glide.RequestBuilder<java.lang.Number>,java.lang.Object)");
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, Object arg1) {",
" return builder;",
" }",
"}"));
}

@Test
public void compilation_withAnnotatedStaticMethod_returningBuilder_nonBuilderParam_fails() {
expectedException.expectMessage(
"@GlideType methods must take a RequestBuilder object as their first and only parameter,"
+ " but given: java.lang.Object");
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(",
" Object arg) {",
" return null;",
" }",
"}"));
}
}
Loading

0 comments on commit 7fccb32

Please sign in to comment.