From ad37e58ae4383b30aa4dd6015eb7113fcf289440 Mon Sep 17 00:00:00 2001 From: Brad Corso Date: Tue, 18 Aug 2026 13:32:15 -0700 Subject: [PATCH] Move full binding graph validation for modules from ModuleValidator to ModuleProcessingStep. This change shifts the full binding graph validation of modules out of ModuleValidator (which is now only responsible for validating the individual module) and into ModuleProcessingStep. This is consistent with how we handle binding graph validation for components, which is done in ComponentProcessingStep rather than ComponentValidator. This also improves error reporting by pointing directly to the errors rather than generating secondary "module has errors" messages on referencing components. RELNOTES=N/A PiperOrigin-RevId: 966775864 --- .../processingstep/ModuleProcessingStep.java | 43 +++++++++++++------ .../codegen/validation/ModuleValidator.java | 30 ------------- .../DuplicateBindingsValidationTest.java | 39 +++++++++++------ .../FullBindingGraphValidationTest.java | 8 ++-- 4 files changed, 61 insertions(+), 59 deletions(-) diff --git a/dagger-compiler/main/java/dagger/internal/codegen/processingstep/ModuleProcessingStep.java b/dagger-compiler/main/java/dagger/internal/codegen/processingstep/ModuleProcessingStep.java index 922283f436e..fd22f2777cf 100644 --- a/dagger-compiler/main/java/dagger/internal/codegen/processingstep/ModuleProcessingStep.java +++ b/dagger-compiler/main/java/dagger/internal/codegen/processingstep/ModuleProcessingStep.java @@ -25,13 +25,16 @@ import androidx.room3.compiler.processing.XProcessingEnv; import androidx.room3.compiler.processing.XTypeElement; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; import dagger.internal.codegen.base.SourceFileGenerator; import dagger.internal.codegen.base.ValidationReport; import dagger.internal.codegen.binding.BindingFactory; +import dagger.internal.codegen.binding.BindingGraph; +import dagger.internal.codegen.binding.BindingGraphFactory; +import dagger.internal.codegen.binding.ComponentDescriptor; import dagger.internal.codegen.binding.ContributionBinding; import dagger.internal.codegen.binding.DelegateDeclaration; import dagger.internal.codegen.binding.ProductionBinding; +import dagger.internal.codegen.validation.BindingGraphValidator; import dagger.internal.codegen.validation.ModuleValidator; import dagger.internal.codegen.writing.InaccessibleMapKeyProxyGenerator; import dagger.internal.codegen.writing.ModuleGenerator; @@ -52,7 +55,9 @@ final class ModuleProcessingStep extends TypeCheckingProcessingStep moduleConstructorProxyGenerator; private final InaccessibleMapKeyProxyGenerator inaccessibleMapKeyProxyGenerator; private final DelegateDeclaration.Factory delegateDeclarationFactory; - private final Set processedModuleElements = Sets.newLinkedHashSet(); + private final ComponentDescriptor.Factory componentDescriptorFactory; + private final BindingGraphFactory bindingGraphFactory; + private final BindingGraphValidator bindingGraphValidator; @Inject ModuleProcessingStep( @@ -62,7 +67,10 @@ final class ModuleProcessingStep extends TypeCheckingProcessingStep producerFactoryGenerator, @ModuleGenerator SourceFileGenerator moduleConstructorProxyGenerator, InaccessibleMapKeyProxyGenerator inaccessibleMapKeyProxyGenerator, - DelegateDeclaration.Factory delegateDeclarationFactory) { + DelegateDeclaration.Factory delegateDeclarationFactory, + ComponentDescriptor.Factory componentDescriptorFactory, + BindingGraphFactory bindingGraphFactory, + BindingGraphValidator bindingGraphValidator) { this.moduleValidator = moduleValidator; this.bindingFactory = bindingFactory; this.factoryGenerator = factoryGenerator; @@ -70,6 +78,9 @@ final class ModuleProcessingStep extends TypeCheckingProcessingStep process( @Override protected void process(XTypeElement module, ImmutableSet annotations) { - if (processedModuleElements.contains(module)) { - return; - } // For backwards compatibility, we allow a companion object to be annotated with @Module even // though it's no longer required. However, we skip processing the companion object itself // because it will now be processed when processing the companion object's enclosing class. @@ -104,14 +112,23 @@ protected void process(XTypeElement module, ImmutableSet annotations } ValidationReport report = moduleValidator.validate(module); report.printMessagesTo(messager); - if (report.isClean()) { - generateForMethodsIn(module); - module.getEnclosedTypeElements().stream() - .filter(XTypeElement::isCompanionObject) - .collect(toOptional()) - .ifPresent(this::generateForMethodsIn); + if (!report.isClean()) { + return; + } + if (bindingGraphValidator.shouldDoFullBindingGraphValidation(module)) { + BindingGraph bindingGraph = + bindingGraphFactory.create( + componentDescriptorFactory.moduleComponentDescriptor(module), + /* createFullBindingGraph= */ true); + if (!bindingGraphValidator.isValid(bindingGraph.topLevelBindingGraph())) { + return; + } } - processedModuleElements.add(module); + generateForMethodsIn(module); + module.getEnclosedTypeElements().stream() + .filter(XTypeElement::isCompanionObject) + .collect(toOptional()) + .ifPresent(this::generateForMethodsIn); } private void generateForMethodsIn(XTypeElement module) { diff --git a/dagger-compiler/main/java/dagger/internal/codegen/validation/ModuleValidator.java b/dagger-compiler/main/java/dagger/internal/codegen/validation/ModuleValidator.java index 213e0bbb36c..5a187376fbd 100644 --- a/dagger-compiler/main/java/dagger/internal/codegen/validation/ModuleValidator.java +++ b/dagger-compiler/main/java/dagger/internal/codegen/validation/ModuleValidator.java @@ -59,12 +59,9 @@ import dagger.internal.codegen.base.DaggerSuperficialValidation; import dagger.internal.codegen.base.ModuleKind; import dagger.internal.codegen.base.ValidationReport; -import dagger.internal.codegen.binding.BindingGraphFactory; -import dagger.internal.codegen.binding.ComponentDescriptor; import dagger.internal.codegen.binding.ComponentRequirement; import dagger.internal.codegen.binding.InjectionAnnotations; import dagger.internal.codegen.binding.MethodSignatureFormatter; -import dagger.internal.codegen.model.BindingGraph; import dagger.internal.codegen.model.Scope; import dagger.internal.codegen.xprocessing.XElements; import dagger.internal.codegen.xprocessing.XTypeNames; @@ -111,9 +108,6 @@ public final class ModuleValidator { private final AnyBindingMethodValidator anyBindingMethodValidator; private final MethodSignatureFormatter methodSignatureFormatter; - private final ComponentDescriptor.Factory componentDescriptorFactory; - private final BindingGraphFactory bindingGraphFactory; - private final BindingGraphValidator bindingGraphValidator; private final InjectionAnnotations injectionAnnotations; private final DaggerSuperficialValidation superficialValidation; private final XProcessingEnv processingEnv; @@ -124,17 +118,11 @@ public final class ModuleValidator { ModuleValidator( AnyBindingMethodValidator anyBindingMethodValidator, MethodSignatureFormatter methodSignatureFormatter, - ComponentDescriptor.Factory componentDescriptorFactory, - BindingGraphFactory bindingGraphFactory, - BindingGraphValidator bindingGraphValidator, InjectionAnnotations injectionAnnotations, DaggerSuperficialValidation superficialValidation, XProcessingEnv processingEnv) { this.anyBindingMethodValidator = anyBindingMethodValidator; this.methodSignatureFormatter = methodSignatureFormatter; - this.componentDescriptorFactory = componentDescriptorFactory; - this.bindingGraphFactory = bindingGraphFactory; - this.bindingGraphValidator = bindingGraphValidator; this.injectionAnnotations = injectionAnnotations; this.superficialValidation = superficialValidation; this.processingEnv = processingEnv; @@ -214,11 +202,6 @@ private ValidationReport validateUncached(XTypeElement module, Set .collect(toOptional()) .ifPresent(companionModule -> validateCompanionModule(companionModule, builder)); - if (builder.build().isClean() - && bindingGraphValidator.shouldDoFullBindingGraphValidation(module)) { - validateModuleBindings(module, builder); - } - return builder.build(); } @@ -634,19 +617,6 @@ private void validateCompanionModule( } } - private void validateModuleBindings(XTypeElement module, ValidationReport.Builder report) { - BindingGraph bindingGraph = - bindingGraphFactory - .create(componentDescriptorFactory.moduleComponentDescriptor(module), true) - .topLevelBindingGraph(); - if (!bindingGraphValidator.isValid(bindingGraph)) { - // Since the validator uses 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/javatests/dagger/internal/codegen/DuplicateBindingsValidationTest.java b/javatests/dagger/internal/codegen/DuplicateBindingsValidationTest.java index a28b906e1c4..0d1a25ba208 100644 --- a/javatests/dagger/internal/codegen/DuplicateBindingsValidationTest.java +++ b/javatests/dagger/internal/codegen/DuplicateBindingsValidationTest.java @@ -739,15 +739,18 @@ public void childBindingConflictsWithParent() { " @Provides Object test.B.BModule.abConflict()"); if (fullBindingGraphValidation) { subject.hasErrorCount(2); - subject.hasErrorContaining("test.A.AModule has errors") + subject + .hasErrorContaining(errorMessage) .onSource(aComponent) - .onLineContaining("@Component("); - subject.hasErrorContaining(errorMessage) + .onLineContaining("interface A {"); + subject + .hasErrorContaining(errorMessage) .onSource(aComponent) .onLineContaining("class AModule"); } else { subject.hasErrorCount(1); - subject.hasErrorContaining(errorMessage) + subject + .hasErrorContaining(errorMessage) .onSource(aComponent) .onLineContaining("interface A {"); } @@ -834,15 +837,18 @@ public void grandchildBindingConflictsWithGrandparent() { " @Provides Object test.C.CModule.acConflict()"); if (fullBindingGraphValidation) { subject.hasErrorCount(2); - subject.hasErrorContaining("test.A.AModule has errors") + subject + .hasErrorContaining(errorMessage) .onSource(aComponent) - .onLineContaining("@Component("); - subject.hasErrorContaining(errorMessage) + .onLineContaining("interface A {"); + subject + .hasErrorContaining(errorMessage) .onSource(aComponent) .onLineContaining("class AModule"); } else { subject.hasErrorCount(1); - subject.hasErrorContaining(errorMessage) + subject + .hasErrorContaining(errorMessage) .onSource(aComponent) .onLineContaining("interface A {"); } @@ -923,16 +929,23 @@ public void grandchildBindingConflictsWithChild() { " @Provides Object test.B.BModule.bcConflict()", " @Provides Object test.C.CModule.bcConflict()"); if (fullBindingGraphValidation) { - subject.hasErrorCount(2); - subject.hasErrorContaining("test.B.BModule has errors") + subject.hasErrorCount(3); + subject + .hasErrorContaining(errorMessage) + .onSource(aComponent) + .onLineContaining("interface A {"); + subject + .hasErrorContaining(errorMessage) .onSource(bComponent) - .onLineContaining("@Subcomponent(modules = B.BModule.class)"); - subject.hasErrorContaining(errorMessage) + .onLineContaining("interface B {"); + subject + .hasErrorContaining(errorMessage) .onSource(bComponent) .onLineContaining("class BModule"); } else { subject.hasErrorCount(1); - subject.hasErrorContaining(errorMessage) + subject + .hasErrorContaining(errorMessage) .onSource(aComponent) .onLineContaining("interface A {"); } diff --git a/javatests/dagger/internal/codegen/FullBindingGraphValidationTest.java b/javatests/dagger/internal/codegen/FullBindingGraphValidationTest.java index 50ad0a6e014..5988b4ed903 100644 --- a/javatests/dagger/internal/codegen/FullBindingGraphValidationTest.java +++ b/javatests/dagger/internal/codegen/FullBindingGraphValidationTest.java @@ -140,12 +140,14 @@ public void includesModuleWithErrors_validationTypeError() { .compile( subject -> { subject.hasErrorCount(2); - subject.hasErrorContainingMatch(MODULE_WITH_ERRORS_MESSAGE.pattern()) + subject + .hasErrorContainingMatch(MODULE_WITH_ERRORS_MESSAGE.pattern()) .onSource(MODULE_WITH_ERRORS) .onLineContaining("interface ModuleWithErrors"); - subject.hasErrorContaining("ModuleWithErrors has errors") + subject + .hasErrorContainingMatch(INCLUDES_MODULE_WITH_ERRORS_MESSAGE.pattern()) .onSource(INCLUDES_MODULE_WITH_ERRORS) - .onLineContaining("ModuleWithErrors.class"); + .onLineContaining("interface IncludesModuleWithErrors"); }); }