Skip to content

Commit

Permalink
Stop generating members injectors for classes without local injection…
Browse files Browse the repository at this point in the history 
… sites and without @Inject constructors.

Fixes #955

RELNOTES=Fixed issue with members injectors being created for types without @Inject fields/constructors.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=300606795
  • Loading branch information
@Chang-Eric @netdpb
Chang-Eric authored and netdpb committed Mar 13, 2020
1 parent 113c498 commit 20f6442
Show file tree
Hide file tree
Showing 4 changed files with 300 additions and 8 deletions.
8 changes: 8 additions & 0 deletions java/dagger/internal/codegen/writing/MembersInjectorGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.squareup.javapoet.MethodSpec.constructorBuilder;
import static com.squareup.javapoet.MethodSpec.methodBuilder;
import static com.squareup.javapoet.TypeSpec.classBuilder;
import static dagger.internal.codegen.binding.InjectionAnnotations.injectedConstructors;
import static dagger.internal.codegen.binding.SourceFiles.bindingTypeElementTypeVariableNames;
import static dagger.internal.codegen.binding.SourceFiles.frameworkFieldUsages;
import static dagger.internal.codegen.binding.SourceFiles.generateBindingFieldsForDependencies;
Expand Down Expand Up @@ -107,6 +108,13 @@ public Optional<TypeSpec.Builder> write(MembersInjectionBinding binding) {
return Optional.empty();
}

// Members injectors for classes with no local injection sites and no @Inject
// constructor are unused.
if (!binding.hasLocalInjectionSites()
&& injectedConstructors(binding.membersInjectedType()).isEmpty()) {
return Optional.empty();
}

statisticsCollector.recordMembersInjectorGenerated();

// We don't want to write out resolved bindings -- we want to write out the generic version.
Expand Down
43 changes: 35 additions & 8 deletions java/dagger/internal/codegen/writing/MembersInjectorProviderCreationExpression.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.squareup.javapoet.CodeBlock;
import dagger.internal.codegen.binding.ProvisionBinding;
import dagger.internal.codegen.writing.FrameworkFieldInitializer.FrameworkInstanceCreationExpression;
import javax.lang.model.element.TypeElement;
import javax.lang.model.type.TypeMirror;

/** A {@code Provider<MembersInjector<Foo>>} creation expression. */
Expand All @@ -46,17 +47,43 @@ public CodeBlock creationExpression() {
TypeMirror membersInjectedType =
getOnlyElement(MoreTypes.asDeclared(binding.key().type()).getTypeArguments());

CodeBlock membersInjector =
binding.injectionSites().isEmpty()
? CodeBlock.of("$T.<$T>noOp()", MEMBERS_INJECTORS, membersInjectedType)
: CodeBlock.of(
"$T.create($L)",
membersInjectorNameForType(MoreTypes.asTypeElement(membersInjectedType)),
componentBindingExpressions.getCreateMethodArgumentsCodeBlock(binding));
boolean castThroughRawType = false;
CodeBlock membersInjector;
if (binding.injectionSites().isEmpty()) {
membersInjector = CodeBlock.of("$T.<$T>noOp()", MEMBERS_INJECTORS, membersInjectedType);
} else {
TypeElement injectedTypeElement = MoreTypes.asTypeElement(membersInjectedType);
while (!hasLocalInjectionSites(injectedTypeElement)) {
// Cast through a raw type since we're going to be using the MembersInjector for the
// parent type.
castThroughRawType = true;
injectedTypeElement = MoreTypes.asTypeElement(injectedTypeElement.getSuperclass());
}

membersInjector = CodeBlock.of(
"$T.create($L)",
membersInjectorNameForType(injectedTypeElement),
componentBindingExpressions.getCreateMethodArgumentsCodeBlock(binding));
}

// TODO(ronshapiro): consider adding a MembersInjectorBindingExpression to return this directly
// (as it's rarely requested as a Provider).
return CodeBlock.of("$T.create($L)", INSTANCE_FACTORY, membersInjector);
CodeBlock providerExpression = CodeBlock.of("$T.create($L)", INSTANCE_FACTORY, membersInjector);
// If needed we cast through raw type around the InstanceFactory type as opposed to the
// MembersInjector since we end up with an InstanceFactory<MembersInjector> as opposed to a
// InstanceFactory<MembersInjector<Foo>> and that becomes unassignable. To fix it would require
// a second cast. If we just cast to the raw type InstanceFactory though, that becomes
// assignable.
return castThroughRawType
? CodeBlock.of("($T) $L", INSTANCE_FACTORY, providerExpression) : providerExpression;
}

private boolean hasLocalInjectionSites(TypeElement injectedTypeElement) {
return binding.injectionSites()
.stream()
.anyMatch(
injectionSite ->
injectionSite.element().getEnclosingElement().equals(injectedTypeElement));
}

@Override
Expand Down
34 changes: 34 additions & 0 deletions javatests/dagger/functional/membersinject/MembersInjectTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@

import static com.google.common.truth.Truth.assertThat;

import dagger.BindsInstance;
import dagger.Component;
import dagger.MembersInjector;
import dagger.functional.multipackage.DaggerMembersInjectionVisibilityComponent;
import dagger.functional.multipackage.MembersInjectionVisibilityComponent;
import dagger.functional.multipackage.a.AGrandchild;
import dagger.functional.multipackage.a.AParent;
import dagger.functional.multipackage.b.BChild;
import javax.inject.Inject;
import javax.inject.Provider;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -93,4 +96,35 @@ public String get() {
injector.injectMembers(child);
assertThat(child.t).isEqualTo("field!");
}

public static final class A extends B {
// No injected members
}

public static class B extends C {
// No injected members
}

public static class C {
@Inject String value;
}

@Component
interface NonLocalMembersComponent {
MembersInjector<A> getAMembersInjector();

@Component.Factory
interface Factory {
NonLocalMembersComponent create(@BindsInstance String value);
}
}

@Test
public void testNonLocalMembersInjection() {
MembersInjector<A> membersInjector = DaggerMembersInjectTest_NonLocalMembersComponent.factory()
.create("test").getAMembersInjector();
A testA = new A();
membersInjector.injectMembers(testA);
assertThat(testA.value).isEqualTo("test");
}
}
223 changes: 223 additions & 0 deletions javatests/dagger/internal/codegen/MembersInjectionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1704,4 +1704,227 @@ public void publicSupertypeHiddenSubtype() {
.generatedSourceFile("test.DaggerTestComponent")
.containsElementsIn(generatedComponent);
}

// Shows that we shouldn't create a members injector for a type that doesn't have
// @Inject fields or @Inject constructor even if it extends and is extended by types that do.
@Test
public void middleClassNoFieldInjection() {
JavaFileObject classA =
JavaFileObjects.forSourceLines(
"test.A",
"package test;",
"",
"import javax.inject.Inject;",
"",
"class A extends B {",
" @Inject String valueA;",
"}");
JavaFileObject classB =
JavaFileObjects.forSourceLines(
"test.B",
"package test;",
"",
"class B extends C {",
"}");
JavaFileObject classC =
JavaFileObjects.forSourceLines(
"test.C",
"package test;",
"",
"import javax.inject.Inject;",
"",
"class C { ",
" @Inject String valueC;",
"}");
JavaFileObject expectedAMembersInjector =
JavaFileObjects.forSourceLines(
"test.A_MembersInjector",
"package test;",
"",
"import dagger.MembersInjector;",
"import dagger.internal.InjectedFieldSignature;",
IMPORT_GENERATED_ANNOTATION,
"import javax.inject.Provider;",
"",
GENERATED_CODE_ANNOTATIONS,
"public final class A_MembersInjector implements MembersInjector<A> {",
" private final Provider<String> valueCProvider;",
" private final Provider<String> valueAProvider;",
"",
" public A_MembersInjector(",
" Provider<String> valueCProvider, Provider<String> valueAProvider) {",
" this.valueCProvider = valueCProvider;",
" this.valueAProvider = valueAProvider;",
" }",
"",
" public static MembersInjector<A> create(",
" Provider<String> valueCProvider, Provider<String> valueAProvider) {",
" return new A_MembersInjector(valueCProvider, valueAProvider);",
" }",
"",
" @Override",
" public void injectMembers(A instance) {",
" C_MembersInjector.injectValueC(instance, valueCProvider.get());",
" injectValueA(instance, valueAProvider.get());",
" }",
"",
" @InjectedFieldSignature(\"test.A.valueA\")",
" public static void injectValueA(Object instance, String valueA) {",
" ((A) instance).valueA = valueA;",
" }",
"}");

JavaFileObject expectedCMembersInjector =
JavaFileObjects.forSourceLines(
"test.C_MembersInjector",
"package test;",
"",
"import dagger.MembersInjector;",
"import dagger.internal.InjectedFieldSignature;",
IMPORT_GENERATED_ANNOTATION,
"import javax.inject.Provider;",
"",
GENERATED_CODE_ANNOTATIONS,
"public final class C_MembersInjector implements MembersInjector<C> {",
" private final Provider<String> valueCProvider;",
"",
" public C_MembersInjector(Provider<String> valueCProvider) {",
" this.valueCProvider = valueCProvider;",
" }",
"",
" public static MembersInjector<C> create(",
" Provider<String> valueCProvider) {",
" return new C_MembersInjector(valueCProvider);",
" }",
"",
" @Override",
" public void injectMembers(C instance) {",
" injectValueC(instance, valueCProvider.get());",
" }",
"",
" @InjectedFieldSignature(\"test.C.valueC\")",
" public static void injectValueC(Object instance, String valueC) {",
" ((C) instance).valueC = valueC;",
" }",
"}");

Compilation compilation =
daggerCompiler()
.withOptions(compilerMode.javacopts())
.compile(classA, classB, classC);
assertThat(compilation).succeeded();
assertThat(compilation)
.generatedSourceFile("test.A_MembersInjector")
.hasSourceEquivalentTo(expectedAMembersInjector);
assertThat(compilation)
.generatedSourceFile("test.C_MembersInjector")
.hasSourceEquivalentTo(expectedCMembersInjector);

try {
assertThat(compilation).generatedSourceFile("test.B_MembersInjector");
// Can't throw an assertion error since it would be caught.
throw new IllegalStateException("Test generated a B_MembersInjector");
} catch (AssertionError expected) {
}
}

// Shows that we do generate a MembersInjector for a type that has an @Inject
// constructor and that extends a type with @Inject fields, even if it has no local field
// injection sites
// TODO(user): Are these even used anymore?
@Test
public void testConstructorInjectedFieldInjection() {
JavaFileObject classA =
JavaFileObjects.forSourceLines(
"test.A",
"package test;",
"",
"import javax.inject.Inject;",
"",
"class A extends B {",
" @Inject A() {}",
"}");
JavaFileObject classB =
JavaFileObjects.forSourceLines(
"test.B",
"package test;",
"",
"import javax.inject.Inject;",
"",
"class B { ",
" @Inject String valueB;",
"}");
JavaFileObject expectedAMembersInjector =
JavaFileObjects.forSourceLines(
"test.A_MembersInjector",
"package test;",
"",
"import dagger.MembersInjector;",
IMPORT_GENERATED_ANNOTATION,
"import javax.inject.Provider;",
"",
GENERATED_CODE_ANNOTATIONS,
"public final class A_MembersInjector implements MembersInjector<A> {",
" private final Provider<String> valueBProvider;",
"",
" public A_MembersInjector(Provider<String> valueBProvider) {",
" this.valueBProvider = valueBProvider;",
" }",
"",
" public static MembersInjector<A> create(Provider<String> valueBProvider) {",
" return new A_MembersInjector(valueBProvider);",
" }",
"",
" @Override",
" public void injectMembers(A instance) {",
" B_MembersInjector.injectValueB(instance, valueBProvider.get());",
" }",
"}");

JavaFileObject expectedBMembersInjector =
JavaFileObjects.forSourceLines(
"test.B_MembersInjector",
"package test;",
"",
"import dagger.MembersInjector;",
"import dagger.internal.InjectedFieldSignature;",
IMPORT_GENERATED_ANNOTATION,
"import javax.inject.Provider;",
"",
GENERATED_CODE_ANNOTATIONS,
"public final class B_MembersInjector implements MembersInjector<B> {",
" private final Provider<String> valueBProvider;",
"",
" public B_MembersInjector(Provider<String> valueBProvider) {",
" this.valueBProvider = valueBProvider;",
" }",
"",
" public static MembersInjector<B> create(",
" Provider<String> valueBProvider) {",
" return new B_MembersInjector(valueBProvider);",
" }",
"",
" @Override",
" public void injectMembers(B instance) {",
" injectValueB(instance, valueBProvider.get());",
" }",
"",
" @InjectedFieldSignature(\"test.B.valueB\")",
" public static void injectValueB(Object instance, String valueB) {",
" ((B) instance).valueB = valueB;",
" }",
"}");

Compilation compilation =
daggerCompiler()
.withOptions(compilerMode.javacopts())
.compile(classA, classB);
assertThat(compilation).succeeded();
assertThat(compilation)
.generatedSourceFile("test.A_MembersInjector")
.hasSourceEquivalentTo(expectedAMembersInjector);
assertThat(compilation)
.generatedSourceFile("test.B_MembersInjector")
.hasSourceEquivalentTo(expectedBMembersInjector);
}
}

0 comments on commit 20f6442

Please sign in to comment.