Skip to content

Commit

Permalink
Ensure that type annotations are placed correctly.
Browse files Browse the repository at this point in the history 
Treating them the same as other annotations leads to problems with nested types like `Map.Entry`. If a field or parameter is a `Map.Entry` that is `@Nullable`, and if `@Nullable` is a `TYPE_USE` annotation, then the declaration must use `Map.@nullable Entry`. JavaPoet will do this correctly if the annotations are applied to the `TypeName` for the declaration.

Also ensure that type annotations are included in the `T` of `Provider<T>` when that appears in field or parameter declarations.

Fixes #949.

RELNOTES=AutoFactory type annotations are now placed correctly even for nested types.
PiperOrigin-RevId: 350930373
  • Loading branch information
@eamonnmcmanus
eamonnmcmanus authored and Google Java Core Libraries committed Jan 9, 2021
1 parent a7cef3f commit f26d2df
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 20 deletions.
71 changes: 55 additions & 16 deletions factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import static javax.lang.model.element.Modifier.PUBLIC;
import static javax.lang.model.element.Modifier.STATIC;

import com.google.auto.common.AnnotationMirrors;
import com.google.auto.common.AnnotationValues;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSetMultimap;
Expand All @@ -43,13 +45,19 @@
import com.squareup.javapoet.TypeSpec;
import com.squareup.javapoet.TypeVariableName;
import java.io.IOException;
import java.lang.annotation.Target;
import java.util.Iterator;
import java.util.List;
import java.util.Optional;
import java.util.stream.Stream;
import javax.annotation.processing.Filer;
import javax.annotation.processing.ProcessingEnvironment;
import javax.inject.Inject;
import javax.inject.Provider;
import javax.lang.model.SourceVersion;
import javax.lang.model.element.AnnotationMirror;
import javax.lang.model.element.Element;
import javax.lang.model.element.VariableElement;
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeMirror;
import javax.lang.model.type.TypeVariable;
Expand Down Expand Up @@ -143,7 +151,7 @@ private void addFactoryMethods(
ImmutableSet<TypeVariableName> factoryTypeVariables) {
for (FactoryMethodDescriptor methodDescriptor : descriptor.methodDescriptors()) {
MethodSpec.Builder method =
MethodSpec.methodBuilder(methodDescriptor.name())
methodBuilder(methodDescriptor.name())
.addTypeVariables(getMethodTypeVariables(methodDescriptor, factoryTypeVariables))
.returns(TypeName.get(methodDescriptor.returnType()))
.varargs(methodDescriptor.isVarArgs());
Expand Down Expand Up @@ -219,17 +227,45 @@ private void addImplementationMethods(
private ImmutableList<ParameterSpec> parameters(Iterable<Parameter> parameters) {
ImmutableList.Builder<ParameterSpec> builder = ImmutableList.builder();
for (Parameter parameter : parameters) {
ParameterSpec.Builder parameterBuilder =
ParameterSpec.builder(resolveTypeName(parameter.type().get()), parameter.name());
Stream.of(parameter.nullable(), parameter.key().qualifier())
.flatMap(Streams::stream)
.map(AnnotationSpec::get)
.forEach(parameterBuilder::addAnnotation);
builder.add(parameterBuilder.build());
TypeName type = resolveTypeName(parameter.type().get());
// Remove TYPE_USE annotations, since resolveTypeName will already have included those in
// the TypeName it returns.
List<AnnotationSpec> annotations =
Stream.of(parameter.nullable(), parameter.key().qualifier())
.flatMap(Streams::stream)
.filter(a -> !isTypeUseAnnotation(a))
.map(AnnotationSpec::get)
.collect(toList());
ParameterSpec parameterSpec =
ParameterSpec.builder(type, parameter.name())
.addAnnotations(annotations)
.build();
builder.add(parameterSpec);
}
return builder.build();
}

private static boolean isTypeUseAnnotation(AnnotationMirror mirror) {
Element annotationElement = mirror.getAnnotationType().asElement();
// This is basically equivalent to:
// Target target = annotationElement.getAnnotation(Target.class);
// return target != null
// && Arrays.asList(annotationElement.getAnnotation(Target.class)).contains(TYPE_USE);
// but that might blow up if the annotation is being compiled at the same time and has an
// undefined identifier in its @Target values. The rigmarole below avoids that problem.
Optional<AnnotationMirror> maybeTargetMirror =
Mirrors.getAnnotationMirror(annotationElement, Target.class);
return maybeTargetMirror
.map(
targetMirror ->
AnnotationValues.getEnums(
AnnotationMirrors.getAnnotationValue(targetMirror, "value"))
.stream()
.map(VariableElement::getSimpleName)
.anyMatch(name -> name.contentEquals("TYPE_USE")))
.orElse(false);
}

private static void addCheckNotNullMethod(
TypeSpec.Builder factory, FactoryDescriptor descriptor) {
if (shouldGenerateCheckNotNull(descriptor)) {
Expand Down Expand Up @@ -284,17 +320,20 @@ private static boolean shouldGenerateCheckNotNull(FactoryDescriptor descriptor)
* {@code @AutoFactory class Foo} has a constructor parameter of type {@code BarFactory} and
* {@code @AutoFactory class Bar} has a constructor parameter of type {@code FooFactory}. We did
* in fact find instances of this in Google's source base.
*
* <p>If the type has type annotations then include those in the returned {@link TypeName}.
*/
private TypeName resolveTypeName(TypeMirror type) {
if (type.getKind() != TypeKind.ERROR) {
return TypeName.get(type);
}
ImmutableSet<PackageAndClass> factoryNames = factoriesBeingCreated.get(type.toString());
if (factoryNames.size() == 1) {
PackageAndClass packageAndClass = Iterables.getOnlyElement(factoryNames);
return ClassName.get(packageAndClass.packageName(), packageAndClass.className());
TypeName typeName = TypeName.get(type);
if (type.getKind() == TypeKind.ERROR) {
ImmutableSet<PackageAndClass> factoryNames = factoriesBeingCreated.get(type.toString());
if (factoryNames.size() == 1) {
PackageAndClass packageAndClass = Iterables.getOnlyElement(factoryNames);
typeName = ClassName.get(packageAndClass.packageName(), packageAndClass.className());
}
}
return TypeName.get(type);
return typeName.annotated(
type.getAnnotationMirrors().stream().map(AnnotationSpec::get).collect(toList()));
}

private static ImmutableSet<TypeVariableName> getFactoryTypeVariables(
Expand Down
7 changes: 7 additions & 0 deletions factory/src/test/java/com/google/auto/factory/processor/AutoFactoryProcessorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/
package com.google.auto.factory.processor;

import static com.google.common.base.StandardSystemProperty.JAVA_SPECIFICATION_VERSION;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.testing.compile.CompilationSubject.assertThat;
import static java.lang.Math.max;
import static java.lang.Math.min;
Expand Down Expand Up @@ -427,6 +429,11 @@ public void customNullableType() {

@Test
public void checkerFrameworkNullableType() {
// TYPE_USE annotations are pretty much unusable with annotation processors on Java 8 because
// of bugs that mean they only appear in the javax.lang.model API when the compiler feels like
// it. Checking for a java.specification.version that does not start with "1." eliminates 8 and
// any earlier version.
assume().that(JAVA_SPECIFICATION_VERSION.value()).doesNotMatch("1\\..*");
Compilation compilation =
javac.compile(JavaFileObjects.forResource("good/CheckerFrameworkNullable.java"));
assertThat(compilation).succeededWithoutWarnings();
Expand Down
15 changes: 12 additions & 3 deletions factory/src/test/resources/expected/CheckerFrameworkNullableFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package tests;

import java.util.Map;
import javax.annotation.processing.Generated;
import javax.inject.Inject;
import javax.inject.Provider;
Expand All @@ -29,19 +30,27 @@ final class CheckerFrameworkNullableFactory {

private final Provider<String> java_lang_StringProvider;

private final Provider<Map.@NullableType Entry<?, ?>> providedNestedNullableTypeProvider;

@Inject
CheckerFrameworkNullableFactory(
Provider<String> java_lang_StringProvider) {
Provider<String> java_lang_StringProvider,
Provider<Map.@NullableType Entry<?, ?>> providedNestedNullableTypeProvider) {
this.java_lang_StringProvider = checkNotNull(java_lang_StringProvider, 1);
this.providedNestedNullableTypeProvider = checkNotNull(providedNestedNullableTypeProvider, 2);
}

CheckerFrameworkNullable create(
@NullableDecl String nullableDecl, @NullableType String nullableType) {
@NullableDecl String nullableDecl,
@NullableType String nullableType,
Map.@NullableType Entry<?, ?> nestedNullableType) {
return new CheckerFrameworkNullable(
nullableDecl,
java_lang_StringProvider.get(),
nullableType,
java_lang_StringProvider.get());
java_lang_StringProvider.get(),
nestedNullableType,
providedNestedNullableTypeProvider.get());
}

private static <T> T checkNotNull(T reference, int argumentIndex) {
Expand Down
5 changes: 4 additions & 1 deletion factory/src/test/resources/good/CheckerFrameworkNullable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import com.google.auto.factory.AutoFactory;
import com.google.auto.factory.Provided;
import java.util.Map;
import org.checkerframework.checker.nullness.compatqual.NullableDecl;
import org.checkerframework.checker.nullness.compatqual.NullableType;

Expand All @@ -27,5 +28,7 @@ final class CheckerFrameworkNullable {
@NullableDecl String nullableDecl,
@Provided @NullableDecl String providedNullableDecl,
@NullableType String nullableType,
@Provided @NullableType String providedNullableType) {}
@Provided @NullableType String providedNullableType,
Map.@NullableType Entry<?, ?> nestedNullableType,
@Provided Map.@NullableType Entry<?, ?> providedNestedNullableType) {}
}

0 comments on commit f26d2df

Please sign in to comment.