From 23fe85f4446dfb52a298eca8653726c7ae5f90be Mon Sep 17 00:00:00 2001
From: nicholasdoglio <nicholasdoglio@gmail.com>
Date: Sat, 8 Apr 2023 09:17:46 -0400
Subject: [PATCH] Adding a rule to suggest constructor injection over Provides
method
---
docs/rules.md | 66 ++++++++++++++++++
.../dagger/DaggerRulesIssueRegistry.kt | 2 +
...onstructorInjectionOverProvidesDetector.kt | 67 +++++++++++++++++++
...ructorInjectionOverProvidesDetectorTest.kt | 47 +++++++++++++
4 files changed, 182 insertions(+)
create mode 100644 lint/dagger/src/main/java/dev/whosnickdoglio/dagger/detectors/ConstructorInjectionOverProvidesDetector.kt
create mode 100644 lint/dagger/src/test/java/dev/whosnickdoglio/dagger/detectors/ConstructorInjectionOverProvidesDetectorTest.kt
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<Class<out UElement>> =
+ 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()
+ }
+}