From 6c45b9d476a6e0495745a5108a470260d9eef727 Mon Sep 17 00:00:00 2001 From: Brad Corso Date: Tue, 14 Dec 2021 12:14:26 -0800 Subject: [PATCH] Remove superficial validation for cases where we don't need it. This CL removes superficial validation in 3 special cases: 1. If a members injected type has no `@Inject` constructor, we don't need to do superficial validation on the type's annotations because Dagger does not create the type, so we ignore scopes even if they're present. 2. For each super class, we don't need to do superficial validation on the type's annotations because Dagger only relies on the scopes of the subtype. 3. For each super class, we don't need to do superficial validation on the constructor because Dagger only relies on the constructor of the subtype. This will hopefully help cases like https://github.com/google/dagger/issues/3090, where the current superficial validation can fail on non-Dagger related types. RELNOTES=N/A PiperOrigin-RevId: 416363991 --- .../codegen/validation/InjectValidator.java | 15 ++++- .../SuperficialInjectValidator.java | 55 +++++++++++++++++-- 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/java/dagger/internal/codegen/validation/InjectValidator.java b/java/dagger/internal/codegen/validation/InjectValidator.java index 46715b488fd..29807ad18c3 100644 --- a/java/dagger/internal/codegen/validation/InjectValidator.java +++ b/java/dagger/internal/codegen/validation/InjectValidator.java @@ -146,7 +146,7 @@ public ValidationReport validate(XTypeElement typeElement) { } private ValidationReport validateUncached(XTypeElement typeElement) { - superficialInjectValidator.throwIfNotValid(typeElement); + superficialInjectValidator.throwIfInjectTypeNotValid(typeElement); ValidationReport.Builder builder = ValidationReport.about(typeElement); builder.addSubreport(validateMembersInjectionType(typeElement)); @@ -169,6 +169,17 @@ private ValidationReport validateUncached(XTypeElement typeElement) { return builder.build(); } + private ValidationReport validateSupertype(XTypeElement typeElement) { + superficialInjectValidator.throwIfInjectSuperTypeNotValid(typeElement); + + // Note: For super types, we don't look at constructors. Even if the super type has an @Inject + // constructor, we don't care in this case because it's being requested as a subtype and Dagger + // will use the subtype's constructor to create the object if it needs to. + return ValidationReport.about(typeElement) + .addSubreport(validateMembersInjectionType(typeElement)) + .build(); + } + private ValidationReport validateConstructor(XConstructorElement constructorElement) { ValidationReport.Builder builder = ValidationReport.about(constructorElement.getEnclosingElement()); @@ -362,7 +373,7 @@ private ValidationReport validateMembersInjectionType(XTypeElement typeElement) checkInjectIntoKotlinObject(typeElement, builder); } if (typeElement.getSuperType() != null) { - ValidationReport report = validate(typeElement.getSuperType().getTypeElement()); + ValidationReport report = validateSupertype(typeElement.getSuperType().getTypeElement()); if (!report.isClean()) { builder.addSubreport(report); } diff --git a/java/dagger/internal/codegen/validation/SuperficialInjectValidator.java b/java/dagger/internal/codegen/validation/SuperficialInjectValidator.java index 28338a584ad..c5927a8fce9 100644 --- a/java/dagger/internal/codegen/validation/SuperficialInjectValidator.java +++ b/java/dagger/internal/codegen/validation/SuperficialInjectValidator.java @@ -18,6 +18,7 @@ import static androidx.room.compiler.processing.compat.XConverters.toJavac; import static dagger.internal.codegen.langmodel.DaggerElements.isAnnotationPresent; +import static javax.lang.model.util.ElementFilter.constructorsIn; import androidx.room.compiler.processing.XTypeElement; import dagger.internal.codegen.base.ClearableCache; @@ -27,20 +28,37 @@ import java.util.Map; import javax.inject.Inject; import javax.inject.Singleton; +import javax.lang.model.element.ElementKind; import javax.lang.model.element.TypeElement; /** Validates inject types in a round. */ +// TODO(bcorso): InjectValidator also handles @AssistedInject constructors, so we should update this +// class to do superficial validation for @AssistedInject constructors too. @Singleton public final class SuperficialInjectValidator implements ClearableCache { - private final Map validatedTypeElements = new HashMap<>(); + // We keep two separate caches because the same type might need validation as both an inject type + // and an inject super type, and we validate different things depending on that context. + private final Map validatedInjectTypeElements = + new HashMap<>(); + private final Map validatedInjectSuperTypeElements = + new HashMap<>(); @Inject SuperficialInjectValidator() {} - public void throwIfNotValid(XTypeElement injectTypeElement) { + public void throwIfInjectTypeNotValid(XTypeElement injectTypeElement) { ValidationException validationException = - validatedTypeElements.computeIfAbsent(injectTypeElement, this::validate); + validatedInjectTypeElements.computeIfAbsent(injectTypeElement, this::validate); + if (validationException != null) { + throw validationException; + } + } + + public void throwIfInjectSuperTypeNotValid(XTypeElement injectSuperTypeElement) { + ValidationException validationException = + validatedInjectSuperTypeElements.computeIfAbsent( + injectSuperTypeElement, this::validateSuperType); if (validationException != null) { throw validationException; } @@ -49,7 +67,7 @@ public void throwIfNotValid(XTypeElement injectTypeElement) { private ValidationException validate(XTypeElement xInjectTypeElement) { // The inject validator inspects: // 1. the type itself - // 2. the type's annotations (needed for scoping) + // 2. the type's annotations, if an @Inject constructor exists (needed for scoping) // 3. the type's superclass (needed for inherited @Inject members) // 4. the direct fields, constructors, and methods annotated with @Inject // TODO(bcorso): Call the #validate() methods from XProcessing instead once we have validation @@ -57,7 +75,12 @@ private ValidationException validate(XTypeElement xInjectTypeElement) { TypeElement injectTypeElement = toJavac(xInjectTypeElement); try { DaggerSuperficialValidation.validateType("class type", injectTypeElement.asType()); - DaggerSuperficialValidation.validateAnnotations(injectTypeElement.getAnnotationMirrors()); + // We only validate annotations if this type has an @Inject constructor. Otherwise, Dagger + // isn't responsible for creating this type, so no need to care about scope annotations. + if (constructorsIn(injectTypeElement.getEnclosedElements()).stream() + .anyMatch(constructor -> isAnnotationPresent(constructor, TypeNames.INJECT))) { + DaggerSuperficialValidation.validateAnnotations(injectTypeElement.getAnnotationMirrors()); + } DaggerSuperficialValidation.validateType( "superclass type", injectTypeElement.getSuperclass()); injectTypeElement.getEnclosedElements().stream() @@ -69,8 +92,28 @@ private ValidationException validate(XTypeElement xInjectTypeElement) { } } + private ValidationException validateSuperType(XTypeElement xInjectSuperTypeElement) { + // Note that we skip validating annotations and constructors in supertypes because scopes and + // @Inject constructors are ignored in super types. + TypeElement injectSupertypeElement = toJavac(xInjectSuperTypeElement); + try { + DaggerSuperficialValidation.validateType("class type", injectSupertypeElement.asType()); + DaggerSuperficialValidation.validateType( + "super type", injectSupertypeElement.getSuperclass()); + injectSupertypeElement.getEnclosedElements().stream() + .filter(element -> isAnnotationPresent(element, TypeNames.INJECT)) + .filter(element -> element.getKind() != ElementKind.CONSTRUCTOR) + .forEach(DaggerSuperficialValidation::validateElement); + return null; + } catch (ValidationException validationException) { + return validationException.append( + "InjectValidator.validateSuperType: " + injectSupertypeElement); + } + } + @Override public void clearCache() { - validatedTypeElements.clear(); + validatedInjectTypeElements.clear(); + validatedInjectSuperTypeElements.clear(); } }