Skip to content

Commit

Permalink
When checking for local contributions, ignore whether the previously …
Browse files Browse the repository at this point in the history
…resolved binding is for a synthetic binding or not. We should still check for @provides Set<T> and @provides @IntoSet T collisions across components (and the same for maps and @provides Optional<T> + @BindsOptionalOf T).

Fixes #975

RELNOTES:
  - Fix a bug where binding collisions were not properly checked across subcomponent boundaries when a parent provided a concrete `Set`/`Map` and a child provided a multibinding contribution with the same key.
  - Also applies to providing a concrete optional binding and `@BindsOptionalOf` declarations.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=186678282
  • Loading branch information
ronshapiro committed Feb 27, 2018
1 parent 43b6067 commit 98b9c3f
Show file tree
Hide file tree
Showing 3 changed files with 191 additions and 22 deletions.
46 changes: 24 additions & 22 deletions java/dagger/internal/codegen/BindingGraphFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Verify.verify;
import static com.google.common.collect.Iterables.any;
import static com.google.common.collect.Iterables.isEmpty;
import static dagger.internal.codegen.ComponentDescriptor.isComponentContributionMethod;
import static dagger.internal.codegen.ComponentRequirement.Kind.BOUND_INSTANCE;
Expand Down Expand Up @@ -900,8 +899,8 @@ private boolean dependsOnLocalBindingsUncached(Key key) {
Resolver.this,
key);
ResolvedBindings previouslyResolvedBindings = getPreviouslyResolvedBindings(key).get();
if (hasLocalMultibindingContributions(previouslyResolvedBindings)
|| hasLocallyPresentOptionalBinding(previouslyResolvedBindings)) {
if (hasLocalMultibindingContributions(key)
|| hasLocalOptionalBindingContribution(previouslyResolvedBindings)) {
return true;
}

Expand Down Expand Up @@ -944,30 +943,33 @@ private boolean dependsOnLocalBindingsUncached(Binding binding) {
}

/**
* Returns {@code true} if {@code resolvedBindings} contains a synthetic multibinding with at
* least one contribution declared within this component's modules.
* Returns {@code true} if there is at least one multibinding contribution declared within
* this component's modules that matches the key.
*/
private boolean hasLocalMultibindingContributions(ResolvedBindings resolvedBindings) {
return any(
resolvedBindings.contributionBindings(),
ContributionBinding::isSyntheticMultibinding)
&& keysMatchingRequest(resolvedBindings.key())
.stream()
.anyMatch(key -> !getLocalExplicitMultibindings(key).isEmpty());
private boolean hasLocalMultibindingContributions(Key requestKey) {
return keysMatchingRequest(requestKey)
.stream()
.anyMatch(key -> !getLocalExplicitMultibindings(key).isEmpty());
}

/**
* Returns {@code true} if {@code resolvedBindings} contains a synthetic optional binding for
* which there is an explicit present binding in this component.
* Returns {@code true} if there is a contribution in this component for an {@code
* Optional<Foo>} key that has not been contributed in a parent.
*/
private boolean hasLocallyPresentOptionalBinding(ResolvedBindings resolvedBindings) {
return resolvedBindings
.contributionBindings()
.stream()
.map(ContributionBinding::kind)
.anyMatch(isEqual(OPTIONAL))
&& !getLocalExplicitBindings(keyFactory.unwrapOptional(resolvedBindings.key()).get())
.isEmpty();
private boolean hasLocalOptionalBindingContribution(ResolvedBindings resolvedBindings) {
if (resolvedBindings
.contributionBindings()
.stream()
.map(ContributionBinding::kind)
.anyMatch(isEqual(OPTIONAL))) {
return !getLocalExplicitBindings(keyFactory.unwrapOptional(resolvedBindings.key()).get())
.isEmpty();
} else {
// If a parent contributes a @Provides Optional<Foo> binding and a child has a
// @BindsOptionalOf Foo method, the two should conflict, even if there is no binding for
// Foo on its own
return !getOptionalBindingDeclarations(resolvedBindings.key()).isEmpty();
}
}
}
}
Expand Down
71 changes: 71 additions & 0 deletions javatests/dagger/internal/codegen/MultibindingTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -191,4 +191,75 @@ public void produceConcreteSet_andRequestSetOfProduced() {
.onLineContaining("setOfProduced()");

}

@Test
public void provideExplicitSetInParent_AndMultibindingContributionInChild() {
JavaFileObject parent =
JavaFileObjects.forSourceLines(
"test.Parent",
"package test;",
"",
"import dagger.Component;",
"import java.util.Set;",
"",
"@Component(modules = ParentModule.class)",
"interface Parent {",
" Set<String> set();",
" Child child();",
"}");
JavaFileObject parentModule =
JavaFileObjects.forSourceLines(
"test.ParentModule",
"package test;",
"",
"import dagger.Module;",
"import dagger.Provides;",
"import java.util.HashSet;",
"import java.util.Set;",
"",
"@Module",
"class ParentModule {",
" @Provides",
" Set<String> set() {",
" return new HashSet();",
" }",
"}");

JavaFileObject child =
JavaFileObjects.forSourceLines(
"test.Child",
"package test;",
"",
"import dagger.Subcomponent;",
"import java.util.Set;",
"",
"@Subcomponent(modules = ChildModule.class)",
"interface Child {",
" Set<String> set();",
"}");
JavaFileObject childModule =
JavaFileObjects.forSourceLines(
"test.ChildModule",
"package test;",
"",
"import dagger.Module;",
"import dagger.multibindings.IntoSet;",
"import dagger.Provides;",
"",
"@Module",
"class ChildModule {",
" @Provides",
" @IntoSet",
" String setContribution() {",
" return new String();",
" }",
"}");

Compilation compilation = daggerCompiler().compile(parent, parentModule, child, childModule);
assertThat(compilation).failed();
assertThat(compilation)
.hadErrorContaining("incompatible bindings or declarations")
.inFile(parent)
.onLineContaining("interface Parent");
}
}
96 changes: 96 additions & 0 deletions javatests/dagger/internal/codegen/OptionalBindingTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
* Copyright (C) 2016 The Dagger Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package dagger.internal.codegen;

import static com.google.testing.compile.CompilationSubject.assertThat;
import static dagger.internal.codegen.Compilers.daggerCompiler;

import com.google.testing.compile.Compilation;
import com.google.testing.compile.JavaFileObjects;
import javax.tools.JavaFileObject;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class OptionalBindingTest {
@Test
public void provideExplicitOptionalInParent_AndBindsOptionalOfInChild() {
JavaFileObject parent =
JavaFileObjects.forSourceLines(
"test.Parent",
"package test;",
"",
"import dagger.Component;",
"import java.util.Optional;",
"",
"@Component(modules = ParentModule.class)",
"interface Parent {",
" Optional<String> optional();",
" Child child();",
"}");
JavaFileObject parentModule =
JavaFileObjects.forSourceLines(
"test.ParentModule",
"package test;",
"",
"import dagger.Module;",
"import dagger.Provides;",
"import java.util.Optional;",
"",
"@Module",
"class ParentModule {",
" @Provides",
" Optional<String> optional() {",
" return Optional.of(new String());",
" }",
"}");

JavaFileObject child =
JavaFileObjects.forSourceLines(
"test.Child",
"package test;",
"",
"import dagger.Subcomponent;",
"import java.util.Optional;",
"",
"@Subcomponent(modules = ChildModule.class)",
"interface Child {",
" Optional<String> optional();",
"}");
JavaFileObject childModule =
JavaFileObjects.forSourceLines(
"test.ChildModule",
"package test;",
"",
"import dagger.BindsOptionalOf;",
"import dagger.Module;",
"",
"@Module",
"interface ChildModule {",
" @BindsOptionalOf",
" String optionalString();",
"}");

Compilation compilation = daggerCompiler().compile(parent, parentModule, child, childModule);
assertThat(compilation).failed();
assertThat(compilation)
.hadErrorContaining("Optional<java.lang.String> is bound multiple times")
.inFile(parent)
.onLineContaining("interface Parent");
}
}

0 comments on commit 98b9c3f

Please sign in to comment.