Skip to content

Commit

Permalink
Fixed the incorrect logic from ProductionBindingRepresentation which …
Browse files Browse the repository at this point in the history
…could have resulted in type casting error.

In the existing logic, if someone is requesting a production binding with Provider, Dagger will end up generating a Producer typed expression.

This cl changes the behavior, so that if a binding request kind for a production binding is PROVIDER, then use derivedFromFrameworkInstanceRequestRepresentation to derive the expression from the corresponding ProducerNodeInstanceRequestRepresentation. This will still fail, but with better error message "request kind PROVIDER cannot be satisfied by production binding" from the BindingGraphPlugin Validator.

RELNOTES=n/a
PiperOrigin-RevId: 629639958
  • Loading branch information
wanyingd1996 authored and Dagger Team committed May 1, 2024
1 parent e209fa9 commit afd1011
Show file tree
Hide file tree
Showing 6 changed files with 108 additions and 32 deletions.
28 changes: 25 additions & 3 deletions java/dagger/internal/codegen/base/RequestKinds.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@
import com.squareup.javapoet.ClassName;
import com.squareup.javapoet.TypeName;
import dagger.internal.codegen.javapoet.TypeNames;
import dagger.internal.codegen.model.Binding;
import dagger.internal.codegen.model.BindingGraph;
import dagger.internal.codegen.model.BindingGraph.ComponentNode;
import dagger.internal.codegen.model.BindingGraph.DependencyEdge;
import dagger.internal.codegen.model.BindingGraph.Node;
import dagger.internal.codegen.model.RequestKind;

/** Utility methods for {@link RequestKind}s. */
Expand Down Expand Up @@ -162,21 +167,38 @@ public static ClassName frameworkClassName(RequestKind requestKind) {
* Returns {@code true} if requests for {@code requestKind} can be satisfied by a production
* binding.
*/
public static boolean canBeSatisfiedByProductionBinding(RequestKind requestKind) {
public static boolean canBeSatisfiedByProductionBinding(
RequestKind requestKind, boolean isEntryPoint) {
switch (requestKind) {
case INSTANCE:
case PROVIDER:
case LAZY:
case PROVIDER_OF_LAZY:
case MEMBERS_INJECTION:
return false;
case PRODUCED: // TODO(b/337087142) Requires implementation for entry point.
case INSTANCE:
return !isEntryPoint;
case PRODUCER:
case PRODUCED:
case FUTURE:
return true;
}
throw new AssertionError();
}

public static boolean dependencyCanBeProduction(DependencyEdge edge, BindingGraph graph) {
Node source = graph.network().incidentNodes(edge).source();
boolean isEntryPoint = source instanceof ComponentNode;
boolean isValidRequest =
canBeSatisfiedByProductionBinding(edge.dependencyRequest().kind(), isEntryPoint);
if (isEntryPoint) {
return isValidRequest;
}
if (source instanceof Binding) {
return isValidRequest && ((Binding) source).isProduction();
}
throw new IllegalArgumentException(
"expected a dagger.internal.codegen.model.Binding or ComponentNode: " + source);
}

private RequestKinds() {}
}
2 changes: 2 additions & 0 deletions java/dagger/internal/codegen/binding/FrameworkType.java
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,8 @@ public static Optional<FrameworkType> forRequestKind(RequestKind requestKind) {
switch (requestKind) {
case PROVIDER:
return Optional.of(FrameworkType.PROVIDER);
case PRODUCER:
return Optional.of(FrameworkType.PRODUCER_NODE);
default:
return Optional.empty();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import static dagger.internal.codegen.base.Formatter.INDENT;
import static dagger.internal.codegen.base.Keys.isValidImplicitProvisionKey;
import static dagger.internal.codegen.base.Keys.isValidMembersInjectionKey;
import static dagger.internal.codegen.base.RequestKinds.canBeSatisfiedByProductionBinding;
import static dagger.internal.codegen.base.RequestKinds.dependencyCanBeProduction;
import static dagger.internal.codegen.binding.DependencyRequestFormatter.DOUBLE_INDENT;
import static dagger.internal.codegen.extension.DaggerStreams.instancesOf;
import static dagger.internal.codegen.extension.DaggerStreams.toImmutableSet;
Expand All @@ -44,7 +44,6 @@
import dagger.internal.codegen.binding.InjectBindingRegistry;
import dagger.internal.codegen.model.Binding;
import dagger.internal.codegen.model.BindingGraph;
import dagger.internal.codegen.model.BindingGraph.ComponentNode;
import dagger.internal.codegen.model.BindingGraph.DependencyEdge;
import dagger.internal.codegen.model.BindingGraph.Edge;
import dagger.internal.codegen.model.BindingGraph.MissingBinding;
Expand Down Expand Up @@ -282,20 +281,6 @@ private boolean allIncomingDependenciesCanUseProduction(
.allMatch(edge -> dependencyCanBeProduction(edge, graph));
}

// TODO(ronshapiro): merge with
// ProvisionDependencyOnProduerBindingValidator.dependencyCanUseProduction
private boolean dependencyCanBeProduction(DependencyEdge edge, BindingGraph graph) {
Node source = graph.network().incidentNodes(edge).source();
if (source instanceof ComponentNode) {
return canBeSatisfiedByProductionBinding(edge.dependencyRequest().kind());
}
if (source instanceof Binding) {
return ((Binding) source).isProduction();
}
throw new IllegalArgumentException(
"expected a dagger.internal.codegen.model.Binding or ComponentNode: " + source);
}

private boolean typeHasInjectionSites(Key key) {
return injectBindingRegistry
.getOrFindMembersInjectionBinding(key)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Verify.verify;
import static dagger.internal.codegen.base.RequestKinds.canBeSatisfiedByProductionBinding;
import static dagger.internal.codegen.base.RequestKinds.dependencyCanBeProduction;
import static dagger.internal.codegen.extension.DaggerStreams.instancesOf;
import static javax.tools.Diagnostic.Kind.ERROR;

Expand Down Expand Up @@ -64,7 +65,7 @@ private Stream<DependencyEdge> provisionDependenciesOnProductionBindings(
return bindingGraph.bindings().stream()
.filter(binding -> binding.isProduction())
.flatMap(binding -> incomingDependencies(binding, bindingGraph))
.filter(edge -> !dependencyCanUseProduction(edge, bindingGraph));
.filter(edge -> !dependencyCanBeProduction(edge, bindingGraph));
}

/** Returns the dependencies on {@code binding}. */
Expand All @@ -74,13 +75,6 @@ private Stream<DependencyEdge> incomingDependencies(Binding binding, BindingGrap
.flatMap(instancesOf(DependencyEdge.class));
}

// TODO(ronshapiro): merge with MissingBindingValidator.dependencyCanUseProduction
private boolean dependencyCanUseProduction(DependencyEdge edge, BindingGraph bindingGraph) {
return edge.isEntryPoint()
? canBeSatisfiedByProductionBinding(edge.dependencyRequest().kind())
: bindingRequestingDependency(edge, bindingGraph).isProduction();
}

/**
* Returns the binding that requests a dependency.
*
Expand Down Expand Up @@ -108,6 +102,12 @@ private String entryPointErrorMessage(DependencyEdge entryPoint) {

private String dependencyErrorMessage(
DependencyEdge dependencyOnProduction, BindingGraph bindingGraph) {
if (!canBeSatisfiedByProductionBinding(
dependencyOnProduction.dependencyRequest().kind(), false)) {
return String.format(
"request kind %s cannot be satisfied by production binding.",
dependencyOnProduction.dependencyRequest().kind());
}
return String.format(
"%s is a provision, which cannot depend on a production.",
bindingRequestingDependency(dependencyOnProduction, bindingGraph).key());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import dagger.internal.codegen.binding.BindingRequest;
import dagger.internal.codegen.binding.FrameworkType;
import dagger.internal.codegen.binding.ProductionBinding;
import dagger.internal.codegen.model.RequestKind;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
Expand All @@ -38,7 +39,7 @@ final class ProductionBindingRepresentation implements BindingRepresentation {
private final ProductionBinding binding;
private final DerivedFromFrameworkInstanceRequestRepresentation.Factory
derivedFromFrameworkInstanceRequestRepresentationFactory;
private final RequestRepresentation frameworkInstanceRequestRepresentation;
private final RequestRepresentation producerNodeInstanceRequestRepresentation;
private final Map<BindingRequest, RequestRepresentation> requestRepresentations = new HashMap<>();

@AssistedInject
Expand Down Expand Up @@ -66,7 +67,7 @@ final class ProductionBindingRepresentation implements BindingRepresentation {
? bindingRepresentations.scope(
binding, unscopedFrameworkInstanceCreationExpressionFactory.create(binding))
: unscopedFrameworkInstanceCreationExpressionFactory.create(binding));
this.frameworkInstanceRequestRepresentation =
this.producerNodeInstanceRequestRepresentation =
producerNodeInstanceRequestRepresentationFactory.create(binding, frameworkInstanceSupplier);
}

Expand All @@ -77,11 +78,11 @@ public RequestRepresentation getRequestRepresentation(BindingRequest request) {
}

private RequestRepresentation getRequestRepresentationUncached(BindingRequest request) {
return request.frameworkType().isPresent()
? frameworkInstanceRequestRepresentation
return request.requestKind().equals(RequestKind.PRODUCER)
? producerNodeInstanceRequestRepresentation
: derivedFromFrameworkInstanceRequestRepresentationFactory.create(
binding,
frameworkInstanceRequestRepresentation,
producerNodeInstanceRequestRepresentation,
request.requestKind(),
FrameworkType.PRODUCER_NODE);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package dagger.internal.codegen;


import androidx.room.compiler.processing.util.Source;
import com.google.common.collect.ImmutableMap;
import dagger.testing.compile.CompilerTests;
Expand Down Expand Up @@ -415,4 +416,69 @@ public void productionScope_injectConstructor() throws Exception {
subject.generatedSource(goldenFileRule.goldenSource("test/DaggerParent"));
});
}

@Test
public void requestProducerNodeWithProvider_failsWithNotSupportedError() {
Source moduleFile =
CompilerTests.javaSource(
"test.ExecutorModule",
"package test;",
"",
"import com.google.common.util.concurrent.MoreExecutors;",
"import dagger.Module;",
"import dagger.Provides;",
"import dagger.producers.Production;",
"import java.util.concurrent.Executor;",
"",
"@Module",
"final class ExecutorModule {",
" @Provides @Production Executor executor() {",
" return MoreExecutors.directExecutor();",
" }",
"}");
Source producerModuleFile =
CompilerTests.javaSource(
"test.SimpleModule",
"package test;",
"",
"import dagger.producers.ProducerModule;",
"import dagger.producers.Produces;",
"import javax.inject.Provider;",
"import java.util.concurrent.Executor;",
"import dagger.producers.Production;",
"",
"@ProducerModule",
"final class SimpleModule {",
" @Produces String str(Provider<Integer> num) {",
" return \"\";",
" }",
" @Produces Integer num() { return 1; }",
"}");
Source componentFile =
CompilerTests.javaSource(
"test.SimpleComponent",
"package test;",
"",
"import com.google.common.util.concurrent.ListenableFuture;",
"import dagger.producers.ProductionComponent;",
"",
"@ProductionComponent(modules = {ExecutorModule.class, SimpleModule.class})",
"interface SimpleComponent {",
" ListenableFuture<String> str();",
"",
" @ProductionComponent.Builder",
" interface Builder {",
" SimpleComponent build();",
" }",
"}");

CompilerTests.daggerCompiler(moduleFile, producerModuleFile, componentFile)
.withProcessingOptions(compilerMode.processorOptions())
.compile(
subject -> {
subject.hasErrorCount(1);
subject.hasErrorContaining(
"request kind PROVIDER cannot be satisfied by production binding");
});
}
}

0 comments on commit afd1011

Please sign in to comment.