Fix a problem with references to Factory classes in other packages.

Detect references to not-yet-generated Factory classes, and associate them with the correct package name if the Factory is one of the ones currently being generated.

Fixes https://github.com/google/auto/issues/297.

RELNOTES=Fix a problem with references to Factory classes in other packages.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=316148042

6 files changed, 169 insertions, 110 deletions

diff --git a/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryDeclaration.java b/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryDeclaration.java
index ad4ccb8b..e2da6eae 100644
--- a/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryDeclaration.java
+++ b/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryDeclaration.java
@@ -62,21 +62,17 @@ abstract class AutoFactoryDeclaration {
   abstract AnnotationMirror mirror();
   abstract ImmutableMap<String, AnnotationValue> valuesMap();
 
-  String getFactoryName() {
-    CharSequence packageName = getPackage(targetType()).getQualifiedName();
-    StringBuilder builder = new StringBuilder(packageName);
-    if (packageName.length() > 0) {
-      builder.append('.');
-    }
+  PackageAndClass getFactoryName() {
+    String packageName = getPackage(targetType()).getQualifiedName().toString();
     if (className().isPresent()) {
-      builder.append(className().get());
-    } else {
-      for (String enclosingSimpleName : targetEnclosingSimpleNames()) {
-        builder.append(enclosingSimpleName).append('_');
-      }
-      builder.append(targetType().getSimpleName()).append("Factory");
+      return PackageAndClass.of(packageName, className().get());
+    }
+    StringBuilder builder = new StringBuilder();
+    for (String enclosingSimpleName : targetEnclosingSimpleNames()) {
+      builder.append(enclosingSimpleName).append('_');
     }
-    return builder.toString();
+    builder.append(targetType().getSimpleName()).append("Factory");
+    return PackageAndClass.of(packageName, builder.toString());
   }
 
   private ImmutableList<String> targetEnclosingSimpleNames() {
diff --git a/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryProcessor.java b/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryProcessor.java
index cf3d5ebd..5cc1d94d 100644
--- a/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryProcessor.java
+++ b/factory/src/main/java/com/google/auto/factory/processor/AutoFactoryProcessor.java
@@ -27,13 +27,10 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
-import com.google.googlejavaformat.java.filer.FormattingFiler;
 import java.io.IOException;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.Comparator;
 import java.util.List;
-import java.util.Map.Entry;
 import java.util.Set;
 import javax.annotation.processing.AbstractProcessor;
 import javax.annotation.processing.Messager;
@@ -68,7 +65,6 @@ public final class AutoFactoryProcessor extends AbstractProcessor {
   private Messager messager;
   private Elements elements;
   private Types types;
-  private FactoryWriter factoryWriter;
 
   @Override
   public synchronized void init(ProcessingEnvironment processingEnv) {
@@ -76,11 +72,6 @@ public final class AutoFactoryProcessor extends AbstractProcessor {
     elements = processingEnv.getElementUtils();
     types = processingEnv.getTypeUtils();
     messager = processingEnv.getMessager();
-    factoryWriter =
-        new FactoryWriter(
-            new FormattingFiler(processingEnv.getFiler()),
-            elements,
-            processingEnv.getSourceVersion());
     providedChecker = new ProvidedChecker(messager);
     declarationFactory = new AutoFactoryDeclaration.Factory(elements, messager);
     factoryDescriptorGenerator =
@@ -103,14 +94,15 @@ public final class AutoFactoryProcessor extends AbstractProcessor {
       providedChecker.checkProvidedParameter(element);
     }
 
-    ImmutableListMultimap.Builder<String, FactoryMethodDescriptor> indexedMethods =
+    ImmutableListMultimap.Builder<PackageAndClass, FactoryMethodDescriptor> indexedMethodsBuilder =
         ImmutableListMultimap.builder();
-    ImmutableSetMultimap.Builder<String, ImplementationMethodDescriptor>
+    ImmutableSetMultimap.Builder<PackageAndClass, ImplementationMethodDescriptor>
         implementationMethodDescriptorsBuilder = ImmutableSetMultimap.builder();
+    // Iterate over the classes and methods that are annotated with @AutoFactory.
     for (Element element : roundEnv.getElementsAnnotatedWith(AutoFactory.class)) {
       Optional<AutoFactoryDeclaration> declaration = declarationFactory.createIfValid(element);
       if (declaration.isPresent()) {
-        String factoryName = declaration.get().getFactoryName();
+        PackageAndClass factoryName = declaration.get().getFactoryName();
         TypeElement extendingType = declaration.get().extendingType();
         implementationMethodDescriptorsBuilder.putAll(
             factoryName, implementationMethods(extendingType, element));
@@ -123,62 +115,61 @@ public final class AutoFactoryProcessor extends AbstractProcessor {
       ImmutableSet<FactoryMethodDescriptor> descriptors =
           factoryDescriptorGenerator.generateDescriptor(element);
       for (FactoryMethodDescriptor descriptor : descriptors) {
-        indexedMethods.put(descriptor.factoryName(), descriptor);
+        indexedMethodsBuilder.put(descriptor.factoryName(), descriptor);
       }
     }
 
-    ImmutableSetMultimap<String, ImplementationMethodDescriptor>
+    ImmutableSetMultimap<PackageAndClass, ImplementationMethodDescriptor>
         implementationMethodDescriptors = implementationMethodDescriptorsBuilder.build();
+    ImmutableListMultimap<PackageAndClass, FactoryMethodDescriptor> indexedMethods =
+        indexedMethodsBuilder.build();
+    ImmutableSetMultimap<String, PackageAndClass> factoriesBeingCreated =
+        simpleNamesToNames(indexedMethods.keySet());
+    FactoryWriter factoryWriter = new FactoryWriter(processingEnv, factoriesBeingCreated);
 
-    for (Entry<String, Collection<FactoryMethodDescriptor>> entry
-        : indexedMethods.build().asMap().entrySet()) {
-      ImmutableSet.Builder<TypeMirror> extending = ImmutableSet.builder();
-      ImmutableSortedSet.Builder<TypeMirror> implementing =
-          ImmutableSortedSet.orderedBy(
-              new Comparator<TypeMirror>() {
-                @Override
-                public int compare(TypeMirror first, TypeMirror second) {
-                  String firstName = MoreTypes.asTypeElement(first).getQualifiedName().toString();
-                  String secondName = MoreTypes.asTypeElement(second).getQualifiedName().toString();
-                  return firstName.compareTo(secondName);
-                }
-              });
-      boolean publicType = false;
-      Boolean allowSubclasses = null;
-      boolean skipCreation = false;
-      for (FactoryMethodDescriptor methodDescriptor : entry.getValue()) {
-        extending.add(methodDescriptor.declaration().extendingType().asType());
-        for (TypeElement implementingType : methodDescriptor.declaration().implementingTypes()) {
-          implementing.add(implementingType.asType());
-        }
-        publicType |= methodDescriptor.publicMethod();
-        if (allowSubclasses == null) {
-          allowSubclasses = methodDescriptor.declaration().allowSubclasses();
-        } else if (!allowSubclasses.equals(methodDescriptor.declaration().allowSubclasses())) {
-          skipCreation = true;
-          messager.printMessage(Kind.ERROR,
-              "Cannot mix allowSubclasses=true and allowSubclasses=false in one factory.",
-              methodDescriptor.declaration().target(),
-              methodDescriptor.declaration().mirror(),
-              methodDescriptor.declaration().valuesMap().get("allowSubclasses"));
-        }
-      }
-      if (!skipCreation) {
-        try {
-          factoryWriter.writeFactory(
-              FactoryDescriptor.create(
-                  entry.getKey(),
-                  Iterables.getOnlyElement(extending.build()),
-                  implementing.build(),
-                  publicType,
-                  ImmutableSet.copyOf(entry.getValue()),
-                  implementationMethodDescriptors.get(entry.getKey()),
-                  allowSubclasses));
-        } catch (IOException e) {
-          messager.printMessage(Kind.ERROR, "failed: " + e);
-        }
-      }
-    }
+    indexedMethods.asMap().forEach(
+        (factoryName, methodDescriptors) -> {
+          // The sets of classes that are mentioned in the `extending` and `implementing` elements,
+          // respectively, of the @AutoFactory annotations for this factory.
+          ImmutableSet.Builder<TypeMirror> extending = newTypeSetBuilder();
+          ImmutableSortedSet.Builder<TypeMirror> implementing = newTypeSetBuilder();
+          boolean publicType = false;
+          Boolean allowSubclasses = null;
+          boolean skipCreation = false;
+          for (FactoryMethodDescriptor methodDescriptor : methodDescriptors) {
+            extending.add(methodDescriptor.declaration().extendingType().asType());
+            for (TypeElement implementingType :
+                methodDescriptor.declaration().implementingTypes()) {
+              implementing.add(implementingType.asType());
+            }
+            publicType |= methodDescriptor.publicMethod();
+            if (allowSubclasses == null) {
+              allowSubclasses = methodDescriptor.declaration().allowSubclasses();
+            } else if (!allowSubclasses.equals(methodDescriptor.declaration().allowSubclasses())) {
+              skipCreation = true;
+              messager.printMessage(Kind.ERROR,
+                  "Cannot mix allowSubclasses=true and allowSubclasses=false in one factory.",
+                  methodDescriptor.declaration().target(),
+                  methodDescriptor.declaration().mirror(),
+                  methodDescriptor.declaration().valuesMap().get("allowSubclasses"));
+            }
+          }
+          if (!skipCreation) {
+            try {
+              factoryWriter.writeFactory(
+                  FactoryDescriptor.create(
+                      factoryName,
+                      Iterables.getOnlyElement(extending.build()),
+                      implementing.build(),
+                      publicType,
+                      ImmutableSet.copyOf(methodDescriptors),
+                      implementationMethodDescriptors.get(factoryName),
+                      allowSubclasses));
+            } catch (IOException e) {
+              messager.printMessage(Kind.ERROR, "failed: " + e);
+            }
+          }
+        });
   }
 
   private ImmutableSet<ImplementationMethodDescriptor> implementationMethods(
@@ -216,8 +207,24 @@ public final class AutoFactoryProcessor extends AbstractProcessor {
     return Iterables.getOnlyElement(types).asType();
   }
 
+  private static ImmutableSetMultimap<String, PackageAndClass> simpleNamesToNames(
+      ImmutableSet<PackageAndClass> names) {
+    // .collect(toImmutableSetMultimap(...)) would make this much simpler but ran into problems in
+    // Google's internal build system because of multiple Guava versions.
+    ImmutableSetMultimap.Builder<String, PackageAndClass> builder = ImmutableSetMultimap.builder();
+    for (PackageAndClass name : names) {
+      builder.put(name.className(), name);
+    }
+    return builder.build();
+  }
+
+  private static ImmutableSortedSet.Builder<TypeMirror> newTypeSetBuilder() {
+    return ImmutableSortedSet.orderedBy(
+        Comparator.comparing(t -> MoreTypes.asTypeElement(t).getQualifiedName().toString()));
+  }
+
   @Override
-  public Set<String> getSupportedAnnotationTypes() {
+  public ImmutableSet<String> getSupportedAnnotationTypes() {
     return ImmutableSet.of(AutoFactory.class.getName(), Provided.class.getName());
   }
 
diff --git a/factory/src/main/java/com/google/auto/factory/processor/FactoryDescriptor.java b/factory/src/main/java/com/google/auto/factory/processor/FactoryDescriptor.java
index 68ae678d..5ed2307a 100644
--- a/factory/src/main/java/com/google/auto/factory/processor/FactoryDescriptor.java
+++ b/factory/src/main/java/com/google/auto/factory/processor/FactoryDescriptor.java
@@ -46,7 +46,7 @@ abstract class FactoryDescriptor {
         }
       };
 
-  abstract String name();
+  abstract PackageAndClass name();
   abstract TypeMirror extendingType();
   abstract ImmutableSet<TypeMirror> implementingTypes();
   abstract boolean publicType();
@@ -76,7 +76,7 @@ abstract class FactoryDescriptor {
   }
 
   static FactoryDescriptor create(
-      String name,
+      PackageAndClass name,
       TypeMirror extendingType,
       ImmutableSet<TypeMirror> implementingTypes,
       boolean publicType,
diff --git a/factory/src/main/java/com/google/auto/factory/processor/FactoryMethodDescriptor.java b/factory/src/main/java/com/google/auto/factory/processor/FactoryMethodDescriptor.java
index c3a0159d..43e5097d 100644
--- a/factory/src/main/java/com/google/auto/factory/processor/FactoryMethodDescriptor.java
+++ b/factory/src/main/java/com/google/auto/factory/processor/FactoryMethodDescriptor.java
@@ -41,7 +41,7 @@ abstract class FactoryMethodDescriptor {
   abstract Builder toBuilder();
   abstract boolean isVarArgs();
 
-  final String factoryName() {
+  final PackageAndClass factoryName() {
     return declaration().getFactoryName();
   }
 
@@ -54,7 +54,7 @@ abstract class FactoryMethodDescriptor {
   }
 
   @AutoValue.Builder
-  static abstract class Builder {
+  abstract static class Builder {
     abstract Builder declaration(AutoFactoryDeclaration declaration);
     abstract Builder name(String name);
     abstract Builder returnType(TypeMirror returnType);
diff --git a/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java b/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java
index 3bb68e1c..53b99cb5 100644
--- a/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java
+++ b/factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java
@@ -29,6 +29,7 @@ import com.google.common.base.Joiner;
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
 import com.squareup.javapoet.AnnotationSpec;
@@ -44,10 +45,12 @@ import com.squareup.javapoet.TypeVariableName;
 import java.io.IOException;
 import java.util.Iterator;
 import javax.annotation.processing.Filer;
+import javax.annotation.processing.ProcessingEnvironment;
 import javax.inject.Inject;
 import javax.inject.Provider;
 import javax.lang.model.SourceVersion;
 import javax.lang.model.element.AnnotationMirror;
+import javax.lang.model.type.TypeKind;
 import javax.lang.model.type.TypeMirror;
 import javax.lang.model.type.TypeVariable;
 import javax.lang.model.util.Elements;
@@ -57,18 +60,22 @@ final class FactoryWriter {
   private final Filer filer;
   private final Elements elements;
   private final SourceVersion sourceVersion;
+  private final ImmutableSetMultimap<String, PackageAndClass> factoriesBeingCreated;
 
-  FactoryWriter(Filer filer, Elements elements, SourceVersion sourceVersion) {
-    this.filer = filer;
-    this.elements = elements;
-    this.sourceVersion = sourceVersion;
+  FactoryWriter(
+      ProcessingEnvironment processingEnv,
+      ImmutableSetMultimap<String, PackageAndClass> factoriesBeingCreated) {
+    this.filer = processingEnv.getFiler();
+    this.elements = processingEnv.getElementUtils();
+    this.sourceVersion = processingEnv.getSourceVersion();
+    this.factoriesBeingCreated = factoriesBeingCreated;
   }
 
   private static final Joiner ARGUMENT_JOINER = Joiner.on(", ");
 
-  void writeFactory(final FactoryDescriptor descriptor)
+  void writeFactory(FactoryDescriptor descriptor)
       throws IOException {
-    String factoryName = getSimpleName(descriptor.name()).toString();
+    String factoryName = descriptor.name().className();
     TypeSpec.Builder factory =
         classBuilder(factoryName)
             .addOriginatingElement(descriptor.declaration().targetType());
@@ -98,7 +105,7 @@ final class FactoryWriter {
     addImplementationMethods(factory, descriptor);
     addCheckNotNullMethod(factory, descriptor);
 
-    JavaFile.builder(getPackage(descriptor.name()), factory.build())
+    JavaFile.builder(descriptor.name().packageName(), factory.build())
         .skipJavaLangImports(true)
         .build()
         .writeTo(filer);
@@ -109,7 +116,7 @@ final class FactoryWriter {
     factory.addTypeVariables(typeVariableNames);
   }
 
-  private static void addConstructorAndProviderFields(
+  private void addConstructorAndProviderFields(
       TypeSpec.Builder factory, FactoryDescriptor descriptor) {
     MethodSpec.Builder constructor = constructorBuilder().addAnnotation(Inject.class);
     if (descriptor.publicType()) {
@@ -118,7 +125,7 @@ final class FactoryWriter {
     Iterator<ProviderField> providerFields = descriptor.providers().values().iterator();
     for (int argumentIndex = 1; providerFields.hasNext(); argumentIndex++) {
       ProviderField provider = providerFields.next();
-      TypeName typeName = TypeName.get(provider.key().type().get()).box();
+      TypeName typeName = resolveTypeName(provider.key().type().get()).box();
       TypeName providerType = ParameterizedTypeName.get(ClassName.get(Provider.class), typeName);
       factory.addField(providerType, provider.name(), PRIVATE, FINAL);
       if (provider.key().qualifier().isPresent()) {
@@ -132,7 +139,7 @@ final class FactoryWriter {
     factory.addMethod(constructor.build());
   }
 
-  private static void addFactoryMethods(
+  private void addFactoryMethods(
       TypeSpec.Builder factory,
       FactoryDescriptor descriptor,
       ImmutableSet<TypeVariableName> factoryTypeVariables) {
@@ -183,7 +190,7 @@ final class FactoryWriter {
     }
   }
 
-  private static void addImplementationMethods(
+  private void addImplementationMethods(
       TypeSpec.Builder factory, FactoryDescriptor descriptor) {
     for (ImplementationMethodDescriptor methodDescriptor :
         descriptor.implementationMethodDescriptors()) {
@@ -215,11 +222,11 @@ final class FactoryWriter {
    * {@link ParameterSpec}s to match {@code parameters}. Note that the type of the {@link
    * ParameterSpec}s match {@link Parameter#type()} and not {@link Key#type()}.
    */
-  private static Iterable<ParameterSpec> parameters(Iterable<Parameter> parameters) {
+  private ImmutableList<ParameterSpec> parameters(Iterable<Parameter> parameters) {
     ImmutableList.Builder<ParameterSpec> builder = ImmutableList.builder();
     for (Parameter parameter : parameters) {
       ParameterSpec.Builder parameterBuilder =
-          ParameterSpec.builder(TypeName.get(parameter.type().get()), parameter.name());
+          ParameterSpec.builder(resolveTypeName(parameter.type().get()), parameter.name());
       for (AnnotationMirror annotation :
           Iterables.concat(parameter.nullable().asSet(), parameter.key().qualifier().asSet())) {
         parameterBuilder.addAnnotation(AnnotationSpec.get(annotation));
@@ -266,23 +273,34 @@ final class FactoryWriter {
     return false;
   }
 
-  private static CharSequence getSimpleName(CharSequence fullyQualifiedName) {
-    int lastDot = lastIndexOf(fullyQualifiedName, '.');
-    return fullyQualifiedName.subSequence(lastDot + 1, fullyQualifiedName.length());
-  }
-
-  private static String getPackage(CharSequence fullyQualifiedName) {
-    int lastDot = lastIndexOf(fullyQualifiedName, '.');
-    return lastDot == -1 ? "" : fullyQualifiedName.subSequence(0, lastDot).toString();
-  }
-
-  private static int lastIndexOf(CharSequence charSequence, char c) {
-    for (int i = charSequence.length() - 1; i >= 0; i--) {
-      if (charSequence.charAt(i) == c) {
-        return i;
-      }
+  /**
+   * Returns an appropriate {@code TypeName} for the given type. If the type is an
+   * {@code ErrorType}, and if it is a simple-name reference to one of the {@code *Factory}
+   * classes that we are going to generate, then we return its fully-qualified name. In every other
+   * case we just return {@code TypeName.get(type)}. Specifically, if it is an {@code ErrorType}
+   * referencing some other type, or referencing one of the classes we are going to generate but
+   * using its fully-qualified name, then we leave it as-is. JavaPoet treats {@code TypeName.get(t)}
+   * the same for {@code ErrorType} as for {@code DeclaredType}, which means that if this is a name
+   * that will eventually be generated then the code we write that references the type will in fact
+   * compile.
+   *
+   * <p>A simpler alternative would be to defer processing to a later round if we find an
+   * {@code @AutoFactory} class that references undefined types, under the assumption that something
+   * else will generate those types in the meanwhile. However, this would fail if for example
+   * {@code @AutoFactory class Foo} has a constructor parameter of type {@code BarFactory} and
+   * {@code @AutoFactory class Bar} has a constructor parameter of type {@code FooFactory}. We did
+   * in fact find instances of this in Google's source base.
+   */
+  private TypeName resolveTypeName(TypeMirror type) {
+    if (type.getKind() != TypeKind.ERROR) {
+      return TypeName.get(type);
+    }
+    ImmutableSet<PackageAndClass> factoryNames = factoriesBeingCreated.get(type.toString());
+    if (factoryNames.size() == 1) {
+      PackageAndClass packageAndClass = Iterables.getOnlyElement(factoryNames);
+      return ClassName.get(packageAndClass.packageName(), packageAndClass.className());
     }
-    return -1;
+    return TypeName.get(type);
   }
 
   private static ImmutableSet<TypeVariableName> getFactoryTypeVariables(
diff --git a/factory/src/main/java/com/google/auto/factory/processor/PackageAndClass.java b/factory/src/main/java/com/google/auto/factory/processor/PackageAndClass.java
new file mode 100644
index 00000000..857fba11
--- /dev/null
+++ b/factory/src/main/java/com/google/auto/factory/processor/PackageAndClass.java
@@ -0,0 +1,38 @@
+/*
+ * Copyright 2020 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.google.auto.factory.processor;
+
+import com.google.auto.value.AutoValue;
+
+/** A Java class name, separated into its package part and its class part. */
+@AutoValue
+abstract class PackageAndClass {
+  /**
+   * The package part of this class name. For {@code java.util.Map.Entry}, it would be {@code
+   * java.util}.
+   */
+  abstract String packageName();
+
+  /**
+   * The class part of this class name. For {@code java.util.Map.Entry}, it would be {@code
+   * Map.Entry}.
+   */
+  abstract String className();
+
+  static PackageAndClass of(String packageName, String className) {
+    return new AutoValue_PackageAndClass(packageName, className);
+  }
+}