Update versions of auto-service and compile-testing, and fix a bug wi…

…th 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

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>

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>

value/src/main/java/com/google/auto/value/extension/serializable/serializer/SerializerFactoryLoader.java

@@ -21,6 +21,8 @@
 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 static SerializerFactory getFactory(ProcessingEnvironment processingEnv)
 
   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()

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,56 +46,76 @@ 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 {
       resourceUrls = Collections.list(loader.getResources(resourceName));
     } 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) {

value/src/test/java/com/google/auto/value/processor/SimpleServiceLoaderTest.java

@@ -28,6 +28,8 @@
 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 void loadOnce() throws Exception {
     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 =