From 492a213c58006797ccc7d22e404bc28ff0a93cef Mon Sep 17 00:00:00 2001 From: Brad Corso Date: Fri, 29 Sep 2023 10:34:47 -0700 Subject: [PATCH] Consolidate logic for whether we generate MembersInjector in one place. The logic for whether we generate a MembersInjector was duplicated in `InjectBindingRegistryImpl` and `MembersInjectorGenerator`. In particular, there are cases where `InjectBindingRegistryImpl` decided that a MembersInjector should be generated, but additional logic in `MembersInjectorGenerator` overrides that decision and decides not to generate the MembersInjector (e.g. if the class has no local injection sites). This led to duplicating of logic in `InjectBindingRegistryImpl` when we're trying to decide if we should output a warning for the case where a MembersInjector should be generated in an upstream library because we needed to repeat the logic in `MembersInjectorGenerator` to know if the MembersInjector will actually be generated. This CL moves the decision on whether to generate a MembersInjector completely into `InjectBindingRegistryImpl`, which removes the duplicated logic and also makes it easier to understand when the MembersInjector will actually be generated since we only need to look in one location. This CL also removes `warnIfNotAlreadyGenerated` since `isCalledFromInjectProcessor` contains all the information we need to decide whether to warn in this case. RELNOTES=N/A PiperOrigin-RevId: 569528883 --- .../validation/InjectBindingRegistryImpl.java | 87 +++++-------------- .../writing/MembersInjectorGenerator.java | 15 ---- .../codegen/ComponentProcessorTest.java | 70 ++++++++++++--- 3 files changed, 81 insertions(+), 91 deletions(-) diff --git a/java/dagger/internal/codegen/validation/InjectBindingRegistryImpl.java b/java/dagger/internal/codegen/validation/InjectBindingRegistryImpl.java index 13bee7f379b..16344d3f3a0 100644 --- a/java/dagger/internal/codegen/validation/InjectBindingRegistryImpl.java +++ b/java/dagger/internal/codegen/validation/InjectBindingRegistryImpl.java @@ -118,10 +118,7 @@ B getBinding(Key key) { } /** Caches the binding and generates it if it needs generation. */ - void tryRegisterBinding( - B binding, - boolean warnIfNotAlreadyGenerated, - boolean isCalledFromInjectProcessor) { + void tryRegisterBinding(B binding, boolean isCalledFromInjectProcessor) { if (processingEnv.getBackend() == XProcessingEnv.Backend.KSP) { Origin origin = toKS(closestEnclosingTypeElement(binding.bindingElement().get())).getOrigin(); @@ -143,21 +140,18 @@ void tryRegisterBinding( @SuppressWarnings("unchecked") B maybeUnresolved = binding.unresolved().isPresent() ? (B) binding.unresolved().get() : binding; - tryToGenerateBinding(maybeUnresolved, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); + tryToGenerateBinding(maybeUnresolved, isCalledFromInjectProcessor); } /** * Tries to generate a binding, not generating if it already is generated. For resolved * bindings, this will try to generate the unresolved version of the binding. */ - void tryToGenerateBinding( - B binding, - boolean warnIfNotAlreadyGenerated, - boolean isCalledFromInjectProcessor) { + void tryToGenerateBinding(B binding, boolean isCalledFromInjectProcessor) { if (shouldGenerateBinding(binding)) { bindingsRequiringGeneration.offer(binding); if (compilerOptions.warnIfInjectionFactoryNotGeneratedUpstream() - && warnIfNotAlreadyGenerated) { + && !isCalledFromInjectProcessor) { messager.printMessage( Kind.NOTE, String.format( @@ -172,6 +166,22 @@ void tryToGenerateBinding( /** Returns true if the binding needs to be generated. */ private boolean shouldGenerateBinding(B binding) { + if (binding instanceof MembersInjectionBinding) { + MembersInjectionBinding membersInjectionBinding = (MembersInjectionBinding) binding; + // Empty members injection bindings are special and don't need source files. + if (membersInjectionBinding.injectionSites().isEmpty()) { + return false; + } + // Members injectors for classes with no local injection sites and no @Inject + // constructor are unused. + boolean hasInjectConstructor = + !(injectedConstructors(membersInjectionBinding.membersInjectedType()).isEmpty() + && assistedInjectedConstructors( + membersInjectionBinding.membersInjectedType()).isEmpty()); + if (!membersInjectionBinding.hasLocalInjectionSites() && !hasInjectConstructor) { + return false; + } + } return !binding.unresolved().isPresent() && !materializedBindingKeys.contains(binding.key()) && !bindingsRequiringGeneration.contains(binding) @@ -225,56 +235,12 @@ public void generateSourcesForRequiredBindings( membersInjectionBindings.generateBindings(membersInjectorGenerator); } - /** - * Registers the binding for generation and later lookup. If the binding is resolved, we also - * attempt to register an unresolved version of it. - */ - private void registerBinding( - ProvisionBinding binding, - boolean warnIfNotAlreadyGenerated, - boolean isCalledFromInjectProcessor) { - provisionBindings.tryRegisterBinding( - binding, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); - } - - /** - * Registers the binding for generation and later lookup. If the binding is resolved, we also - * attempt to register an unresolved version of it. - */ - private void registerBinding( - MembersInjectionBinding binding, - boolean warnIfNotAlreadyGenerated, - boolean isCalledFromInjectProcessor) { - /* - * We generate MembersInjector classes for types with @Inject constructors only if they have any - * injection sites. - * - * We generate MembersInjector classes for types without @Inject constructors only if they have - * local (non-inherited) injection sites. - * - * Warn only when registering bindings post-hoc for those types. - */ - if (warnIfNotAlreadyGenerated) { - boolean hasInjectConstructor = - !(injectedConstructors(binding.membersInjectedType()).isEmpty() - && assistedInjectedConstructors(binding.membersInjectedType()).isEmpty()); - warnIfNotAlreadyGenerated = - hasInjectConstructor - ? !binding.injectionSites().isEmpty() - : binding.hasLocalInjectionSites(); - } - - membersInjectionBindings - .tryRegisterBinding(binding, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); - } - @Override public Optional tryRegisterInjectConstructor( XConstructorElement constructorElement) { return tryRegisterConstructor( constructorElement, Optional.empty(), - /* warnIfNotAlreadyGenerated= */ false, /* isCalledFromInjectProcessor= */ true); } @@ -282,7 +248,6 @@ public Optional tryRegisterInjectConstructor( private Optional tryRegisterConstructor( XConstructorElement constructorElement, Optional resolvedType, - boolean warnIfNotAlreadyGenerated, boolean isCalledFromInjectProcessor) { XTypeElement typeElement = constructorElement.getEnclosingElement(); @@ -301,10 +266,9 @@ private Optional tryRegisterConstructor( } ProvisionBinding binding = bindingFactory.injectionBinding(constructorElement, resolvedType); - registerBinding(binding, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); + provisionBindings.tryRegisterBinding(binding, isCalledFromInjectProcessor); if (!binding.injectionSites().isEmpty()) { - tryRegisterMembersInjectedType( - typeElement, resolvedType, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); + tryRegisterMembersInjectedType(typeElement, resolvedType, isCalledFromInjectProcessor); } return Optional.of(binding); } @@ -322,7 +286,6 @@ public Optional tryRegisterInjectField(XFieldElement fi return tryRegisterMembersInjectedType( asTypeElement(fieldElement.getEnclosingElement()), Optional.empty(), - /* warnIfNotAlreadyGenerated= */ false, /* isCalledFromInjectProcessor= */ true); } @@ -339,7 +302,6 @@ public Optional tryRegisterInjectMethod(XMethodElement return tryRegisterMembersInjectedType( asTypeElement(methodElement.getEnclosingElement()), Optional.empty(), - /* warnIfNotAlreadyGenerated= */ false, /* isCalledFromInjectProcessor= */ true); } @@ -347,7 +309,6 @@ public Optional tryRegisterInjectMethod(XMethodElement private Optional tryRegisterMembersInjectedType( XTypeElement typeElement, Optional resolvedType, - boolean warnIfNotAlreadyGenerated, boolean isCalledFromInjectProcessor) { // Validating here shouldn't have a performance penalty because the validator caches its reports ValidationReport report = injectValidator.validateForMembersInjection(typeElement); @@ -364,7 +325,7 @@ private Optional tryRegisterMembersInjectedType( } MembersInjectionBinding binding = bindingFactory.membersInjectionBinding(type, resolvedType); - registerBinding(binding, warnIfNotAlreadyGenerated, isCalledFromInjectProcessor); + membersInjectionBindings.tryRegisterBinding(binding, isCalledFromInjectProcessor); for (Optional supertype = nonObjectSuperclass(type); supertype.isPresent(); supertype = nonObjectSuperclass(supertype.get())) { @@ -404,7 +365,6 @@ public Optional getOrFindProvisionBinding(Key key) { tryRegisterConstructor( constructor, Optional.of(type), - /* warnIfNotAlreadyGenerated= */ true, /* isCalledFromInjectProcessor= */ false)); } @@ -421,7 +381,6 @@ public Optional getOrFindMembersInjectionBinding(Key ke return tryRegisterMembersInjectedType( key.type().xprocessing().getTypeElement(), Optional.of(key.type().xprocessing()), - /* warnIfNotAlreadyGenerated= */ true, /* isCalledFromInjectProcessor= */ false); } diff --git a/java/dagger/internal/codegen/writing/MembersInjectorGenerator.java b/java/dagger/internal/codegen/writing/MembersInjectorGenerator.java index 82d12ddb5c7..ea2233c44b5 100644 --- a/java/dagger/internal/codegen/writing/MembersInjectorGenerator.java +++ b/java/dagger/internal/codegen/writing/MembersInjectorGenerator.java @@ -20,8 +20,6 @@ import static com.squareup.javapoet.MethodSpec.constructorBuilder; import static com.squareup.javapoet.MethodSpec.methodBuilder; import static com.squareup.javapoet.TypeSpec.classBuilder; -import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.assistedInjectedConstructors; -import static dagger.internal.codegen.binding.InjectionAnnotations.injectedConstructors; import static dagger.internal.codegen.binding.SourceFiles.bindingTypeElementTypeVariableNames; import static dagger.internal.codegen.binding.SourceFiles.generateBindingFieldsForDependencies; import static dagger.internal.codegen.binding.SourceFiles.membersInjectorNameForType; @@ -90,19 +88,6 @@ public XElement originatingElement(MembersInjectionBinding binding) { @Override public ImmutableList topLevelTypes(MembersInjectionBinding binding) { - // Empty members injection bindings are special and don't need source files. - if (binding.injectionSites().isEmpty()) { - return ImmutableList.of(); - } - - // Members injectors for classes with no local injection sites and no @Inject - // constructor are unused. - if (!binding.hasLocalInjectionSites() - && injectedConstructors(binding.membersInjectedType()).isEmpty() - && assistedInjectedConstructors(binding.membersInjectedType()).isEmpty()) { - return ImmutableList.of(); - } - // We don't want to write out resolved bindings -- we want to write out the generic version. checkState( diff --git a/javatests/dagger/internal/codegen/ComponentProcessorTest.java b/javatests/dagger/internal/codegen/ComponentProcessorTest.java index 931ba903662..a6dc2d6c2ac 100644 --- a/javatests/dagger/internal/codegen/ComponentProcessorTest.java +++ b/javatests/dagger/internal/codegen/ComponentProcessorTest.java @@ -16,7 +16,13 @@ package dagger.internal.codegen; +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; +import static org.junit.Assert.assertThrows; + +import androidx.room.compiler.processing.util.CompilationResultSubject; import androidx.room.compiler.processing.util.Source; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.squareup.javapoet.AnnotationSpec; import com.squareup.javapoet.MethodSpec; @@ -27,6 +33,7 @@ import dagger.testing.golden.GoldenFileRule; import java.util.Collection; import javax.inject.Inject; +import javax.tools.Diagnostic; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -1380,21 +1387,60 @@ public void unprocessedMembersInjectorNotes() { subject -> { subject.hasErrorCount(0); subject.hasWarningCount(0); - subject.hasNoteContaining( - String.format( - "Generating a MembersInjector for dagger.internal.codegen.%s.", - "ComponentProcessorTestClasses.LocalInjectMemberNoConstructor")); - subject.hasNoteContaining( - String.format( - "Generating a MembersInjector for dagger.internal.codegen.%s.", - "ComponentProcessorTestClasses.LocalInjectMemberWithConstructor")); - subject.hasNoteContaining( - String.format( - "Generating a MembersInjector for dagger.internal.codegen.%s.", - "ComponentProcessorTestClasses.ParentInjectMemberWithConstructor")); + + String generatedFileTemplate = + "dagger/internal/codegen/ComponentProcessorTestClasses_%s_MembersInjector.java"; + String noteTemplate = + "Generating a MembersInjector for " + + "dagger.internal.codegen.ComponentProcessorTestClasses.%s."; + + // Assert that we generate sources and notes for the following classes. + ImmutableList.of( + "LocalInjectMemberNoConstructor", + "LocalInjectMemberWithConstructor", + "ParentInjectMemberWithConstructor") + .forEach( + className -> { + subject.generatedSourceFileWithPath( + String.format(generatedFileTemplate, className)); + subject.hasNoteContaining(String.format(noteTemplate, className)); + }); + + // Assert that we **do not** generate sources and notes for the following classes. + ImmutableList.of( + "ParentInjectMemberNoConstructor", + "NoInjectMemberNoConstructor", + "NoInjectMemberWithConstructor") + .forEach( + className -> { + assertFileNotGenerated( + subject, String.format(generatedFileTemplate, className)); + assertDoesNotHaveNoteContaining( + subject, String.format(noteTemplate, className)); + }); }); } + private void assertFileNotGenerated(CompilationResultSubject subject, String filePath) { + // TODO(b/303653163): replace this with better XProcessing API once we have the ability to get a + // list of all generated sources. + AssertionError error = + assertThrows( + AssertionError.class, + () -> subject.generatedSourceFileWithPath(filePath)); + assertThat(error).hasMessageThat().contains("Didn't generate file"); + } + + private void assertDoesNotHaveNoteContaining(CompilationResultSubject subject, String content) { + assertThat( + subject + .getCompilationResult() + .getDiagnostics() + .get(Diagnostic.Kind.NOTE) + .stream() + .filter(diagnostic -> diagnostic.getMsg().contains(content))).isEmpty(); + } + @Test public void scopeAnnotationOnInjectConstructorNotValid() { Source aScope =