From 51d049ff958f97bceba324882c478cb1a10f6e15 Mon Sep 17 00:00:00 2001 From: dpb Date: Thu, 21 Jun 2018 10:03:44 -0700 Subject: [PATCH] Check conflicting entry points. RELNOTES=If two entry point methods with different keys are inherited from different supertypes of a component type, Dagger reports an error. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=201542278 --- .../internal/codegen/ComponentValidator.java | 150 ++++++++-- .../codegen/MethodSignatureFormatter.java | 30 +- .../codegen/ConflictingEntryPointsTest.java | 262 ++++++++++++++++++ 3 files changed, 411 insertions(+), 31 deletions(-) create mode 100644 javatests/dagger/internal/codegen/ConflictingEntryPointsTest.java diff --git a/java/dagger/internal/codegen/ComponentValidator.java b/java/dagger/internal/codegen/ComponentValidator.java index 89c8195d9de..671f0857a4d 100644 --- a/java/dagger/internal/codegen/ComponentValidator.java +++ b/java/dagger/internal/codegen/ComponentValidator.java @@ -16,8 +16,12 @@ package dagger.internal.codegen; +import static com.google.auto.common.MoreElements.asType; import static com.google.auto.common.MoreElements.getLocalAndInheritedMethods; +import static com.google.auto.common.MoreTypes.asDeclared; import static com.google.auto.common.MoreTypes.asExecutable; +import static com.google.common.base.Verify.verify; +import static com.google.common.collect.Multimaps.asMap; import static dagger.internal.codegen.ConfigurationAnnotations.enclosedBuilders; import static dagger.internal.codegen.ConfigurationAnnotations.getComponentDependencies; import static dagger.internal.codegen.ConfigurationAnnotations.getComponentModules; @@ -25,15 +29,19 @@ import static dagger.internal.codegen.ConfigurationAnnotations.getTransitiveModules; import static dagger.internal.codegen.DaggerElements.getAnnotationMirror; import static dagger.internal.codegen.DaggerElements.getAnyAnnotation; +import static dagger.internal.codegen.DaggerStreams.toImmutableSet; +import static java.util.Comparator.comparing; import static javax.lang.model.element.ElementKind.CLASS; import static javax.lang.model.element.ElementKind.INTERFACE; import static javax.lang.model.element.Modifier.ABSTRACT; import static javax.lang.model.type.TypeKind.VOID; +import static javax.lang.model.util.ElementFilter.methodsIn; import com.google.auto.common.MoreElements; import com.google.auto.common.MoreTypes; import com.google.auto.value.AutoValue; import com.google.common.collect.FluentIterable; +import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -44,8 +52,11 @@ import dagger.Component; import dagger.Reusable; import dagger.internal.codegen.ComponentDescriptor.Kind; +import dagger.model.DependencyRequest; +import dagger.model.Key; import dagger.producers.ProductionComponent; import java.lang.annotation.Annotation; +import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Optional; @@ -71,17 +82,23 @@ final class ComponentValidator { private final Types types; private final ModuleValidator moduleValidator; private final BuilderValidator builderValidator; + private final MethodSignatureFormatter methodSignatureFormatter; + private final DependencyRequestFactory dependencyRequestFactory; @Inject ComponentValidator( DaggerElements elements, Types types, ModuleValidator moduleValidator, - BuilderValidator builderValidator) { + BuilderValidator builderValidator, + MethodSignatureFormatter methodSignatureFormatter, + DependencyRequestFactory dependencyRequestFactory) { this.elements = elements; this.types = types; this.moduleValidator = moduleValidator; this.builderValidator = builderValidator; + this.methodSignatureFormatter = methodSignatureFormatter; + this.dependencyRequestFactory = dependencyRequestFactory; } @AutoValue @@ -99,14 +116,14 @@ public ComponentValidationReport validate( final TypeElement subject, Set validatedSubcomponents, Set validatedSubcomponentBuilders) { - ValidationReport.Builder builder = ValidationReport.about(subject); + ValidationReport.Builder report = ValidationReport.about(subject); ComponentDescriptor.Kind componentKind = ComponentDescriptor.Kind.forAnnotatedElement(subject).get(); if (!subject.getKind().equals(INTERFACE) && !(subject.getKind().equals(CLASS) && subject.getModifiers().contains(ABSTRACT))) { - builder.addError( + report.addError( String.format( "@%s may only be applied to an interface or abstract class", componentKind.annotationType().getSimpleName()), @@ -116,14 +133,14 @@ public ComponentValidationReport validate( ImmutableList builders = enclosedBuilders(subject, componentKind.builderAnnotationType()); if (builders.size() > 1) { - builder.addError( + report.addError( String.format(ErrorMessages.builderMsgsFor(componentKind).moreThanOne(), builders), subject); } Optional reusableAnnotation = getAnnotationMirror(subject, Reusable.class); if (reusableAnnotation.isPresent()) { - builder.addError( + report.addError( "@Reusable cannot be applied to components or subcomponents", subject, reusableAnnotation.get()); @@ -143,7 +160,7 @@ public ComponentValidationReport validate( TypeMirror returnType = resolvedMethod.getReturnType(); if (!resolvedMethod.getTypeVariables().isEmpty()) { - builder.addError("Component methods cannot have type variables", method); + report.addError("Component methods cannot have type variables", method); } // abstract methods are ones we have to implement, so they each need to be validated @@ -163,7 +180,7 @@ public ComponentValidationReport validate( if (subcomponentAnnotation.isPresent()) { referencedSubcomponents.put(MoreTypes.asElement(returnType), method); validateSubcomponentMethod( - builder, + report, ComponentDescriptor.Kind.forAnnotatedElement( MoreTypes.asTypeElement(returnType)) .get(), @@ -176,7 +193,7 @@ public ComponentValidationReport validate( referencedSubcomponents.put( MoreTypes.asElement(returnType).getEnclosingElement(), method); validateSubcomponentBuilderMethod( - builder, method, parameters, returnType, validatedSubcomponentBuilders); + report, method, parameters, returnType, validatedSubcomponentBuilders); } else { // if it's not a subcomponent... switch (parameters.size()) { @@ -189,14 +206,14 @@ public ComponentValidationReport validate( TypeMirror onlyParameter = Iterables.getOnlyElement(parameterTypes); if (!(returnType.getKind().equals(VOID) || types.isSameType(returnType, onlyParameter))) { - builder.addError( + report.addError( "Members injection methods may only return the injected type or void.", method); } break; default: // this isn't any method that we know how to implement... - builder.addError( + report.addError( "This method isn't a valid provision method, members injection method or " + "subcomponent factory method. Dagger cannot implement this method", method); @@ -205,10 +222,12 @@ public ComponentValidationReport validate( } }); + checkConflictingEntryPoints(report); + Maps.filterValues(referencedSubcomponents.asMap(), methods -> methods.size() > 1) .forEach( (subcomponent, methods) -> - builder.addError( + report.addError( String.format( ErrorMessages.SubcomponentBuilderMessages.INSTANCE .moreThanOneRefToSubcomponent(), @@ -219,9 +238,9 @@ public ComponentValidationReport validate( AnnotationMirror componentMirror = getAnnotationMirror(subject, componentKind.annotationType()).get(); if (componentKind.isTopLevel()) { - validateComponentDependencies(builder, getComponentDependencies(componentMirror)); + validateComponentDependencies(report, getComponentDependencies(componentMirror)); } - builder.addSubreport( + report.addSubreport( moduleValidator.validateReferencedModules( subject, componentMirror, componentKind.moduleKinds(), new HashSet<>())); @@ -235,20 +254,99 @@ public ComponentValidationReport validate( for (Element subcomponent : Sets.difference(referencedSubcomponents.keySet(), validatedSubcomponents)) { ComponentValidationReport subreport = - validate( - MoreElements.asType(subcomponent), - validatedSubcomponents, - validatedSubcomponentBuilders); - builder.addItems(subreport.report().items()); + validate(asType(subcomponent), validatedSubcomponents, validatedSubcomponentBuilders); + report.addItems(subreport.report().items()); allSubcomponents.addAll(subreport.referencedSubcomponents()); } return new AutoValue_ComponentValidator_ComponentValidationReport( - allSubcomponents.build(), builder.build()); + allSubcomponents.build(), report.build()); + } + + private void checkConflictingEntryPoints(ValidationReport.Builder report) { + DeclaredType componentType = asDeclared(report.getSubject().asType()); + + // Collect entry point methods that are not overridden by others. If the "same" method is + // inherited from more than one supertype, each will be in the multimap. + SetMultimap entryPointMethods = HashMultimap.create(); + + methodsIn(elements.getAllMembers(report.getSubject())) + .stream() + .filter( + method -> isEntryPoint(method, asExecutable(types.asMemberOf(componentType, method)))) + .forEach( + method -> + addMethodUnlessOverridden( + method, entryPointMethods.get(method.getSimpleName().toString()))); + + for (Set methods : asMap(entryPointMethods).values()) { + if (distinctKeys(methods, report.getSubject()).size() > 1) { + reportConflictingEntryPoints(methods, report); + } + } + } + + private boolean isEntryPoint(ExecutableElement method, ExecutableType methodType) { + return method.getModifiers().contains(ABSTRACT) + && method.getParameters().isEmpty() + && !methodType.getReturnType().getKind().equals(VOID) + && methodType.getTypeVariables().isEmpty(); + } + + private ImmutableSet distinctKeys(Set methods, TypeElement component) { + return methods + .stream() + .map(method -> dependencyRequest(method, component)) + .map(DependencyRequest::key) + .collect(toImmutableSet()); + } + + private DependencyRequest dependencyRequest(ExecutableElement method, TypeElement component) { + ExecutableType methodType = + asExecutable(types.asMemberOf(asDeclared(component.asType()), method)); + return ComponentDescriptor.Kind.forAnnotatedElement(component).get().isProducer() + ? dependencyRequestFactory.forComponentProductionMethod(method, methodType) + : dependencyRequestFactory.forComponentProvisionMethod(method, methodType); + } + + private void addMethodUnlessOverridden(ExecutableElement method, Set methods) { + if (methods.stream().noneMatch(existingMethod -> overridesAsDeclared(existingMethod, method))) { + methods.removeIf(existingMethod -> overridesAsDeclared(method, existingMethod)); + methods.add(method); + } + } + + /** + * Returns {@code true} if {@code overrider} overrides {@code overridden} considered from within + * the type that declares {@code overrider}. + */ + // TODO(dpb): Does this break for ECJ? + private boolean overridesAsDeclared(ExecutableElement overridder, ExecutableElement overridden) { + return elements.overrides(overridder, overridden, asType(overridder.getEnclosingElement())); + } + + private void reportConflictingEntryPoints( + Collection methods, ValidationReport.Builder report) { + verify( + methods.stream().map(ExecutableElement::getEnclosingElement).distinct().count() + == methods.size(), + "expected each method to be declared on a different type: %s", + methods); + StringBuilder message = new StringBuilder("conflicting entry point declarations:"); + methodSignatureFormatter + .typedFormatter(asDeclared(report.getSubject().asType())) + .formatIndentedList( + message, + ImmutableList.sortedCopyOf( + comparing( + method -> asType(method.getEnclosingElement()).getQualifiedName().toString()), + methods), + 1); + report.addError(message.toString()); } private void validateSubcomponentMethod( - final ValidationReport.Builder builder, + final ValidationReport.Builder report, final ComponentDescriptor.Kind subcomponentKind, ExecutableElement method, List parameters, @@ -292,7 +390,7 @@ public Optional visitDeclared(DeclaredType t, Void p) { null); if (moduleType.isPresent()) { if (variableTypes.contains(moduleType.get())) { - builder.addError( + report.addError( String.format( "A module may only occur once an an argument in a Subcomponent factory " + "method, but %s was already passed.", @@ -300,7 +398,7 @@ public Optional visitDeclared(DeclaredType t, Void p) { parameter); } if (!transitiveModules.contains(moduleType.get())) { - builder.addError( + report.addError( String.format( "%s is present as an argument to the %s factory method, but is not one of the" + " modules used to implement the subcomponent.", @@ -310,7 +408,7 @@ public Optional visitDeclared(DeclaredType t, Void p) { } variableTypes.add(moduleType.get()); } else { - builder.addError( + report.addError( String.format( "Subcomponent factory methods may only accept modules, but %s is not.", parameterType), @@ -320,14 +418,14 @@ public Optional visitDeclared(DeclaredType t, Void p) { } private void validateSubcomponentBuilderMethod( - ValidationReport.Builder builder, + ValidationReport.Builder report, ExecutableElement method, List parameters, TypeMirror returnType, Set validatedSubcomponentBuilders) { if (!parameters.isEmpty()) { - builder.addError( + report.addError( ErrorMessages.SubcomponentBuilderMessages.INSTANCE.builderMethodRequiresNoArgs(), method); } @@ -337,7 +435,7 @@ private void validateSubcomponentBuilderMethod( // TODO(sameb): The builder validator right now assumes the element is being compiled // in this pass, which isn't true here. We should change error messages to spit out // this method as the subject and add the original subject to the message output. - builder.addItems(builderValidator.validate(builderElement).items()); + report.addItems(builderValidator.validate(builderElement).items()); } } diff --git a/java/dagger/internal/codegen/MethodSignatureFormatter.java b/java/dagger/internal/codegen/MethodSignatureFormatter.java index 6ffee05d08e..67535572c61 100644 --- a/java/dagger/internal/codegen/MethodSignatureFormatter.java +++ b/java/dagger/internal/codegen/MethodSignatureFormatter.java @@ -46,6 +46,22 @@ final class MethodSignatureFormatter extends Formatter { this.types = types; } + /** + * A formatter that uses the type where the method is declared for the annotations and name of the + * method, but the method's resolved type as a member of {@code declaredType} for the key. + */ + Formatter typedFormatter(DeclaredType declaredType) { + return new Formatter() { + @Override + public String format(ExecutableElement method) { + return MethodSignatureFormatter.this.format( + method, + MoreTypes.asExecutable(types.asMemberOf(declaredType, method)), + MoreElements.asType(method.getEnclosingElement())); + } + }; + } + @Override public String format(ExecutableElement method) { return format(method, Optional.empty()); } @@ -55,14 +71,18 @@ final class MethodSignatureFormatter extends Formatter { * present. */ public String format(ExecutableElement method, Optional container) { - StringBuilder builder = new StringBuilder(); TypeElement type = MoreElements.asType(method.getEnclosingElement()); ExecutableType executableType = MoreTypes.asExecutable(method.asType()); if (container.isPresent()) { executableType = MoreTypes.asExecutable(types.asMemberOf(container.get(), method)); type = MoreElements.asType(container.get().asElement()); } + return format(method, executableType, type); + } + private String format( + ExecutableElement method, ExecutableType methodType, TypeElement declaringType) { + StringBuilder builder = new StringBuilder(); // TODO(cgruber): AnnotationMirror formatter. List annotations = method.getAnnotationMirrors(); if (!annotations.isEmpty()) { @@ -75,15 +95,15 @@ public String format(ExecutableElement method, Optional container) } builder.append(' '); } - builder.append(nameOfType(executableType.getReturnType())); + builder.append(nameOfType(methodType.getReturnType())); builder.append(' '); - builder.append(type.getQualifiedName()); + builder.append(declaringType.getQualifiedName()); builder.append('.'); builder.append(method.getSimpleName()); builder.append('('); - checkState(method.getParameters().size() == executableType.getParameterTypes().size()); + checkState(method.getParameters().size() == methodType.getParameterTypes().size()); Iterator parameters = method.getParameters().iterator(); - Iterator parameterTypes = executableType.getParameterTypes().iterator(); + Iterator parameterTypes = methodType.getParameterTypes().iterator(); for (int i = 0; parameters.hasNext(); i++) { if (i > 0) { builder.append(", "); diff --git a/javatests/dagger/internal/codegen/ConflictingEntryPointsTest.java b/javatests/dagger/internal/codegen/ConflictingEntryPointsTest.java new file mode 100644 index 00000000000..ea904474bac --- /dev/null +++ b/javatests/dagger/internal/codegen/ConflictingEntryPointsTest.java @@ -0,0 +1,262 @@ +/* + * Copyright (C) 2018 The Dagger Authors. + * + * 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 dagger.internal.codegen; + +import static com.google.testing.compile.CompilationSubject.assertThat; +import static dagger.internal.codegen.Compilers.daggerCompiler; +import static dagger.internal.codegen.TestUtils.message; + +import com.google.testing.compile.Compilation; +import com.google.testing.compile.JavaFileObjects; +import javax.tools.JavaFileObject; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class ConflictingEntryPointsTest { + + @Test + public void covariantType() { + JavaFileObject base1 = + JavaFileObjects.forSourceLines( + "test.Base1", // + "package test;", + "", + "interface Base1 {", + " Long foo();", + "}"); + JavaFileObject base2 = + JavaFileObjects.forSourceLines( + "test.Base2", // + "package test;", + "", + "interface Base2 {", + " Number foo();", + "}"); + JavaFileObject component = + JavaFileObjects.forSourceLines( + "test.TestComponent", + "package test;", + "", + "import dagger.BindsInstance;", + "import dagger.Component;", + "", + "@Component", + "interface TestComponent extends Base1, Base2 {", + "", + " @Component.Builder", + " interface Builder {", + " @BindsInstance Builder foo(Long foo);", + " @BindsInstance Builder foo(Number foo);", + " TestComponent build();", + " }", + "}"); + Compilation compilation = daggerCompiler().compile(base1, base2, component); + assertThat(compilation).failed(); + assertThat(compilation) + .hadErrorContaining( + message( + "conflicting entry point declarations:", + "Long test.Base1.foo()", + "Number test.Base2.foo()")) + .inFile(component) + .onLineContaining("interface TestComponent "); + } + + @Test + public void covariantTypeFromGenericSupertypes() { + JavaFileObject base1 = + JavaFileObjects.forSourceLines( + "test.Base1", // + "package test;", + "", + "interface Base1 {", + " T foo();", + "}"); + JavaFileObject base2 = + JavaFileObjects.forSourceLines( + "test.Base2", // + "package test;", + "", + "interface Base2 {", + " T foo();", + "}"); + JavaFileObject component = + JavaFileObjects.forSourceLines( + "test.TestComponent", + "package test;", + "", + "import dagger.BindsInstance;", + "import dagger.Component;", + "", + "@Component", + "interface TestComponent extends Base1, Base2 {", + "", + " @Component.Builder", + " interface Builder {", + " @BindsInstance Builder foo(Long foo);", + " @BindsInstance Builder foo(Number foo);", + " TestComponent build();", + " }", + "}"); + Compilation compilation = daggerCompiler().compile(base1, base2, component); + assertThat(compilation).failed(); + assertThat(compilation) + .hadErrorContaining( + message( + "conflicting entry point declarations:", + "Long test.Base1.foo()", + "Number test.Base2.foo()")) + .inFile(component) + .onLineContaining("interface TestComponent "); + } + + @Test + public void differentQualifier() { + JavaFileObject base1 = + JavaFileObjects.forSourceLines( + "test.Base1", // + "package test;", + "", + "interface Base1 {", + " Object foo();", + "}"); + JavaFileObject base2 = + JavaFileObjects.forSourceLines( + "test.Base2", // + "package test;", + "", + "import javax.inject.Named;", + "", + "interface Base2 {", + " @Named(\"foo\") Object foo();", + "}"); + JavaFileObject component = + JavaFileObjects.forSourceLines( + "test.TestComponent", + "package test;", + "", + "import dagger.BindsInstance;", + "import dagger.Component;", + "import javax.inject.Named;", + "", + "@Component", + "interface TestComponent extends Base1, Base2 {", + "", + " @Component.Builder", + " interface Builder {", + " @BindsInstance Builder foo(Object foo);", + " @BindsInstance Builder namedFoo(@Named(\"foo\") Object foo);", + " TestComponent build();", + " }", + "}"); + Compilation compilation = daggerCompiler().compile(base1, base2, component); + assertThat(compilation).failed(); + assertThat(compilation) + .hadErrorContaining( + message( + "conflicting entry point declarations:", + "Object test.Base1.foo()", + "@Named(\"foo\") Object test.Base2.foo()")) + .inFile(component) + .onLineContaining("interface TestComponent "); + } + + @Test + public void sameKey() { + JavaFileObject base1 = + JavaFileObjects.forSourceLines( + "test.Base1", // + "package test;", + "", + "interface Base1 {", + " Object foo();", + "}"); + JavaFileObject base2 = + JavaFileObjects.forSourceLines( + "test.Base2", // + "package test;", + "", + "interface Base2 {", + " Object foo();", + "}"); + JavaFileObject component = + JavaFileObjects.forSourceLines( + "test.TestComponent", + "package test;", + "", + "import dagger.BindsInstance;", + "import dagger.Component;", + "", + "@Component", + "interface TestComponent extends Base1, Base2 {", + "", + " @Component.Builder", + " interface Builder {", + " @BindsInstance Builder foo(Object foo);", + " TestComponent build();", + " }", + "}"); + Compilation compilation = daggerCompiler().compile(base1, base2, component); + assertThat(compilation).succeeded(); + } + + @Test + public void sameQualifiedKey() { + JavaFileObject base1 = + JavaFileObjects.forSourceLines( + "test.Base1", // + "package test;", + "", + "import javax.inject.Named;", + "", + "interface Base1 {", + " @Named(\"foo\") Object foo();", + "}"); + JavaFileObject base2 = + JavaFileObjects.forSourceLines( + "test.Base2", // + "package test;", + "", + "import javax.inject.Named;", + "", + "interface Base2 {", + " @Named(\"foo\") Object foo();", + "}"); + JavaFileObject component = + JavaFileObjects.forSourceLines( + "test.TestComponent", + "package test;", + "", + "import dagger.BindsInstance;", + "import dagger.Component;", + "import javax.inject.Named;", + "", + "@Component", + "interface TestComponent extends Base1, Base2 {", + "", + " @Component.Builder", + " interface Builder {", + " @BindsInstance Builder foo(@Named(\"foo\") Object foo);", + " TestComponent build();", + " }", + "}"); + Compilation compilation = daggerCompiler().compile(base1, base2, component); + assertThat(compilation).succeeded(); + } +}