Skip to content

Commit

Permalink
Fix a bug that causing invalid code generated when using multiple Aut…
Browse files Browse the repository at this point in the history 
…oFactories implementing same interface.

-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=92862023
  • Loading branch information
@cgruber
Oleksandr Gumen authored and cgruber committed May 6, 2015
1 parent 2f7ceda commit b1aca4d
Show file tree
Hide file tree
Showing 5 changed files with 184 additions and 57 deletions.
45 changes: 27 additions & 18 deletions factory/src/main/java/com/google/auto/factory/processor/AutoFactoryProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
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.common.collect.Multimaps;
Expand Down Expand Up @@ -97,25 +98,30 @@ private void doProcess(RoundEnvironment roundEnv) {

ImmutableListMultimap.Builder<String, FactoryMethodDescriptor> indexedMethods =
ImmutableListMultimap.builder();
ImmutableSet.Builder<ImplementationMethodDescriptor> implementationMethodDescriptors =
ImmutableSet.builder();
ImmutableSetMultimap.Builder<String, ImplementationMethodDescriptor>
implementationMethodDescriptorsBuilder = ImmutableSetMultimap.builder();
for (Element element : roundEnv.getElementsAnnotatedWith(AutoFactory.class)) {
Optional<AutoFactoryDeclaration> declaration = declarationFactory.createIfValid(element);
if (declaration.isPresent()) {
String factoryName = declaration.get().getFactoryName(
elements.getPackageOf(element).getQualifiedName(),
getAnnotatedType(element).getSimpleName());

TypeElement extendingType = declaration.get().extendingType();
List<ExecutableElement> supertypeMethods =
ElementFilter.methodsIn(elements.getAllMembers(extendingType));
for (ExecutableElement supertypeMethod : supertypeMethods) {
if (supertypeMethod.getModifiers().contains(Modifier.ABSTRACT)) {
ExecutableType methodType = Elements2.getExecutableElementAsMemberOf(
types, supertypeMethod, extendingType);
implementationMethodDescriptors.add(new ImplementationMethodDescriptor.Builder()
.name(supertypeMethod.getSimpleName().toString())
.returnType(getAnnotatedType(element).getQualifiedName().toString())
.publicMethod()
.passedParameters(Parameter.forParameterList(
supertypeMethod.getParameters(), methodType.getParameterTypes()))
.build());
implementationMethodDescriptorsBuilder.put(factoryName,
new ImplementationMethodDescriptor.Builder()
.name(supertypeMethod.getSimpleName().toString())
.returnType(getAnnotatedType(element).getQualifiedName().toString())
.publicMethod()
.passedParameters(Parameter.forParameterList(
supertypeMethod.getParameters(), methodType.getParameterTypes()))
.build());
}
}
for (TypeElement implementingType : declaration.get().implementingTypes()) {
Expand All @@ -125,13 +131,14 @@ private void doProcess(RoundEnvironment roundEnv) {
if (interfaceMethod.getModifiers().contains(Modifier.ABSTRACT)) {
ExecutableType methodType = Elements2.getExecutableElementAsMemberOf(
types, interfaceMethod, implementingType);
implementationMethodDescriptors.add(new ImplementationMethodDescriptor.Builder()
.name(interfaceMethod.getSimpleName().toString())
.returnType(getAnnotatedType(element).getQualifiedName().toString())
.publicMethod()
.passedParameters(Parameter.forParameterList(
interfaceMethod.getParameters(), methodType.getParameterTypes()))
.build());
implementationMethodDescriptorsBuilder.put(factoryName,
new ImplementationMethodDescriptor.Builder()
.name(interfaceMethod.getSimpleName().toString())
.returnType(getAnnotatedType(element).getQualifiedName().toString())
.publicMethod()
.passedParameters(Parameter.forParameterList(
interfaceMethod.getParameters(), methodType.getParameterTypes()))
.build());
}
}
}
Expand All @@ -147,6 +154,9 @@ private void doProcess(RoundEnvironment roundEnv) {
}));
}

ImmutableSetMultimap<String, ImplementationMethodDescriptor>
implementationMethodDescriptors = implementationMethodDescriptorsBuilder.build();

for (Entry<String, Collection<FactoryMethodDescriptor>> entry
: indexedMethods.build().asMap().entrySet()) {
ImmutableSet.Builder<String> extending = ImmutableSet.builder();
Expand Down Expand Up @@ -180,8 +190,7 @@ private void doProcess(RoundEnvironment roundEnv) {
implementing.build(),
publicType,
ImmutableSet.copyOf(entry.getValue()),
// TODO(gak): this needs to be indexed too
implementationMethodDescriptors.build(),
implementationMethodDescriptors.get(entry.getKey()),
allowSubclasses));
} catch (IOException e) {
messager.printMessage(Kind.ERROR, "failed");
Expand Down
92 changes: 53 additions & 39 deletions factory/src/test/java/com/google/auto/factory/processor/AutoFactoryProcessorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/
package com.google.auto.factory.processor;

import static com.google.common.truth.Truth.assert_;
import static com.google.common.truth.Truth.assertAbout;
import static com.google.testing.compile.JavaSourceSubjectFactory.javaSource;
import static com.google.testing.compile.JavaSourcesSubjectFactory.javaSources;

Expand All @@ -34,15 +34,15 @@
@RunWith(JUnit4.class)
public class AutoFactoryProcessorTest {
@Test public void simpleClass() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/SimpleClass.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
.and().generatesSources(JavaFileObjects.forResource("expected/SimpleClassFactory.java"));
}

@Test public void simpleClassNonFinal() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/SimpleClassNonFinal.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
Expand All @@ -51,23 +51,23 @@ public class AutoFactoryProcessorTest {
}

@Test public void publicClass() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/PublicClass.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
.and().generatesSources(JavaFileObjects.forResource("expected/PublicClassFactory.java"));
}

@Test public void simpleClassCustomName() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/SimpleClassCustomName.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
.and().generatesSources(JavaFileObjects.forResource("expected/CustomNamedFactory.java"));
}

@Test public void simpleClassMixedDeps() {
assert_().about(javaSources())
assertAbout(javaSources())
.that(ImmutableSet.of(
JavaFileObjects.forResource("good/SimpleClassMixedDeps.java"),
JavaFileObjects.forResource("support/AQualifier.java")))
Expand All @@ -78,7 +78,7 @@ public class AutoFactoryProcessorTest {
}

@Test public void simpleClassPassedDeps() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/SimpleClassPassedDeps.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
Expand All @@ -87,7 +87,7 @@ public class AutoFactoryProcessorTest {
}

@Test public void simpleClassProvidedDeps() {
assert_().about(javaSources())
assertAbout(javaSources())
.that(ImmutableSet.of(
JavaFileObjects.forResource("support/AQualifier.java"),
JavaFileObjects.forResource("support/BQualifier.java"),
Expand All @@ -99,7 +99,7 @@ public class AutoFactoryProcessorTest {
}

@Test public void constructorAnnotated() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/ConstructorAnnotated.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
Expand All @@ -108,7 +108,7 @@ public class AutoFactoryProcessorTest {
}

@Test public void constructorAnnotatedNonFinal() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/ConstructorAnnotatedNonFinal.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
Expand All @@ -117,7 +117,7 @@ public class AutoFactoryProcessorTest {
}

@Test public void simpleClassImplementingMarker() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/SimpleClassImplementingMarker.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
Expand All @@ -126,7 +126,7 @@ public class AutoFactoryProcessorTest {
}

@Test public void simpleClassImplementingSimpleInterface() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/SimpleClassImplementingSimpleInterface.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
Expand All @@ -135,7 +135,7 @@ public class AutoFactoryProcessorTest {
}

@Test public void mixedDepsImplementingInterfaces() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/MixedDepsImplementingInterfaces.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
Expand All @@ -145,31 +145,31 @@ public class AutoFactoryProcessorTest {

@Test public void failsWithMixedFinals() {
JavaFileObject file = JavaFileObjects.forResource("bad/MixedFinals.java");
assert_().about(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
.withErrorContaining(
"Cannot mix allowSubclasses=true and allowSubclasses=false in one factory.")
.in(file).onLine(25).atColumn(3)
.and().withErrorContaining(
"Cannot mix allowSubclasses=true and allowSubclasses=false in one factory.")
.in(file).onLine(26).atColumn(3);
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
.withErrorContaining(
"Cannot mix allowSubclasses=true and allowSubclasses=false in one factory.")
.in(file).onLine(25).atColumn(3)
.and().withErrorContaining(
"Cannot mix allowSubclasses=true and allowSubclasses=false in one factory.")
.in(file).onLine(26).atColumn(3);
}

@Test public void failsOnGenericClass() {
JavaFileObject file = JavaFileObjects.forResource("bad/GenericClass.java");
assert_().about(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
.withErrorContaining("AutoFactory does not support generic types")
.in(file).onLine(21).atColumn(14);
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
.withErrorContaining("AutoFactory does not support generic types")
.in(file).onLine(21).atColumn(14);
}

@Test public void providedButNoAutoFactory() {
JavaFileObject file = JavaFileObjects.forResource("bad/ProvidedButNoAutoFactory.java");
assert_().about(javaSource())
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
Expand All @@ -180,7 +180,7 @@ public class AutoFactoryProcessorTest {

@Test public void providedOnMethodParameter() {
JavaFileObject file = JavaFileObjects.forResource("bad/ProvidedOnMethodParameter.java");
assert_().about(javaSource())
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
Expand All @@ -191,7 +191,7 @@ public class AutoFactoryProcessorTest {

@Test public void invalidCustomName() {
JavaFileObject file = JavaFileObjects.forResource("bad/InvalidCustomName.java");
assert_().about(javaSource())
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
Expand All @@ -200,7 +200,7 @@ public class AutoFactoryProcessorTest {
}

@Test public void factoryExtendingAbstractClass() {
assert_().about(javaSource())
assertAbout(javaSource())
.that(JavaFileObjects.forResource("good/FactoryExtendingAbstractClass.java"))
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
Expand All @@ -211,7 +211,7 @@ public class AutoFactoryProcessorTest {
@Test public void factoryExtendingAbstractClass_withConstructorParams() {
JavaFileObject file =
JavaFileObjects.forResource("good/FactoryExtendingAbstractClassWithConstructorParams.java");
assert_().about(javaSource())
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
Expand All @@ -225,15 +225,15 @@ public class AutoFactoryProcessorTest {
@Test public void factoryExtendingAbstractClass_multipleConstructors() {
JavaFileObject file = JavaFileObjects.forResource(
"good/FactoryExtendingAbstractClassWithMultipleConstructors.java");
assert_().about(javaSource())
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError();
}

@Test public void factoryExtendingInterface() {
JavaFileObject file = JavaFileObjects.forResource("bad/InterfaceSupertype.java");
assert_().about(javaSource())
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
Expand All @@ -244,7 +244,7 @@ public class AutoFactoryProcessorTest {

@Test public void factoryExtendingEnum() {
JavaFileObject file = JavaFileObjects.forResource("bad/EnumSupertype.java");
assert_().about(javaSource())
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
Expand All @@ -256,7 +256,7 @@ public class AutoFactoryProcessorTest {

@Test public void factoryExtendingFinalClass() {
JavaFileObject file = JavaFileObjects.forResource("bad/FinalSupertype.java");
assert_().about(javaSource())
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.failsToCompile()
Expand All @@ -268,11 +268,25 @@ public class AutoFactoryProcessorTest {
@Test public void factoryImplementingGenericInterfaceExtension() {
JavaFileObject file =
JavaFileObjects.forResource("good/FactoryImplementingGenericInterfaceExtension.java");
assert_().about(javaSource())
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
.and().generatesSources(JavaFileObjects.forResource(
"expected/FactoryImplementingGenericInterfaceExtension.java"));
}

@Test public void multipleFactoriesImpementingInterface() {
JavaFileObject file =
JavaFileObjects.forResource("good/MultipleFactoriesImplementingInterface.java");
assertAbout(javaSource())
.that(file)
.processedWith(new AutoFactoryProcessor())
.compilesWithoutError()
.and().generatesSources(
JavaFileObjects.forResource(
"expected/MultipleFactoriesImplementingInterfaceAFactory.java"),
JavaFileObjects.forResource(
"expected/MultipleFactoriesImplementingInterfaceBFactory.java"));
}
}
36 changes: 36 additions & 0 deletions factory/src/test/resources/expected/MultipleFactoriesImplementingInterfaceAFactory.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright (C) 2015 Google, Inc.
*
* 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 tests;

import javax.annotation.Generated;
import javax.inject.Inject;
import tests.MultipleFactoriesImplementingInterface.Base.Factory;

@Generated("com.google.auto.factory.processor.AutoFactoryProcessor")
final class MultipleFactoriesImplementingInterfaceAFactory implements Factory {
@Inject
MultipleFactoriesImplementingInterfaceAFactory() {}

MultipleFactoriesImplementingInterface.MultipleFactoriesImplementingInterfaceA create() {
return new MultipleFactoriesImplementingInterface.MultipleFactoriesImplementingInterfaceA();
}

@Override
public MultipleFactoriesImplementingInterface.MultipleFactoriesImplementingInterfaceA
abstractNonDefaultCreate() {
return create();
}
}

0 comments on commit b1aca4d

Please sign in to comment.