From 973be86a43a702e2d1c5e6a76769ee243babd9fa Mon Sep 17 00:00:00 2001 From: Vadym Shavalda <208031585+vadym-shavalda@users.noreply.github.com> Date: Wed, 23 Apr 2025 08:53:45 -0700 Subject: [PATCH] [PATCH] Add helpful message when using @Assisted with @Inject. Closes https://github.com/google/dagger/pull/4692. RELNOTES=N/A PiperOrigin-RevId: 750608984 --- .../codegen/DelegateComponentProcessor.java | 5 + .../AssistedProcessingStep.java | 77 ++---------- .../codegen/validation/AssistedValidator.java | 115 ++++++++++++++++++ .../codegen/validation/InjectValidator.java | 54 ++++---- .../codegen/AssistedFactoryErrorsTest.java | 26 ++++ 5 files changed, 186 insertions(+), 91 deletions(-) create mode 100644 dagger-compiler/main/java/dagger/internal/codegen/validation/AssistedValidator.java diff --git a/dagger-compiler/main/java/dagger/internal/codegen/DelegateComponentProcessor.java b/dagger-compiler/main/java/dagger/internal/codegen/DelegateComponentProcessor.java index 6793731b950..b41c02faa3d 100644 --- a/dagger-compiler/main/java/dagger/internal/codegen/DelegateComponentProcessor.java +++ b/dagger-compiler/main/java/dagger/internal/codegen/DelegateComponentProcessor.java @@ -45,6 +45,7 @@ import dagger.internal.codegen.kotlin.KotlinMetadataFactory; import dagger.internal.codegen.processingstep.ProcessingStepsModule; import dagger.internal.codegen.validation.AnyBindingMethodValidator; +import dagger.internal.codegen.validation.AssistedValidator; import dagger.internal.codegen.validation.BindingMethodValidatorsModule; import dagger.internal.codegen.validation.ComponentCreatorValidator; import dagger.internal.codegen.validation.ComponentValidator; @@ -186,6 +187,10 @@ interface ProcessingRoundCacheModule { @IntoSet ClearableCache componentCreatorValidator(ComponentCreatorValidator cache); + @Binds + @IntoSet + ClearableCache assistedValidator(AssistedValidator cache); + @Binds @IntoSet ClearableCache kotlinMetadata(KotlinMetadataFactory cache); diff --git a/dagger-compiler/main/java/dagger/internal/codegen/processingstep/AssistedProcessingStep.java b/dagger-compiler/main/java/dagger/internal/codegen/processingstep/AssistedProcessingStep.java index 86023f26207..13407ccf94c 100644 --- a/dagger-compiler/main/java/dagger/internal/codegen/processingstep/AssistedProcessingStep.java +++ b/dagger-compiler/main/java/dagger/internal/codegen/processingstep/AssistedProcessingStep.java @@ -16,21 +16,10 @@ package dagger.internal.codegen.processingstep; -import static androidx.room.compiler.processing.XElementKt.isConstructor; -import static androidx.room.compiler.processing.XElementKt.isMethod; -import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.assistedFactoryMethod; -import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.isAssistedFactoryType; -import static dagger.internal.codegen.xprocessing.XElements.asMethod; -import static dagger.internal.codegen.xprocessing.XElements.closestEnclosingTypeElement; -import static dagger.internal.codegen.xprocessing.XElements.getSimpleName; - import androidx.room.compiler.codegen.XClassName; -import androidx.room.compiler.processing.XExecutableElement; import androidx.room.compiler.processing.XExecutableParameterElement; -import androidx.room.compiler.processing.XTypeElement; import com.google.common.collect.ImmutableSet; -import dagger.internal.codegen.binding.InjectionAnnotations; -import dagger.internal.codegen.validation.ValidationReport; +import dagger.internal.codegen.validation.AssistedValidator; import dagger.internal.codegen.xprocessing.XTypeNames; import javax.inject.Inject; @@ -40,11 +29,11 @@ *

This processing step should run after {@link AssistedFactoryProcessingStep}. */ final class AssistedProcessingStep extends TypeCheckingProcessingStep { - private final InjectionAnnotations injectionAnnotations; + private final AssistedValidator assistedValidator; @Inject - AssistedProcessingStep(InjectionAnnotations injectionAnnotations) { - this.injectionAnnotations = injectionAnnotations; + AssistedProcessingStep(AssistedValidator assistedValidator) { + this.assistedValidator = assistedValidator; } @Override @@ -55,59 +44,11 @@ public ImmutableSet annotationClassNames() { @Override protected void process( XExecutableParameterElement assisted, ImmutableSet annotations) { - new AssistedValidator().validate(assisted).printMessagesTo(messager); - } - - private final class AssistedValidator { - ValidationReport validate(XExecutableParameterElement assisted) { - ValidationReport.Builder report = ValidationReport.about(assisted); - - XExecutableElement enclosingElement = assisted.getEnclosingElement(); - if (!isAssistedInjectConstructor(enclosingElement) - && !isAssistedFactoryCreateMethod(enclosingElement) - // The generated java stubs for kotlin data classes contain a "copy" method that has - // the same parameters (and annotations) as the constructor, so just ignore it. - && !isKotlinDataClassCopyMethod(enclosingElement)) { - report.addError( - "@Assisted parameters can only be used within an @AssistedInject-annotated " - + "constructor.", - assisted); - } - - injectionAnnotations - .getQualifiers(assisted) - .forEach( - qualifier -> - report.addError( - "Qualifiers cannot be used with @Assisted parameters.", assisted, qualifier)); - - return report.build(); + // If the AssistedValidator already validated this element as part of InjectValidator, then we + // don't need to report the errors again. + if (assistedValidator.containsCache(assisted)) { + return; } - } - - private boolean isAssistedInjectConstructor(XExecutableElement executableElement) { - return isConstructor(executableElement) - && executableElement.hasAnnotation(XTypeNames.ASSISTED_INJECT); - } - - private boolean isAssistedFactoryCreateMethod(XExecutableElement executableElement) { - if (isMethod(executableElement)) { - XTypeElement enclosingElement = closestEnclosingTypeElement(executableElement); - return isAssistedFactoryType(enclosingElement) - // This assumes we've already validated AssistedFactory and that a valid method exists. - && assistedFactoryMethod(enclosingElement).equals(executableElement); - } - return false; - } - - private boolean isKotlinDataClassCopyMethod(XExecutableElement executableElement) { - // Note: This is a best effort. Technically, we could check the return type and parameters of - // the copy method to verify it's the one associated with the constructor, but I'd rather keep - // this simple to avoid encoding too many details of kapt's stubs. At worst, we'll be allowing - // an @Assisted annotation that has no affect, which is already true for many of Dagger's other - // annotations. - return isMethod(executableElement) - && getSimpleName(asMethod(executableElement)).contentEquals("copy") - && closestEnclosingTypeElement(executableElement.getEnclosingElement()).isDataClass(); + assistedValidator.validate(assisted).printMessagesTo(messager); } } diff --git a/dagger-compiler/main/java/dagger/internal/codegen/validation/AssistedValidator.java b/dagger-compiler/main/java/dagger/internal/codegen/validation/AssistedValidator.java new file mode 100644 index 00000000000..82464b703b6 --- /dev/null +++ b/dagger-compiler/main/java/dagger/internal/codegen/validation/AssistedValidator.java @@ -0,0 +1,115 @@ +/* + * Copyright (C) 2025 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.validation; + +import static androidx.room.compiler.processing.XElementKt.isConstructor; +import static androidx.room.compiler.processing.XElementKt.isMethod; +import static com.google.common.base.Preconditions.checkArgument; +import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.assistedFactoryMethod; +import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.isAssistedFactoryType; +import static dagger.internal.codegen.xprocessing.XElements.asMethod; +import static dagger.internal.codegen.xprocessing.XElements.closestEnclosingTypeElement; +import static dagger.internal.codegen.xprocessing.XElements.getSimpleName; + +import androidx.room.compiler.processing.XExecutableElement; +import androidx.room.compiler.processing.XExecutableParameterElement; +import androidx.room.compiler.processing.XTypeElement; +import dagger.internal.codegen.base.ClearableCache; +import dagger.internal.codegen.binding.InjectionAnnotations; +import dagger.internal.codegen.xprocessing.XTypeNames; +import java.util.HashMap; +import java.util.Map; +import javax.inject.Inject; +import javax.inject.Singleton; + +/** Validates an {@link dagger.assisted.Assisted}-annotated parameter. */ +@Singleton +public final class AssistedValidator implements ClearableCache { + private final InjectionAnnotations injectionAnnotations; + private final Map cache = new HashMap<>(); + + @Inject + AssistedValidator(InjectionAnnotations injectionAnnotations) { + this.injectionAnnotations = injectionAnnotations; + } + + @Override + public void clearCache() { + cache.clear(); + } + + public boolean containsCache(XExecutableParameterElement assisted) { + return cache.containsKey(assisted); + } + + public ValidationReport validate(XExecutableParameterElement assisted) { + checkArgument(assisted.hasAnnotation(XTypeNames.ASSISTED)); + return cache.computeIfAbsent(assisted, this::validateUncached); + } + + + private ValidationReport validateUncached(XExecutableParameterElement assisted) { + ValidationReport.Builder report = ValidationReport.about(assisted); + + XExecutableElement enclosingElement = assisted.getEnclosingElement(); + if (!isAssistedInjectConstructor(enclosingElement) + && !isAssistedFactoryCreateMethod(enclosingElement) + // The generated java stubs for kotlin data classes contain a "copy" method that has + // the same parameters (and annotations) as the constructor, so just ignore it. + && !isKotlinDataClassCopyMethod(enclosingElement)) { + report.addError( + "@Assisted parameters can only be used within an @AssistedInject-annotated " + + "constructor.", + assisted); + } + + injectionAnnotations + .getQualifiers(assisted) + .forEach( + qualifier -> + report.addError( + "Qualifiers cannot be used with @Assisted parameters.", assisted, qualifier)); + + return report.build(); + } + + private boolean isAssistedInjectConstructor(XExecutableElement executableElement) { + return isConstructor(executableElement) + && executableElement.hasAnnotation(XTypeNames.ASSISTED_INJECT); + } + + private boolean isAssistedFactoryCreateMethod(XExecutableElement executableElement) { + if (isMethod(executableElement)) { + XTypeElement enclosingElement = closestEnclosingTypeElement(executableElement); + return isAssistedFactoryType(enclosingElement) + // This assumes we've already validated AssistedFactory and that a valid method exists. + && assistedFactoryMethod(enclosingElement).equals(executableElement); + } + return false; + } + + private boolean isKotlinDataClassCopyMethod(XExecutableElement executableElement) { + // Note: This is a best effort. Technically, we could check the return type and parameters of + // the copy method to verify it's the one associated with the constructor, but I'd rather keep + // this simple to avoid encoding too many details of kapt's stubs. At worst, we'll be allowing + // an @Assisted annotation that has no affect, which is already true for many of Dagger's other + // annotations. + return isMethod(executableElement) + && getSimpleName(asMethod(executableElement)).contentEquals("copy") + && closestEnclosingTypeElement(executableElement.getEnclosingElement()).isDataClass(); + } +} diff --git a/dagger-compiler/main/java/dagger/internal/codegen/validation/InjectValidator.java b/dagger-compiler/main/java/dagger/internal/codegen/validation/InjectValidator.java index 4bb8645c8b4..7ea333bd893 100644 --- a/dagger-compiler/main/java/dagger/internal/codegen/validation/InjectValidator.java +++ b/dagger-compiler/main/java/dagger/internal/codegen/validation/InjectValidator.java @@ -19,17 +19,15 @@ import static com.google.common.collect.Iterables.getOnlyElement; import static dagger.internal.codegen.base.Util.reentrantComputeIfAbsent; import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.assistedInjectedConstructors; +import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.isAssistedParameter; import static dagger.internal.codegen.binding.InjectionAnnotations.injectedConstructors; import static dagger.internal.codegen.binding.SourceFiles.factoryNameForElement; import static dagger.internal.codegen.binding.SourceFiles.membersInjectorNameForType; import static dagger.internal.codegen.extension.DaggerStreams.toImmutableList; import static dagger.internal.codegen.xprocessing.XElements.closestEnclosingTypeElement; -import static dagger.internal.codegen.xprocessing.XElements.getAnyAnnotation; import static dagger.internal.codegen.xprocessing.XMethodElements.hasTypeParameters; -import static dagger.internal.codegen.xprocessing.XTypeNames.injectTypeNames; import static dagger.internal.codegen.xprocessing.XTypes.isSubtype; -import androidx.room.compiler.codegen.XClassName; import androidx.room.compiler.codegen.XTypeName; import androidx.room.compiler.processing.XAnnotation; import androidx.room.compiler.processing.XConstructorElement; @@ -49,7 +47,6 @@ import dagger.internal.codegen.compileroption.CompilerOptions; import dagger.internal.codegen.langmodel.Accessibility; import dagger.internal.codegen.model.Scope; -import dagger.internal.codegen.xprocessing.XAnnotations; import dagger.internal.codegen.xprocessing.XTypeNames; import java.util.HashMap; import java.util.Map; @@ -72,6 +69,7 @@ public final class InjectValidator implements ClearableCache { private final MethodSignatureFormatter methodSignatureFormatter; private final InternalValidator validator; private final InternalValidator validatorWhenGeneratingCode; + private final AssistedValidator assistedValidator; @Inject InjectValidator( @@ -80,12 +78,14 @@ public final class InjectValidator implements ClearableCache { CompilerOptions compilerOptions, InjectionAnnotations injectionAnnotations, DaggerSuperficialValidation superficialValidation, - MethodSignatureFormatter methodSignatureFormatter) { + MethodSignatureFormatter methodSignatureFormatter, + AssistedValidator assistedValidator) { this.processingEnv = processingEnv; this.dependencyRequestValidator = dependencyRequestValidator; this.injectionAnnotations = injectionAnnotations; this.superficialValidation = superficialValidation; this.methodSignatureFormatter = methodSignatureFormatter; + this.assistedValidator = assistedValidator; // When validating types that require a generated factory class we need to error on private and // static inject members even if the compiler options are set to not error. @@ -191,21 +191,24 @@ private ValidationReport validateConstructor(XConstructorElement constructorElem ValidationReport.Builder builder = ValidationReport.about(constructorElement.getEnclosingElement()); - if (InjectionAnnotations.hasInjectAnnotation(constructorElement) - && constructorElement.hasAnnotation(XTypeNames.ASSISTED_INJECT)) { + boolean isInjectConstructor = InjectionAnnotations.hasInjectAnnotation(constructorElement); + boolean isAssistedInjectConstructor = + InjectionAnnotations.hasAssistedInjectAnnotation(constructorElement); + final String injectAnnotationName; + if (isInjectConstructor && isAssistedInjectConstructor) { builder.addError("Constructors cannot be annotated with both @Inject and @AssistedInject"); + // The rest of the validation assumes that only one of the annotations is present so return + // early if there are both. + return builder.build(); + } else if (isInjectConstructor) { + injectAnnotationName = "Inject"; + } else if (isAssistedInjectConstructor) { + injectAnnotationName = "AssistedInject"; + } else { + throw new AssertionError( + "No @Inject or @AssistedInject annotation found: " + constructorElement); } - XClassName injectAnnotation = - getAnyAnnotation( - constructorElement, - ImmutableSet.builder() - .addAll(injectTypeNames()) - .add(XTypeNames.ASSISTED_INJECT) - .build()) - .map(XAnnotations::asClassName) - .get(); - if (constructorElement.isPrivate()) { builder.addError( "Dagger does not support injection into private constructors", constructorElement); @@ -221,7 +224,7 @@ private ValidationReport validateConstructor(XConstructorElement constructorElem builder.addError( String.format( "@Qualifier annotations are not allowed on @%s constructors", - injectAnnotation.getSimpleName()), + injectAnnotationName), constructorElement, qualifier); } @@ -229,7 +232,7 @@ private ValidationReport validateConstructor(XConstructorElement constructorElem String scopeErrorMsg = String.format( "@Scope annotations are not allowed on @%s constructors", - injectAnnotation.getSimpleName()); + injectAnnotationName); if (InjectionAnnotations.hasInjectAnnotation(constructorElement)) { scopeErrorMsg += "; annotate the class instead"; @@ -243,14 +246,19 @@ private ValidationReport validateConstructor(XConstructorElement constructorElem for (XExecutableParameterElement parameter : constructorElement.getParameters()) { superficialValidation.validateTypeOf(parameter); - validateDependencyRequest(builder, parameter); + if (isAssistedParameter(parameter)) { + builder.addSubreport(assistedValidator.validate(parameter)); + } else { + // Only validate dependency requests for non-assisted parameters. + validateDependencyRequest(builder, parameter); + } } if (throwsCheckedExceptions(constructorElement)) { builder.addItem( String.format( "Dagger does not support checked exceptions on @%s constructors", - injectAnnotation.getSimpleName()), + injectAnnotationName), privateMemberDiagnosticKind, constructorElement); } @@ -262,7 +270,7 @@ private ValidationReport validateConstructor(XConstructorElement constructorElem builder.addError( String.format( "@%s is nonsense on the constructor of an abstract class", - injectAnnotation.getSimpleName()), + injectAnnotationName), constructorElement); } @@ -271,7 +279,7 @@ private ValidationReport validateConstructor(XConstructorElement constructorElem String.format( "@%s constructors are invalid on inner classes. " + "Did you mean to make the class static?", - injectAnnotation.getSimpleName()), + injectAnnotationName), constructorElement); } diff --git a/javatests/dagger/internal/codegen/AssistedFactoryErrorsTest.java b/javatests/dagger/internal/codegen/AssistedFactoryErrorsTest.java index a266fd111a6..f11b81b9105 100644 --- a/javatests/dagger/internal/codegen/AssistedFactoryErrorsTest.java +++ b/javatests/dagger/internal/codegen/AssistedFactoryErrorsTest.java @@ -768,6 +768,32 @@ public void testMultipleInjectAnnotations() { }); } + @Test + public void testInjectWithAssistedAnnotations() { + Source foo = + CompilerTests.javaSource( + "test.Foo", + "package test;", + "", + "import dagger.assisted.Assisted;", + "import javax.inject.Inject;", + "", + "class Foo {", + " @Inject", + " Foo(@Assisted int i) {}", + "}"); + + CompilerTests.daggerCompiler(foo) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining( + "@Assisted parameters can only be used within an @AssistedInject-annotated " + + "constructor"); + }); + } + @Test public void testAssistedInjectWithNoAssistedParametersIsNotInjectable() { Source foo =