Skip to content

Commit

Permalink
Refactor InjectValidator to have a single entry point.
Browse files Browse the repository at this point in the history 
This CL refactors the InjectValidator to have a single entry point: `InjectValidator#validate(XTypeElement)` rather than allowing validation on the constructor and members injection types separately.

The main purpose of this refactor is to make it easier to fix #3075 by only needing to add validation at a single entry point.

However, having a single entry point on the TypeElement also allows us to organize the report better (e.g. we now check that there's only 1 @Inject constructor on the type rather than on each individual constructor), and cache on the TypeElement rather than each individual ConstructorElement.

RELNOTES=N/A
PiperOrigin-RevId: 413451893
  • Loading branch information
@bcorso
bcorso authored and Dagger Team committed Dec 1, 2021
1 parent 49dc28b commit b4f80d1
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 52 deletions.
2 changes: 1 addition & 1 deletion java/dagger/internal/codegen/bindinggraphvalidation/InjectBindingValidator.java
View file
Expand Up @@ -56,7 +56,7 @@ public void visitGraph(BindingGraph bindingGraph, DiagnosticReporter diagnosticR

private void validateInjectionBinding(Binding node, DiagnosticReporter diagnosticReporter) {
ValidationReport typeReport =
injectValidator.validateType(
injectValidator.validate(
toXProcessing(asTypeElement(node.key().type().java()), processingEnv));
for (Item item : typeReport.allItems()) {
diagnosticReporter.reportBinding(item.kind(), node, item.message());
Expand Down
29 changes: 16 additions & 13 deletions java/dagger/internal/codegen/validation/InjectBindingRegistryImpl.java
View file
Expand Up @@ -106,7 +106,7 @@ void generateBindings(SourceFileGenerator<B> generator) throws SourceFileGenerat
TypeMirror type = binding.key().type().java();
if (!type.getKind().equals(DECLARED)
|| injectValidatorWhenGeneratingCode
.validateType(toXProcessing(asTypeElement(type), processingEnv))
.validate(toXProcessing(asTypeElement(type), processingEnv))
.isClean()) {
generator.generate(binding);
}
Expand Down Expand Up @@ -259,19 +259,21 @@ private Optional<ProvisionBinding> tryRegisterConstructor(
Optional<XType> resolvedType,
boolean warnIfNotAlreadyGenerated) {
XTypeElement typeElement = constructorElement.getEnclosingElement();

// Validating here shouldn't have a performance penalty because the validator caches its reports
ValidationReport report = injectValidator.validate(typeElement);
report.printMessagesTo(messager);
if (!report.isClean()) {
return Optional.empty();
}

XType type = typeElement.getType();
Key key = keyFactory.forInjectConstructorWithResolvedType(type);
ProvisionBinding cachedBinding = provisionBindings.getBinding(key);
if (cachedBinding != null) {
return Optional.of(cachedBinding);
}

ValidationReport report = injectValidator.validateConstructor(constructorElement);
report.printMessagesTo(messager);
if (!report.isClean()) {
return Optional.empty();
}

ProvisionBinding binding = bindingFactory.injectionBinding(constructorElement, resolvedType);
registerBinding(binding, warnIfNotAlreadyGenerated);
if (!binding.injectionSites().isEmpty()) {
Expand Down Expand Up @@ -311,19 +313,20 @@ public Optional<MembersInjectionBinding> tryRegisterInjectMethod(XMethodElement
@CanIgnoreReturnValue
private Optional<MembersInjectionBinding> tryRegisterMembersInjectedType(
XTypeElement typeElement, Optional<XType> resolvedType, boolean warnIfNotAlreadyGenerated) {
// Validating here shouldn't have a performance penalty because the validator caches its reports
ValidationReport report = injectValidator.validate(typeElement);
report.printMessagesTo(messager);
if (!report.isClean()) {
return Optional.empty();
}

XType type = typeElement.getType();
Key key = keyFactory.forInjectConstructorWithResolvedType(type);
MembersInjectionBinding cachedBinding = membersInjectionBindings.getBinding(key);
if (cachedBinding != null) {
return Optional.of(cachedBinding);
}

ValidationReport report = injectValidator.validateMembersInjectionType(typeElement);
report.printMessagesTo(messager);
if (!report.isClean()) {
return Optional.empty();
}

MembersInjectionBinding binding = bindingFactory.membersInjectionBinding(type, resolvedType);
registerBinding(binding, warnIfNotAlreadyGenerated);
for (Optional<DeclaredType> supertype = types.nonObjectSuperclass(type);
Expand Down
63 changes: 30 additions & 33 deletions java/dagger/internal/codegen/validation/InjectValidator.java
View file
Expand Up @@ -18,6 +18,7 @@

import static androidx.room.compiler.processing.compat.XConverters.toJavac;
import static androidx.room.compiler.processing.compat.XConverters.toXProcessing;
import static com.google.common.collect.Iterables.getOnlyElement;
import static dagger.internal.codegen.base.Scopes.scopesOf;
import static dagger.internal.codegen.base.Util.reentrantComputeIfAbsent;
import static dagger.internal.codegen.binding.AssistedInjectionAnnotations.assistedInjectedConstructors;
Expand Down Expand Up @@ -50,7 +51,6 @@
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Stream;
import javax.inject.Inject;
import javax.inject.Singleton;
import javax.tools.Diagnostic;
Expand All @@ -70,7 +70,7 @@ public final class InjectValidator implements ClearableCache {
private final Optional<Diagnostic.Kind> privateAndStaticInjectionDiagnosticKind;
private final InjectionAnnotations injectionAnnotations;
private final KotlinMetadataUtil metadataUtil;
private final Map<XConstructorElement, ValidationReport> reports = new HashMap<>();
private final Map<XTypeElement, ValidationReport> reports = new HashMap<>();

@Inject
InjectValidator(
Expand Down Expand Up @@ -135,11 +135,34 @@ public InjectValidator whenGeneratingCode() {
metadataUtil);
}

public ValidationReport validateConstructor(XConstructorElement constructorElement) {
return reentrantComputeIfAbsent(reports, constructorElement, this::validateConstructorUncached);
public ValidationReport validate(XTypeElement typeElement) {
return reentrantComputeIfAbsent(reports, typeElement, this::validateUncached);
}

private ValidationReport validateConstructorUncached(XConstructorElement constructorElement) {
private ValidationReport validateUncached(XTypeElement typeElement) {
ValidationReport.Builder builder = ValidationReport.about(typeElement);
builder.addSubreport(validateMembersInjectionType(typeElement));

ImmutableSet<XConstructorElement> injectConstructors =
ImmutableSet.<XConstructorElement>builder()
.addAll(injectedConstructors(typeElement))
.addAll(assistedInjectedConstructors(typeElement))
.build();

switch (injectConstructors.size()) {
case 0:
break; // Nothing to validate.
case 1:
builder.addSubreport(validateConstructor(getOnlyElement(injectConstructors)));
break;
default:
builder.addError("Types may only contain one injected constructor", typeElement);
}

return builder.build();
}

private ValidationReport validateConstructor(XConstructorElement constructorElement) {
ValidationReport.Builder builder =
ValidationReport.about(constructorElement.getEnclosingElement());

Expand Down Expand Up @@ -216,17 +239,6 @@ private ValidationReport validateConstructorUncached(XConstructorElement constru
constructorElement);
}

// This is computationally expensive, but probably preferable to a giant index
ImmutableSet<XConstructorElement> injectConstructors =
ImmutableSet.<XConstructorElement>builder()
.addAll(injectedConstructors(enclosingElement))
.addAll(assistedInjectedConstructors(enclosingElement))
.build();

if (injectConstructors.size() > 1) {
builder.addError("Types may only contain one injected constructor", constructorElement);
}

ImmutableSet<Scope> scopes = scopesOf(enclosingElement);
if (injectAnnotation.equals(TypeNames.ASSISTED_INJECT)) {
for (Scope scope : scopes) {
Expand Down Expand Up @@ -314,7 +326,7 @@ private void validateDependencyRequest(
dependencyRequestValidator.checkNotProducer(builder, parameter);
}

public ValidationReport validateMembersInjectionType(XTypeElement typeElement) {
private ValidationReport validateMembersInjectionType(XTypeElement typeElement) {
// TODO(beder): This element might not be currently compiled, so this error message could be
// left in limbo. Find an appropriate way to display the error message in that case.
ValidationReport.Builder builder = ValidationReport.about(typeElement);
Expand Down Expand Up @@ -343,29 +355,14 @@ public ValidationReport validateMembersInjectionType(XTypeElement typeElement) {
checkInjectIntoKotlinObject(typeElement, builder);
}
if (typeElement.getSuperType() != null) {
ValidationReport report = validateType(typeElement.getSuperType().getTypeElement());
ValidationReport report = validate(typeElement.getSuperType().getTypeElement());
if (!report.isClean()) {
builder.addSubreport(report);
}
}
return builder.build();
}

public ValidationReport validateType(XTypeElement typeElement) {
ValidationReport.Builder builder = ValidationReport.about(typeElement);
ValidationReport membersInjectionReport = validateMembersInjectionType(typeElement);
if (!membersInjectionReport.isClean()) {
builder.addSubreport(membersInjectionReport);
}
Stream.concat(
injectedConstructors(typeElement).stream(),
assistedInjectedConstructors(typeElement).stream())
.map(this::validateConstructor)
.filter(report -> !report.isClean())
.forEach(builder::addSubreport);
return builder.build();
}

/** Returns true if the given method element declares a checked exception. */
private boolean throwsCheckedExceptions(XConstructorElement constructorElement) {
XType runtimeException = processingEnv.findType(TypeNames.RUNTIME_EXCEPTION);
Expand Down
7 changes: 2 additions & 5 deletions javatests/dagger/internal/codegen/InjectConstructorFactoryGeneratorTest.java
View file
Expand Up @@ -547,14 +547,11 @@ public final class InjectConstructorFactoryGeneratorTest {
"}");
Compilation compilation = daggerCompiler().compile(file);
assertThat(compilation).failed();
assertThat(compilation).hadErrorCount(1);
assertThat(compilation)
.hadErrorContaining("Types may only contain one injected constructor")
.inFile(file)
.onLine(6);
assertThat(compilation)
.hadErrorContaining("Types may only contain one injected constructor")
.inFile(file)
.onLine(8);
.onLine(5);
}

@Test public void multipleQualifiersOnInjectConstructorParameter() {
Expand Down

0 comments on commit b4f80d1

Please sign in to comment.