diff options
Diffstat (limited to 'value/src')
6 files changed, 87 insertions, 216 deletions
diff --git a/value/src/it/functional/pom.xml b/value/src/it/functional/pom.xml index 414d6c59..bf92f4a1 100644 --- a/value/src/it/functional/pom.xml +++ b/value/src/it/functional/pom.xml @@ -32,7 +32,7 @@ <version>HEAD-SNAPSHOT</version> <name>Auto-Value Functional Integration Test</name> <properties> - <kotlin.version>1.8.22</kotlin.version> + <kotlin.version>1.9.10</kotlin.version> </properties> <dependencies> <dependency> @@ -92,7 +92,7 @@ <dependency> <groupId>dev.gradleplugins</groupId> <artifactId>gradle-test-kit</artifactId> - <version>8.1</version> + <version>8.3</version> <scope>test</scope> </dependency> <dependency> 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 b7bee429..82d31bf8 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 @@ -17,6 +17,7 @@ package com.google.auto.value.processor; import static com.google.auto.common.AnnotationMirrors.getAnnotationValue; import static com.google.auto.common.GeneratedAnnotations.generatedAnnotation; +import static com.google.auto.common.MoreElements.asType; import static com.google.auto.common.MoreElements.getPackage; import static com.google.auto.common.MoreElements.isAnnotationPresent; import static com.google.auto.common.MoreStreams.toImmutableList; @@ -1215,8 +1216,24 @@ abstract class AutoValueishProcessor extends AbstractProcessor { private ImmutableList<AnnotationMirror> propertyFieldAnnotations( TypeElement type, ExecutableElement method) { + // We need to exclude type annotations from the ones being output on the method, since + // they will be output as part of the field's type. + Set<String> returnTypeAnnotations = + getReturnTypeAnnotations(method, this::annotationAppliesToFields); if (!hasAnnotationMirror(method, COPY_ANNOTATIONS_NAME)) { - return ImmutableList.of(); + // If there's no @CopyAnnotations, we will still copy a @Nullable annotation, if (1) it is not + // a TYPE_USE annotation (those appear as part of the type in the generated code) and (2) it + // applies to fields. All known non-TYPE_USE @Nullable annotations do apply to fields, but we + // check just in case. + return method.getAnnotationMirrors().stream() + .filter( + a -> { + TypeElement annotationType = asType(a.getAnnotationType().asElement()); + return isNullable(a) + && !returnTypeAnnotations.contains(annotationType.getQualifiedName().toString()) + && annotationAppliesToFields(annotationType); + }) + .collect(toImmutableList()); } ImmutableSet<String> excludedAnnotations = ImmutableSet.<String>builder() @@ -1224,10 +1241,6 @@ abstract class AutoValueishProcessor extends AbstractProcessor { .add(Override.class.getCanonicalName()) .build(); - // We need to exclude type annotations from the ones being output on the method, since - // they will be output as part of the field's type. - Set<String> returnTypeAnnotations = - getReturnTypeAnnotations(method, this::annotationAppliesToFields); Set<String> nonFieldAnnotations = method.getAnnotationMirrors().stream() .map(a -> a.getAnnotationType().asElement()) diff --git a/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java b/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java index b0872009..591b0cc8 100644 --- a/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java +++ b/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java @@ -403,6 +403,13 @@ abstract class BuilderMethodClassifier<E extends Element> { TypeMirror returnType = methodMirror.getReturnType(); if (typeUtils.isSubtype(builderType.asType(), returnType) && !MoreTypes.isTypeOf(Object.class, returnType)) { + if (nullableAnnotationFor(method, returnType).isPresent()) { + errorReporter. + reportWarning( + method, + "[%sBuilderSetterNullable] Setter methods always return the Builder so @Nullable" + + " is not appropriate", autoWhat()); + } // We allow the return type to be a supertype (other than Object), to support step builders. TypeMirror parameterType = Iterables.getOnlyElement(methodMirror.getParameterTypes()); propertyNameToSetters.put( diff --git a/value/src/main/java/com/google/auto/value/processor/TemplateVars.java b/value/src/main/java/com/google/auto/value/processor/TemplateVars.java index d9e3337b..73253629 100644 --- a/value/src/main/java/com/google/auto/value/processor/TemplateVars.java +++ b/value/src/main/java/com/google/auto/value/processor/TemplateVars.java @@ -15,27 +15,28 @@ */ package com.google.auto.value.processor; -import com.google.common.base.Throwables; +import static java.nio.charset.StandardCharsets.UTF_8; + +import com.google.common.base.Ascii; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.io.ByteStreams; import com.google.escapevelocity.Template; import java.io.File; import java.io.FileInputStream; -import java.io.FilterInputStream; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; import java.io.Reader; -import java.io.UnsupportedEncodingException; +import java.io.StringReader; +import java.io.UncheckedIOException; import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; -import java.nio.charset.StandardCharsets; import java.util.Map; import java.util.TreeMap; -import java.util.jar.JarEntry; import java.util.jar.JarFile; /** @@ -95,7 +96,7 @@ abstract class TemplateVars { * concrete subclass of TemplateVars) into the template returned by {@link #parsedTemplate()}. */ String toText() { - Map<String, Object> vars = toVars(); + ImmutableMap<String, Object> vars = toVars(); return parsedTemplate().evaluate(vars); } @@ -121,69 +122,42 @@ abstract class TemplateVars { static Template parsedTemplateForResource(String resourceName) { try { - return Template.parseFrom(resourceName, TemplateVars::readerFromResource); - } catch (UnsupportedEncodingException e) { - throw new AssertionError(e); - } catch (IOException | NullPointerException | IllegalStateException e) { - // https://github.com/google/auto/pull/439 says that we can get NullPointerException. - // https://github.com/google/auto/issues/715 says that we can get IllegalStateException - return retryParseAfterException(resourceName, e); - } - } - - private static Template retryParseAfterException(String resourceName, Exception exception) { - try { return Template.parseFrom(resourceName, TemplateVars::readerFromUrl); - } catch (IOException t) { - // Chain the original exception so we can see both problems. - Throwables.getRootCause(exception).initCause(t); - throw new AssertionError(exception); + } catch (IOException e) { + throw new UncheckedIOException(e); } } - private static Reader readerFromResource(String resourceName) { - InputStream in = TemplateVars.class.getResourceAsStream(resourceName); - if (in == null) { - throw new IllegalArgumentException("Could not find resource: " + resourceName); - } - return new InputStreamReader(in, StandardCharsets.UTF_8); - } - // This is an ugly workaround for https://bugs.openjdk.java.net/browse/JDK-6947916, as // reported in https://github.com/google/auto/issues/365. // The issue is that sometimes the InputStream returned by JarURLCollection.getInputStream() // can be closed prematurely, which leads to an IOException saying "Stream closed". - // We catch all IOExceptions, and fall back on logic that opens the jar file directly and - // loads the resource from it. Since that doesn't use JarURLConnection, it shouldn't be - // susceptible to the same bug. We only use this as fallback logic rather than doing it always, - // because jars are memory-mapped by URLClassLoader, so loading a resource in the usual way - // through the getResourceAsStream should be a lot more efficient than reopening the jar. + // To avoid that issue, we open the jar file directly and load the resource from it. Since that + // doesn't use JarURLConnection, it shouldn't be susceptible to the same bug. private static Reader readerFromUrl(String resourceName) throws IOException { URL resourceUrl = TemplateVars.class.getResource(resourceName); if (resourceUrl == null) { - // This is unlikely, since getResourceAsStream has already succeeded for the same resource. throw new IllegalArgumentException("Could not find resource: " + resourceName); } - InputStream in; try { - if (resourceUrl.getProtocol().equalsIgnoreCase("file")) { - in = inputStreamFromFile(resourceUrl); - } else if (resourceUrl.getProtocol().equalsIgnoreCase("jar")) { - in = inputStreamFromJar(resourceUrl); + if (Ascii.equalsIgnoreCase(resourceUrl.getProtocol(), "file")) { + return readerFromFile(resourceUrl); + } else if (Ascii.equalsIgnoreCase(resourceUrl.getProtocol(), "jar")) { + return readerFromJar(resourceUrl); } else { - throw new AssertionError("Template fallback logic fails for: " + resourceUrl); + throw new AssertionError("Template search logic fails for: " + resourceUrl); } } catch (URISyntaxException e) { throw new IOException(e); } - return new InputStreamReader(in, StandardCharsets.UTF_8); } - private static InputStream inputStreamFromJar(URL resourceUrl) - throws URISyntaxException, IOException { + private static Reader readerFromJar(URL resourceUrl) throws URISyntaxException, IOException { // Jar URLs look like this: jar:file:/path/to/file.jar!/entry/within/jar // So take apart the URL to open the jar /path/to/file.jar and read the entry // entry/within/jar from it. + // We could use the methods from JarURLConnection here, but that would risk provoking the same + // problem that prompted this workaround in the first place. String resourceUrlString = resourceUrl.toString().substring("jar:".length()); int bang = resourceUrlString.lastIndexOf('!'); String entryName = resourceUrlString.substring(bang + 1); @@ -191,28 +165,19 @@ abstract class TemplateVars { entryName = entryName.substring(1); } URI jarUri = new URI(resourceUrlString.substring(0, bang)); - JarFile jar = new JarFile(new File(jarUri)); - JarEntry entry = jar.getJarEntry(entryName); - InputStream in = jar.getInputStream(entry); - // We have to be careful not to close the JarFile before the stream has been read, because - // that would also close the stream. So we defer closing the JarFile until the stream is closed. - return new FilterInputStream(in) { - @Override - public void close() throws IOException { - super.close(); - jar.close(); - } - }; + try (JarFile jar = new JarFile(new File(jarUri)); + InputStream in = jar.getInputStream(jar.getJarEntry(entryName))) { + String contents = new String(ByteStreams.toByteArray(in), UTF_8); + return new StringReader(contents); + } } - // We don't really expect this case to arise, since the bug we're working around concerns jars - // not individual files. However, when running the test for this workaround from Maven, we do - // have files. That does mean the test is basically useless there, but Google's internal build - // system does run it using a jar, so we do have coverage. - private static InputStream inputStreamFromFile(URL resourceUrl) + // In most execution environments, we'll be dealing with a jar, but we handle individual files + // just for cases like running our tests with Maven. + private static Reader readerFromFile(URL resourceUrl) throws IOException, URISyntaxException { File resourceFile = new File(resourceUrl.toURI()); - return new FileInputStream(resourceFile); + return new InputStreamReader(new FileInputStream(resourceFile), UTF_8); } private static Object fieldValue(Field field, Object container) { 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 01ec81fe..d30198cf 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 @@ -1163,6 +1163,7 @@ public class AutoValueCompilationTest { "final class AutoValue_Baz<T extends Number> extends Baz<T> {", " private final int anInt;", " private final byte[] aByteArray;", + " @Nullable", " private final int[] aNullableIntArray;", " private final List<T> aList;", " private final ImmutableMap<T, String> anImmutableMap;", @@ -2038,6 +2039,37 @@ public class AutoValueCompilationTest { } @Test + public void autoValueBuilderSetterReturnsNullable() { + JavaFileObject javaFileObject = + JavaFileObjects.forSourceLines( + "foo.bar.Baz", + "package foo.bar;", + "", + "import com.google.auto.value.AutoValue;", + "import javax.annotation.Nullable;", + "", + "@AutoValue", + "public abstract class Baz {", + " abstract String blam();", + "", + " @AutoValue.Builder", + " public interface Builder {", + " @Nullable Builder blam(String x);", + " Baz build();", + " }", + "}"); + Compilation compilation = + javac() + .withProcessors(new AutoValueProcessor(), new AutoValueBuilderProcessor()) + .compile(javaFileObject); + assertThat(compilation) + .hadWarningContaining( + "Setter methods always return the Builder so @Nullable is not appropriate") + .inFile(javaFileObject) + .onLineContaining("Builder blam(String x)"); + } + + @Test public void autoValueBuilderWrongTypeSetterWithCopyOf() { JavaFileObject javaFileObject = JavaFileObjects.forSourceLines( diff --git a/value/src/test/java/com/google/auto/value/processor/TemplateVarsTest.java b/value/src/test/java/com/google/auto/value/processor/TemplateVarsTest.java index bca7bcab..621a4122 100644 --- a/value/src/test/java/com/google/auto/value/processor/TemplateVarsTest.java +++ b/value/src/test/java/com/google/auto/value/processor/TemplateVarsTest.java @@ -15,29 +15,15 @@ */ package com.google.auto.value.processor; -import static com.google.common.base.StandardSystemProperty.JAVA_CLASS_PATH; -import static com.google.common.base.StandardSystemProperty.PATH_SEPARATOR; import static com.google.common.truth.Truth.assertThat; -import static java.util.logging.Level.WARNING; import static org.junit.Assert.fail; -import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; -import com.google.common.reflect.Reflection; import com.google.escapevelocity.Template; -import java.io.File; import java.io.IOException; -import java.io.InputStream; import java.io.Reader; import java.io.StringReader; -import java.net.MalformedURLException; -import java.net.URL; -import java.net.URLClassLoader; import java.util.List; -import java.util.Set; -import java.util.TreeSet; -import java.util.concurrent.Callable; -import java.util.logging.Logger; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -172,136 +158,4 @@ public class TemplateVarsTest { } catch (IllegalArgumentException expected) { } } - - @Test - public void testBrokenInputStream_IOException() throws Exception { - doTestBrokenInputStream(new IOException("BrokenInputStream")); - } - - @Test - public void testBrokenInputStream_NullPointerException() throws Exception { - doTestBrokenInputStream(new NullPointerException("BrokenInputStream")); - } - - @Test - public void testBrokenInputStream_IllegalStateException() throws Exception { - doTestBrokenInputStream(new IllegalStateException("BrokenInputStream")); - } - - // This is a complicated test that tries to simulates the failures that are worked around in - // Template.parsedTemplateForResource. Those failures means that the InputStream returned by - // ClassLoader.getResourceAsStream sometimes throws IOException or NullPointerException or - // IllegalStateException while it is being read. To simulate that, we make a second ClassLoader - // with the same configuration as the one that runs this test, and we override getResourceAsStream - // so that it wraps the returned InputStream in a BrokenInputStream, which throws an exception - // after a certain number of characters. We check that that exception was indeed seen, and that - // we did indeed try to read the resource we're interested in, and that we succeeded in loading a - // Template nevertheless. - private void doTestBrokenInputStream(Exception exception) throws Exception { - URLClassLoader shadowLoader = new ShadowLoader(getClass().getClassLoader(), exception); - Runnable brokenInputStreamTest = - (Runnable) - shadowLoader - .loadClass(BrokenInputStreamTest.class.getName()) - .getConstructor() - .newInstance(); - brokenInputStreamTest.run(); - } - - private static class ShadowLoader extends URLClassLoader implements Callable<Set<String>> { - - private static final Logger logger = Logger.getLogger(ShadowLoader.class.getName()); - - private final Exception exception; - private final Set<String> result = new TreeSet<String>(); - - ShadowLoader(ClassLoader original, Exception exception) { - super(getClassPathUrls(original), original.getParent()); - this.exception = exception; - } - - private static URL[] getClassPathUrls(ClassLoader original) { - return original instanceof URLClassLoader - ? ((URLClassLoader) original).getURLs() - : parseJavaClassPath(); - } - - /** - * Returns the URLs in the class path specified by the {@code java.class.path} {@linkplain - * System#getProperty system property}. - */ - // TODO(b/65488446): Use a new public API. - private static URL[] parseJavaClassPath() { - ImmutableList.Builder<URL> urls = ImmutableList.builder(); - for (String entry : Splitter.on(PATH_SEPARATOR.value()).split(JAVA_CLASS_PATH.value())) { - try { - try { - urls.add(new File(entry).toURI().toURL()); - } catch (SecurityException e) { // File.toURI checks to see if the file is a directory - urls.add(new URL("file", null, new File(entry).getAbsolutePath())); - } - } catch (MalformedURLException e) { - logger.log(WARNING, "malformed classpath entry: " + entry, e); - } - } - return urls.build().toArray(new URL[0]); - } - - @Override - public Set<String> call() throws Exception { - return result; - } - - @Override - public InputStream getResourceAsStream(String resource) { - // Make sure this is actually the resource we are expecting. If we're using JaCoCo or the - // like, we might end up reading some other resource, and we don't want to break that. - if (resource.startsWith("com/google/auto")) { - result.add(resource); - return new BrokenInputStream(super.getResourceAsStream(resource)); - } else { - return super.getResourceAsStream(resource); - } - } - - private class BrokenInputStream extends InputStream { - private final InputStream original; - private int count = 0; - - BrokenInputStream(InputStream original) { - this.original = original; - } - - @Override - public int read() throws IOException { - if (++count > 10) { - result.add("threw"); - if (exception instanceof IOException) { - throw (IOException) exception; - } - throw (RuntimeException) exception; - } - return original.read(); - } - } - } - - public static class BrokenInputStreamTest implements Runnable { - @Override - public void run() { - Template template = TemplateVars.parsedTemplateForResource("autovalue.vm"); - assertThat(template).isNotNull(); - String resourceName = - Reflection.getPackageName(getClass()).replace('.', '/') + "/autovalue.vm"; - @SuppressWarnings("unchecked") - Callable<Set<String>> myLoader = (Callable<Set<String>>) getClass().getClassLoader(); - try { - Set<String> result = myLoader.call(); - assertThat(result).contains(resourceName); - assertThat(result).contains("threw"); - } catch (Exception e) { - throw new AssertionError(e); - } - } - } } |