Skip to content

Commit

Permalink
Fix error message for an @Binds @IntoSet implementation with duplic…
Browse files Browse the repository at this point in the history
…ate 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
  • Loading branch information
bcorso authored and Dagger Team committed Dec 18, 2023
1 parent be77d24 commit 8d01223
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -59,7 +60,8 @@ private void checkForDuplicateSetContributions(
Multimap<Key, Binding> 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));
}
}

Expand All @@ -80,20 +82,26 @@ 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).
*
* <p>If the binding cannot be dereferenced (because it leads to a missing binding or duplicate
* bindings) then {@link Optional#empty()} is returned.
*/
private Optional<Key> dereferenceDelegateBinding(Binding binding, BindingGraph bindingGraph) {
ImmutableSet<Binding> 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.
Binding delegate = Iterables.getOnlyElement(delegateSet);
if (delegate.kind().equals(DELEGATE)) {
return dereferenceDelegateBinding(delegate, bindingGraph);
}
return delegate.key();
return Optional.of(delegate.key());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<Foo> 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<Foo> 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(
Expand Down

0 comments on commit 8d01223

Please sign in to comment.