Update versions of auto-service and compile-testing, and fix a bug with SimpleServiceLoader.

Updating the compile-testing version revealed a bug in AutoValue. If the same jar is present more than once in the class path (perhaps in different versions), then we get `META-INF/services` contributions from both. That could lead to us instantiating the same AutoValueExtension more than once. The bug has been fixed by uniquifying the list of classes before instantiating them.

We specifically saw this bug because compile-testing now has a dependency on AutoValue, which meant that that AutoValue was in the classpath when compiling tests, in addition to the version of AutoValue being tested.

RELNOTES=Fixed a bug which could lead to the same AutoValue extension being run more than once.
PiperOrigin-RevId: 339463884

5 files changed, 104 insertions, 34 deletions

diff --git a/value/pom.xml b/value/pom.xml
index 69c77e58..f4d89da6 100644
--- a/value/pom.xml
+++ b/value/pom.xml
@@ -107,8 +107,7 @@
       <dependency>
         <groupId>com.google.testing.compile</groupId>
         <artifactId>compile-testing</artifactId>
-        <!-- TODO(cpovirk): Why does updating to 0.19 break tests? -->
-        <version>0.18</version>
+        <version>0.19</version>
       </dependency>
       <dependency>
         <groupId>com.google.truth</groupId>
diff --git a/value/processor/pom.xml b/value/processor/pom.xml
index 7a1cbafa..edcb3766 100644
--- a/value/processor/pom.xml
+++ b/value/processor/pom.xml
@@ -49,8 +49,7 @@
     <dependency>
       <groupId>com.google.auto.service</groupId>
       <artifactId>auto-service</artifactId>
-      <!-- TODO(cpovirk): Update to 1.0-rc7. But doing so breaks function tests, oddly under JDK9 but not JDK8. The error is "not applicable in this type context" on a @Nullable annotation, so perhaps it's a compiler bug? -->
-      <version>1.0-rc6</version>
+      <version>1.0-rc7</version>
       <scope>provided</scope>
     </dependency>
     <dependency>
@@ -151,6 +150,14 @@
             <include>com/google/auto/value/extension/serializable/processor/**/*.java</include>
             <include>com/google/auto/value/extension/serializable/serializer/**/*.java</include>
           </includes>
+          <compilerArgs>
+            <!-- This is something of a hack to allow tests to pass. Ideally we would build
+                 TestStringSerializerFactory as a separate artifact, to avoid a problem when it
+                 is built at the same time as @AutoValue classes that might end up finding it.
+                 But by allowing a missing class to be ignored, we avoid crashing if there is a
+                 META-INF/services entry for a class that the compiler has not yet generated. -->
+            <arg>-AallowedMissingSerializableExtensionClasses=.*TestStringSerializerFactory</arg>
+          </compilerArgs>
         </configuration>
       </plugin>
       <plugin>
diff --git a/value/src/main/java/com/google/auto/value/extension/serializable/serializer/SerializerFactoryLoader.java b/value/src/main/java/com/google/auto/value/extension/serializable/serializer/SerializerFactoryLoader.java
index 12984b05..e81b5867 100644
--- a/value/src/main/java/com/google/auto/value/extension/serializable/serializer/SerializerFactoryLoader.java
+++ b/value/src/main/java/com/google/auto/value/extension/serializable/serializer/SerializerFactoryLoader.java
@@ -21,6 +21,8 @@ import com.google.auto.value.extension.serializable.serializer.interfaces.Serial
 import com.google.auto.value.processor.SimpleServiceLoader;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
+import java.util.Optional;
+import java.util.regex.Pattern;
 import javax.annotation.processing.ProcessingEnvironment;
 import javax.tools.Diagnostic;
 
@@ -40,10 +42,18 @@ public final class SerializerFactoryLoader {
 
   private static ImmutableList<SerializerExtension> loadExtensions(
       ProcessingEnvironment processingEnv) {
+    // The below is a workaround for a test-building bug. We don't expect to support it indefinitely
+    // so don't depend on it.
+    String allowedMissingClasses =
+        processingEnv.getOptions().get("allowedMissingSerializableExtensionClasses");
+    Optional<Pattern> allowedMissingClassesPattern =
+        Optional.ofNullable(allowedMissingClasses).map(Pattern::compile);
     try {
       return ImmutableList.copyOf(
           SimpleServiceLoader.load(
-              SerializerExtension.class, SerializerFactoryLoader.class.getClassLoader()));
+              SerializerExtension.class,
+              SerializerFactoryLoader.class.getClassLoader(),
+              allowedMissingClassesPattern));
     } catch (Throwable t) {
       processingEnv
           .getMessager()
diff --git a/value/src/main/java/com/google/auto/value/processor/SimpleServiceLoader.java b/value/src/main/java/com/google/auto/value/processor/SimpleServiceLoader.java
index 55f9ae4e..23f6674c 100644
--- a/value/src/main/java/com/google/auto/value/processor/SimpleServiceLoader.java
+++ b/value/src/main/java/com/google/auto/value/processor/SimpleServiceLoader.java
@@ -15,20 +15,23 @@
  */
 package com.google.auto.value.processor;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.stream.Collectors.toList;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Streams;
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.net.URL;
 import java.net.URLConnection;
-import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.ServiceConfigurationError;
+import java.util.regex.Pattern;
 
 /**
  * A replacement for {@link java.util.ServiceLoader} that avoids certain long-standing bugs. This
@@ -43,6 +46,11 @@ public final class SimpleServiceLoader {
   private SimpleServiceLoader() {}
 
   public static <T> ImmutableList<T> load(Class<? extends T> service, ClassLoader loader) {
+    return load(service, loader, Optional.empty());
+  }
+
+  public static <T> ImmutableList<T> load(
+      Class<? extends T> service, ClassLoader loader, Optional<Pattern> allowedMissingClasses) {
     String resourceName = "META-INF/services/" + service.getName();
     List<URL> resourceUrls;
     try {
@@ -50,49 +58,64 @@ public final class SimpleServiceLoader {
     } catch (IOException e) {
       throw new ServiceConfigurationError("Could not look up " + resourceName, e);
     }
-    ImmutableList.Builder<T> providers = ImmutableList.builder();
+    ImmutableSet.Builder<Class<? extends T>> providerClasses = ImmutableSet.builder();
     for (URL resourceUrl : resourceUrls) {
       try {
-        providers.addAll(providersFromUrl(resourceUrl, service, loader));
+        providerClasses.addAll(
+            providerClassesFromUrl(resourceUrl, service, loader, allowedMissingClasses));
       } catch (IOException e) {
         throw new ServiceConfigurationError("Could not read " + resourceUrl, e);
       }
     }
+    ImmutableList.Builder<T> providers = ImmutableList.builder();
+    for (Class<? extends T> providerClass : providerClasses.build()) {
+      try {
+        T provider = providerClass.getConstructor().newInstance();
+        providers.add(provider);
+      } catch (ReflectiveOperationException e) {
+        throw new ServiceConfigurationError("Could not construct " + providerClass.getName(), e);
+      }
+    }
     return providers.build();
   }
 
-  private static <T> ImmutableList<T> providersFromUrl(
-      URL resourceUrl, Class<T> service, ClassLoader loader) throws IOException {
-    ImmutableList.Builder<T> providers = ImmutableList.builder();
+  private static <T> ImmutableSet<Class<? extends T>> providerClassesFromUrl(
+      URL resourceUrl,
+      Class<? extends T> service,
+      ClassLoader loader,
+      Optional<Pattern> allowedMissingClasses)
+      throws IOException {
+    ImmutableSet.Builder<Class<? extends T>> providerClasses = ImmutableSet.builder();
     URLConnection urlConnection = resourceUrl.openConnection();
     urlConnection.setUseCaches(false);
+    List<String> lines;
     try (InputStream in = urlConnection.getInputStream();
-        BufferedReader reader =
-            new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8))) {
-      for (String line : reader.lines().collect(toList())) {
-        Optional<String> maybeClassName = parseClassName(line);
-        if (maybeClassName.isPresent()) {
-          String className = maybeClassName.get();
-          Class<?> c;
-          try {
-            c = Class.forName(className, false, loader);
-          } catch (ClassNotFoundException e) {
-            throw new ServiceConfigurationError("Could not load " + className, e);
-          }
-          if (!service.isAssignableFrom(c)) {
-            throw new ServiceConfigurationError(
-                "Class " + className + " is not assignable to " + service.getName());
-          }
-          try {
-            Object provider = c.getConstructor().newInstance();
-            providers.add(service.cast(provider));
-          } catch (ReflectiveOperationException e) {
-            throw new ServiceConfigurationError("Could not construct " + className, e);
-          }
+        BufferedReader reader = new BufferedReader(new InputStreamReader(in, UTF_8))) {
+      lines = reader.lines().collect(toList());
+    }
+    List<String> classNames =
+        lines.stream()
+            .map(SimpleServiceLoader::parseClassName)
+            .flatMap(Streams::stream)
+            .collect(toList());
+    for (String className : classNames) {
+      Class<?> c;
+      try {
+        c = Class.forName(className, false, loader);
+      } catch (ClassNotFoundException e) {
+        if (allowedMissingClasses.isPresent()
+            && allowedMissingClasses.get().matcher(className).matches()) {
+          continue;
         }
+        throw new ServiceConfigurationError("Could not load " + className, e);
+      }
+      if (!service.isAssignableFrom(c)) {
+        throw new ServiceConfigurationError(
+            "Class " + className + " is not assignable to " + service.getName());
       }
-      return providers.build();
+      providerClasses.add(c.asSubclass(service));
     }
+    return providerClasses.build();
   }
 
   private static Optional<String> parseClassName(String line) {
diff --git a/value/src/test/java/com/google/auto/value/processor/SimpleServiceLoaderTest.java b/value/src/test/java/com/google/auto/value/processor/SimpleServiceLoaderTest.java
index fab18056..5e2d230d 100644
--- a/value/src/test/java/com/google/auto/value/processor/SimpleServiceLoaderTest.java
+++ b/value/src/test/java/com/google/auto/value/processor/SimpleServiceLoaderTest.java
@@ -28,6 +28,8 @@ import java.io.PrintWriter;
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Enumeration;
 import java.util.List;
 import java.util.ServiceConfigurationError;
@@ -54,6 +56,35 @@ public final class SimpleServiceLoaderTest {
     assertThat(classes).containsExactly(String.class, StringBuilder.class).inOrder();
   }
 
+  // Sometimes you can have the same jar appear more than once in the classpath, perhaps in
+  // different versions. In that case we don't want to instantiate the same class more than once.
+  // This test checks that we don't.
+  @Test
+  public void loadWithDuplicates() throws Exception {
+    ClassLoader loader1 =
+        loaderForJarWithEntries(
+            CharSequence.class.getName(), String.class.getName(), StringBuilder.class.getName());
+    ClassLoader loader2 =
+        loaderForJarWithEntries(
+            CharSequence.class.getName(), String.class.getName(), StringBuilder.class.getName());
+    ClassLoader combinedLoader =
+        new ClassLoader() {
+          @Override
+          public Enumeration<URL> getResources(String name) throws IOException {
+            List<URL> urls = new ArrayList<>(Collections.list(loader1.getResources(name)));
+            urls.addAll(Collections.list(loader2.getResources(name)));
+            return Collections.enumeration(urls);
+          }
+        };
+
+    ImmutableList<CharSequence> providers =
+        SimpleServiceLoader.load(CharSequence.class, combinedLoader);
+
+    assertThat(providers).contains("");
+    List<Class<?>> classes = providers.stream().map(Object::getClass).collect(toList());
+    assertThat(classes).containsExactly(String.class, StringBuilder.class).inOrder();
+  }
+
   @Test
   public void blankLinesAndComments() throws Exception {
     ClassLoader loader =