diff --git a/docs/rules.md b/docs/rules.md index eadc7bdf..8be55ffc 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -63,6 +63,72 @@ here: [Keeping the Daggers Sharp](https://developer.squareup.com/blog/keeping-th [//]: # (TODO mention `AppComponentFactory` and `FragmentFactory`) +## Prefer `@Inject` annotating classes in your project domain over providing them in a Dagger module + +When you want to add something to the Dagger graph that is a **class we own** (A class that has been written by a Zillow +engineer and the source code can be modified), you should use constructor injection over writing a `@Provides` method in +a Dagger `@Module` to add this class to the Dagger graph. +Methods are only for things Dagger can't figure +out how to provide on its own. +These are the three most common scenarios for when you should use a `@Provides` method: + +1. Adding a third party dependency to your Dagger Graph. + When you're using something like Room, Retrofit, or + `SharedPreferences` you cannot access their code to properly annotate their classes with an `@Inject` annotation for + Dagger to pick them up. +2. Adding a class to the Dagger graph that doesn't have a straight forward constructor but instead uses a `Factory` + or `Builder`. +3. Adding a duplicate class to the Dagger graph using a `@Named` or Qualifier annotation. + Sometimes you need multiple + instances of a single class (maybe one per feature or one for prod vs testing) so you can create a custom qualifier + annotation or use `@Named` to differentiate them. + +TODO mention dependency inversion + +A common pattern that arises when using dependency injection is to have an interface/implementation pair to allow to +easily swap implementations for testing (maybe a `Repository` that hits a real API in prod and fake data for tests). +In +these scenarios, you want to make sure your class depends on the interface and not the concrete implementation to make +swapping implementations easy. +A common mistake is to do this via a `@Provides` method instead of `@Binds` method. + +```kotlin +interface Greeter { + fun hello(): String +} + +class GreeterImpl @Inject constructor() : Greeter { + override fun hello(): String = "Hello World" +} + +// Bad! +@Module +abstract class GreeterModule { + @Provides + fun provideGreeter(): Greeter = GreeterImpl() +} + +// Good! +@Module +abstract class GreeterModule { + + @Binds + abstract fun provideGreeter(impl: GreeterImpl): Greeter +} +``` + +We want to do this for two major reasons. +The first being if we avoid using a `@Provides` method we can avoid +maintaining the constructor contract in two different places, at the class definition and also the `@Provides` method. +The `Greeter` example is simple with a no parameter constructor but imagine it having six dependencies, we'd need wire +that up correctly in both the constructor and `@Provides` method and then maintain it in two places if a new dependency +is added. +The second reason is by using `@Binds` Dagger will generate less code! +For `@Binds` functions no extra code is +actually generated. +Dagger can swap in the concrete implementation wherever the interface is used but if using +a `Provides` method Dagger would have to generate another class to supply this dependency to its consumers. + ### Methods annotated with `@Binds` must be abstract Methods annotated with the [`@Binds` annotation](https://dagger.dev/api/latest/dagger/Binds.html) need to be abstract. diff --git a/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/DaggerRulesIssueRegistry.kt b/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/DaggerRulesIssueRegistry.kt index e0e30f68..214cbdc9 100644 --- a/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/DaggerRulesIssueRegistry.kt +++ b/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/DaggerRulesIssueRegistry.kt @@ -11,6 +11,7 @@ import com.android.tools.lint.detector.api.Issue import com.google.auto.service.AutoService import dev.whosnickdoglio.dagger.detectors.ComponentMustBeAbstractDetector import dev.whosnickdoglio.dagger.detectors.ConstructorInjectionOverFieldInjectionDetector +import dev.whosnickdoglio.dagger.detectors.ConstructorInjectionOverProvidesDetector import dev.whosnickdoglio.dagger.detectors.CorrectBindsUsageDetector import dev.whosnickdoglio.dagger.detectors.MissingModuleAnnotationDetector import dev.whosnickdoglio.dagger.detectors.MultipleScopesDetector @@ -26,6 +27,7 @@ public class DaggerRulesIssueRegistry : IssueRegistry() { ConstructorInjectionOverFieldInjectionDetector.ISSUE, CorrectBindsUsageDetector.ISSUE_BINDS_ABSTRACT, CorrectBindsUsageDetector.ISSUE_CORRECT_RETURN_TYPE, + ConstructorInjectionOverProvidesDetector.ISSUE, MissingModuleAnnotationDetector.ISSUE, MultipleScopesDetector.ISSUE, StaticProvidesDetector.ISSUE, diff --git a/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/detectors/ConstructorInjectionOverProvidesDetector.kt b/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/detectors/ConstructorInjectionOverProvidesDetector.kt new file mode 100644 index 00000000..739fb890 --- /dev/null +++ b/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/detectors/ConstructorInjectionOverProvidesDetector.kt @@ -0,0 +1,67 @@ +/* + * Copyright (C) 2024 Nicholas Doglio + * SPDX-License-Identifier: MIT + */ +package dev.whosnickdoglio.dagger.detectors + +import com.android.tools.lint.client.api.UElementHandler +import com.android.tools.lint.detector.api.Category +import com.android.tools.lint.detector.api.Detector +import com.android.tools.lint.detector.api.Implementation +import com.android.tools.lint.detector.api.Issue +import com.android.tools.lint.detector.api.JavaContext +import com.android.tools.lint.detector.api.Scope +import com.android.tools.lint.detector.api.Severity +import com.android.tools.lint.detector.api.SourceCodeScanner +import com.android.tools.lint.detector.api.TextFormat +import dev.whosnickdoglio.lint.annotations.dagger.PROVIDES +import org.jetbrains.uast.UAnnotation +import org.jetbrains.uast.UCallExpression +import org.jetbrains.uast.UElement +import org.jetbrains.uast.util.isConstructorCall + +internal class ConstructorInjectionOverProvidesDetector : Detector(), SourceCodeScanner { + + override fun getApplicableUastTypes(): List> = + listOf(UAnnotation::class.java) + + override fun createUastHandler(context: JavaContext): UElementHandler = + object : UElementHandler() { + override fun visitAnnotation(node: UAnnotation) { + if (node.qualifiedName == PROVIDES) { + val method = node.uastParent as? UCallExpression ?: return + if (method.isConstructorCall()) { + context.report( + issue = ISSUE, + location = context.getLocation(method), + message = ISSUE.getExplanation(TextFormat.TEXT), + ) + } + } + } + } + + companion object { + + private val implementation = + Implementation( + ConstructorInjectionOverProvidesDetector::class.java, + Scope.JAVA_FILE_SCOPE, + ) + val ISSUE = + Issue.create( + id = "ConstructorInjectionOverProvidesMethods", + briefDescription = "@Provides method used instead of constructor injection", + explanation = + """ + `@Provides` methods are great for adding third party libraries or classes that require Builders or Factories \ + to the Dagger graph but for classes with simple constructors you should just add a `@Inject` annotation to the constructor + """, + category = Category.CORRECTNESS, + priority = 5, + severity = Severity.WARNING, + implementation = implementation, + ) + .setEnabledByDefault(false) + } +} diff --git a/lint/dagger/src/test/java/dev/whosnickdoglio/dagger/detectors/ConstructorInjectionOverProvidesDetectorTest.kt b/lint/dagger/src/test/java/dev/whosnickdoglio/dagger/detectors/ConstructorInjectionOverProvidesDetectorTest.kt new file mode 100644 index 00000000..39a4d5c9 --- /dev/null +++ b/lint/dagger/src/test/java/dev/whosnickdoglio/dagger/detectors/ConstructorInjectionOverProvidesDetectorTest.kt @@ -0,0 +1,47 @@ +/* + * Copyright (C) 2024 Nicholas Doglio + * SPDX-License-Identifier: MIT + */ +package dev.whosnickdoglio.dagger.detectors + +import com.android.tools.lint.checks.infrastructure.TestFiles.kotlin +import com.android.tools.lint.checks.infrastructure.TestLintTask.lint +import dev.whosnickdoglio.stubs.daggerAnnotations +import org.junit.Test + +class ConstructorInjectionOverProvidesDetectorTest { + + // TODO java and companion object tests + // TODO multiple @Provides + + @Test + fun `class that could use constructor injection triggers provides warning`() { + lint() + .files( + daggerAnnotations, + kotlin( + """ + package com.test.android + + import dagger.Provides + import dagger.Module + + class MyClass + + @Module + object MyModule { + + @Provides + fun provideMyClass(): MyClass { + return MyClass() + } + } + """ + ) + .indented(), + ) + .issues(ConstructorInjectionOverProvidesDetector.ISSUE) + .run() + .expectClean() + } +}