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 =