Merge pull request #313 from google/internal_duplication_improvement

Improve duplicate descriptor checking.

factory/src/main/java/com/google/auto/factory/processor/FactoryDescriptor.java

@@ -15,18 +15,17 @@
  */
 package com.google.auto.factory.processor;
 
-import static com.google.common.base.Preconditions.checkNotNull;
-
 import com.google.auto.common.MoreTypes;
 import com.google.auto.value.AutoValue;
 import com.google.common.base.CharMatcher;
+import com.google.common.collect.ImmutableBiMap;
 import com.google.common.collect.ImmutableMap;
 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 java.util.Collection;
-import java.util.LinkedHashSet;
 import java.util.Map.Entry;
 
 import javax.lang.model.type.TypeMirror;
@@ -84,49 +83,110 @@ static FactoryDescriptor create(
           break;
       }
     }
+
+    ImmutableBiMap<FactoryMethodDescriptor, ImplementationMethodDescriptor>
+        duplicateMethodDescriptors =
+            createDuplicateMethodDescriptorsBiMap(
+                methodDescriptors, implementationMethodDescriptors);
+
+    ImmutableSet<FactoryMethodDescriptor> deduplicatedMethodDescriptors =
+        getDeduplicatedMethodDescriptors(methodDescriptors, duplicateMethodDescriptors);
+
+    ImmutableSet<ImplementationMethodDescriptor> deduplicatedImplementationMethodDescriptors =
+        ImmutableSet.copyOf(
+            Sets.difference(implementationMethodDescriptors, duplicateMethodDescriptors.values()));
+
     return new AutoValue_FactoryDescriptor(
         name,
         extendingType,
         implementingTypes,
         publicType,
-        methodDescriptors,
-        dedupeMethods(methodDescriptors, implementationMethodDescriptors),
+        deduplicatedMethodDescriptors,
+        deduplicatedImplementationMethodDescriptors,
         allowSubclasses,
         providersBuilder.build());
   }
 
   /**
-   * Removes methods with matching signatures from the set of
-   * {@link ImplementationMethodDescriptor}s.
+   * Creates a bi-map of duplicate {@link ImplementationMethodDescriptor}s by their respective
+   * {@link FactoryMethodDescriptor}.
    */
-  private static ImmutableSet<ImplementationMethodDescriptor> dedupeMethods(
-      ImmutableSet<FactoryMethodDescriptor> methodDescriptors,
-      ImmutableSet<ImplementationMethodDescriptor> implementationMethodDescriptors) {
-
-    checkNotNull(implementationMethodDescriptors);
-    LinkedHashSet<ImplementationMethodDescriptor> dedupedMethods =
-        new LinkedHashSet<ImplementationMethodDescriptor>(implementationMethodDescriptors);
-
-    for (ImplementationMethodDescriptor implementationMethod : implementationMethodDescriptors) {
-      for (FactoryMethodDescriptor factoryMethod : methodDescriptors) {
-        if (implementationMethod.name().equals(factoryMethod.name())
-            && parameterTypesEqual(
-                implementationMethod.passedParameters(), factoryMethod.passedParameters())) {
-          dedupedMethods.remove(implementationMethod);
+  private static ImmutableBiMap<FactoryMethodDescriptor, ImplementationMethodDescriptor>
+      createDuplicateMethodDescriptorsBiMap(
+          ImmutableSet<FactoryMethodDescriptor> factoryMethodDescriptors,
+          ImmutableSet<ImplementationMethodDescriptor> implementationMethodDescriptors) {
+
+    ImmutableBiMap.Builder<FactoryMethodDescriptor, ImplementationMethodDescriptor> builder =
+        ImmutableBiMap.builder();
+
+    for (FactoryMethodDescriptor factoryMethodDescriptor : factoryMethodDescriptors) {
+      for (ImplementationMethodDescriptor implementationMethodDescriptor :
+          implementationMethodDescriptors) {
+
+        boolean areDuplicateMethodDescriptors =
+            areDuplicateMethodDescriptors(factoryMethodDescriptor, implementationMethodDescriptor);
+        if (areDuplicateMethodDescriptors) {
+          builder.put(factoryMethodDescriptor, implementationMethodDescriptor);
+          break;
         }
       }
     }
-    return ImmutableSet.copyOf(dedupedMethods);
+
+    return builder.build();
   }
 
   /**
-   * Returns whether the two {@link Iterable}s of {@link Parameter}s are equal solely by type.
+   * Returns a set of deduplicated {@link FactoryMethodDescriptor}s from the set of original
+   * descriptors and the bi-map of duplicate descriptors.
+   *
+   * <p>Modifies the duplicate {@link FactoryMethodDescriptor}s such that they are overriding and
+   * reflect properties from the {@link ImplementationMethodDescriptor} they are implementing.
    */
-  private static boolean parameterTypesEqual(
-      Iterable<Parameter> first, Iterable<Parameter> second) {
+  private static ImmutableSet<FactoryMethodDescriptor> getDeduplicatedMethodDescriptors(
+      ImmutableSet<FactoryMethodDescriptor> methodDescriptors,
+      ImmutableBiMap<FactoryMethodDescriptor, ImplementationMethodDescriptor>
+          duplicateMethodDescriptors) {
+
+    ImmutableSet.Builder<FactoryMethodDescriptor> deduplicatedMethodDescriptors =
+        ImmutableSet.builder();
+
+    for (FactoryMethodDescriptor methodDescriptor : methodDescriptors) {
+      ImplementationMethodDescriptor duplicateMethodDescriptor =
+          duplicateMethodDescriptors.get(methodDescriptor);
+
+      FactoryMethodDescriptor newMethodDescriptor =
+         (duplicateMethodDescriptor != null)
+              ? methodDescriptor
+                  .toBuilder()
+                  .overridingMethod(true)
+                  .publicMethod(duplicateMethodDescriptor.publicMethod())
+                  .returnType(duplicateMethodDescriptor.returnType())
+                  .build()
+              : methodDescriptor;
+      deduplicatedMethodDescriptors.add(newMethodDescriptor);
+    }
+
+    return deduplicatedMethodDescriptors.build();
+  }
+
+  /**
+   * Returns true if the given {@link FactoryMethodDescriptor} and
+   * {@link ImplementationMethodDescriptor} are duplicates.
+   *
+   * <p>Descriptors are duplicates if they have the same name and if they have the same passed types
+   * in the same order.
+   */
+  private static boolean areDuplicateMethodDescriptors(
+      FactoryMethodDescriptor factory,
+      ImplementationMethodDescriptor implementation) {
+
+    if (!factory.name().equals(implementation.name())) {
+      return false;
+    }
 
+    // Descriptors are identical if they have the same passed types in the same order.
     return MoreTypes.equivalence().pairwise().equivalent(
-        Iterables.transform(first, Parameter.parameterToType),
-        Iterables.transform(second, Parameter.parameterToType));
+        Iterables.transform(factory.passedParameters(), Parameter.parameterToType),
+        Iterables.transform(implementation.passedParameters(), Parameter.parameterToType));
   }
 }

factory/src/main/java/com/google/auto/factory/processor/FactoryMethodDescriptor.java

@@ -15,13 +15,14 @@
  */
 package com.google.auto.factory.processor;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
-import javax.lang.model.type.TypeMirror;
 
-import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Preconditions.checkState;
+import javax.lang.model.type.TypeMirror;
 
 /**
  * A value object representing a factory method to be generated.
@@ -34,9 +35,11 @@ abstract class FactoryMethodDescriptor {
   abstract String name();
   abstract TypeMirror returnType();
   abstract boolean publicMethod();
+  abstract boolean overridingMethod();
   abstract ImmutableSet<Parameter> passedParameters();
   abstract ImmutableSet<Parameter> providedParameters();
   abstract ImmutableSet<Parameter> creationParameters();
+  abstract Builder toBuilder();
 
   final String factoryName() {
     return declaration().getFactoryName();
@@ -45,7 +48,8 @@ final String factoryName() {
   static Builder builder(AutoFactoryDeclaration declaration) {
     return new AutoValue_FactoryMethodDescriptor.Builder()
         .declaration(checkNotNull(declaration))
-        .publicMethod(false);
+        .publicMethod(false)
+        .overridingMethod(false);
   }
 
   @AutoValue.Builder
@@ -54,6 +58,7 @@ static abstract class Builder {
     abstract Builder name(String name);
     abstract Builder returnType(TypeMirror returnType);
     abstract Builder publicMethod(boolean publicMethod);
+    abstract Builder overridingMethod(boolean overridingMethod);
     abstract Builder passedParameters(Iterable<Parameter> passedParameters);
     abstract Builder providedParameters(Iterable<Parameter> providedParameters);
     abstract Builder creationParameters(Iterable<Parameter> creationParameters);

factory/src/main/java/com/google/auto/factory/processor/FactoryWriter.java

@@ -106,6 +106,9 @@ void writeFactory(final FactoryDescriptor descriptor)
       MethodSpec.Builder method =
           MethodSpec.methodBuilder(methodDescriptor.name())
               .returns(TypeName.get(methodDescriptor.returnType()));
+      if (methodDescriptor.overridingMethod()) {
+        method.addAnnotation(Override.class);
+      }
       if (methodDescriptor.publicMethod()) {
         method.addModifiers(PUBLIC);
       }

factory/src/test/resources/expected/FactoryImplementingCreateMethod_ConcreteClassFactory.java

@@ -23,25 +23,28 @@
     value = "com.google.auto.factory.processor.AutoFactoryProcessor",
     comments = "https://github.com/google/auto/tree/master/factory"
 )
-public final class FactoryImplementingCreateMethod_ConcreteClassFactory
+final class FactoryImplementingCreateMethod_ConcreteClassFactory
     implements FactoryImplementingCreateMethod.FactoryInterfaceWithCreateMethod {
 
   @Inject
-  public FactoryImplementingCreateMethod_ConcreteClassFactory() {}
+  FactoryImplementingCreateMethod_ConcreteClassFactory() {}
 
+  @Override
   public FactoryImplementingCreateMethod.ConcreteClass create() {
     return new FactoryImplementingCreateMethod.ConcreteClass();
   }
 
+  @Override
   public FactoryImplementingCreateMethod.ConcreteClass create(int aDifferentArgumentName) {
     return new FactoryImplementingCreateMethod.ConcreteClass(aDifferentArgumentName);
   }
 
+  @Override
   public FactoryImplementingCreateMethod.ConcreteClass create(List<Integer> genericWithDifferentArgumentName) {
     return new FactoryImplementingCreateMethod.ConcreteClass(genericWithDifferentArgumentName);
   }
 
-  public FactoryImplementingCreateMethod.ConcreteClass create(int a, boolean b) {
+  FactoryImplementingCreateMethod.ConcreteClass create(int a, boolean b) {
     return new FactoryImplementingCreateMethod.ConcreteClass(a, b);
   }
 }

factory/src/test/resources/good/FactoryImplementingCreateMethod.java

@@ -32,7 +32,7 @@ interface FactoryInterfaceWithCreateMethod {
   }
 
   @AutoFactory(implementing = FactoryInterfaceWithCreateMethod.class)
-  public static class ConcreteClass implements Interface {
+  static class ConcreteClass implements Interface {
     // Will generate a method with a signature that matches one from the interface.
     ConcreteClass() {}