Adding a rule to suggest constructor injection over Provides method

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.

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,

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)
+    }
+}

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()
+    }
+}