From 8d0122322b912c1d9b6d4605e9126ba450abf7d6 Mon Sep 17 00:00:00 2001 From: Brad Corso Date: Mon, 18 Dec 2023 12:19:44 -0800 Subject: [PATCH] Fix error message for an `@Binds @IntoSet` implementation with duplicate bindings. This CL fixes a case where an `@Binds @IntoSet` that delegates to an implementation with duplicate bindings was throwing an `IllegalArgumentException` rather than reporting an error properly. An example of the problematic code is shown below: ```java @Module interface FooModule { @Binds @IntoSet Foo bindFoo(FooImpl impl); @Provides static FooImpl provideFooImpl1() { ... } @Provides static FooImpl provideFooImpl2() { ... } } ``` The above code now reports the duplicate binding rather than throwing an `IllegalArgumentException`. RELNOTES=Fixed error message for an `@Binds @IntoSet` implementation with duplicate bindings. PiperOrigin-RevId: 591976664 --- .../SetMultibindingValidator.java | 24 ++++-- .../SetMultibindingValidationTest.java | 79 +++++++++++++++++++ 2 files changed, 95 insertions(+), 8 deletions(-) diff --git a/java/dagger/internal/codegen/bindinggraphvalidation/SetMultibindingValidator.java b/java/dagger/internal/codegen/bindinggraphvalidation/SetMultibindingValidator.java index f767da85101..30ebf09b222 100644 --- a/java/dagger/internal/codegen/bindinggraphvalidation/SetMultibindingValidator.java +++ b/java/dagger/internal/codegen/bindinggraphvalidation/SetMultibindingValidator.java @@ -30,6 +30,7 @@ import dagger.internal.codegen.model.DiagnosticReporter; import dagger.internal.codegen.model.Key; import dagger.internal.codegen.validation.ValidationBindingGraphPlugin; +import java.util.Optional; import javax.inject.Inject; /** Validates that there are not multiple set binding contributions to the same binding. */ @@ -59,7 +60,8 @@ private void checkForDuplicateSetContributions( Multimap dereferencedBindsTargets = HashMultimap.create(); for (Binding dep : bindingGraph.requestedBindings(binding)) { if (dep.kind().equals(DELEGATE)) { - dereferencedBindsTargets.put(dereferenceDelegateBinding(dep, bindingGraph), dep); + dereferenceDelegateBinding(dep, bindingGraph) + .ifPresent(dereferencedKey -> dereferencedBindsTargets.put(dereferencedKey, dep)); } } @@ -80,13 +82,19 @@ private void checkForDuplicateSetContributions( }); } - /** Returns the delegate target of a delegate binding (going through other delegates as well). */ - private Key dereferenceDelegateBinding(Binding binding, BindingGraph bindingGraph) { + /** + * Returns the dereferenced key of a delegate binding (going through other delegates as well). + * + *

If the binding cannot be dereferenced (because it leads to a missing binding or duplicate + * bindings) then {@link Optional#empty()} is returned. + */ + private Optional dereferenceDelegateBinding(Binding binding, BindingGraph bindingGraph) { ImmutableSet delegateSet = bindingGraph.requestedBindings(binding); - if (delegateSet.isEmpty()) { - // There may not be a delegate if the delegate is missing. In this case, we just take the - // requested key and return that. - return Iterables.getOnlyElement(binding.dependencies()).key(); + if (delegateSet.size() != 1) { + // If there isn't exactly 1 delegate then it means either a MissingBinding or DuplicateBinding + // error will be reported. Just return nothing rather than trying to dereference further, as + // anything we report here will just be noise on top of the other error anyway. + return Optional.empty(); } // If there is a binding, first we check if that is a delegate binding so we can dereference // that binding if needed. @@ -94,6 +102,6 @@ private Key dereferenceDelegateBinding(Binding binding, BindingGraph bindingGrap if (delegate.kind().equals(DELEGATE)) { return dereferenceDelegateBinding(delegate, bindingGraph); } - return delegate.key(); + return Optional.of(delegate.key()); } } diff --git a/javatests/dagger/internal/codegen/bindinggraphvalidation/SetMultibindingValidationTest.java b/javatests/dagger/internal/codegen/bindinggraphvalidation/SetMultibindingValidationTest.java index 8eaccaede37..e68099ae4b8 100644 --- a/javatests/dagger/internal/codegen/bindinggraphvalidation/SetMultibindingValidationTest.java +++ b/javatests/dagger/internal/codegen/bindinggraphvalidation/SetMultibindingValidationTest.java @@ -95,6 +95,85 @@ public SetMultibindingValidationTest(CompilerMode compilerMode) { }); } + // Regression test for b/316582741 to ensure the duplicate binding gets reported rather than + // causing a crash. + @Test public void testSetBindingsToDuplicateBinding() { + Source module = + CompilerTests.javaSource( + "test.TestModule", + "package test;", + "", + "import dagger.Binds;", + "import dagger.Module;", + "import dagger.Provides;", + "import dagger.multibindings.IntoSet;", + "", + "@Module", + "interface TestModule {", + " @Binds @IntoSet Foo bindFoo(FooImpl impl);", + "", + " @Provides static FooImpl provideFooImpl() { return null; }", + "", + " @Provides static FooImpl provideFooImplAgain() { return null; }", + "}"); + Source component = + CompilerTests.javaSource( + "test.TestComponent", + "package test;", + "", + "import dagger.Component;", + "import java.util.Set;", + "", + "@Component(modules = TestModule.class)", + "interface TestComponent {", + " Set setOfFoo();", + "}"); + CompilerTests.daggerCompiler(FOO, FOO_IMPL, module, component) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining("FooImpl is bound multiple times"); + }); + } + + @Test public void testSetBindingsToMissingBinding() { + Source module = + CompilerTests.javaSource( + "test.TestModule", + "package test;", + "", + "import dagger.Binds;", + "import dagger.Module;", + "import dagger.multibindings.IntoSet;", + "", + "@Module", + "interface TestModule {", + " @Binds @IntoSet Foo bindFoo(MissingFooImpl impl);", + "", + " static class MissingFooImpl implements Foo {}", + "}"); + Source component = + CompilerTests.javaSource( + "test.TestComponent", + "package test;", + "", + "import dagger.Component;", + "import java.util.Set;", + "", + "@Component(modules = TestModule.class)", + "interface TestComponent {", + " Set setOfFoo();", + "}"); + CompilerTests.daggerCompiler(FOO, module, component) + .withProcessingOptions(compilerMode.processorOptions()) + .compile( + subject -> { + subject.hasErrorCount(1); + subject.hasErrorContaining("MissingFooImpl cannot be provided"); + }); + } + @Test public void testMultipleSetBindingsToSameFooThroughMultipleBinds() { Source module = CompilerTests.javaSource(