From 325b516ac6a53d3fc973d247b5231fafda9870a2 Mon Sep 17 00:00:00 2001 From: dpb Date: Mon, 3 Dec 2018 08:56:02 -0800 Subject: [PATCH] Add module-level validation. When -Adagger.moduleBindingValidation=ERROR or =WARNING is set, then each module is processed as a kind of component (without generating any code) in order to validate the bindings installed by it and all included modules. Any binding graph errors, such as duplicate bindngs (but not including missing bindings), will be reported as errors or warnings depending on the option. SPI plugins will also run on those module-level binding graphs. Relnotes: Add module-level validation. When `-Adagger.moduleBindingValidation=ERROR` or `=WARNING` is set, then each module is processed as a kind of component (without generating any code) in order to validate the bindings installed by it and all included modules. Any binding graph errors, such as duplicate bindngs (but not including missing bindings), will be reported as errors or warnings depending on the option. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=223803671 --- java/dagger/internal/codegen/BUILD | 18 +- .../codegen/BindingGraphConverter.java | 3 +- .../internal/codegen/BindingGraphFactory.java | 7 +- .../codegen/BindingGraphValidationModule.java | 22 ++ .../codegen/BindsInstanceProcessingStep.java | 1 + .../internal/codegen/BuilderValidator.java | 2 +- .../internal/codegen/CompilerOptions.java | 30 +- .../internal/codegen/ComponentDescriptor.java | 279 +++++++++++------- .../codegen/ComponentHjarProcessingStep.java | 2 +- .../codegen/ComponentProcessingStep.java | 4 +- .../internal/codegen/ComponentValidator.java | 27 +- .../codegen/ConfigurationAnnotations.java | 4 +- .../DependsOnProductionExecutorValidator.java | 6 +- .../codegen/DiagnosticReporterFactory.java | 87 ++++-- .../IncompatiblyScopedBindingsValidator.java | 61 ++-- .../codegen/KytheBindingGraphFactory.java | 5 +- .../codegen/MissingBindingValidator.java | 3 + .../internal/codegen/ModuleDescriptor.java | 26 ++ .../internal/codegen/ModuleValidation.java | 31 ++ .../internal/codegen/ModuleValidator.java | 36 ++- .../SubcomponentFactoryMethodValidator.java | 4 + .../internal/codegen/ValidationReport.java | 26 +- java/dagger/model/BindingGraph.java | 32 +- java/dagger/model/BindingGraphProxies.java | 5 +- .../DependencyCycleValidationTest.java | 78 +++-- .../DuplicateBindingsValidationTest.java | 197 ++++++++++--- .../MapMultibindingValidationTest.java | 28 ++ .../codegen/ModuleBindingValidationTest.java | 124 ++++++++ .../NullableBindingValidationTest.java | 29 ++ .../ProductionComponentProcessorTest.java | 17 +- .../ProductionGraphValidationTest.java | 77 ++--- .../codegen/ScopingValidationTest.java | 43 ++- 32 files changed, 1026 insertions(+), 288 deletions(-) create mode 100644 java/dagger/internal/codegen/ModuleValidation.java create mode 100644 javatests/dagger/internal/codegen/ModuleBindingValidationTest.java diff --git a/java/dagger/internal/codegen/BUILD b/java/dagger/internal/codegen/BUILD index 2a0c07ceef9..049222a27cd 100644 --- a/java/dagger/internal/codegen/BUILD +++ b/java/dagger/internal/codegen/BUILD @@ -125,8 +125,11 @@ java_library( "AnnotationExpression.java", "Binding.java", "BindingDeclaration.java", + "BindingDeclarationFormatter.java", "BindingFactory.java", "BindingGraph.java", + "BindingGraphConverter.java", + "BindingGraphFactory.java", "BindingNode.java", "BindingRequest.java", "BindingType.java", @@ -142,6 +145,7 @@ java_library( "DelegateDeclaration.java", "DependencyEdgeImpl.java", "DependencyRequestFactory.java", + "DependencyRequestFormatter.java", "DependencyVariableNamer.java", # Used by SourceFiles "ErrorMessages.java", # Consider splitting this up as it pulls in too much "FrameworkDependency.java", @@ -154,6 +158,7 @@ java_library( "MapKeys.java", "MembersInjectionBinding.java", "MethodSignature.java", + "MethodSignatureFormatter.java", "ModuleDescriptor.java", "MultibindingDeclaration.java", "OptionalBindingDeclaration.java", @@ -174,7 +179,7 @@ java_library( name = "validation", srcs = [ "AnyBindingMethodValidator.java", - "BindingDeclarationFormatter.java", + "BindingGraphPlugins.java", "BindingMethodProcessingStep.java", "BindingMethodValidator.java", "BindsInstanceProcessingStep.java", @@ -184,12 +189,12 @@ java_library( "ComponentDescriptorValidator.java", "ComponentHierarchyValidator.java", "ComponentValidator.java", - "DependencyRequestFormatter.java", "DependencyRequestValidator.java", + "DiagnosticReporterFactory.java", "InjectValidator.java", "MapKeyValidator.java", "MembersInjectionValidator.java", - "MethodSignatureFormatter.java", + "ModuleValidation.java", "ModuleValidator.java", "MultibindingAnnotationsProcessingStep.java", "MultibindsMethodValidator.java", @@ -208,10 +213,8 @@ java_library( java_library( name = "binding_graph_validation", srcs = [ - "BindingGraphPlugins.java", "DependencyCycleValidator.java", "DependsOnProductionExecutorValidator.java", - "DiagnosticReporterFactory.java", "DuplicateBindingsValidator.java", "IncompatiblyScopedBindingsValidator.java", "InjectBindingValidator.java", @@ -220,7 +223,6 @@ java_library( "NullableBindingValidator.java", "ProvisionDependencyOnProducerBindingValidator.java", "SubcomponentFactoryMethodValidator.java", - "Validation.java", ], plugins = CODEGEN_PLUGINS, tags = ["maven:merged"], @@ -324,8 +326,6 @@ simple_jar( java_library( name = "processor", srcs = [ - "BindingGraphConverter.java", - "BindingGraphFactory.java", "BindingGraphValidationModule.java", "BindingMethodValidatorsModule.java", "ComponentGenerator.java", @@ -342,6 +342,7 @@ java_library( "SourceFileGeneratorsModule.java", "SpiModule.java", "SystemComponentsModule.java", + "Validation.java", ], plugins = CODEGEN_PLUGINS, resource_jars = [":processor_manifest_files.jar"], @@ -376,7 +377,6 @@ java_library( deps = [ ":base", ":binding", - ":binding_graph_validation", ":kythe_plugin", ":processor", ":validation", diff --git a/java/dagger/internal/codegen/BindingGraphConverter.java b/java/dagger/internal/codegen/BindingGraphConverter.java index f09004a93a2..57f46899f91 100644 --- a/java/dagger/internal/codegen/BindingGraphConverter.java +++ b/java/dagger/internal/codegen/BindingGraphConverter.java @@ -70,7 +70,8 @@ dagger.model.BindingGraph convert(BindingGraph rootGraph) { unreachableNodes(traverser.network.asGraph(), rootComponentNode(traverser.network)) .forEach(traverser.network::removeNode); - return BindingGraphProxies.bindingGraph(traverser.network); + return BindingGraphProxies.bindingGraph( + traverser.network, rootGraph.componentDescriptor().kind().isForModuleValidation()); } // TODO(dpb): Example of BindingGraph logic applied to derived networks. diff --git a/java/dagger/internal/codegen/BindingGraphFactory.java b/java/dagger/internal/codegen/BindingGraphFactory.java index a89d64a3e0f..82d63a2d35f 100644 --- a/java/dagger/internal/codegen/BindingGraphFactory.java +++ b/java/dagger/internal/codegen/BindingGraphFactory.java @@ -111,8 +111,11 @@ private BindingGraph create( ImmutableSet.Builder delegatesBuilder = ImmutableSet.builder(); ImmutableSet.Builder optionalsBuilder = ImmutableSet.builder(); - // binding for the component itself - explicitBindingsBuilder.add(bindingFactory.componentBinding(componentDescriptor.typeElement())); + if (!componentDescriptor.kind().isForModuleValidation()) { + // binding for the component itself + explicitBindingsBuilder.add( + bindingFactory.componentBinding(componentDescriptor.typeElement())); + } // Collect Component dependencies. for (ComponentRequirement dependency : componentDescriptor.dependencies()) { diff --git a/java/dagger/internal/codegen/BindingGraphValidationModule.java b/java/dagger/internal/codegen/BindingGraphValidationModule.java index c35bc2e6cb3..48c28b79e2d 100644 --- a/java/dagger/internal/codegen/BindingGraphValidationModule.java +++ b/java/dagger/internal/codegen/BindingGraphValidationModule.java @@ -96,4 +96,26 @@ static BindingGraphPlugins validationPlugins( return new BindingGraphPlugins( validationPlugins, filer, types, elements, processingOptions, diagnosticReporterFactory); } + + @Provides + @Singleton + @ModuleValidation + static BindingGraphPlugins moduleValidationPlugins( + @Validation Set validationPlugins, + Filer filer, + Types types, + Elements elements, + @ProcessingOptions Map processingOptions, + DiagnosticReporterFactory diagnosticReporterFactory, + CompilerOptions compilerOptions) { + return new BindingGraphPlugins( + validationPlugins, + filer, + types, + elements, + processingOptions, + diagnosticReporterFactory + .treatingErrorsAs(compilerOptions.moduleBindingValidationType()) + .withoutPrintingEntryPoints()); + } } diff --git a/java/dagger/internal/codegen/BindsInstanceProcessingStep.java b/java/dagger/internal/codegen/BindsInstanceProcessingStep.java index fc7fc71ecee..055e6acea9d 100644 --- a/java/dagger/internal/codegen/BindsInstanceProcessingStep.java +++ b/java/dagger/internal/codegen/BindsInstanceProcessingStep.java @@ -45,6 +45,7 @@ final class BindsInstanceProcessingStep extends TypeCheckingProcessingStep> COMPONENT_ANNOTATIONS = Stream.of(ComponentDescriptor.Kind.values()) + .filter(kind -> !kind.isForModuleValidation()) .map(ComponentDescriptor.Kind::annotationType) .collect(toImmutableSet()); private static final ImmutableSet> MODULE_ANNOTATIONS = diff --git a/java/dagger/internal/codegen/BuilderValidator.java b/java/dagger/internal/codegen/BuilderValidator.java index 543cd5691dd..f69637e2539 100644 --- a/java/dagger/internal/codegen/BuilderValidator.java +++ b/java/dagger/internal/codegen/BuilderValidator.java @@ -68,7 +68,7 @@ public ValidationReport validate(TypeElement subject) { Element componentElement = subject.getEnclosingElement(); ErrorMessages.ComponentBuilderMessages msgs = ErrorMessages.builderMsgsFor(componentKind); Class componentAnnotation = componentKind.annotationType(); - Class builderAnnotation = componentKind.builderAnnotationType(); + Class builderAnnotation = componentKind.builderAnnotationType().get(); checkArgument(subject.getAnnotation(builderAnnotation) != null); if (!isAnnotationPresent(componentElement, componentAnnotation)) { diff --git a/java/dagger/internal/codegen/CompilerOptions.java b/java/dagger/internal/codegen/CompilerOptions.java index 95512d359c0..5c2647571ef 100644 --- a/java/dagger/internal/codegen/CompilerOptions.java +++ b/java/dagger/internal/codegen/CompilerOptions.java @@ -25,6 +25,7 @@ import static dagger.internal.codegen.FeatureStatus.DISABLED; import static dagger.internal.codegen.FeatureStatus.ENABLED; import static dagger.internal.codegen.ValidationType.ERROR; +import static dagger.internal.codegen.ValidationType.NONE; import static dagger.internal.codegen.ValidationType.WARNING; import static java.util.EnumSet.allOf; @@ -104,6 +105,10 @@ boolean doCheckForNulls() { abstract boolean useGradleIncrementalProcessing(); + abstract ValidationType moduleBindingValidationType(); + + abstract Diagnostic.Kind moduleHasDifferentScopesDiagnosticKind(); + static Builder builder() { return new AutoValue_CompilerOptions.Builder() .headerCompilation(false) @@ -158,6 +163,10 @@ Builder warnIfInjectionFactoryNotGeneratedUpstream( Builder useGradleIncrementalProcessing(boolean enabled); + Builder moduleBindingValidationType(ValidationType validationType); + + Builder moduleHasDifferentScopesDiagnosticKind(Diagnostic.Kind kind); + @CheckReturnValue CompilerOptions build(); } @@ -296,6 +305,16 @@ private enum Validation implements Option { PRIVATE_MEMBER_VALIDATION(kindSetter(Builder::privateMemberValidationKind), ERROR, WARNING), STATIC_MEMBER_VALIDATION(kindSetter(Builder::staticMemberValidationKind), ERROR, WARNING), + + /** Whether to validate partial binding graphs associated with modules. */ + MODULE_BINDING_VALIDATION(Builder::moduleBindingValidationType, NONE, ERROR, WARNING), + + /** + * How to report conflicting scoped bindings when validating partial binding graphs associated + * with modules. + */ + MODULE_HAS_DIFFERENT_SCOPES_VALIDATION( + kindSetter(Builder::moduleHasDifferentScopesDiagnosticKind), ERROR, WARNING), ; static BiConsumer kindSetter( @@ -304,20 +323,21 @@ static BiConsumer kindSetter( setter.accept(builder, validationType.diagnosticKind().get()); } + final ValidationType defaultType; final ImmutableSet validTypes; final BiConsumer setter; Validation(BiConsumer setter) { - this.setter = setter; - this.validTypes = immutableEnumSet(allOf(ValidationType.class)); + this(setter, ERROR, WARNING, NONE); } Validation( BiConsumer setter, - ValidationType validType, + ValidationType defaultType, ValidationType... moreValidTypes) { this.setter = setter; - this.validTypes = immutableEnumSet(validType, moreValidTypes); + this.defaultType = defaultType; + this.validTypes = immutableEnumSet(defaultType, moreValidTypes); } @Override @@ -326,7 +346,7 @@ public void set(Builder builder, ProcessingEnvironment processingEnvironment) { } ValidationType validationType(ProcessingEnvironment processingEnvironment) { - return CompilerOptions.valueOf(processingEnvironment, toString(), ERROR, validTypes); + return CompilerOptions.valueOf(processingEnvironment, toString(), defaultType, validTypes); } @Override diff --git a/java/dagger/internal/codegen/ComponentDescriptor.java b/java/dagger/internal/codegen/ComponentDescriptor.java index 2ba801b9036..6eecaa6996c 100644 --- a/java/dagger/internal/codegen/ComponentDescriptor.java +++ b/java/dagger/internal/codegen/ComponentDescriptor.java @@ -22,6 +22,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Verify.verify; import static com.google.common.collect.Iterables.getOnlyElement; +import static com.google.common.collect.Sets.immutableEnumSet; import static dagger.internal.codegen.ConfigurationAnnotations.enclosedBuilders; import static dagger.internal.codegen.ConfigurationAnnotations.getComponentDependencies; import static dagger.internal.codegen.ConfigurationAnnotations.getComponentModules; @@ -34,6 +35,7 @@ import static dagger.internal.codegen.InjectionAnnotations.getQualifier; import static dagger.internal.codegen.Scopes.productionScope; import static dagger.internal.codegen.Scopes.scopesOf; +import static java.util.EnumSet.allOf; import static javax.lang.model.element.Modifier.ABSTRACT; import static javax.lang.model.type.TypeKind.DECLARED; import static javax.lang.model.type.TypeKind.VOID; @@ -48,20 +50,20 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; import dagger.BindsInstance; import dagger.Component; import dagger.Lazy; import dagger.Module; import dagger.Subcomponent; import dagger.model.DependencyRequest; +import dagger.model.RequestKind; import dagger.model.Scope; import dagger.producers.CancellationPolicy; +import dagger.producers.ProducerModule; import dagger.producers.ProductionComponent; import dagger.producers.ProductionSubcomponent; import java.lang.annotation.Annotation; import java.util.EnumSet; -import java.util.LinkedHashSet; import java.util.List; import java.util.Optional; import java.util.Set; @@ -78,7 +80,14 @@ import javax.lang.model.util.Types; /** - * The logical representation of a {@link Component} or {@link ProductionComponent} definition. + * A component declaration. + * + *

Represents one type annotated with {@code @Component}, {@code Subcomponent}, + * {@code @ProductionComponent}, or {@code @ProductionSubcomponent}. + * + *

When validating bindings installed in modules, a {@link ComponentDescriptor} can also + * represent a synthetic component for the module, where there is an entry point for each binding in + * the module. */ @AutoValue abstract class ComponentDescriptor { @@ -87,10 +96,23 @@ enum Kind { SUBCOMPONENT(Subcomponent.class, Subcomponent.Builder.class, false), PRODUCTION_COMPONENT(ProductionComponent.class, ProductionComponent.Builder.class, true), PRODUCTION_SUBCOMPONENT( - ProductionSubcomponent.class, ProductionSubcomponent.Builder.class, false); + ProductionSubcomponent.class, ProductionSubcomponent.Builder.class, false), + + /** + * This descriptor was generated from a {@link Module} instead of a component type in order to + * validate the module's bindings. + */ + MODULE(Module.class, Optional.empty(), true), + + /** + * This descriptor was generated from a {@link ProducerModule} instead of a component type in + * order to validate the module's bindings. + */ + PRODUCER_MODULE(ProducerModule.class, Optional.empty(), true), + ; private final Class annotationType; - private final Class builderType; + private final Optional> builderType; private final boolean isTopLevel; /** @@ -122,7 +144,9 @@ static Optional forAnnotatedElement(TypeElement element) { static Optional forAnnotatedBuilderElement(TypeElement element) { Set kinds = EnumSet.noneOf(Kind.class); for (Kind kind : values()) { - if (isAnnotationPresent(element, kind.builderAnnotationType())) { + if (kind.builderAnnotationType() + .filter(builderAnnotation -> isAnnotationPresent(element, builderAnnotation)) + .isPresent()) { kinds.add(kind); } } @@ -135,6 +159,13 @@ static Optional forAnnotatedBuilderElement(TypeElement element) { Class annotationType, Class builderType, boolean isTopLevel) { + this(annotationType, Optional.of(builderType), isTopLevel); + } + + Kind( + Class annotationType, + Optional> builderType, + boolean isTopLevel) { this.annotationType = annotationType; this.builderType = builderType; this.isTopLevel = isTopLevel; @@ -144,52 +175,65 @@ Class annotationType() { return annotationType; } - Class builderAnnotationType() { + /** + * Returns the {@code @Builder} annotation type for this kind of component, or empty if the + * descriptor is {@linkplain #isForModuleValidation() for a module} in order to validate its + * bindings. + */ + Optional> builderAnnotationType() { return builderType; } ImmutableSet moduleKinds() { - switch (this) { - case COMPONENT: - case SUBCOMPONENT: - return Sets.immutableEnumSet(ModuleDescriptor.Kind.MODULE); - case PRODUCTION_COMPONENT: - case PRODUCTION_SUBCOMPONENT: - return Sets.immutableEnumSet( - ModuleDescriptor.Kind.MODULE, ModuleDescriptor.Kind.PRODUCER_MODULE); - default: - throw new AssertionError(this); - } + return isProducer() + ? immutableEnumSet(allOf(ModuleDescriptor.Kind.class)) + : immutableEnumSet(ModuleDescriptor.Kind.MODULE); } ImmutableSet subcomponentKinds() { - switch (this) { - case COMPONENT: - case SUBCOMPONENT: - return ImmutableSet.of(SUBCOMPONENT, PRODUCTION_SUBCOMPONENT); - case PRODUCTION_COMPONENT: - case PRODUCTION_SUBCOMPONENT: - return ImmutableSet.of(PRODUCTION_SUBCOMPONENT); - default: - throw new AssertionError(); - } + return isProducer() + ? immutableEnumSet(PRODUCTION_SUBCOMPONENT) + : immutableEnumSet(SUBCOMPONENT, PRODUCTION_SUBCOMPONENT); } + /** + * Returns {@code true} if the descriptor is for a top-level (not a child) component or is for + * {@linkplain #isForModuleValidation() module-validation}. + */ boolean isTopLevel() { return isTopLevel; } + /** Returns {@code true} if the descriptor is for a production component or module. */ boolean isProducer() { switch (this) { case COMPONENT: case SUBCOMPONENT: + case MODULE: return false; + case PRODUCTION_COMPONENT: case PRODUCTION_SUBCOMPONENT: + case PRODUCER_MODULE: return true; - default: - throw new AssertionError(); } + throw new AssertionError(this); + } + + /** Returns {@code true} if the descriptor is for a module in order to validate its bindings. */ + boolean isForModuleValidation() { + switch (this) { + case MODULE: + case PRODUCER_MODULE: + return true; + + case COMPONENT: + case SUBCOMPONENT: + case PRODUCTION_COMPONENT: + case PRODUCTION_SUBCOMPONENT: + return false; + } + throw new AssertionError(this); } } @@ -339,12 +383,39 @@ final ImmutableSet entryPointMethods() { .collect(toImmutableSet()); } - /** The entry point dependency requests on the component type. */ + /** + * The entry points. + * + *

For descriptors that are generated from a module in order to validate the module's bindings, + * these will be requests for every key for every binding declared in the module (erasing + * multibinding contribution identifiers so that we get the multibinding key). + * + *

In order not to trigger a validation error if the requested binding is nullable, each + * request will be nullable. + * + *

In order not to trigger a validation error if the requested binding is a production binding, + * each request will be for a {@link com.google.common.util.concurrent.ListenableFuture} of the + * key type. + */ final ImmutableSet entryPoints() { - return entryPointMethods() - .stream() - .map(method -> method.dependencyRequest().get()) - .collect(toImmutableSet()); + if (kind().isForModuleValidation()) { + return modules().stream() + .flatMap(module -> module.allBindingKeys().stream()) + .map(key -> key.toBuilder().multibindingContributionIdentifier(Optional.empty()).build()) + .map( + key -> + DependencyRequest.builder() + .key(key) + // TODO(dpb): Futures only in ProducerModules, instances elsewhere? + .kind(RequestKind.FUTURE) + .isNullable(true) + .build()) + .collect(toImmutableSet()); + } else { + return entryPointMethods().stream() + .map(method -> method.dependencyRequest().get()) + .collect(toImmutableSet()); + } } // TODO(gak): Consider making this non-optional and revising the @@ -475,31 +546,31 @@ static final class Factory { } /** - * Returns a component descriptor for a type annotated with either {@link Component @Component} - * or {@link ProductionComponent @ProductionComponent}. This is also compatible with {@link - * Subcomponent @Subcomponent} or {@link ProductionSubcomponent @ProductionSubcomponent} when - * generating ahead-of-time subcomponents. + * Returns a component descriptor for a type. + * + *

The type must be annotated with a top-level component annotation unless ahead-of-time + * subcomponents are being generated or we are creating a descriptor for a module in order to + * validate its bindings. */ - ComponentDescriptor forComponent(TypeElement componentType) { - Optional kind = Kind.forAnnotatedElement(componentType); + ComponentDescriptor forTypeElement(TypeElement typeElement) { + Optional kind = Kind.forAnnotatedElement(typeElement); checkArgument( - kind.isPresent(), "%s must have a component or subcomponent annotation", componentType); + kind.isPresent(), + "%s must have a component or subcomponent or module annotation", + typeElement); if (!compilerOptions.aheadOfTimeSubcomponents()) { - checkArgument(kind.get().isTopLevel(), - "%s must be annotated with @Component or @ProductionComponent.", - componentType); + checkArgument(kind.get().isTopLevel(), "%s must be a top-level component.", typeElement); } - return create(componentType, kind.get()); + return create(typeElement, kind.get()); } - private ComponentDescriptor create(TypeElement componentDefinitionType, Kind kind) { - AnnotationMirror componentMirror = - getAnnotationMirror(componentDefinitionType, kind.annotationType()).get(); - DeclaredType declaredComponentType = MoreTypes.asDeclared(componentDefinitionType.asType()); + private ComponentDescriptor create(TypeElement typeElement, Kind kind) { + AnnotationMirror componentAnnotation = + getAnnotationMirror(typeElement, kind.annotationType()).get(); + DeclaredType declaredComponentType = MoreTypes.asDeclared(typeElement.asType()); ImmutableSet componentDependencies = - kind.isTopLevel() - ? getComponentDependencies(componentMirror) - .stream() + kind.isTopLevel() && !kind.isForModuleValidation() + ? getComponentDependencies(componentAnnotation).stream() .map(ComponentRequirement::forDependency) .collect(toImmutableSet()) : ImmutableSet.of(); @@ -516,12 +587,16 @@ private ComponentDescriptor create(TypeElement componentDefinitionType, Kind kin } } - ImmutableSet modules = - getComponentModules(componentMirror).stream() - .map(moduleType -> moduleDescriptorFactory.create(asTypeElement(moduleType))) - .collect(toImmutableSet()); + ImmutableSet modules = + kind.isForModuleValidation() + ? ImmutableSet.of(typeElement) + : getComponentModules(componentAnnotation).stream() + .map(MoreTypes::asTypeElement) + .collect(toImmutableSet()); + + ImmutableSet transitiveModules = + moduleDescriptorFactory.transitiveModules(modules); - ImmutableSet transitiveModules = transitiveModules(modules); ImmutableSet.Builder subcomponentsFromModules = ImmutableSet.builder(); for (ModuleDescriptor module : transitiveModules) { for (SubcomponentDeclaration subcomponentDeclaration : module.subcomponentDeclarations()) { @@ -530,51 +605,57 @@ private ComponentDescriptor create(TypeElement componentDefinitionType, Kind kin create(subcomponent, Kind.forAnnotatedElement(subcomponent).get())); } } - ImmutableSet unimplementedMethods = - elements.getUnimplementedMethods(componentDefinitionType); ImmutableSet.Builder componentMethodsBuilder = ImmutableSet.builder(); - ImmutableBiMap.Builder subcomponentsByFactoryMethod = ImmutableBiMap.builder(); ImmutableBiMap.Builder subcomponentsByBuilderMethod = ImmutableBiMap.builder(); - for (ExecutableElement componentMethod : unimplementedMethods) { - ExecutableType resolvedMethod = - MoreTypes.asExecutable(types.asMemberOf(declaredComponentType, componentMethod)); - ComponentMethodDescriptor componentMethodDescriptor = - getDescriptorForComponentMethod(componentDefinitionType, kind, componentMethod); - componentMethodsBuilder.add(componentMethodDescriptor); - switch (componentMethodDescriptor.kind()) { - case SUBCOMPONENT: - case PRODUCTION_SUBCOMPONENT: - subcomponentsByFactoryMethod.put( - componentMethodDescriptor, - create( - MoreElements.asType(MoreTypes.asElement(resolvedMethod.getReturnType())), - componentMethodDescriptor.kind().componentKind())); - break; - case SUBCOMPONENT_BUILDER: - case PRODUCTION_SUBCOMPONENT_BUILDER: - subcomponentsByBuilderMethod.put( - componentMethodDescriptor, - create( - MoreElements.asType( - MoreTypes.asElement(resolvedMethod.getReturnType()).getEnclosingElement()), - componentMethodDescriptor.kind().componentKind())); - break; - default: // nothing special to do for other methods. + if (!kind.isForModuleValidation()) { + ImmutableSet unimplementedMethods = + elements.getUnimplementedMethods(typeElement); + for (ExecutableElement componentMethod : unimplementedMethods) { + ExecutableType resolvedMethod = + MoreTypes.asExecutable(types.asMemberOf(declaredComponentType, componentMethod)); + ComponentMethodDescriptor componentMethodDescriptor = + getDescriptorForComponentMethod(typeElement, kind, componentMethod); + componentMethodsBuilder.add(componentMethodDescriptor); + switch (componentMethodDescriptor.kind()) { + case SUBCOMPONENT: + case PRODUCTION_SUBCOMPONENT: + subcomponentsByFactoryMethod.put( + componentMethodDescriptor, + create( + MoreElements.asType(MoreTypes.asElement(resolvedMethod.getReturnType())), + componentMethodDescriptor.kind().componentKind())); + break; + + case SUBCOMPONENT_BUILDER: + case PRODUCTION_SUBCOMPONENT_BUILDER: + subcomponentsByBuilderMethod.put( + componentMethodDescriptor, + create( + MoreElements.asType( + MoreTypes.asElement(resolvedMethod.getReturnType()) + .getEnclosingElement()), + componentMethodDescriptor.kind().componentKind())); + break; + + default: // nothing special to do for other methods. + } } } ImmutableList enclosedBuilders = - enclosedBuilders(componentDefinitionType, kind.builderAnnotationType()); + kind.builderAnnotationType() + .map(builderAnnotationType -> enclosedBuilders(typeElement, builderAnnotationType)) + .orElse(ImmutableList.of()); Optional builderType = Optional.ofNullable(getOnlyElement(enclosedBuilders, null)); Optional builderSpec = createBuilderSpec(builderType); - ImmutableSet scopes = scopesOf(componentDefinitionType); + ImmutableSet scopes = scopesOf(typeElement); if (kind.isProducer()) { scopes = ImmutableSet.builder().addAll(scopes).add(productionScope(elements)).build(); @@ -582,8 +663,8 @@ private ComponentDescriptor create(TypeElement componentDefinitionType, Kind kin return new AutoValue_ComponentDescriptor( kind, - componentMirror, - componentDefinitionType, + componentAnnotation, + typeElement, componentDependencies, transitiveModules, dependenciesByDependencyMethod.build(), @@ -706,24 +787,6 @@ private ComponentRequirement requirementForBuilderMethod( ? ComponentRequirement.forModule(type) : ComponentRequirement.forDependency(type); } - - private ImmutableSet transitiveModules( - Iterable topLevelModules) { - Set transitiveModules = new LinkedHashSet<>(); - for (ModuleDescriptor module : topLevelModules) { - addTransitiveModules(transitiveModules, module); - } - return ImmutableSet.copyOf(transitiveModules); - } - - private void addTransitiveModules( - Set transitiveModules, ModuleDescriptor module) { - if (transitiveModules.add(module)) { - for (TypeElement includedModule : module.includedModules()) { - addTransitiveModules(transitiveModules, moduleDescriptorFactory.create(includedModule)); - } - } - } } /** diff --git a/java/dagger/internal/codegen/ComponentHjarProcessingStep.java b/java/dagger/internal/codegen/ComponentHjarProcessingStep.java index 6b07406b262..6685cc46010 100644 --- a/java/dagger/internal/codegen/ComponentHjarProcessingStep.java +++ b/java/dagger/internal/codegen/ComponentHjarProcessingStep.java @@ -116,7 +116,7 @@ protected void process( validationReport.report().printMessagesTo(messager); if (validationReport.report().isClean()) { new EmptyComponentGenerator(filer, elements, sourceVersion) - .generate(componentDescriptorFactory.forComponent(componentTypeElement), messager); + .generate(componentDescriptorFactory.forTypeElement(componentTypeElement), messager); } } diff --git a/java/dagger/internal/codegen/ComponentProcessingStep.java b/java/dagger/internal/codegen/ComponentProcessingStep.java index 2b9e28cf660..47d140bfabb 100644 --- a/java/dagger/internal/codegen/ComponentProcessingStep.java +++ b/java/dagger/internal/codegen/ComponentProcessingStep.java @@ -133,7 +133,7 @@ protected void process( if (!isClean(validationReport)) { return; } - ComponentDescriptor componentDescriptor = componentDescriptorFactory.forComponent(element); + ComponentDescriptor componentDescriptor = componentDescriptorFactory.forTypeElement(element); ValidationReport componentDescriptorReport = componentDescriptorValidator.validate(componentDescriptor); componentDescriptorReport.printMessagesTo(messager); @@ -151,7 +151,7 @@ protected void process( if (!subcomponentIsClean(element)) { return; } - ComponentDescriptor componentDescriptor = componentDescriptorFactory.forComponent(element); + ComponentDescriptor componentDescriptor = componentDescriptorFactory.forTypeElement(element); BindingGraph bindingGraph = bindingGraphFactory.create(componentDescriptor); // TODO(b/72748365): Do subgraph validation. generateComponent(bindingGraph); diff --git a/java/dagger/internal/codegen/ComponentValidator.java b/java/dagger/internal/codegen/ComponentValidator.java index f4a779aaf8a..c904bf7a48f 100644 --- a/java/dagger/internal/codegen/ComponentValidator.java +++ b/java/dagger/internal/codegen/ComponentValidator.java @@ -30,6 +30,7 @@ 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.presentValues; import static dagger.internal.codegen.DaggerStreams.toImmutableSet; import static java.util.Comparator.comparing; import static javax.lang.model.element.ElementKind.CLASS; @@ -40,7 +41,6 @@ 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; @@ -52,6 +52,7 @@ import dagger.Component; import dagger.Reusable; import dagger.internal.codegen.ComponentDescriptor.Kind; +import dagger.internal.codegen.ErrorMessages.SubcomponentBuilderMessages; import dagger.model.DependencyRequest; import dagger.model.Key; import dagger.producers.CancellationPolicy; @@ -144,7 +145,10 @@ public ComponentValidationReport validate( } ImmutableList builders = - enclosedBuilders(subject, componentKind.builderAnnotationType()); + componentKind + .builderAnnotationType() + .map(builderAnnotationType -> enclosedBuilders(subject, builderAnnotationType)) + .orElse(ImmutableList.of()); if (builders.size() > 1) { report.addError( String.format(ErrorMessages.builderMsgsFor(componentKind).moreThanOne(), builders), @@ -180,15 +184,16 @@ public ComponentValidationReport validate( Optional subcomponentAnnotation = checkForAnnotations( returnType, - FluentIterable.from(componentKind.subcomponentKinds()) - .transform(Kind::annotationType) - .toSet()); + componentKind.subcomponentKinds().stream() + .map(Kind::annotationType) + .collect(toImmutableSet())); Optional subcomponentBuilderAnnotation = checkForAnnotations( returnType, - FluentIterable.from(componentKind.subcomponentKinds()) - .transform(Kind::builderAnnotationType) - .toSet()); + componentKind.subcomponentKinds().stream() + .map(kind -> kind.builderAnnotationType()) + .flatMap(presentValues()) + .collect(toImmutableSet())); if (subcomponentAnnotation.isPresent()) { referencedSubcomponents.put(MoreTypes.asElement(returnType), method); validateSubcomponentMethod( @@ -245,8 +250,7 @@ public ComponentValidationReport validate( (subcomponent, methods) -> report.addError( String.format( - ErrorMessages.SubcomponentBuilderMessages.INSTANCE - .moreThanOneRefToSubcomponent(), + SubcomponentBuilderMessages.INSTANCE.moreThanOneRefToSubcomponent(), subcomponent, methods), subject)); @@ -440,8 +444,7 @@ private void validateSubcomponentBuilderMethod( Set validatedSubcomponentBuilders) { if (!parameters.isEmpty()) { - report.addError( - ErrorMessages.SubcomponentBuilderMessages.INSTANCE.builderMethodRequiresNoArgs(), method); + report.addError(SubcomponentBuilderMessages.INSTANCE.builderMethodRequiresNoArgs(), method); } // If we haven't already validated the subcomponent builder itself, validate it now. diff --git a/java/dagger/internal/codegen/ConfigurationAnnotations.java b/java/dagger/internal/codegen/ConfigurationAnnotations.java index 48b31651c04..dbfcdbc3206 100644 --- a/java/dagger/internal/codegen/ConfigurationAnnotations.java +++ b/java/dagger/internal/codegen/ConfigurationAnnotations.java @@ -102,7 +102,9 @@ static boolean isSubcomponentBuilder(Element element) { */ static ImmutableList getModules( TypeElement annotatedType, AnnotationMirror annotation) { - if (ComponentDescriptor.Kind.forAnnotatedElement(annotatedType).isPresent()) { + if (ComponentDescriptor.Kind.forAnnotatedElement(annotatedType) + .filter(kind -> !kind.isForModuleValidation()) + .isPresent()) { return asAnnotationValues(getAnnotationValue(annotation, MODULES_ATTRIBUTE)); } if (ModuleDescriptor.Kind.forAnnotatedElement(annotatedType).isPresent()) { diff --git a/java/dagger/internal/codegen/DependsOnProductionExecutorValidator.java b/java/dagger/internal/codegen/DependsOnProductionExecutorValidator.java index 0717406b211..e611d22fb11 100644 --- a/java/dagger/internal/codegen/DependsOnProductionExecutorValidator.java +++ b/java/dagger/internal/codegen/DependsOnProductionExecutorValidator.java @@ -16,9 +16,11 @@ package dagger.internal.codegen; +import static dagger.internal.codegen.DaggerStreams.instancesOf; import static javax.tools.Diagnostic.Kind.ERROR; import dagger.model.BindingGraph; +import dagger.model.BindingGraph.MaybeBinding; import dagger.model.Key; import dagger.spi.BindingGraphPlugin; import dagger.spi.DiagnosticReporter; @@ -52,7 +54,9 @@ public void visitGraph(BindingGraph bindingGraph, DiagnosticReporter diagnosticR Key productionImplementationExecutorKey = keyFactory.forProductionImplementationExecutor(); Key productionExecutorKey = keyFactory.forProductionExecutor(); - bindingGraph.bindings(productionExecutorKey).stream() + bindingGraph.network().nodes().stream() + .flatMap(instancesOf(MaybeBinding.class)) + .filter(node -> node.key().equals(productionExecutorKey)) .flatMap(productionExecutor -> bindingGraph.requestingBindings(productionExecutor).stream()) .filter(binding -> !binding.key().equals(productionImplementationExecutorKey)) .forEach(binding -> reportError(diagnosticReporter, binding)); diff --git a/java/dagger/internal/codegen/DiagnosticReporterFactory.java b/java/dagger/internal/codegen/DiagnosticReporterFactory.java index cf94e5370f8..4625a9f833a 100644 --- a/java/dagger/internal/codegen/DiagnosticReporterFactory.java +++ b/java/dagger/internal/codegen/DiagnosticReporterFactory.java @@ -19,11 +19,11 @@ import static com.google.auto.common.MoreTypes.asTypeElement; import static com.google.common.base.Predicates.equalTo; import static com.google.common.base.Verify.verify; +import static com.google.common.collect.Iterables.filter; import static com.google.common.collect.Iterables.getLast; import static com.google.common.collect.Iterables.indexOf; import static com.google.common.collect.Iterables.transform; import static com.google.common.collect.Lists.asList; -import static com.google.common.collect.Sets.filter; import static dagger.internal.codegen.DaggerElements.DECLARATION_ORDER; import static dagger.internal.codegen.DaggerElements.closestEnclosingTypeElement; import static dagger.internal.codegen.DaggerElements.elementEncloses; @@ -34,6 +34,7 @@ import static java.util.Collections.min; import static java.util.Comparator.comparing; import static java.util.Comparator.comparingInt; +import static javax.tools.Diagnostic.Kind.ERROR; import com.google.auto.common.MoreElements; import com.google.common.cache.CacheBuilder; @@ -72,14 +73,41 @@ final class DiagnosticReporterFactory { private final DaggerTypes types; private final Messager messager; private final DependencyRequestFormatter dependencyRequestFormatter; + private final ValidationType validationType; + private final boolean printingEntryPoints; @Inject DiagnosticReporterFactory( DaggerTypes types, Messager messager, DependencyRequestFormatter dependencyRequestFormatter) { + this(types, messager, dependencyRequestFormatter, ValidationType.ERROR, true); + } + + private DiagnosticReporterFactory( + DaggerTypes types, + Messager messager, + DependencyRequestFormatter dependencyRequestFormatter, + ValidationType validationType, + boolean printingEntryPoints) { this.types = types; this.messager = messager; this.dependencyRequestFormatter = dependencyRequestFormatter; + this.validationType = validationType; + this.printingEntryPoints = printingEntryPoints; + } + + /** Returns a factory that treats all reported errors as some other kind instead. */ + DiagnosticReporterFactory treatingErrorsAs(ValidationType validationType) { + if (validationType.equals(this.validationType)) { + return this; + } + return new DiagnosticReporterFactory( + types, messager, dependencyRequestFormatter, validationType, printingEntryPoints); + } + /** Returns a factory that does not print dependency traces from entry points to the error. */ + DiagnosticReporterFactory withoutPrintingEntryPoints() { + return new DiagnosticReporterFactory( + types, messager, dependencyRequestFormatter, validationType, false); } /** Creates a reporter for a binding graph and a plugin. */ @@ -143,6 +171,7 @@ public void reportComponent( Diagnostic.Kind diagnosticKind, ComponentNode componentNode, String messageFormat) { StringBuilder message = new StringBuilder(messageFormat); appendComponentPathUnlessAtRoot(message, componentNode); + // TODO(dpb): Report at the component node component. printMessage(diagnosticKind, message, rootComponent); } @@ -221,15 +250,23 @@ private Node source(Edge edge) { void printMessage( Diagnostic.Kind diagnosticKind, CharSequence message, Element elementToReport) { + if (diagnosticKind.equals(ERROR)) { + if (!validationType.diagnosticKind().isPresent()) { + return; + } + diagnosticKind = validationType.diagnosticKind().get(); + } reportedDiagnosticKinds.add(diagnosticKind); StringBuilder fullMessage = new StringBuilder(); appendBracketPrefix(fullMessage, plugin); + // TODO(ronshapiro): should we create a HashSet out of elementEncloses() so we don't // need to do an O(n) contains() each time? if (!elementEncloses(rootComponent, elementToReport)) { appendBracketPrefix(fullMessage, elementToString(elementToReport)); elementToReport = rootComponent; } + messager.printMessage(diagnosticKind, fullMessage.append(message), elementToReport); } @@ -274,30 +311,44 @@ private final class DiagnosticInfo { @Override public String toString() { StringBuilder message = - new StringBuilder(dependencyTrace.size() * 100 /* a guess heuristic */); - - // Print the dependency trace. - dependencyTrace.forEach( - edge -> dependencyRequestFormatter.appendFormatLine(message, edge.dependencyRequest())); - appendComponentPathUnlessAtRoot(message, source(getLast(dependencyTrace))); + printingEntryPoints + ? new StringBuilder(dependencyTrace.size() * 100 /* a guess heuristic */) + : new StringBuilder(); + + // Print the dependency trace if we're printing entry points + if (printingEntryPoints) { + dependencyTrace.forEach( + edge -> + dependencyRequestFormatter.appendFormatLine(message, edge.dependencyRequest())); + appendComponentPathUnlessAtRoot(message, source(getLast(dependencyTrace))); + } - // List any other dependency requests. - ImmutableSet otherRequests = + // Print any dependency requests that aren't shown as part of the dependency trace. + ImmutableSet requestsToPrint = requests.stream() - .filter(request -> !request.isEntryPoint()) // skip entry points, listed below - // skip the request from the dependency trace above - .filter(request -> !request.equals(dependencyTrace.get(0))) + .filter( + // if printing entry points, skip them and the request at the head of the + // dependency trace here. + printingEntryPoints + ? request -> + !request.isEntryPoint() && !request.equals(dependencyTrace.get(0)) + : request -> true) + .filter(request -> request.dependencyRequest().requestElement().isPresent()) .map(request -> request.dependencyRequest().requestElement().get()) .collect(toImmutableSet()); - if (!otherRequests.isEmpty()) { - message.append("\nIt is also requested at:"); - for (Element otherRequest : otherRequests) { - message.append("\n ").append(elementToString(otherRequest)); + if (!requestsToPrint.isEmpty()) { + message + .append("\nIt is") + .append(printingEntryPoints ? " also " : " ") + .append("requested at:"); + for (Element request : requestsToPrint) { + message.append("\n ").append(elementToString(request)); } } - // List the remaining entry points, showing which component they're in. - if (entryPoints.size() > 1) { + // Print the remaining entry points, showing which component they're in, if we're printing + // entry points. + if (printingEntryPoints && entryPoints.size() > 1) { message.append("\nThe following other entry points also depend on it:"); entryPoints.stream() .filter(entryPoint -> !entryPoint.equals(getLast(dependencyTrace))) diff --git a/java/dagger/internal/codegen/IncompatiblyScopedBindingsValidator.java b/java/dagger/internal/codegen/IncompatiblyScopedBindingsValidator.java index 4dfb49ec92e..9eaddddbc5d 100644 --- a/java/dagger/internal/codegen/IncompatiblyScopedBindingsValidator.java +++ b/java/dagger/internal/codegen/IncompatiblyScopedBindingsValidator.java @@ -19,18 +19,22 @@ import static dagger.internal.codegen.DaggerElements.closestEnclosingTypeElement; import static dagger.internal.codegen.Formatter.INDENT; import static dagger.internal.codegen.Scopes.getReadableSource; +import static dagger.model.BindingKind.INJECTION; import static java.util.stream.Collectors.joining; import static javax.tools.Diagnostic.Kind.ERROR; import com.google.auto.common.MoreElements; import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.Multimaps; +import dagger.model.Binding; import dagger.model.BindingGraph; import dagger.model.BindingGraph.ComponentNode; import dagger.spi.BindingGraphPlugin; import dagger.spi.DiagnosticReporter; +import java.util.Optional; import java.util.Set; import javax.inject.Inject; +import javax.tools.Diagnostic; /** * Reports an error for any component that uses bindings with scopes that are not assigned to the @@ -39,10 +43,13 @@ final class IncompatiblyScopedBindingsValidator implements BindingGraphPlugin { private final MethodSignatureFormatter methodSignatureFormatter; + private final CompilerOptions compilerOptions; @Inject - IncompatiblyScopedBindingsValidator(MethodSignatureFormatter methodSignatureFormatter) { + IncompatiblyScopedBindingsValidator( + MethodSignatureFormatter methodSignatureFormatter, CompilerOptions compilerOptions) { this.methodSignatureFormatter = methodSignatureFormatter; + this.compilerOptions = compilerOptions; } @Override @@ -57,14 +64,19 @@ public void visitGraph(BindingGraph bindingGraph, DiagnosticReporter diagnosticR for (dagger.model.Binding binding : bindingGraph.bindings()) { binding .scope() + .filter(scope -> !scope.isReusable()) .ifPresent( scope -> { - if (scope.isReusable()) { - return; - } ComponentNode componentNode = bindingGraph.componentNode(binding.componentPath()).get(); - if (!componentNode.scopes().contains(scope)) { + if (bindingGraph.isModuleBindingGraph() && componentNode.componentPath().atRoot()) { + // @Inject bindings in the root "component" for module binding graphs will appear + // at the properly scoped ancestor component of the component that installs the + // module, so ignore them here. + if (!binding.kind().equals(INJECTION)) { + incompatibleBindings.put(componentNode, binding); + } + } else if (!componentNode.scopes().contains(scope)) { incompatibleBindings.put(componentNode, binding); } }); @@ -72,27 +84,42 @@ public void visitGraph(BindingGraph bindingGraph, DiagnosticReporter diagnosticR Multimaps.asMap(incompatibleBindings.build()) .forEach( (componentNode, bindings) -> - diagnosticReporter.reportComponent( - ERROR, componentNode, incompatibleBindingScopesError(componentNode, bindings))); + report(componentNode, bindings, bindingGraph, diagnosticReporter)); } - private String incompatibleBindingScopesError( - ComponentNode componentNode, Set bindings) { + private void report( + ComponentNode componentNode, + Set bindings, + BindingGraph bindingGraph, + DiagnosticReporter diagnosticReporter) { + Diagnostic.Kind diagnosticKind = ERROR; StringBuilder message = new StringBuilder(componentNode.componentPath().currentComponent().getQualifiedName()); - if (!componentNode.scopes().isEmpty()) { + + if (bindingGraph.isModuleBindingGraph() && componentNode.componentPath().atRoot()) { + // The root "component" of a module binding graph is a module, which will have no scopes + // attached. We want to report if there is more than one scope in that component. + if (bindings.stream().map(Binding::scope).map(Optional::get).distinct().count() <= 1) { + return; + } + message.append(" contains bindings with different scopes:"); + diagnosticKind = compilerOptions.moduleHasDifferentScopesDiagnosticKind(); + } else if (componentNode.scopes().isEmpty()) { + message.append(" (unscoped) may not reference scoped bindings:"); + } else { message .append(" scoped with ") .append( componentNode.scopes().stream().map(Scopes::getReadableSource).collect(joining(" "))) - .append(" may not reference bindings with different scopes:\n"); - } else { - message.append(" (unscoped) may not reference scoped bindings:\n"); + .append(" may not reference bindings with different scopes:"); } + // TODO(ronshapiro): Should we group by scope? - for (dagger.model.Binding binding : bindings) { - message.append(INDENT); + for (Binding binding : bindings) { + message.append('\n').append(INDENT); + // TODO(dpb): Use BindingDeclarationFormatter. + // But that doesn't print scopes for @Inject-constructed types. switch (binding.kind()) { case DELEGATE: case PROVISION: @@ -112,9 +139,7 @@ private String incompatibleBindingScopesError( default: throw new AssertionError(binding); } - - message.append("\n"); } - return message.toString(); + diagnosticReporter.reportComponent(diagnosticKind, componentNode, message.toString()); } } diff --git a/java/dagger/internal/codegen/KytheBindingGraphFactory.java b/java/dagger/internal/codegen/KytheBindingGraphFactory.java index c3a6a238fd0..e9ea2428d5c 100644 --- a/java/dagger/internal/codegen/KytheBindingGraphFactory.java +++ b/java/dagger/internal/codegen/KytheBindingGraphFactory.java @@ -57,7 +57,8 @@ final class KytheBindingGraphFactory { Optional create(TypeElement type) { if (MoreElements.isAnnotationPresent(type, Component.class) || MoreElements.isAnnotationPresent(type, ProductionComponent.class)) { - return Optional.of(bindingGraphFactory.create(componentDescriptorFactory.forComponent(type))); + return Optional.of( + bindingGraphFactory.create(componentDescriptorFactory.forTypeElement(type))); } return Optional.empty(); } @@ -77,6 +78,8 @@ static CompilerOptions createCompilerOptions() { .fastInit(false) .experimentalAndroidMode2(false) .aheadOfTimeSubcomponents(false) + .moduleBindingValidationType(ValidationType.NONE) + .moduleHasDifferentScopesDiagnosticKind(Diagnostic.Kind.NOTE) .build() .validate(); } diff --git a/java/dagger/internal/codegen/MissingBindingValidator.java b/java/dagger/internal/codegen/MissingBindingValidator.java index 727b90885ff..ddd71c16ea1 100644 --- a/java/dagger/internal/codegen/MissingBindingValidator.java +++ b/java/dagger/internal/codegen/MissingBindingValidator.java @@ -54,6 +54,9 @@ public String pluginName() { @Override public void visitGraph(BindingGraph graph, DiagnosticReporter diagnosticReporter) { + if (graph.isModuleBindingGraph()) { + return; // Don't report missing bindings when validating a module. + } graph .missingBindings() .forEach(missingBinding -> reportMissingBinding(missingBinding, graph, diagnosticReporter)); diff --git a/java/dagger/internal/codegen/ModuleDescriptor.java b/java/dagger/internal/codegen/ModuleDescriptor.java index 1210e22eaec..8ac4e22d543 100644 --- a/java/dagger/internal/codegen/ModuleDescriptor.java +++ b/java/dagger/internal/codegen/ModuleDescriptor.java @@ -22,6 +22,7 @@ import static com.google.common.base.CaseFormat.UPPER_CAMEL; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Verify.verify; +import static com.google.common.collect.Iterables.transform; import static dagger.internal.codegen.ConfigurationAnnotations.getModuleAnnotation; import static dagger.internal.codegen.ConfigurationAnnotations.getModuleIncludes; import static dagger.internal.codegen.DaggerElements.getAnnotationMirror; @@ -37,20 +38,24 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; +import com.google.common.graph.Traverser; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.squareup.javapoet.ClassName; import dagger.Binds; import dagger.BindsOptionalOf; import dagger.Module; import dagger.Provides; +import dagger.model.Key; import dagger.multibindings.Multibinds; import dagger.producers.ProducerModule; import dagger.producers.Produces; import java.lang.annotation.Annotation; +import java.util.Collection; import java.util.EnumSet; import java.util.LinkedHashSet; import java.util.Optional; import java.util.Set; +import java.util.stream.Stream; import javax.inject.Inject; import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.ExecutableElement; @@ -80,6 +85,19 @@ abstract class ModuleDescriptor { abstract Kind kind(); + /** Returns the keys of all bindings declared by this module. */ + ImmutableSet allBindingKeys() { + return Stream.of( + bindings(), + delegateDeclarations(), + multibindingDeclarations(), + optionalDeclarations(), + subcomponentDeclarations()) + .flatMap(Collection::stream) + .map(BindingDeclaration::key) + .collect(toImmutableSet()); + } + enum Kind { MODULE(Module.class, Provides.class), PRODUCER_MODULE(ProducerModule.class, Produces.class); @@ -200,6 +218,14 @@ ModuleDescriptor create(TypeElement moduleElement) { Kind.forAnnotatedElement(moduleElement).get()); } + /** Returns all the modules transitively included by given modules, including the arguments. */ + ImmutableSet transitiveModules(Iterable modules) { + return ImmutableSet.copyOf( + Traverser.forGraph( + (ModuleDescriptor module) -> transform(module.includedModules(), this::create)) + .depthFirstPreOrder(transform(modules, this::create))); + } + @CanIgnoreReturnValue private Set collectIncludedModules( Set includedModules, diff --git a/java/dagger/internal/codegen/ModuleValidation.java b/java/dagger/internal/codegen/ModuleValidation.java new file mode 100644 index 00000000000..209ba6feb84 --- /dev/null +++ b/java/dagger/internal/codegen/ModuleValidation.java @@ -0,0 +1,31 @@ +/* + * 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 java.lang.annotation.Documented; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import javax.inject.Qualifier; + +/** + * Qualifier annotation for the {@link dagger.spi.BindingGraphPlugin}s that are used to implement + * core Dagger validation for module binding graphs. + */ +@Documented +@Retention(RetentionPolicy.RUNTIME) +@Qualifier +@interface ModuleValidation {} diff --git a/java/dagger/internal/codegen/ModuleValidator.java b/java/dagger/internal/codegen/ModuleValidator.java index 89715eaf0d6..d15dd43c66c 100644 --- a/java/dagger/internal/codegen/ModuleValidator.java +++ b/java/dagger/internal/codegen/ModuleValidator.java @@ -52,6 +52,7 @@ import dagger.Binds; import dagger.Module; import dagger.Subcomponent; +import dagger.model.BindingGraph; import dagger.multibindings.Multibinds; import dagger.producers.ProducerModule; import dagger.producers.ProductionSubcomponent; @@ -111,6 +112,11 @@ final class ModuleValidator { private final DaggerElements elements; private final AnyBindingMethodValidator anyBindingMethodValidator; private final MethodSignatureFormatter methodSignatureFormatter; + private final ComponentDescriptor.Factory componentDescriptorFactory; + private final BindingGraphFactory bindingGraphFactory; + private final BindingGraphConverter bindingGraphConverter; + private final BindingGraphPlugins moduleValidationPlugins; + private final CompilerOptions compilerOptions; private final Map> cache = new HashMap<>(); private final Set knownModules = new HashSet<>(); @@ -119,11 +125,21 @@ final class ModuleValidator { Types types, DaggerElements elements, AnyBindingMethodValidator anyBindingMethodValidator, - MethodSignatureFormatter methodSignatureFormatter) { + MethodSignatureFormatter methodSignatureFormatter, + ComponentDescriptor.Factory componentDescriptorFactory, + BindingGraphFactory bindingGraphFactory, + BindingGraphConverter bindingGraphConverter, + @ModuleValidation BindingGraphPlugins moduleValidationPlugins, + CompilerOptions compilerOptions) { this.types = types; this.elements = elements; this.anyBindingMethodValidator = anyBindingMethodValidator; this.methodSignatureFormatter = methodSignatureFormatter; + this.componentDescriptorFactory = componentDescriptorFactory; + this.bindingGraphFactory = bindingGraphFactory; + this.bindingGraphConverter = bindingGraphConverter; + this.moduleValidationPlugins = moduleValidationPlugins; + this.compilerOptions = compilerOptions; } /** @@ -218,6 +234,11 @@ private ValidationReport validateUncached( validateNoScopeAnnotationsOnModuleElement(module, moduleKind, builder); validateSelfCycles(module, builder); + if (builder.build().isClean() + && !compilerOptions.moduleBindingValidationType().equals(ValidationType.NONE)) { + validateModuleBindings(module, builder); + } + return builder.build(); } @@ -570,6 +591,19 @@ public Void visitArray(List values, AnnotationValue p null); } + private void validateModuleBindings( + TypeElement module, ValidationReport.Builder report) { + BindingGraph bindingGraph = + bindingGraphConverter.convert( + bindingGraphFactory.create(componentDescriptorFactory.forTypeElement(module))); + if (moduleValidationPlugins.pluginsReportErrors(bindingGraph)) { + // Since the plugins use a DiagnosticReporter to report errors, the ValdiationReport won't + // have any Items for them. We have to tell the ValidationReport that some errors were + // reported for the subject. + report.markDirty(); + } + } + private static String formatListForErrorMessage(List things) { switch (things.size()) { case 0: diff --git a/java/dagger/internal/codegen/SubcomponentFactoryMethodValidator.java b/java/dagger/internal/codegen/SubcomponentFactoryMethodValidator.java index 306b0b84f7f..f0dea099441 100644 --- a/java/dagger/internal/codegen/SubcomponentFactoryMethodValidator.java +++ b/java/dagger/internal/codegen/SubcomponentFactoryMethodValidator.java @@ -61,6 +61,10 @@ public String pluginName() { @Override public void visitGraph(BindingGraph bindingGraph, DiagnosticReporter diagnosticReporter) { + if (bindingGraph.isModuleBindingGraph()) { + // We don't know all the modules that might be owned by the child until we know the root. + return; + } bindingGraph.network().edges().stream() .flatMap(instancesOf(ChildFactoryMethodEdge.class)) .forEach( diff --git a/java/dagger/internal/codegen/ValidationReport.java b/java/dagger/internal/codegen/ValidationReport.java index ef2142144e4..0a3765a43bd 100644 --- a/java/dagger/internal/codegen/ValidationReport.java +++ b/java/dagger/internal/codegen/ValidationReport.java @@ -67,8 +67,20 @@ ImmutableSet> allReports() { return ImmutableSet.copyOf(SUBREPORTS.depthFirstPreOrder(this)); } - /** Returns {@code true} if there are no errors in this report or any subreports. */ + /** + * {@code true} if {@link #isClean()} should return {@code false} even if there are no error items + * in this report. + */ + abstract boolean markedDirty(); + + /** + * Returns {@code true} if there are no errors in this report or any subreports and {@link + * #markedDirty()} is {@code false}. + */ boolean isClean() { + if (markedDirty()) { + return false; + } for (Item item : items()) { switch (item.kind()) { case ERROR: @@ -150,6 +162,7 @@ static final class Builder { private final T subject; private final ImmutableSet.Builder items = ImmutableSet.builder(); private final ImmutableSet.Builder> subreports = ImmutableSet.builder(); + private boolean markedDirty; private Builder(T subject) { this.subject = subject; @@ -253,6 +266,14 @@ private Builder addItem( return this; } + /** + * If called, then {@link #isClean()} will return {@code false} even if there are no error items + * in the report. + */ + void markDirty() { + this.markedDirty = true; + } + Builder addSubreport(ValidationReport subreport) { subreports.add(subreport); return this; @@ -260,7 +281,8 @@ Builder addSubreport(ValidationReport subreport) { @CheckReturnValue ValidationReport build() { - return new AutoValue_ValidationReport<>(subject, items.build(), subreports.build()); + return new AutoValue_ValidationReport<>( + subject, items.build(), subreports.build(), markedDirty); } } } diff --git a/java/dagger/model/BindingGraph.java b/java/dagger/model/BindingGraph.java index d5bc7875516..74f51a2c12e 100644 --- a/java/dagger/model/BindingGraph.java +++ b/java/dagger/model/BindingGraph.java @@ -40,7 +40,25 @@ import javax.lang.model.element.TypeElement; /** - * The immutable graph of bindings, dependency requests, and components for a valid root component. + * A graph of bindings, dependency requests, and components. + * + *

A {@link BindingGraph} represents one of the following: + * + *

    + *
  • an entire component hierarchy rooted at a {@link dagger.Component} or {@link + * dagger.producers.ProductionComponent} + *
  • a partial component hierarchy rooted at a {@link dagger.Subcomponent} or {@link + * dagger.producers.ProductionSubcomponent} (only when {@code + * -Adagger.experimentalAheadOfTimeSubcomponents=enabled} is passed to the compiler) + *
  • the bindings installed by a {@link Module} or {@link dagger.producers.ProducerModule}, + * including all subcomponents generated by {@link Module#subcomponents()} ()} and {@link + * dagger.producers.ProducerModule#subcomponents()} ()} + *
+ * + * In the case of a {@link BindingGraph} representing a module, the root {@link ComponentNode} will + * actually represent the module type, and there will be an entry point edge (with no request + * element) for every binding (except multibinding contributions) in the module, including its + * transitively included modules. * *

Nodes

* @@ -72,8 +90,8 @@ @AutoValue public abstract class BindingGraph { - static BindingGraph create(Network network) { - return new AutoValue_BindingGraph(ImmutableNetwork.copyOf(network)); + static BindingGraph create(Network network, boolean isModuleBindingGraph) { + return new AutoValue_BindingGraph(ImmutableNetwork.copyOf(network), isModuleBindingGraph); } BindingGraph() {} @@ -86,6 +104,14 @@ public final String toString() { return network().toString(); } + /** + * Returns {@code true} if this graph was constructed from a module for module binding validation. + * + * @see Module binding + * validation + */ + public abstract boolean isModuleBindingGraph(); + /** Returns the bindings. */ public final ImmutableSet bindings() { return nodes(Binding.class); diff --git a/java/dagger/model/BindingGraphProxies.java b/java/dagger/model/BindingGraphProxies.java index 73163d8fe95..4d514f32836 100644 --- a/java/dagger/model/BindingGraphProxies.java +++ b/java/dagger/model/BindingGraphProxies.java @@ -28,8 +28,9 @@ */ public final class BindingGraphProxies { /** Creates a new {@link BindingGraph}. */ - public static BindingGraph bindingGraph(Network network) { - return BindingGraph.create(network); + public static BindingGraph bindingGraph( + Network network, boolean isModuleBindingGraph) { + return BindingGraph.create(network, isModuleBindingGraph); } /** Creates a new {@link MissingBinding}. */ diff --git a/javatests/dagger/internal/codegen/DependencyCycleValidationTest.java b/javatests/dagger/internal/codegen/DependencyCycleValidationTest.java index 7ac72f2b93b..4a57da3c77c 100644 --- a/javatests/dagger/internal/codegen/DependencyCycleValidationTest.java +++ b/javatests/dagger/internal/codegen/DependencyCycleValidationTest.java @@ -30,34 +30,43 @@ @RunWith(JUnit4.class) public class DependencyCycleValidationTest { @Test public void cyclicDependency() { - JavaFileObject component = JavaFileObjects.forSourceLines("test.Outer", - "package test;", - "", - "import dagger.Component;", - "import dagger.Module;", - "import dagger.Provides;", - "import javax.inject.Inject;", - "", - "final class Outer {", - " static class A {", - " @Inject A(C cParam) {}", - " }", - "", - " static class B {", - " @Inject B(A aParam) {}", - " }", - "", - " static class C {", - " @Inject C(B bParam) {}", - " }", - "", - " @Component()", - " interface CComponent {", - " C getC();", - " }", - "}"); + JavaFileObject component = + JavaFileObjects.forSourceLines( + "test.Outer", + "package test;", + "", + "import dagger.Binds;", + "import dagger.Component;", + "import dagger.Module;", + "import dagger.Provides;", + "import javax.inject.Inject;", + "", + "final class Outer {", + " static class A {", + " @Inject A(C cParam) {}", + " }", + "", + " static class B {", + " @Inject B(A aParam) {}", + " }", + "", + " static class C {", + " @Inject C(B bParam) {}", + " }", + "", + " @Module", + " interface MModule {", + " @Binds Object object(C c);", + " }", + "", + " @Component()", + " interface CComponent {", + " C getC();", + " }", + "}"); - Compilation compilation = daggerCompiler().compile(component); + Compilation compilation = + daggerCompiler().withOptions("-Adagger.moduleBindingValidation=ERROR").compile(component); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -73,6 +82,21 @@ public class DependencyCycleValidationTest { " test.Outer.CComponent.getC()")) .inFile(component) .onLineContaining("interface CComponent"); + + assertThat(compilation) + .hadErrorContaining( + message( + "Found a dependency cycle:", + " test.Outer.C is injected at", + " test.Outer.A(cParam)", + " test.Outer.A is injected at", + " test.Outer.B(aParam)", + " test.Outer.B is injected at", + " test.Outer.C(bParam)", + "It is requested at:", + " test.Outer.MModule.object(c)")) + .inFile(component) + .onLineContaining("interface MModule"); } @Test public void cyclicDependencyNotIncludingEntryPoint() { diff --git a/javatests/dagger/internal/codegen/DuplicateBindingsValidationTest.java b/javatests/dagger/internal/codegen/DuplicateBindingsValidationTest.java index e980f2bedfe..ac54941e6cd 100644 --- a/javatests/dagger/internal/codegen/DuplicateBindingsValidationTest.java +++ b/javatests/dagger/internal/codegen/DuplicateBindingsValidationTest.java @@ -17,20 +17,36 @@ package dagger.internal.codegen; import static com.google.testing.compile.CompilationSubject.assertThat; -import static com.google.testing.compile.Compiler.javac; import static dagger.internal.codegen.Compilers.daggerCompiler; import static dagger.internal.codegen.TestUtils.message; +import static org.junit.Assume.assumeFalse; +import com.google.common.collect.ImmutableList; 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; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; -@RunWith(JUnit4.class) +@RunWith(Parameterized.class) public class DuplicateBindingsValidationTest { + + @Parameters(name = "moduleBindingValidation={0}") + public static ImmutableList parameters() { + return ImmutableList.copyOf(new Object[][] {{false}, {true}}); + } + + private final boolean moduleBindingValidation; + + public DuplicateBindingsValidationTest(boolean moduleBindingValidation) { + this.moduleBindingValidation = moduleBindingValidation; + } + @Test public void duplicateExplicitBindings_ProvidesAndComponentProvision() { + assumeFalse(moduleBindingValidation); + JavaFileObject component = JavaFileObjects.forSourceLines("test.Outer", "package test;", "", @@ -65,7 +81,8 @@ public class DuplicateBindingsValidationTest { " }", "}"); - Compilation compilation = daggerCompiler().compile(component); + Compilation compilation = + daggerCompiler().withOptions(moduleBindingValidationOption()).compile(component); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -106,6 +123,9 @@ public class DuplicateBindingsValidationTest { " @Provides A provideA2(String s) { return new A() {}; }", " }", "", + " @Module(includes = { Module1.class, Module2.class})", + " abstract static class Module3 {}", + "", " @Component(modules = { Module1.class, Module2.class})", " interface TestComponent {", " A getA();", @@ -113,7 +133,8 @@ public class DuplicateBindingsValidationTest { " }", "}"); - Compilation compilation = daggerCompiler().compile(component); + Compilation compilation = + daggerCompiler().withOptions(moduleBindingValidationOption()).compile(component); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -123,8 +144,20 @@ public class DuplicateBindingsValidationTest { " @Provides test.Outer.A test.Outer.Module2.provideA2(String)")) .inFile(component) .onLineContaining("interface TestComponent"); + + if (moduleBindingValidation) { + assertThat(compilation) + .hadErrorContaining( + message( + "test.Outer.A is bound multiple times:", + " @Provides test.Outer.A test.Outer.Module1.provideA1()", + " @Provides test.Outer.A test.Outer.Module2.provideA2(String)")) + .inFile(component) + .onLineContaining("class Module3"); + } + // The duplicate bindngs are also requested from B, but we don't want to report them again. - assertThat(compilation).hadErrorCount(1); + assertThat(compilation).hadErrorCount(moduleBindingValidation ? 2 : 1); } @Test @@ -157,13 +190,17 @@ public void duplicateExplicitBindings_ProvidesVsBinds() { " @Binds abstract A bindA2(B b);", " }", "", + " @Module(includes = { Module1.class, Module2.class})", + " abstract static class Module3 {}", + "", " @Component(modules = { Module1.class, Module2.class})", " interface TestComponent {", " A getA();", " }", "}"); - Compilation compilation = daggerCompiler().compile(component); + Compilation compilation = + daggerCompiler().withOptions(moduleBindingValidationOption()).compile(component); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -173,6 +210,17 @@ public void duplicateExplicitBindings_ProvidesVsBinds() { " @Binds test.Outer.A test.Outer.Module2.bindA2(test.Outer.B)")) .inFile(component) .onLineContaining("interface TestComponent"); + + if (moduleBindingValidation) { + assertThat(compilation) + .hadErrorContaining( + message( + "test.Outer.A is bound multiple times:", + " @Provides test.Outer.A test.Outer.Module1.provideA1()", + " @Binds test.Outer.A test.Outer.Module2.bindA2(test.Outer.B)")) + .inFile(component) + .onLineContaining("class Module3"); + } } @Test @@ -210,13 +258,17 @@ public void duplicateExplicitBindings_multibindingsAndExplicitSets() { " @Provides Set stringSet() { return new HashSet(); }", " }", "", + " @Module(includes = { TestModule1.class, TestModule2.class})", + " abstract static class TestModule3 {}", + "", " @Component(modules = { TestModule1.class, TestModule2.class })", " interface TestComponent {", " Set getStringSet();", " }", "}"); - Compilation compilation = daggerCompiler().compile(component); + Compilation compilation = + daggerCompiler().withOptions(moduleBindingValidationOption()).compile(component); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -231,7 +283,8 @@ public void duplicateExplicitBindings_multibindingsAndExplicitSets() { " Unique bindings and declarations:", " @Provides Set test.Outer.TestModule2.stringSet()")) .inFile(component) - .onLineContaining("interface TestComponent"); + .onLineContaining( + moduleBindingValidation ? "class TestModule3" : "interface TestComponent"); } @Test @@ -274,13 +327,17 @@ public void duplicateExplicitBindings_multibindingsAndExplicitMaps() { " }", " }", "", + " @Module(includes = { TestModule1.class, TestModule2.class})", + " abstract static class TestModule3 {}", + "", " @Component(modules = { TestModule1.class, TestModule2.class })", " interface TestComponent {", " Map getStringMap();", " }", "}"); - Compilation compilation = daggerCompiler().compile(component); + Compilation compilation = + daggerCompiler().withOptions(moduleBindingValidationOption()).compile(component); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -298,7 +355,8 @@ public void duplicateExplicitBindings_multibindingsAndExplicitMaps() { " Unique bindings and declarations:", " @Provides Map test.Outer.TestModule2.stringMap()")) .inFile(component) - .onLineContaining("interface TestComponent"); + .onLineContaining( + moduleBindingValidation ? "class TestModule3" : "interface TestComponent"); } @Test @@ -326,13 +384,17 @@ public void duplicateExplicitBindings_UniqueBindingAndMultibindingDeclaration_Se " @Provides Set stringSet() { return new HashSet(); }", " }", "", + " @Module(includes = { TestModule1.class, TestModule2.class})", + " abstract static class TestModule3 {}", + "", " @Component(modules = { TestModule1.class, TestModule2.class })", " interface TestComponent {", " Set getStringSet();", " }", "}"); - Compilation compilation = daggerCompiler().compile(component); + Compilation compilation = + daggerCompiler().withOptions(moduleBindingValidationOption()).compile(component); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -344,7 +406,8 @@ public void duplicateExplicitBindings_UniqueBindingAndMultibindingDeclaration_Se " Unique bindings and declarations:", " @Provides Set test.Outer.TestModule2.stringSet()")) .inFile(component) - .onLineContaining("interface TestComponent"); + .onLineContaining( + moduleBindingValidation ? "class TestModule3" : "interface TestComponent"); } @Test @@ -374,13 +437,17 @@ public void duplicateExplicitBindings_UniqueBindingAndMultibindingDeclaration_Ma " }", " }", "", + " @Module(includes = { TestModule1.class, TestModule2.class})", + " abstract static class TestModule3 {}", + "", " @Component(modules = { TestModule1.class, TestModule2.class })", " interface TestComponent {", " Map getStringMap();", " }", "}"); - Compilation compilation = daggerCompiler().compile(component); + Compilation compilation = + daggerCompiler().withOptions(moduleBindingValidationOption()).compile(component); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -393,7 +460,8 @@ public void duplicateExplicitBindings_UniqueBindingAndMultibindingDeclaration_Ma " Unique bindings and declarations:", " @Provides Map test.Outer.TestModule2.stringMap()")) .inFile(component) - .onLineContaining("interface TestComponent"); + .onLineContaining( + moduleBindingValidation ? "class TestModule3" : "interface TestComponent"); } @Test public void duplicateBindings_TruncateAfterLimit() { @@ -470,6 +538,22 @@ public void duplicateExplicitBindings_UniqueBindingAndMultibindingDeclaration_Ma " @Provides A provideA() { return new A() {}; }", " }", "", + " @Module(includes = {", + " Module01.class,", + " Module02.class,", + " Module03.class,", + " Module04.class,", + " Module05.class,", + " Module06.class,", + " Module07.class,", + " Module08.class,", + " Module09.class,", + " Module10.class,", + " Module11.class,", + " Module12.class", + " })", + " abstract static class Modules {}", + "", " @Component(modules = {", " Module01.class,", " Module02.class,", @@ -489,7 +573,8 @@ public void duplicateExplicitBindings_UniqueBindingAndMultibindingDeclaration_Ma " }", "}"); - Compilation compilation = daggerCompiler().compile(component); + Compilation compilation = + daggerCompiler().withOptions(moduleBindingValidationOption()).compile(component); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -507,7 +592,7 @@ public void duplicateExplicitBindings_UniqueBindingAndMultibindingDeclaration_Ma " @Provides test.Outer.A test.Outer.Module10.provideA()", " and 2 others")) .inFile(component) - .onLineContaining("interface TestComponent"); + .onLineContaining(moduleBindingValidation ? "class Modules" : "interface TestComponent"); } @Test @@ -525,9 +610,9 @@ public void childBindingConflictsWithParent() { "interface A {", " Object conflict();", "", - " B b();", + " B.Builder b();", "", - " @Module", + " @Module(subcomponents = B.class)", " static class AModule {", " @Provides static Object abConflict() {", " return \"a\";", @@ -547,6 +632,11 @@ public void childBindingConflictsWithParent() { "interface B {", " Object conflict();", "", + " @Subcomponent.Builder", + " interface Builder {", + " B build();", + " }", + "", " @Module", " static class BModule {", " @Provides static Object abConflict() {", @@ -555,7 +645,10 @@ public void childBindingConflictsWithParent() { " }", "}"); - Compilation compilation = daggerCompiler().compile(aComponent, bComponent); + Compilation compilation = + daggerCompiler() + .withOptions(moduleBindingValidationOption()) + .compile(aComponent, bComponent); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -564,7 +657,7 @@ public void childBindingConflictsWithParent() { " @Provides Object test.A.AModule.abConflict()", " @Provides Object test.B.BModule.abConflict()")) .inFile(aComponent) - .onLineContaining("interface A {"); + .onLineContaining(moduleBindingValidation ? "class AModule" : "interface A {"); } @Test @@ -582,9 +675,9 @@ public void grandchildBindingConflictsWithGrandparent() { "interface A {", " Object conflict();", "", - " B b();", + " B.Builder b();", "", - " @Module", + " @Module(subcomponents = B.class)", " static class AModule {", " @Provides static Object acConflict() {", " return \"a\";", @@ -600,7 +693,12 @@ public void grandchildBindingConflictsWithGrandparent() { "", "@Subcomponent", "interface B {", - " C c();", + " C.Builder c();", + "", + " @Subcomponent.Builder", + " interface Builder {", + " B build();", + " }", "}"); JavaFileObject cComponent = JavaFileObjects.forSourceLines( @@ -615,6 +713,11 @@ public void grandchildBindingConflictsWithGrandparent() { "interface C {", " Object conflict();", "", + " @Subcomponent.Builder", + " interface Builder {", + " C build();", + " }", + "", " @Module", " static class CModule {", " @Provides static Object acConflict() {", @@ -623,7 +726,10 @@ public void grandchildBindingConflictsWithGrandparent() { " }", "}"); - Compilation compilation = daggerCompiler().compile(aComponent, bComponent, cComponent); + Compilation compilation = + daggerCompiler() + .withOptions(moduleBindingValidationOption()) + .compile(aComponent, bComponent, cComponent); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -632,7 +738,7 @@ public void grandchildBindingConflictsWithGrandparent() { " @Provides Object test.A.AModule.acConflict()", " @Provides Object test.C.CModule.acConflict()")) .inFile(aComponent) - .onLineContaining("interface A {"); + .onLineContaining(moduleBindingValidation ? "class AModule" : "interface A {"); } @Test @@ -661,9 +767,9 @@ public void grandchildBindingConflictsWithChild() { "interface B {", " Object conflict();", "", - " C c();", + " C.Builder c();", "", - " @Module", + " @Module(subcomponents = C.class)", " static class BModule {", " @Provides static Object bcConflict() {", " return \"b\";", @@ -683,6 +789,11 @@ public void grandchildBindingConflictsWithChild() { "interface C {", " Object conflict();", "", + " @Subcomponent.Builder", + " interface Builder {", + " C build();", + " }", + "", " @Module", " static class CModule {", " @Provides static Object bcConflict() {", @@ -691,7 +802,10 @@ public void grandchildBindingConflictsWithChild() { " }", "}"); - Compilation compilation = daggerCompiler().compile(aComponent, bComponent, cComponent); + Compilation compilation = + daggerCompiler() + .withOptions(moduleBindingValidationOption()) + .compile(aComponent, bComponent, cComponent); assertThat(compilation).failed(); assertThat(compilation) .hadErrorContaining( @@ -699,8 +813,8 @@ public void grandchildBindingConflictsWithChild() { "java.lang.Object is bound multiple times:", " @Provides Object test.B.BModule.bcConflict()", " @Provides Object test.C.CModule.bcConflict()")) - .inFile(aComponent) - .onLineContaining("interface A {"); + .inFile(moduleBindingValidation ? bComponent : aComponent) + .onLineContaining(moduleBindingValidation ? "class BModule" : "interface A {"); } @Test @@ -717,9 +831,9 @@ public void grandchildBindingConflictsWithParentWithNullableViolationAsWarning() "", "@Component(modules = ParentConflictsWithChild.ParentModule.class)", "interface ParentConflictsWithChild {", - " Child child();", + " Child.Builder child();", "", - " @Module", + " @Module(subcomponents = Child.class)", " static class ParentModule {", " @Provides @Nullable static Object nullableParentChildConflict() {", " return \"parent\";", @@ -739,6 +853,11 @@ public void grandchildBindingConflictsWithParentWithNullableViolationAsWarning() "interface Child {", " Object parentChildConflictThatViolatesNullability();", "", + " @Subcomponent.Builder", + " interface Builder {", + " Child build();", + " }", + "", " @Module", " static class ChildModule {", " @Provides static Object nonNullableParentChildConflict() {", @@ -748,9 +867,8 @@ public void grandchildBindingConflictsWithParentWithNullableViolationAsWarning() "}"); Compilation compilation = - javac() - .withOptions("-Adagger.nullableValidation=WARNING") - .withProcessors(new ComponentProcessor()) + daggerCompiler() + .withOptions("-Adagger.nullableValidation=WARNING", moduleBindingValidationOption()) .compile(parentConflictsWithChild, child); assertThat(compilation).failed(); assertThat(compilation) @@ -761,7 +879,12 @@ public void grandchildBindingConflictsWithParentWithNullableViolationAsWarning() " @Provides @javax.annotation.Nullable Object" + " test.ParentConflictsWithChild.ParentModule.nullableParentChildConflict()")) .inFile(parentConflictsWithChild) - .onLine(9); + .onLineContaining( + moduleBindingValidation ? "class ParentModule" : "interface ParentConflictsWithChild"); + } + + private String moduleBindingValidationOption() { + return "-Adagger.moduleBindingValidation=" + (moduleBindingValidation ? "ERROR" : "NONE"); } @Test diff --git a/javatests/dagger/internal/codegen/MapMultibindingValidationTest.java b/javatests/dagger/internal/codegen/MapMultibindingValidationTest.java index 487fd1054d5..b2ed61456c1 100644 --- a/javatests/dagger/internal/codegen/MapMultibindingValidationTest.java +++ b/javatests/dagger/internal/codegen/MapMultibindingValidationTest.java @@ -70,6 +70,19 @@ public void duplicateMapKeys() { assertThat(compilation).hadErrorContaining("provideObjectForAKeyAgain()"); assertThat(compilation).hadErrorCount(1); + compilation = + daggerCompiler().withOptions("-Adagger.moduleBindingValidation=ERROR").compile(module); + assertThat(compilation).failed(); + assertThat(compilation) + .hadErrorContaining( + "The same map key is bound more than once for " + + "java.util.Map>") + .inFile(module) + .onLineContaining("class MapModule"); + assertThat(compilation).hadErrorContaining("provideObjectForAKey()"); + assertThat(compilation).hadErrorContaining("provideObjectForAKeyAgain()"); + assertThat(compilation).hadErrorCount(1); + // If there's Map and Map>, report only Map. compilation = daggerCompiler() @@ -198,6 +211,21 @@ public void inconsistentMapKeyAnnotations() { assertThat(compilation).hadErrorContaining("provideObjectForBKey()"); assertThat(compilation).hadErrorCount(1); + compilation = + daggerCompiler() + .withOptions("-Adagger.moduleBindingValidation=ERROR") + .compile(module, stringKeyTwoFile); + assertThat(compilation).failed(); + assertThat(compilation) + .hadErrorContaining( + "java.util.Map>" + + " uses more than one @MapKey annotation type") + .inFile(module) + .onLineContaining("class MapModule"); + assertThat(compilation).hadErrorContaining("provideObjectForAKey()"); + assertThat(compilation).hadErrorContaining("provideObjectForBKey()"); + assertThat(compilation).hadErrorCount(1); + // If there's Map and Map>, report only Map. compilation = daggerCompiler() diff --git a/javatests/dagger/internal/codegen/ModuleBindingValidationTest.java b/javatests/dagger/internal/codegen/ModuleBindingValidationTest.java new file mode 100644 index 00000000000..dee9a8ff776 --- /dev/null +++ b/javatests/dagger/internal/codegen/ModuleBindingValidationTest.java @@ -0,0 +1,124 @@ +/* + * 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 ModuleBindingValidationTest { + private static final JavaFileObject MODULE = + JavaFileObjects.forSourceLines( + "test.TestModule", + "package test;", + "", + "import dagger.Binds;", + "import dagger.Module;", + "", + "@Module", + "interface TestModule {", + " @Binds Object toString(String string);", + " @Binds Object toLong(Long l);", + "}"); + + private static final JavaFileObject INCLUDING_MODULE = + JavaFileObjects.forSourceLines( + "test.IncludingModule", + "package test;", + "", + "import dagger.Module;", + "", + "@Module(includes = TestModule.class)", + "interface IncludingModule {}"); + + @Test + public void moduleBindingValidationErrors() { + Compilation compilation = + daggerCompiler() + .withOptions("-Adagger.moduleBindingValidation=ERROR") + .compile(MODULE, INCLUDING_MODULE); + assertThat(compilation).failed(); + + // Make sure the module-level error doesn't show a dependency trace afterwards (note the $). + assertThat(compilation) + .hadErrorContainingMatch( + message( + "^\\Q[Dagger/DuplicateBindings] java.lang.Object is bound multiple times:", + " @Binds Object test.TestModule.toLong(Long)", + " @Binds Object test.TestModule.toString(String)\\E$")) + .inFile(MODULE) + .onLineContaining("interface TestModule"); + + assertThat(compilation) + .hadErrorContaining("test.TestModule has errors") + .inFile(INCLUDING_MODULE) + .onLineContaining("TestModule.class"); + + // The duplicate bindings error is reported only once, for TestModule, and not again for + // IncludingModule. + assertThat(compilation).hadErrorCount(2); + } + + @Test + public void moduleBindingValidationWarning() { + Compilation compilation = + daggerCompiler() + .withOptions("-Adagger.moduleBindingValidation=WARNING") + .compile(MODULE, INCLUDING_MODULE); + assertThat(compilation).succeeded(); + + assertThat(compilation) + .hadWarningContainingMatch( + message( + "^\\Q[Dagger/DuplicateBindings] java.lang.Object is bound multiple times:", + " @Binds Object test.TestModule.toLong(Long)", + " @Binds Object test.TestModule.toString(String)\\E$")) + .inFile(MODULE) + .onLineContaining("interface TestModule"); + + // Make sure the module-level error doesn't show a dependency trace. + assertThat(compilation) + .hadWarningContainingMatch( + message( + "^\\Q[Dagger/DuplicateBindings] java.lang.Object is bound multiple times:", + " @Binds Object test.TestModule.toLong(Long)", + " @Binds Object test.TestModule.toString(String)\\E$")) + .inFile(INCLUDING_MODULE) + .onLineContaining("interface IncludingModule"); + + // If module binding validation reports warnings, the duplicate bindings warning occurs twice: + // once for TestModule and once for IncludingModule, which includes it. + assertThat(compilation).hadWarningCount(2); + } + + @Test + public void moduleBindingValidationNone() { + Compilation compilation = + daggerCompiler() + .withOptions("-Adagger.moduleBindingValidation=NONE") + .compile(MODULE, INCLUDING_MODULE); + assertThat(compilation).succeededWithoutWarnings(); + } +} diff --git a/javatests/dagger/internal/codegen/NullableBindingValidationTest.java b/javatests/dagger/internal/codegen/NullableBindingValidationTest.java index 710f7772150..eb0dc234334 100644 --- a/javatests/dagger/internal/codegen/NullableBindingValidationTest.java +++ b/javatests/dagger/internal/codegen/NullableBindingValidationTest.java @@ -387,4 +387,33 @@ public void nullCheckForOptionalProviderOfLazy() { Compilation compilation = daggerCompiler().compile(NULLABLE, a, module, component); assertThat(compilation).succeeded(); } + + @Test + public void moduleValidation() { + JavaFileObject module = + JavaFileObjects.forSourceLines( + "test.TestModule", + "package test;", + "", + "import dagger.Binds;", + "import dagger.Module;", + "import dagger.Provides;", + "", + "@Module", + "abstract class TestModule {", + " @Provides @Nullable static String nullableString() { return null; }", + " @Binds abstract Object object(String string);", + "}"); + + Compilation compilation = + daggerCompiler() + .withOptions("-Adagger.moduleBindingValidation=ERROR") + .compile(module, NULLABLE); + assertThat(compilation).failed(); + assertThat(compilation) + .hadErrorContaining( + nullableToNonNullable( + "java.lang.String", + "@Provides @test.Nullable String test.TestModule.nullableString()")); + } } diff --git a/javatests/dagger/internal/codegen/ProductionComponentProcessorTest.java b/javatests/dagger/internal/codegen/ProductionComponentProcessorTest.java index 7e891560bc7..48af3ec5c44 100644 --- a/javatests/dagger/internal/codegen/ProductionComponentProcessorTest.java +++ b/javatests/dagger/internal/codegen/ProductionComponentProcessorTest.java @@ -158,10 +158,23 @@ public void dependsOnProductionExecutor() { "}"); Compilation compilation = daggerCompiler() - .withOptions(compilerMode.javacopts()) .compile(moduleFile, producerModuleFile, componentFile); assertThat(compilation).failed(); - assertThat(compilation).hadErrorContaining("may not depend on the production executor"); + assertThat(compilation) + .hadErrorContaining("java.lang.String may not depend on the production executor") + .inFile(componentFile) + .onLineContaining("interface SimpleComponent"); + + compilation = + daggerCompiler() + .withOptions("-Adagger.moduleBindingValidation=ERROR") + .compile(producerModuleFile); + assertThat(compilation).failed(); + assertThat(compilation) + .hadErrorContaining("java.lang.String may not depend on the production executor") + .inFile(producerModuleFile) + .onLineContaining("class SimpleModule"); + // TODO(dpb): Report at the binding if enclosed in the module. } @Test diff --git a/javatests/dagger/internal/codegen/ProductionGraphValidationTest.java b/javatests/dagger/internal/codegen/ProductionGraphValidationTest.java index 1029c1cd587..7106dd9e756 100644 --- a/javatests/dagger/internal/codegen/ProductionGraphValidationTest.java +++ b/javatests/dagger/internal/codegen/ProductionGraphValidationTest.java @@ -110,39 +110,40 @@ public class ProductionGraphValidationTest { } @Test public void provisionDependsOnProduction() { - JavaFileObject component = JavaFileObjects.forSourceLines("test.TestClass", - "package test;", - "", - "import com.google.common.util.concurrent.ListenableFuture;", - "import dagger.Module;", - "import dagger.Provides;", - "import dagger.producers.ProducerModule;", - "import dagger.producers.Produces;", - "import dagger.producers.ProductionComponent;", - "", - "final class TestClass {", - " interface A {}", - " interface B {}", - "", - " @Module", - " final class AModule {", - " @Provides A a(B b) {", - " return null;", - " }", - " }", - "", - " @ProducerModule", - " final class BModule {", - " @Produces ListenableFuture b() {", - " return null;", - " }", - " }", - "", - " @ProductionComponent(modules = {ExecutorModule.class, AModule.class, BModule.class})", - " interface AComponent {", - " ListenableFuture getA();", - " }", - "}"); + JavaFileObject component = + JavaFileObjects.forSourceLines( + "test.TestClass", + "package test;", + "", + "import com.google.common.util.concurrent.ListenableFuture;", + "import dagger.Provides;", + "import dagger.producers.ProducerModule;", + "import dagger.producers.Produces;", + "import dagger.producers.ProductionComponent;", + "", + "final class TestClass {", + " interface A {}", + " interface B {}", + "", + " @ProducerModule(includes = BModule.class)", + " final class AModule {", + " @Provides A a(B b) {", + " return null;", + " }", + " }", + "", + " @ProducerModule", + " final class BModule {", + " @Produces ListenableFuture b() {", + " return null;", + " }", + " }", + "", + " @ProductionComponent(modules = {ExecutorModule.class, AModule.class})", + " interface AComponent {", + " ListenableFuture getA();", + " }", + "}"); Compilation compilation = daggerCompiler().compile(EXECUTOR_MODULE, component); assertThat(compilation).failed(); @@ -150,6 +151,16 @@ public class ProductionGraphValidationTest { .hadErrorContaining("test.TestClass.A is a provision, which cannot depend on a production.") .inFile(component) .onLineContaining("interface AComponent"); + + compilation = + daggerCompiler() + .withOptions("-Adagger.moduleBindingValidation=ERROR") + .compile(EXECUTOR_MODULE, component); + assertThat(compilation).failed(); + assertThat(compilation) + .hadErrorContaining("test.TestClass.A is a provision, which cannot depend on a production.") + .inFile(component) + .onLineContaining("class AModule"); } @Test public void provisionEntryPointDependsOnProduction() { diff --git a/javatests/dagger/internal/codegen/ScopingValidationTest.java b/javatests/dagger/internal/codegen/ScopingValidationTest.java index cbe35801337..659c423842e 100644 --- a/javatests/dagger/internal/codegen/ScopingValidationTest.java +++ b/javatests/dagger/internal/codegen/ScopingValidationTest.java @@ -237,7 +237,48 @@ public void componentWithScopeIncludesIncompatiblyScopedBindings_Fail() { " @test.PerTest class test.ScopedType", " @Provides @test.PerTest String test.ScopedModule.string()", " @Provides @test.Per(test.MyComponent.class) boolean " - + "test.ScopedModule.bool()")); + + "test.ScopedModule.bool()")) + .inFile(componentFile) + .onLineContaining("interface MyComponent"); + + compilation = + daggerCompiler() + .withOptions("-Adagger.moduleBindingValidation=ERROR") + .compile(componentFile, scopeFile, scopeWithAttribute, typeFile, moduleFile); + // The @Inject binding for ScopedType should not appear here, but the @Singleton binding should. + assertThat(compilation) + .hadErrorContaining( + message( + "test.ScopedModule contains bindings with different scopes:", + " @Provides @test.PerTest String test.ScopedModule.string()", + " @Provides @Singleton float test.ScopedModule.floatingPoint()", + " @Provides @test.Per(test.MyComponent.class) boolean " + + "test.ScopedModule.bool()")) + .inFile(moduleFile) + .onLineContaining("class ScopedModule"); + } + + @Test + public void moduleBindingValidationDoesNotReportForOneScope() { + Compilation compilation = + daggerCompiler() + .withOptions("-Adagger.moduleBindingValidation=ERROR") + .compile( + JavaFileObjects.forSourceLines( + "test.TestModule", + "package test;", + "", + "import dagger.Module;", + "import dagger.Provides;", + "import javax.inject.Singleton;", + "", + "@Module", + "interface TestModule {", + " @Provides @Singleton static Object object() { return \"object\"; }", + " @Provides @Singleton static String string() { return \"string\"; }", + " @Provides static int integer() { return 4; }", + "}")); + assertThat(compilation).succeededWithoutWarnings(); } @Test