Skip to content

Commit

Permalink
Remove superficial validation for cases where we don't need it.
Browse files Browse the repository at this point in the history
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 #3090, where the current superficial validation can fail on non-Dagger related types.

RELNOTES=N/A
PiperOrigin-RevId: 416363991
  • Loading branch information
bcorso authored and Dagger Team committed Dec 14, 2021
1 parent 3d683fe commit 6c45b9d
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 8 deletions.
15 changes: 13 additions & 2 deletions java/dagger/internal/codegen/validation/InjectValidator.java
Expand Up @@ -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));

Expand All @@ -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());
Expand Down Expand Up @@ -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);
}
Expand Down
Expand Up @@ -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;
Expand All @@ -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<XTypeElement, ValidationException> 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<XTypeElement, ValidationException> validatedInjectTypeElements =
new HashMap<>();
private final Map<XTypeElement, ValidationException> 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;
}
Expand All @@ -49,15 +67,20 @@ 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
// for types other than elements, e.g. annotations, annotation values, and types.
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()
Expand All @@ -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();
}
}

0 comments on commit 6c45b9d

Please sign in to comment.