Skip to content

Commit 03b237f

Browse files
bcorsoDagger Team
bcorso
authored and
Dagger Team
committed
Ban scoping on @Binds that delegate to @Produces implementations.
This CL bans scoping on `@Binds` methods that delegate to `@Produces` methods. Note that scoping on `@Produces` methods themselves is already banned because production bindings are implicitly scoped. There's a few benefits to banning this. First, allowing users to arbitrarily add scopes can be misleading because the scopes will just be ignored. Second, this change allows us to clean up places in `BindingFactory` that required knowledge of the `BindingType` (which is something we're currently trying to remove). RELNOTES=Ban scoping on production bindings. PiperOrigin-RevId: 673020697
1 parent 5918d11 commit 03b237f

File tree

5 files changed

+252
-45
lines changed

5 files changed

+252
-45
lines changed

‎java/dagger/internal/codegen/binding/BindingFactory.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,7 @@ public ProductionBinding producesMethodBinding(XMethodElement method, XTypeEleme
219219
.explicitDependencies(
220220
dependencyRequestFactory.forRequiredResolvedVariables(
221221
method.getParameters(), methodType.getParameterTypes()))
222+
.scope(injectionAnnotations.getScope(method))
222223
.unresolved(
223224
methodType.isSameType(method.getExecutableType())
224225
? Optional.empty()
@@ -402,10 +403,7 @@ private DelegateBinding delegateBinding(
402403
bindingType == BindingType.PRODUCTION
403404
? keyFactory.forDelegateBinding(delegateDeclaration, TypeNames.PRODUCER)
404405
: keyFactory.forDelegateBinding(delegateDeclaration, TypeNames.PROVIDER))
405-
.scope(
406-
bindingType == BindingType.PRODUCTION
407-
? Optional.empty()
408-
: injectionAnnotations.getScope(delegateDeclaration.bindingElement().get()))
406+
.scope(injectionAnnotations.getScope(delegateDeclaration.bindingElement().get()))
409407
.build();
410408
}
411409

‎java/dagger/internal/codegen/bindinggraphvalidation/BindingGraphValidationModule.java

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,9 @@ static ImmutableSet<ValidationBindingGraphPlugin> providePlugins(
4242
MissingBindingValidator validation7,
4343
NullableBindingValidator validation8,
4444
ProvisionDependencyOnProducerBindingValidator validation9,
45-
SetMultibindingValidator validation10,
46-
SubcomponentFactoryMethodValidator validation11) {
45+
InvalidProductionBindingScopeValidator validation10,
46+
SetMultibindingValidator validation11,
47+
SubcomponentFactoryMethodValidator validation12) {
4748
ImmutableSet<ValidationBindingGraphPlugin> plugins =
4849
ImmutableSet.of(
4950
validation1,
@@ -56,7 +57,8 @@ static ImmutableSet<ValidationBindingGraphPlugin> providePlugins(
5657
validation8,
5758
validation9,
5859
validation10,
59-
validation11);
60+
validation11,
61+
validation12);
6062
if (compilerOptions.experimentalDaggerErrorMessages()) {
6163
return ImmutableSet.of(factory.create(plugins));
6264
} else {

‎java/dagger/internal/codegen/bindinggraphvalidation/InvalidProductionBindingScopeValidator.java

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/*
2+
* Copyright (C) 2024 The Dagger Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package dagger.internal.codegen.bindinggraphvalidation;
18+
19+
import static javax.tools.Diagnostic.Kind.ERROR;
20+
21+
import dagger.internal.codegen.model.Binding;
22+
import dagger.internal.codegen.model.BindingGraph;
23+
import dagger.internal.codegen.model.DiagnosticReporter;
24+
import dagger.internal.codegen.validation.ValidationBindingGraphPlugin;
25+
import javax.inject.Inject;
26+
27+
/** Reports an error for each production binding type that is invalidly scoped. */
28+
final class InvalidProductionBindingScopeValidator extends ValidationBindingGraphPlugin {
29+
30+
@Inject
31+
InvalidProductionBindingScopeValidator() {}
32+
33+
@Override
34+
public String pluginName() {
35+
return "Dagger/InvalidProductionBindingScope";
36+
}
37+
38+
@Override
39+
public void visitGraph(BindingGraph bindingGraph, DiagnosticReporter reporter) {
40+
// Note: ProducesMethodValidator validates that @Produces methods aren't scoped, but here we
41+
// take that a step further and validate that anything that transitively depends on a @Produces
42+
// method is also not scoped (i.e. all production binding types).
43+
bindingGraph.bindings().stream()
44+
.filter(Binding::isProduction)
45+
.filter(binding -> binding.scope().isPresent())
46+
.forEach(binding -> reporter.reportBinding(ERROR, binding, errorMessage(binding)));
47+
}
48+
49+
private String errorMessage(Binding binding) {
50+
return String.format(
51+
"%s cannot be scoped because it delegates to an @Produces method.",
52+
binding);
53+
}
54+
}

‎javatests/dagger/functional/producers/binds/SimpleBindingModule.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
import java.util.concurrent.Executor;
3838
import javax.inject.Named;
3939
import javax.inject.Qualifier;
40-
import javax.inject.Singleton;
4140

4241
@ProducerModule(includes = {
4342
SimpleBindingModule.ExecutorModule.class,
@@ -54,7 +53,6 @@ abstract class SimpleBindingModule {
5453
abstract Foo<? extends Number> bindFooOfNumbers(Foo<Integer> fooOfIntegers);
5554

5655
@Binds
57-
@Singleton
5856
@SomeQualifier
5957
abstract Foo<String> bindQualifiedFooOfStrings(FooOfStrings impl);
6058

‎javatests/dagger/internal/codegen/ProductionComponentProcessorTest.java

Lines changed: 191 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,24 @@ public static Collection<Object[]> parameters() {
3535
return CompilerMode.TEST_PARAMETERS;
3636
}
3737

38+
private static final Source EXECUTOR_MODULE =
39+
CompilerTests.javaSource(
40+
"test.ExecutorModule",
41+
"package test;",
42+
"",
43+
"import com.google.common.util.concurrent.MoreExecutors;",
44+
"import dagger.Module;",
45+
"import dagger.Provides;",
46+
"import dagger.producers.Production;",
47+
"import java.util.concurrent.Executor;",
48+
"",
49+
"@Module",
50+
"final class ExecutorModule {",
51+
" @Provides @Production Executor executor() {",
52+
" return MoreExecutors.directExecutor();",
53+
" }",
54+
"}");
55+
3856
@Rule public GoldenFileRule goldenFileRule = new GoldenFileRule();
3957

4058
private final CompilerMode compilerMode;
@@ -122,23 +140,6 @@ public ProductionComponentProcessorTest(CompilerMode compilerMode) {
122140

123141
@Test
124142
public void dependsOnProductionExecutor() throws Exception {
125-
Source moduleFile =
126-
CompilerTests.javaSource(
127-
"test.ExecutorModule",
128-
"package test;",
129-
"",
130-
"import com.google.common.util.concurrent.MoreExecutors;",
131-
"import dagger.Module;",
132-
"import dagger.Provides;",
133-
"import dagger.producers.Production;",
134-
"import java.util.concurrent.Executor;",
135-
"",
136-
"@Module",
137-
"final class ExecutorModule {",
138-
" @Provides @Production Executor executor() {",
139-
" return MoreExecutors.directExecutor();",
140-
" }",
141-
"}");
142143
Source producerModuleFile =
143144
CompilerTests.javaSource(
144145
"test.SimpleModule",
@@ -175,7 +176,7 @@ public void dependsOnProductionExecutor() throws Exception {
175176
"}");
176177

177178
String errorMessage = "String may not depend on the production executor";
178-
CompilerTests.daggerCompiler(moduleFile, producerModuleFile, componentFile)
179+
CompilerTests.daggerCompiler(EXECUTOR_MODULE, producerModuleFile, componentFile)
179180
.withProcessingOptions(compilerMode.processorOptions())
180181
.compile(
181182
subject -> {
@@ -419,23 +420,6 @@ public void productionScope_injectConstructor() throws Exception {
419420

420421
@Test
421422
public void requestProducerNodeWithProvider_failsWithNotSupportedError() {
422-
Source moduleFile =
423-
CompilerTests.javaSource(
424-
"test.ExecutorModule",
425-
"package test;",
426-
"",
427-
"import com.google.common.util.concurrent.MoreExecutors;",
428-
"import dagger.Module;",
429-
"import dagger.Provides;",
430-
"import dagger.producers.Production;",
431-
"import java.util.concurrent.Executor;",
432-
"",
433-
"@Module",
434-
"final class ExecutorModule {",
435-
" @Provides @Production Executor executor() {",
436-
" return MoreExecutors.directExecutor();",
437-
" }",
438-
"}");
439423
Source producerModuleFile =
440424
CompilerTests.javaSource(
441425
"test.SimpleModule",
@@ -472,7 +456,7 @@ public void requestProducerNodeWithProvider_failsWithNotSupportedError() {
472456
" }",
473457
"}");
474458

475-
CompilerTests.daggerCompiler(moduleFile, producerModuleFile, componentFile)
459+
CompilerTests.daggerCompiler(EXECUTOR_MODULE, producerModuleFile, componentFile)
476460
.withProcessingOptions(compilerMode.processorOptions())
477461
.compile(
478462
subject -> {
@@ -481,4 +465,175 @@ public void requestProducerNodeWithProvider_failsWithNotSupportedError() {
481465
"request kind PROVIDER cannot be satisfied by production binding");
482466
});
483467
}
468+
469+
@Test
470+
public void productionBindingKind_failsIfScoped() {
471+
Source component =
472+
CompilerTests.javaSource(
473+
"test.TestComponent",
474+
"package test;",
475+
"",
476+
"import com.google.common.util.concurrent.ListenableFuture;",
477+
"import dagger.producers.ProductionComponent;",
478+
"",
479+
"@ProductionComponent(modules = {ExecutorModule.class, TestModule.class})",
480+
"interface TestComponent {",
481+
" ListenableFuture<String> str();",
482+
"}");
483+
Source module =
484+
CompilerTests.javaSource(
485+
"test.TestModule",
486+
"package test;",
487+
"",
488+
"import dagger.producers.ProducerModule;",
489+
"import dagger.producers.Produces;",
490+
"import dagger.producers.ProductionScope;",
491+
"import javax.inject.Provider;",
492+
"import java.util.concurrent.Executor;",
493+
"import dagger.producers.Production;",
494+
"",
495+
"@ProducerModule",
496+
"interface TestModule {",
497+
" @ProductionScope",
498+
" @Produces",
499+
" static String provideString() { return \"\"; }",
500+
"}");
501+
502+
CompilerTests.daggerCompiler(component, module, EXECUTOR_MODULE)
503+
.withProcessingOptions(compilerMode.processorOptions())
504+
.compile(
505+
subject -> {
506+
subject.hasErrorCount(1);
507+
subject.hasErrorContaining("@Produces methods cannot be scoped");
508+
});
509+
}
510+
511+
@Test
512+
public void delegateToProductionBindingKind_failsIfScoped() {
513+
Source component =
514+
CompilerTests.javaSource(
515+
"test.TestComponent",
516+
"package test;",
517+
"",
518+
"import com.google.common.util.concurrent.ListenableFuture;",
519+
"import dagger.producers.ProductionComponent;",
520+
"",
521+
"@ProductionComponent(modules = {ExecutorModule.class, TestModule.class})",
522+
"interface TestComponent {",
523+
" ListenableFuture<Foo> foo();",
524+
"}");
525+
Source module =
526+
CompilerTests.javaSource(
527+
"test.TestModule",
528+
"package test;",
529+
"",
530+
"import dagger.Binds;",
531+
"import dagger.producers.ProducerModule;",
532+
"import dagger.producers.Produces;",
533+
"import dagger.producers.ProductionScope;",
534+
"import javax.inject.Provider;",
535+
"import java.util.concurrent.Executor;",
536+
"import dagger.producers.Production;",
537+
"",
538+
"@ProducerModule",
539+
"interface TestModule {",
540+
" @ProductionScope",
541+
" @Binds",
542+
" Foo bind(FooImpl impl);",
543+
"",
544+
" @Produces",
545+
" static FooImpl fooImpl() { return new FooImpl(); }",
546+
"}");
547+
Source foo =
548+
CompilerTests.javaSource(
549+
"test.Foo",
550+
"package test;",
551+
"",
552+
"interface Foo {}");
553+
Source fooImpl =
554+
CompilerTests.javaSource(
555+
"test.FooImpl",
556+
"package test;",
557+
"",
558+
"final class FooImpl implements Foo {}");
559+
560+
CompilerTests.daggerCompiler(component, module, foo, fooImpl, EXECUTOR_MODULE)
561+
.withProcessingOptions(compilerMode.processorOptions())
562+
.compile(
563+
subject -> {
564+
subject.hasErrorCount(1);
565+
subject.hasErrorContaining(
566+
"@ProductionScope @Binds Foo TestModule.bind(FooImpl) cannot be scoped "
567+
+ "because it delegates to an @Produces method");
568+
});
569+
}
570+
571+
@Test
572+
public void multipleDelegatesToProductionBindingKind_failsIfScoped() {
573+
Source component =
574+
CompilerTests.javaSource(
575+
"test.TestComponent",
576+
"package test;",
577+
"",
578+
"import com.google.common.util.concurrent.ListenableFuture;",
579+
"import dagger.producers.ProductionComponent;",
580+
"",
581+
"@ProductionComponent(modules = {ExecutorModule.class, TestModule.class})",
582+
"interface TestComponent {",
583+
" ListenableFuture<FooSuper> fooSuper();",
584+
"}");
585+
Source module =
586+
CompilerTests.javaSource(
587+
"test.TestModule",
588+
"package test;",
589+
"",
590+
"import dagger.Binds;",
591+
"import dagger.producers.ProducerModule;",
592+
"import dagger.producers.Produces;",
593+
"import dagger.producers.ProductionScope;",
594+
"import javax.inject.Provider;",
595+
"import java.util.concurrent.Executor;",
596+
"import dagger.producers.Production;",
597+
"",
598+
"@ProducerModule",
599+
"interface TestModule {",
600+
" @ProductionScope",
601+
" @Binds",
602+
" FooSuper bindFooSuper(Foo impl);",
603+
"",
604+
" @Binds",
605+
" Foo bindFoo(FooImpl impl);",
606+
"",
607+
" @Produces",
608+
" static FooImpl fooImpl() { return new FooImpl(); }",
609+
"}");
610+
Source fooSuper =
611+
CompilerTests.javaSource(
612+
"test.FooSuper",
613+
"package test;",
614+
"",
615+
"interface FooSuper {}");
616+
Source foo =
617+
CompilerTests.javaSource(
618+
"test.Foo",
619+
"package test;",
620+
"",
621+
"interface Foo extends FooSuper {}");
622+
Source fooImpl =
623+
CompilerTests.javaSource(
624+
"test.FooImpl",
625+
"package test;",
626+
"",
627+
"final class FooImpl implements Foo {}");
628+
629+
CompilerTests.daggerCompiler(component, module, fooSuper, foo, fooImpl, EXECUTOR_MODULE)
630+
.withProcessingOptions(compilerMode.processorOptions())
631+
.compile(
632+
subject -> {
633+
subject.hasErrorCount(1);
634+
subject.hasErrorContaining(
635+
"@ProductionScope @Binds FooSuper TestModule.bindFooSuper(Foo) cannot be scoped "
636+
+ "because it delegates to an @Produces method");
637+
});
638+
}
484639
}

0 commit comments

Comments
 (0)