Handle type-use nullness annotations and resolve conflicting `@NotNull`/`@Nullable` annotations in AutoValue and AutoBuilder. RELNOTES=Handle type-use nullness annotations and resolve conflicting `@NotNull`/`@Nullable` annotations in AutoValue and AutoBuilder PiperOrigin-RevId: 957233671
diff --git a/value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java b/value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java index b42af90..9478d5a 100644 --- a/value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java +++ b/value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java
@@ -233,6 +233,9 @@ * {@code Optional.empty()}); * <li>we have found a {@code @Nullable} type annotation that can be applied. * </ul> + * + * <p>If the property has a non-null annotation (e.g., {@code @NonNull}), it is stripped from + * the builder field type because the field starts off as null. */ public String getBuilderFieldType() { if (annotatedType.getType().getKind().isPrimitive() @@ -241,7 +244,13 @@ || availableNullableTypeAnnotations.isEmpty()) { return type; } - return TypeEncoder.encodeWithAnnotations(annotatedType, availableNullableTypeAnnotations); + Set<TypeMirror> excludedAnnotationTypes = + annotatedType.annotations().stream() + .filter(Nullables::isNonNull) + .map(AnnotationMirror::getAnnotationType) + .collect(toCollection(TypeMirrorSet::new)); + return TypeEncoder.encodeWithAnnotations( + annotatedType, availableNullableTypeAnnotations, excludedAnnotationTypes); } /** @@ -551,9 +560,11 @@ ImmutableSet.Builder<Property> props = ImmutableSet.builder(); propertyMethodsAndTypes.forEach( (propertyMethod, returnType) -> { + Set<TypeMirror> excludedAnnotationTypes = + new TypeMirrorSet(getExcludedAnnotationTypes(propertyMethod)); String propertyTypeString = TypeEncoder.encodeWithAnnotations( - returnType, ImmutableList.of(), getExcludedAnnotationTypes(propertyMethod)); + returnType, ImmutableList.of(), excludedAnnotationTypes); String propertyName = methodToPropertyName.get(propertyMethod); String identifier = methodToIdentifier.get(propertyMethod); ImmutableList<String> fieldAnnotations = @@ -744,16 +755,12 @@ private static OptionalInt nullableAnnotationIndex(List<? extends AnnotationMirror> annotations) { return IntStream.range(0, annotations.size()) - .filter(i -> isNullableAnnotation(annotations.get(i))) + .filter(i -> Nullables.isNullable(annotations.get(i))) .findFirst(); } - private static boolean isNullableAnnotation(AnnotationMirror annotation) { - return annotation.getAnnotationType().asElement().getSimpleName().contentEquals("Nullable"); - } - private static boolean isNullable(TypeMirror type) { - return nullableAnnotationIndex(type.getAnnotationMirrors()).isPresent(); + return Nullables.nullableIn(type.getAnnotationMirrors()).isPresent(); } private static boolean isTypeVariableWithNullableBound(TypeMirror type) { @@ -1251,7 +1258,7 @@ .filter( a -> { TypeElement annotationType = asType(a.getAnnotationType().asElement()); - return isNullableAnnotation(a) + return Nullables.isNullable(a) && !returnTypeAnnotations.contains(annotationType.getQualifiedName().toString()) && annotationAppliesToFields(annotationType); })
diff --git a/value/src/main/java/com/google/auto/value/processor/Nullables.java b/value/src/main/java/com/google/auto/value/processor/Nullables.java index a8da3b9..39aaeb6 100644 --- a/value/src/main/java/com/google/auto/value/processor/Nullables.java +++ b/value/src/main/java/com/google/auto/value/processor/Nullables.java
@@ -25,6 +25,7 @@ import java.util.List; import java.util.Objects; import java.util.Optional; +import java.util.function.Predicate; import java.util.stream.Stream; import javax.annotation.processing.ProcessingEnvironment; import javax.lang.model.SourceVersion; @@ -32,9 +33,11 @@ import javax.lang.model.element.AnnotationValue; import javax.lang.model.element.Element; import javax.lang.model.element.ExecutableElement; +import javax.lang.model.element.Name; import javax.lang.model.type.ArrayType; import javax.lang.model.type.DeclaredType; import javax.lang.model.type.IntersectionType; +import javax.lang.model.type.PrimitiveType; import javax.lang.model.type.TypeMirror; import javax.lang.model.type.TypeVariable; import javax.lang.model.type.WildcardType; @@ -57,6 +60,17 @@ this.nullableTypeAnnotation = nullableTypeAnnotation; } + static boolean isNullable(AnnotationMirror annotation) { + return annotation.getAnnotationType().asElement().getSimpleName().contentEquals("Nullable"); + } + + static boolean isNonNull(AnnotationMirror annotation) { + Name simpleName = annotation.getAnnotationType().asElement().getSimpleName(); + return simpleName.contentEquals("NotNull") + || simpleName.contentEquals("NonNull") + || simpleName.contentEquals("Nonnull"); + } + /** * Make an instance where the default {@code @Nullable} type annotation is discovered by looking * for methods whose parameter or return types have such an annotation. If there are none, use a @@ -121,33 +135,55 @@ }; } - private static Optional<AnnotationMirror> nullableIn(TypeMirror type) { - return new NullableFinder().visit(type); + static Optional<AnnotationMirror> nullableIn(TypeMirror type) { + return new NullnessFinder(Nullables::isNullable).visit(type); } - private static Optional<AnnotationMirror> nullableIn( - List<? extends AnnotationMirror> annotations) { + static Optional<AnnotationMirror> nullableIn(List<? extends AnnotationMirror> annotations) { return annotations.stream() - .filter(a -> a.getAnnotationType().asElement().getSimpleName().contentEquals("Nullable")) - .map(a -> (AnnotationMirror) a) // get rid of the pesky wildcard + .filter(Nullables::isNullable) + .map(a -> (AnnotationMirror) a) .findFirst(); } - private static class NullableFinder extends SimpleTypeVisitor8<Optional<AnnotationMirror>, Void> { + static Optional<AnnotationMirror> nonNullIn(TypeMirror type) { + return new NullnessFinder(Nullables::isNonNull).visit(type); + } + + static Optional<AnnotationMirror> nonNullIn(List<? extends AnnotationMirror> annotations) { + return annotations.stream() + .filter(Nullables::isNonNull) + .map(a -> (AnnotationMirror) a) + .findFirst(); + } + + private static class NullnessFinder extends SimpleTypeVisitor8<Optional<AnnotationMirror>, Void> { + private final Predicate<AnnotationMirror> predicate; private final TypeMirrorSet visiting = new TypeMirrorSet(); - NullableFinder() { + NullnessFinder(Predicate<AnnotationMirror> predicate) { super(Optional.empty()); + this.predicate = predicate; } - // Primitives can't be @Nullable so we don't check that. + private Optional<AnnotationMirror> findIn(List<? extends AnnotationMirror> annotations) { + return annotations.stream().filter(predicate).map(a -> (AnnotationMirror) a).findFirst(); + } + + @Override + public Optional<AnnotationMirror> visitPrimitive(PrimitiveType t, Void unused) { + // Type annotations can appear on primitive types (e.g., @Nullable int or @NonNull int). + // While null-checking frameworks might not support this, we handle it for robustness + // and consistency with TypeEncoder. + return findIn(t.getAnnotationMirrors()); + } @Override public Optional<AnnotationMirror> visitDeclared(DeclaredType t, Void unused) { if (!visiting.add(t)) { return Optional.empty(); } - return nullableIn(t.getAnnotationMirrors()) + return findIn(t.getAnnotationMirrors()) .map(Optional::of) .orElseGet(() -> visitAll(t.getTypeArguments())); } @@ -157,21 +193,21 @@ if (!visiting.add(t)) { return Optional.empty(); } - return nullableIn(t.getAnnotationMirrors()) + return findIn(t.getAnnotationMirrors()) .map(Optional::of) .orElseGet(() -> visitAll(ImmutableList.of(t.getUpperBound(), t.getLowerBound()))); } @Override public Optional<AnnotationMirror> visitArray(ArrayType t, Void unused) { - return nullableIn(t.getAnnotationMirrors()) + return findIn(t.getAnnotationMirrors()) .map(Optional::of) .orElseGet(() -> visit(t.getComponentType())); } @Override public Optional<AnnotationMirror> visitWildcard(WildcardType t, Void unused) { - return nullableIn(t.getAnnotationMirrors()) + return findIn(t.getAnnotationMirrors()) .map(Optional::of) .orElseGet( () -> @@ -183,7 +219,7 @@ @Override public Optional<AnnotationMirror> visitIntersection(IntersectionType t, Void unused) { - return nullableIn(t.getAnnotationMirrors()) + return findIn(t.getAnnotationMirrors()) .map(Optional::of) .orElseGet(() -> visitAll(t.getBounds())); }
diff --git a/value/src/main/java/com/google/auto/value/processor/TypeEncoder.java b/value/src/main/java/com/google/auto/value/processor/TypeEncoder.java index ef0061a..74a08e9 100644 --- a/value/src/main/java/com/google/auto/value/processor/TypeEncoder.java +++ b/value/src/main/java/com/google/auto/value/processor/TypeEncoder.java
@@ -442,35 +442,67 @@ @Override public StringBuilder visitDeclared(DeclaredType type, StringBuilder sb) { List<? extends AnnotationMirror> annotationMirrors = getTypeAnnotations.apply(type); + TypeMirror enclosing = EclipseHack.getEnclosingType(type); + List<? extends AnnotationMirror> enclosingAnnotations = + enclosing.getKind().equals(TypeKind.DECLARED) + ? getTypeAnnotations.apply(enclosing) + : ImmutableList.of(); + // If the type or its enclosing type has a @Nullable annotation, we strip any + // non-null annotations from the type to avoid conflicting annotations (e.g. @Nullable + // @NonNull). + boolean hasNullable = + annotationMirrors.stream().anyMatch(Nullables::isNullable) + || enclosingAnnotations.stream().anyMatch(Nullables::isNullable); + if (hasNullable) { + annotationMirrors = + annotationMirrors.stream().filter(a -> !Nullables.isNonNull(a)).collect(toList()); + } if (annotationMirrors.isEmpty()) { super.visitDeclared(type, sb); } else { - TypeMirror enclosing = EclipseHack.getEnclosingType(type); - if (enclosing.getKind().equals(TypeKind.DECLARED)) { - // We have something like com.example.Outer<Double>.@Annot Inner. - // We'll recursively encode com.example.Outer<Double> first, - // which if it is also annotated might result in a mouthful like - // `«com.example.Outer`@`org.annots.Nullable``»com.example.Outer`<`java.lang.Double`> . - // That annotation will have been added by a recursive call to this method. - // Then we'll add the annotation on the .Inner class, which we know is there because - // annotationMirrors is not empty. That means we'll append .@`org.annots.Annot` Inner . - visit2(enclosing, sb); - sb.append("."); - appendAnnotationsWithExclusions(annotationMirrors, sb); - sb.append(type.asElement().getSimpleName()); - } else { - // This isn't an inner class, so we have the simpler (but still complicated) case of - // needing to place the annotation correctly in cases like java.util.@Nullable Map . - // See the class doc comment for an explanation of « and » here. - String className = className(type); - sb.append("`«").append(className).append("`"); - appendAnnotationsWithExclusions(annotationMirrors, sb); - sb.append("`»").append(className).append("`"); - } - appendTypeArguments(type, sb); + visitAnnotatedDeclared(type, annotationMirrors, sb); } return sb; } + + private void visitEnclosingDeclared(DeclaredType enclosing, StringBuilder sb) { + List<? extends AnnotationMirror> annotationMirrors = getTypeAnnotations.apply(enclosing); + // Enclosing instances should NEVER have @NotNull annotations. + annotationMirrors = + annotationMirrors.stream().filter(a -> !Nullables.isNonNull(a)).collect(toList()); + if (annotationMirrors.isEmpty()) { + super.visitDeclared(enclosing, sb); + } else { + visitAnnotatedDeclared(enclosing, annotationMirrors, sb); + } + } + + private void visitAnnotatedDeclared( + DeclaredType type, List<? extends AnnotationMirror> annotationMirrors, StringBuilder sb) { + TypeMirror enclosing = EclipseHack.getEnclosingType(type); + if (enclosing.getKind().equals(TypeKind.DECLARED)) { + // We have something like com.example.Outer<Double>.@Annot Inner. + // We'll recursively encode com.example.Outer<Double> first, + // which if it is also annotated might result in a mouthful like + // `«com.example.Outer`@`org.annots.Nullable``»com.example.Outer`<`java.lang.Double`> . + // That annotation will have been added by a recursive call to this method. + // Then we'll add the annotation on the .Inner class, which we know is there because + // annotationMirrors is not empty. That means we'll append .@`org.annots.Annot` Inner . + visitEnclosingDeclared(MoreTypes.asDeclared(enclosing), sb); + sb.append("."); + appendAnnotationsWithExclusions(annotationMirrors, sb); + sb.append(type.asElement().getSimpleName()); + } else { + // This isn't an inner class, so we have the simpler (but still complicated) case of + // needing to place the annotation correctly in cases like java.util.@Nullable Map . + // See the class doc comment for an explanation of « and » here. + String className = className(type); + sb.append("`«").append(className).append("`"); + appendAnnotationsWithExclusions(annotationMirrors, sb); + sb.append("`»").append(className).append("`"); + } + appendTypeArguments(type, sb); + } } private static class TypeRewriter {
diff --git a/value/src/test/java/com/google/auto/value/processor/AutoBuilderCompilationTest.java b/value/src/test/java/com/google/auto/value/processor/AutoBuilderCompilationTest.java index 2fb26d5..4484f9f 100644 --- a/value/src/test/java/com/google/auto/value/processor/AutoBuilderCompilationTest.java +++ b/value/src/test/java/com/google/auto/value/processor/AutoBuilderCompilationTest.java
@@ -1112,8 +1112,7 @@ "@Generated(\"com.google.auto.value.processor.AutoBuilderProcessor\")", "class AutoBuilder_Baz_Builder implements Baz.Builder {", "", - " // TODO(b/540040170): conflicting @Baz.NotNull @Nullable annotations", - " private @Baz.NotNull @Nullable String aString;", + " private @Nullable String aString;", "", " AutoBuilder_Baz_Builder() {", " }", @@ -1145,6 +1144,42 @@ .hasSourceEquivalentTo(expectedOutput); } + @Test + public void nullablePrimitiveTypeUseAnnotation() { + JavaFileObject javaFileObject = + JavaFileObjects.forSourceLines( + "foo.bar.Baz", + "package foo.bar;", + "", + "import com.google.auto.value.AutoBuilder;", + "import java.lang.annotation.ElementType;", + "import java.lang.annotation.Target;", + "", + "public class Baz {", + " @Target(ElementType.TYPE_USE)", + " public @interface Nullable {}", + "", + " private final int thing;", + "", + " public Baz(@Nullable int thing) {", + " this.thing = thing;", + " }", + "", + " @AutoBuilder", + " public interface Builder {", + " Builder thing(int thing);", + " Baz build();", + " }", + "}"); + Compilation compilation = + javac().withProcessors(new AutoBuilderProcessor()).compile(javaFileObject); + assertThat(compilation).failed(); + assertThat(compilation) + .hadErrorContaining("[AutoBuilderNullPrimitive] Primitive types cannot be @Nullable") + .inFile(javaFileObject) + .onLineContaining("Baz(@Nullable int thing)"); + } + private static String sorted(String... imports) { return stream(imports).sorted().collect(joining("\n")); }
diff --git a/value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java b/value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java index 0660a32..46988ac 100644 --- a/value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java +++ b/value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java
@@ -4151,8 +4151,7 @@ " }", "", " static final class Builder extends Baz.Builder {", - " // TODO(b/540040170): conflicting @Baz.NotNull @Nullable annotations", - " private @Baz.NotNull @Nullable String aString;", + " private @Nullable String aString;", " Builder() {", " }", " @Override", @@ -4227,15 +4226,15 @@ "@Generated(\"com.google.auto.value.processor.AutoValueProcessor\")", "final class AutoValue_Baz extends Baz {", "", - " private final Baz.@Baz.NotNull Outer.@Baz.Nullable Inner inner;", + " private final Baz.Outer.@Baz.Nullable Inner inner;", "", " private AutoValue_Baz(", - " Baz.@Baz.NotNull Outer.@Baz.Nullable Inner inner) {", + " Baz.Outer.@Baz.Nullable Inner inner) {", " this.inner = inner;", " }", "", " @Override", - " public Baz.@Baz.NotNull Outer.@Baz.Nullable Inner inner() {", + " public Baz.Outer.@Baz.Nullable Inner inner() {", " return inner;", " }", "", @@ -4268,13 +4267,11 @@ " }", "", " static final class Builder extends Baz.Builder {", - " // TODO: The unfixed processor generates Baz.@Baz.NotNull Outer.@Baz.Nullable" - + " Inner. Remove @Baz.NotNull when fix is submitted.", - " private Baz.@Baz.NotNull Outer.@Baz.Nullable Inner inner;", + " private Baz.Outer.@Baz.Nullable Inner inner;", " Builder() {", " }", " @Override", - " public Baz.Builder setInner(Baz.@Baz.NotNull Outer.@Baz.Nullable Inner inner) {", + " public Baz.Builder setInner(Baz.Outer.@Baz.Nullable Inner inner) {", " this.inner = inner;", " return this;", " }",