From 0efa2a2d06e292c145463119c71445bb66b9504a Mon Sep 17 00:00:00 2001 From: Belhorma Bendebiche Date: Fri, 19 Apr 2019 17:35:26 -0400 Subject: [PATCH] Don't emit `@SafeVarargs` for reifiable types Attempt to detect if a type is reifiable, and omit the annotation in those cases. https://docs.oracle.com/javase/specs/jls/se7/html/jls-4.html However one problem as noted in the comments is we can't get at a ParameterizedTypeName's enclosing type. So for instance, this code will treat `Foo.Bar` as reifiable, even though it isn't. One idea to get around it would be to `toString()` the type and check if it contains any type parameters, although that seems kinda messy. --- .../processor/AutoMatterProcessor.java | 37 ++++++++++++++++++- .../automatter/AutoMatterProcessorTest.java | 33 +++++++++++++++++ .../expected/CollectionFieldsBuilder.java | 12 ++---- .../CollectionInterfaceFieldBuilder.java | 3 +- .../NullableCollectionFieldsBuilder.java | 6 +-- ...ncreteExtensionOfGenericParentBuilder.java | 3 +- 6 files changed, 76 insertions(+), 18 deletions(-) diff --git a/processor/src/main/java/io/norberg/automatter/processor/AutoMatterProcessor.java b/processor/src/main/java/io/norberg/automatter/processor/AutoMatterProcessor.java index d776a5d9..58c39ed7 100644 --- a/processor/src/main/java/io/norberg/automatter/processor/AutoMatterProcessor.java +++ b/processor/src/main/java/io/norberg/automatter/processor/AutoMatterProcessor.java @@ -29,6 +29,8 @@ import com.squareup.javapoet.WildcardTypeName; import io.norberg.automatter.AutoMatter; import java.io.IOException; +import java.lang.reflect.Parameter; +import java.lang.reflect.ParameterizedType; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -43,6 +45,7 @@ import java.util.Set; import java.util.TreeMap; import java.util.TreeSet; +import java.util.function.Function; import java.util.stream.Stream; import javax.annotation.processing.AbstractProcessor; import javax.annotation.processing.Filer; @@ -524,7 +527,9 @@ private MethodSpec collectionVarargSetter(final Descriptor d, final ExecutableEl .varargs() .returns(builderType(d)); - ensureSafeVarargs(setter); + if (!isReifiable(itemType)) { + ensureSafeVarargs(setter); + } collectionNullGuard(setter, field); @@ -533,7 +538,6 @@ private MethodSpec collectionVarargSetter(final Descriptor d, final ExecutableEl } private void ensureSafeVarargs(MethodSpec.Builder setter) { - // TODO: Add SafeVarargs annotation only for non-reifiable types. AnnotationSpec safeVarargsAnnotation = AnnotationSpec.builder(SafeVarargs.class).build(); setter @@ -1426,6 +1430,35 @@ private static String capitalizeFirstLetter(String s) { return s.substring(0, 1).toUpperCase() + (s.length() > 1 ? s.substring(1) : ""); } + private static boolean isUnboundedWildcard(TypeName type) { + if (type instanceof WildcardTypeName) { + WildcardTypeName t = (WildcardTypeName) type; + return t.lowerBounds.isEmpty() && t.upperBounds.stream().allMatch(bound -> bound.equals(TypeName.OBJECT)); + } + return false; + } + + public static boolean isReifiable(TypeName type) { + if (type.isPrimitive()) return true; + if (type.isBoxedPrimitive()) return true; + + // TODO: handle nested types, per JLS ยง4.7 + // JavaPoet does not expose ParameterizedTypeName's enclosing type, + // so we can't verify that the enclosing type is reifiable. + + if (type instanceof ParameterizedTypeName) { + ParameterizedTypeName parameterized = (ParameterizedTypeName) type; + return parameterized.typeArguments.stream().allMatch(AutoMatterProcessor::isUnboundedWildcard); + } else if (type instanceof ArrayTypeName) { + ArrayTypeName array = (ArrayTypeName) type; + return isReifiable(array.componentType); + } else if (type instanceof ClassName) { + return true; + } + + return false; + } + @Override public SourceVersion getSupportedSourceVersion() { return SourceVersion.latestSupported(); diff --git a/processor/src/test/java/io/norberg/automatter/AutoMatterProcessorTest.java b/processor/src/test/java/io/norberg/automatter/AutoMatterProcessorTest.java index 7b00986a..2e8b1eaa 100644 --- a/processor/src/test/java/io/norberg/automatter/AutoMatterProcessorTest.java +++ b/processor/src/test/java/io/norberg/automatter/AutoMatterProcessorTest.java @@ -5,14 +5,23 @@ import static com.google.testing.compile.Compiler.javac; import static com.google.testing.compile.JavaSourceSubjectFactory.javaSource; import static com.google.testing.compile.JavaSourcesSubjectFactory.javaSources; +import static com.squareup.javapoet.WildcardTypeName.subtypeOf; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import com.google.common.collect.ImmutableSet; import com.google.common.io.Resources; import com.google.testing.compile.Compilation; import com.google.testing.compile.JavaFileObjects; +import com.squareup.javapoet.ArrayTypeName; +import com.squareup.javapoet.ClassName; +import com.squareup.javapoet.ParameterizedTypeName; +import com.squareup.javapoet.TypeName; +import com.squareup.javapoet.WildcardTypeName; import io.norberg.automatter.processor.AutoMatterProcessor; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.List; import java.util.Set; import java.util.stream.Collectors; import javax.tools.Diagnostic; @@ -420,4 +429,28 @@ private boolean hasJutOptional() { return false; } } + + private TypeName[] wildcards(final int size) { + final WildcardTypeName wildcard = subtypeOf(ClassName.get(Object.class)); + final TypeName[] wildcards = new TypeName[size]; + for (int i = 0; i < size; i++) { + wildcards[i] = wildcard; + } + return wildcards; + } + + @Test + public void testReifiable() { + ParameterizedTypeName listOfObject = ParameterizedTypeName.get(ClassName.get("java.util", "List"), TypeName.OBJECT); + assertTrue(AutoMatterProcessor.isReifiable(TypeName.BOOLEAN)); + assertTrue(AutoMatterProcessor.isReifiable(TypeName.get(String.class))); + assertTrue(AutoMatterProcessor.isReifiable(ClassName.get("java.util", "List"))); + assertTrue(AutoMatterProcessor.isReifiable(ParameterizedTypeName.get(ClassName.get("java.util", "List"), wildcards(1)[0]))); + assertFalse(AutoMatterProcessor.isReifiable(listOfObject)); + assertTrue(AutoMatterProcessor.isReifiable(ArrayTypeName.of(String.class))); + assertFalse(AutoMatterProcessor.isReifiable(ArrayTypeName.of(listOfObject))); + + // TODO: handle nested classes + //assertFalse(AutoMatterProcessor.isReifiable(listOfObject.nestedClass("Foo"))); + } } diff --git a/processor/src/test/resources/expected/CollectionFieldsBuilder.java b/processor/src/test/resources/expected/CollectionFieldsBuilder.java index 52f6ac0c..cc8edfb0 100644 --- a/processor/src/test/resources/expected/CollectionFieldsBuilder.java +++ b/processor/src/test/resources/expected/CollectionFieldsBuilder.java @@ -114,8 +114,7 @@ public CollectionFieldsBuilder strings(Iterator strings) { return this; } - @SafeVarargs - public final CollectionFieldsBuilder strings(String... strings) { + public CollectionFieldsBuilder strings(String... strings) { if (strings == null) { throw new NullPointerException("strings"); } @@ -473,8 +472,7 @@ public CollectionFieldsBuilder numbers(Iterator numbers) { return this; } - @SafeVarargs - public final CollectionFieldsBuilder numbers(Long... numbers) { + public CollectionFieldsBuilder numbers(Long... numbers) { if (numbers == null) { throw new NullPointerException("numbers"); } @@ -541,8 +539,7 @@ public CollectionFieldsBuilder sortedNumbers(Iterator sortedNumb return this; } - @SafeVarargs - public final CollectionFieldsBuilder sortedNumbers(Long... sortedNumbers) { + public CollectionFieldsBuilder sortedNumbers(Long... sortedNumbers) { if (sortedNumbers == null) { throw new NullPointerException("sortedNumbers"); } @@ -609,8 +606,7 @@ public CollectionFieldsBuilder navigableNumbers(Iterator navigab return this; } - @SafeVarargs - public final CollectionFieldsBuilder navigableNumbers(Long... navigableNumbers) { + public CollectionFieldsBuilder navigableNumbers(Long... navigableNumbers) { if (navigableNumbers == null) { throw new NullPointerException("navigableNumbers"); } diff --git a/processor/src/test/resources/expected/CollectionInterfaceFieldBuilder.java b/processor/src/test/resources/expected/CollectionInterfaceFieldBuilder.java index 5c24a155..e691f287 100644 --- a/processor/src/test/resources/expected/CollectionInterfaceFieldBuilder.java +++ b/processor/src/test/resources/expected/CollectionInterfaceFieldBuilder.java @@ -69,8 +69,7 @@ public CollectionInterfaceFieldBuilder strings(Iterator string return this; } - @SafeVarargs - public final CollectionInterfaceFieldBuilder strings(String... strings) { + public CollectionInterfaceFieldBuilder strings(String... strings) { if (strings == null) { throw new NullPointerException("strings"); } diff --git a/processor/src/test/resources/expected/NullableCollectionFieldsBuilder.java b/processor/src/test/resources/expected/NullableCollectionFieldsBuilder.java index d3b3579a..c58797e2 100644 --- a/processor/src/test/resources/expected/NullableCollectionFieldsBuilder.java +++ b/processor/src/test/resources/expected/NullableCollectionFieldsBuilder.java @@ -79,8 +79,7 @@ public NullableCollectionFieldsBuilder strings(Iterator string return this; } - @SafeVarargs - public final NullableCollectionFieldsBuilder strings(String... strings) { + public NullableCollectionFieldsBuilder strings(String... strings) { if (strings == null) { this.strings = null; return this; @@ -198,8 +197,7 @@ public NullableCollectionFieldsBuilder numbers(Iterator numbers) return this; } - @SafeVarargs - public final NullableCollectionFieldsBuilder numbers(Long... numbers) { + public NullableCollectionFieldsBuilder numbers(Long... numbers) { if (numbers == null) { this.numbers = null; return this; diff --git a/processor/src/test/resources/expected/inheritance/ConcreteExtensionOfGenericParentBuilder.java b/processor/src/test/resources/expected/inheritance/ConcreteExtensionOfGenericParentBuilder.java index 7d6a876c..9a582651 100644 --- a/processor/src/test/resources/expected/inheritance/ConcreteExtensionOfGenericParentBuilder.java +++ b/processor/src/test/resources/expected/inheritance/ConcreteExtensionOfGenericParentBuilder.java @@ -74,8 +74,7 @@ public ConcreteExtensionOfGenericParentBuilder foos(Iterator return this; } - @SafeVarargs - public final ConcreteExtensionOfGenericParentBuilder foos(Integer... foos) { + public ConcreteExtensionOfGenericParentBuilder foos(Integer... foos) { if (foos == null) { throw new NullPointerException("foos"); }