Skip to content

Commit

Permalink
Removes unused (@Deprecated) module setter methods that contain onl…
Browse files Browse the repository at this point in the history
…y `static`/`abstract` binding methods.

Note that this doesn't remove all of the deprecated setter methods. In particular, this CL removes setter methods only for the case where a module instance is never needed because it contains only `static` and `abstract` methods.

However, this CL does not remove the deprecated setter methods in the general case where a module instance is not needed because it is unused in the component. That case is a bit trickier because it depends on the usage of the bindings rather than properties of the module itself.

RELNOTES=Potentially breaking change. Dagger no longer generates module setter methods for modules that have only static/abstract methods. These methods were previously marked as deprecated, as the module instances passed in aren't actually used by the Dagger component.
PiperOrigin-RevId: 578651221
  • Loading branch information
bcorso authored and Dagger Team committed Nov 1, 2023
1 parent d9d0a8e commit ed47d4b
Show file tree
Hide file tree
Showing 29 changed files with 18 additions and 362 deletions.
28 changes: 2 additions & 26 deletions java/dagger/hilt/processor/internal/root/RootMetadata.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import static dagger.internal.codegen.extension.DaggerStreams.toImmutableSet;

import androidx.room.compiler.processing.XConstructorElement;
import androidx.room.compiler.processing.XMethodElement;
import androidx.room.compiler.processing.XProcessingEnv;
import androidx.room.compiler.processing.XTypeElement;
import com.google.common.base.Supplier;
Expand Down Expand Up @@ -196,33 +195,10 @@ private ImmutableSetMultimap<ClassName, ClassName> getScopesByComponentUncached(
}

private static boolean daggerCanConstruct(XTypeElement type) {
if (type.isKotlinObject()) {
// Treat Kotlin object modules as if Dagger can construct them (it technically can't, but it
// doesn't need to as it can use them since all their provision methods are static).
if (!Processors.requiresModuleInstance(type)) {
return true;
}

return !isInnerClass(type)
&& !hasNonDaggerAbstractMethod(type)
&& (hasOnlyStaticProvides(type) || hasVisibleEmptyConstructor(type));
}

private static boolean isInnerClass(XTypeElement type) {
return type.isNested() && !type.isStatic();
}

private static boolean hasNonDaggerAbstractMethod(XTypeElement type) {
// TODO(erichang): Actually this isn't really supported b/28989613
return type.getDeclaredMethods().stream()
.filter(XMethodElement::isAbstract)
.anyMatch(method -> !Processors.hasDaggerAbstractMethodAnnotation(method));
}

private static boolean hasOnlyStaticProvides(XTypeElement type) {
// TODO(erichang): Check for @Produces too when we have a producers story
return type.getDeclaredMethods().stream()
.filter(method -> method.hasAnnotation(ClassNames.PROVIDES))
.allMatch(XMethodElement::isStatic);
return hasVisibleEmptyConstructor(type) && (!type.isNested() || type.isStatic());
}

private static boolean hasVisibleEmptyConstructor(XTypeElement type) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public boolean requiresAPassedInstance() {
* <p>Alternatively, if the module is a Kotlin Object then the binding methods are considered
* {@code static}, requiring no module instance.
*/
private boolean requiresModuleInstance() {
public boolean requiresModuleInstance() {
if (typeElement().isKotlinObject() || typeElement().isCompanionObject()) {
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,14 @@ private static Stream<ComponentRequirement> componentRequirements(ComponentDescr
&& isElementAccessibleFrom(
module.moduleElement(),
component.typeElement().getClassName().packageName()))
.map(module -> ComponentRequirement.forModule(module.moduleElement().getType())));
.map(module -> ComponentRequirement.forModule(module.moduleElement().getType()))
// If the user hasn't defined an explicit creator/builder then we need to prune out the
// module requirements that don't require a module instance to match the non-hjar
// implementation.
.filter(
requirement ->
component.creatorDescriptor().isPresent()
|| requirement.requiresModuleInstance()));
}

private boolean hasBindsInstanceMethods(ComponentDescriptor componentDescriptor) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,12 @@ private Optional<MethodSpec> createSetterMethod(
case NEEDED:
return Optional.of(normalSetterMethod(requirement));
case UNNEEDED:
// If this is a generated Builder, then remove the setter methods for modules that don't
// require an instance.
if (!componentDescriptor().creatorDescriptor().isPresent()
&& !requirement.requiresModuleInstance()) {
return Optional.empty();
}
// TODO(bcorso): Don't generate noop setters for any unneeded requirements.
// However, since this is a breaking change we can at least avoid trying
// to generate noop setters for impossible cases like when the requirement type
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ static dagger.functional.basic.subpackage.NestedType provideSomeType() {

@Test
public void typeNameWontClashWithNestedTypeName() {
TestComponent component =
DaggerComponentNestedTypeTest_TestComponent.builder().testModule(new TestModule()).build();
TestComponent component = DaggerComponentNestedTypeTest_TestComponent.create();
assertThat(component.nestedType()).isNotNull();
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package test;

import dagger.internal.DaggerGenerated;
import dagger.internal.Preconditions;
import javax.annotation.processing.Generated;

@DaggerGenerated
Expand Down Expand Up @@ -31,15 +30,6 @@ final class DaggerParent {
private Builder() {
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder parentModule(ParentModule parentModule) {
Preconditions.checkNotNull(parentModule);
return this;
}

public Parent build() {
return new ParentImpl();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package test;

import dagger.internal.DaggerGenerated;
import dagger.internal.Preconditions;
import javax.annotation.processing.Generated;

@DaggerGenerated
Expand Down Expand Up @@ -31,15 +30,6 @@ final class DaggerParent {
private Builder() {
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder parentModule(ParentModule parentModule) {
Preconditions.checkNotNull(parentModule);
return this;
}

public Parent build() {
return new ParentImpl();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package test;

import dagger.internal.DaggerGenerated;
import dagger.internal.Preconditions;
import javax.annotation.processing.Generated;

@DaggerGenerated
Expand Down Expand Up @@ -31,60 +30,6 @@ final class DaggerTestComponent {
private Builder() {
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder testModule(TestModule testModule) {
Preconditions.checkNotNull(testModule);
return this;
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder parentTestIncluded(ParentTestIncluded parentTestIncluded) {
Preconditions.checkNotNull(parentTestIncluded);
return this;
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder alwaysIncluded(AlwaysIncluded alwaysIncluded) {
Preconditions.checkNotNull(alwaysIncluded);
return this;
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder depModule(DepModule depModule) {
Preconditions.checkNotNull(depModule);
return this;
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder parentDepIncluded(ParentDepIncluded parentDepIncluded) {
Preconditions.checkNotNull(parentDepIncluded);
return this;
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder refByDep(RefByDep refByDep) {
Preconditions.checkNotNull(refByDep);
return this;
}

public TestComponent build() {
return new TestComponentImpl();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package test;

import dagger.internal.DaggerGenerated;
import dagger.internal.Preconditions;
import javax.annotation.processing.Generated;

@DaggerGenerated
Expand Down Expand Up @@ -31,60 +30,6 @@ final class DaggerTestComponent {
private Builder() {
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder testModule(TestModule testModule) {
Preconditions.checkNotNull(testModule);
return this;
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder parentTestIncluded(ParentTestIncluded parentTestIncluded) {
Preconditions.checkNotNull(parentTestIncluded);
return this;
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder alwaysIncluded(AlwaysIncluded alwaysIncluded) {
Preconditions.checkNotNull(alwaysIncluded);
return this;
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder depModule(DepModule depModule) {
Preconditions.checkNotNull(depModule);
return this;
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder parentDepIncluded(ParentDepIncluded parentDepIncluded) {
Preconditions.checkNotNull(parentDepIncluded);
return this;
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder refByDep(RefByDep refByDep) {
Preconditions.checkNotNull(refByDep);
return this;
}

public TestComponent build() {
return new TestComponentImpl();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package test;

import dagger.internal.DaggerGenerated;
import dagger.internal.Preconditions;
import javax.annotation.processing.Generated;

@DaggerGenerated
Expand Down Expand Up @@ -31,15 +30,6 @@ final class DaggerParent {
private Builder() {
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder testModule(TestModule testModule) {
Preconditions.checkNotNull(testModule);
return this;
}

public Parent build() {
return new ParentImpl();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package test;

import dagger.internal.DaggerGenerated;
import dagger.internal.Preconditions;
import javax.annotation.processing.Generated;

@DaggerGenerated
Expand Down Expand Up @@ -31,15 +30,6 @@ final class DaggerParent {
private Builder() {
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder testModule(TestModule testModule) {
Preconditions.checkNotNull(testModule);
return this;
}

public Parent build() {
return new ParentImpl();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@ package test;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import dagger.internal.DaggerGenerated;
import dagger.internal.DoubleCheck;
import dagger.internal.Preconditions;
import java.util.List;
import javax.annotation.processing.Generated;
import javax.inject.Provider;
import other.InaccessiblesModule;
import other.InaccessiblesModule_InaccessiblesFactory;
import other.UsesInaccessibles;
import other.UsesInaccessibles_Factory;
Expand Down Expand Up @@ -40,15 +38,6 @@ final class DaggerTestComponent {
private Builder() {
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder inaccessiblesModule(InaccessiblesModule inaccessiblesModule) {
Preconditions.checkNotNull(inaccessiblesModule);
return this;
}

public TestComponent build() {
return new TestComponentImpl();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@ package test;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import dagger.internal.DaggerGenerated;
import dagger.internal.DoubleCheck;
import dagger.internal.Preconditions;
import java.util.List;
import javax.annotation.processing.Generated;
import javax.inject.Provider;
import other.InaccessiblesModule;
import other.InaccessiblesModule_InaccessiblesFactory;
import other.UsesInaccessibles;
import other.UsesInaccessibles_Factory;
Expand Down Expand Up @@ -40,15 +38,6 @@ final class DaggerTestComponent {
private Builder() {
}

/**
* @deprecated This module is declared, but an instance is not used in the component. This method is a no-op. For more, see https://dagger.dev/unused-modules.
*/
@Deprecated
public Builder inaccessiblesModule(InaccessiblesModule inaccessiblesModule) {
Preconditions.checkNotNull(inaccessiblesModule);
return this;
}

public TestComponent build() {
return new TestComponentImpl();
}
Expand Down
Loading

0 comments on commit ed47d4b

Please sign in to comment.