diff --git a/docs/rules.md b/docs/rules.md index eadc7bdf..63751590 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -63,6 +63,8 @@ here: [Keeping the Daggers Sharp](https://developer.squareup.com/blog/keeping-th [//]: # (TODO mention `AppComponentFactory` and `FragmentFactory`) +## Prefer using @Binds over @Provides + ### 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..5f154684 100644 --- a/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/DaggerRulesIssueRegistry.kt +++ b/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/DaggerRulesIssueRegistry.kt @@ -12,6 +12,7 @@ import com.google.auto.service.AutoService import dev.whosnickdoglio.dagger.detectors.ComponentMustBeAbstractDetector import dev.whosnickdoglio.dagger.detectors.ConstructorInjectionOverFieldInjectionDetector import dev.whosnickdoglio.dagger.detectors.CorrectBindsUsageDetector +import dev.whosnickdoglio.dagger.detectors.FavorBindsOverProvidesDetector import dev.whosnickdoglio.dagger.detectors.MissingModuleAnnotationDetector import dev.whosnickdoglio.dagger.detectors.MultipleScopesDetector import dev.whosnickdoglio.dagger.detectors.ScopedWithoutInjectAnnotationDetector @@ -26,6 +27,7 @@ public class DaggerRulesIssueRegistry : IssueRegistry() { ConstructorInjectionOverFieldInjectionDetector.ISSUE, CorrectBindsUsageDetector.ISSUE_BINDS_ABSTRACT, CorrectBindsUsageDetector.ISSUE_CORRECT_RETURN_TYPE, + FavorBindsOverProvidesDetector.ISSUE, MissingModuleAnnotationDetector.ISSUE, MultipleScopesDetector.ISSUE, StaticProvidesDetector.ISSUE, diff --git a/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/detectors/FavorBindsOverProvidesDetector.kt b/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/detectors/FavorBindsOverProvidesDetector.kt new file mode 100644 index 00000000..bfda4f53 --- /dev/null +++ b/lint/dagger/src/main/java/dev/whosnickdoglio/dagger/detectors/FavorBindsOverProvidesDetector.kt @@ -0,0 +1,110 @@ +/* + * 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 com.android.tools.lint.detector.api.asCall +import com.intellij.psi.util.InheritanceUtil +import dev.whosnickdoglio.lint.annotations.dagger.PROVIDES +import org.jetbrains.uast.UAnnotation +import org.jetbrains.uast.UClass +import org.jetbrains.uast.UElement +import org.jetbrains.uast.UMethod +import org.jetbrains.uast.toUElementOfType +import org.jetbrains.uast.util.isConstructorCall + +// TODO make this configurable + +internal class FavorBindsOverProvidesDetector : 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) { + // TODO check if provided class has `@Inject` on it's constructor + val providesMethod = node.uastParent as? UMethod ?: return + + val parameterIsReturnedAsSuper = + providesMethod.checkIfMethodJustPassesThroughParameter(context) + + val constructorCall = + providesMethod.checkIfMethodBodyJustCallsConstructor(context) + + if (parameterIsReturnedAsSuper || constructorCall) { + context.report( + issue = ISSUE, + location = context.getNameLocation(providesMethod), + message = ISSUE.getExplanation(TextFormat.TEXT), + ) + } + } + } + } + + private fun UMethod.checkIfMethodJustPassesThroughParameter(context: JavaContext): Boolean { + val methodReturnType = context.evaluator.getTypeClass(this.returnType) + val parameterType = context.evaluator.getTypeClass(this.uastParameters.firstOrNull()?.type) + + // CHeck if type is specifically the same type and not a supertype + return InheritanceUtil.isInheritor( + parameterType, + true, + methodReturnType?.qualifiedName.orEmpty(), + ) + } + + private fun UMethod.checkIfMethodBodyJustCallsConstructor(context: JavaContext): Boolean { + val methodReturnType = + context.evaluator.getTypeClass(this.returnType)?.toUElementOfType() + + val bodyReturnType = + context.evaluator + .getTypeClass(uastBody?.asCall()?.returnType) + ?.toUElementOfType() + + val isConstructorCall = uastBody?.isConstructorCall() == true + + val bodyReturnsSubClassOfReturnType = + InheritanceUtil.isInheritor( + bodyReturnType, + true, + methodReturnType?.qualifiedName.orEmpty(), + ) + val returnCondition = isConstructorCall && bodyReturnsSubClassOfReturnType + + return returnCondition + } + + companion object { + + internal val allowList = setOf("Factory", "Builder") + + private val implementation = + Implementation(FavorBindsOverProvidesDetector::class.java, Scope.JAVA_FILE_SCOPE) + val ISSUE = + Issue.create( + id = "FavorBindsOverProvides", + briefDescription = "Using @Provides instead of @Binds", + explanation = "plz use @Binds instead of @Provides", + category = Category.CORRECTNESS, + priority = 5, + severity = Severity.WARNING, + implementation = implementation, + ) + .setEnabledByDefault(false) + } +} diff --git a/lint/dagger/src/test/java/dev/whosnickdoglio/dagger/detectors/FavorBindsOverProvidesDetectorTest.kt b/lint/dagger/src/test/java/dev/whosnickdoglio/dagger/detectors/FavorBindsOverProvidesDetectorTest.kt new file mode 100644 index 00000000..95f60474 --- /dev/null +++ b/lint/dagger/src/test/java/dev/whosnickdoglio/dagger/detectors/FavorBindsOverProvidesDetectorTest.kt @@ -0,0 +1,171 @@ +/* + * Copyright (C) 2024 Nicholas Doglio + * SPDX-License-Identifier: MIT + */ +package dev.whosnickdoglio.dagger.detectors + +import com.android.tools.lint.checks.infrastructure.TestFiles +import com.android.tools.lint.checks.infrastructure.TestLintTask +import dev.whosnickdoglio.stubs.daggerAnnotations +import org.junit.Test + +class FavorBindsOverProvidesDetectorTest { + + // TODO add more complicated methods (multiple params, setup before calling constructor, + // returning a lambda etc) + // or valid @Provides use cases + + @Test + fun `kotlin returning @Provides method parameter directly with supertype as return type shows an warning`() { + TestLintTask.lint() + .files( + daggerAnnotations, + TestFiles.kotlin( + """ + import dagger.Provides + import dagger.Module + + interface Greeter + class GreeterImpl: Greeter + + @Module + object MyModule { + + @Provides + fun provideAnotherGreeter(impl: GreeterImpl): Greeter = impl + } + """ + ) + .indented(), + ) + .issues(FavorBindsOverProvidesDetector.ISSUE) + .run() + .expect( + """ + src/Greeter.kt:11: Warning: plz use @Binds instead of @Provides [FavorBindsOverProvides] + fun provideAnotherGreeter(impl: GreeterImpl): Greeter = impl + ~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """ + .trimIndent() + ) + .expectWarningCount(1) + } + + @Test + fun `java returning @Provides method parameter directly with supertype as return type shows an warning`() { + TestLintTask.lint() + .files( + daggerAnnotations, + TestFiles.java( + """ + import dagger.Provides; + import dagger.Module; + + interface Greeter {} + class GreeterImpl extends Greeter {} + + @Module + class MyModule { + + @Provides + Greeter provideAnotherGreeter(GreeterImpl impl) { + return impl; + } + } + """ + ) + .indented(), + ) + .issues(FavorBindsOverProvidesDetector.ISSUE) + .run() + .expect( + """ + src/Greeter.java:11: Warning: plz use @Binds instead of @Provides [FavorBindsOverProvides] + Greeter provideAnotherGreeter(GreeterImpl impl) { + ~~~~~~~~~~~~~~~~~~~~~ + 0 errors, 1 warnings + """ + .trimIndent() + ) + .expectWarningCount(1) + } + + @Test + fun `kotlin calling constructor directly in @Provides method instead of using @Binds method shows a warning`() { + TestLintTask.lint() + .files( + daggerAnnotations, + TestFiles.kotlin( + """ + import dagger.Provides + import dagger.Module + + interface Greeter + class GreeterImpl: Greeter + + @Module + object MyModule { + + @Provides fun provideGreeter(): Greeter = GreeterImpl() + } + """ + ) + .indented(), + ) + .issues(FavorBindsOverProvidesDetector.ISSUE) + .run() + .expect("") + .expectWarningCount(1) + } + + @Test + fun `java calling constructor directly in @Provides method instead of using @Binds method shows a warning`() { + TestLintTask.lint() + .files( + daggerAnnotations, + TestFiles.java( + """ + import dagger.Provides; + import dagger.Module; + + interface Greeter {} + class GreeterImpl implements Greeter {} + + @Module + class MyModule { + + @Provides Greeter provideGreeter() { + return GreeterImpl(); + } + } + """ + ) + .indented(), + ) + .issues(FavorBindsOverProvidesDetector.ISSUE) + .run() + .expectErrorCount(1) + .expect("") + } + + @Test + fun `kotlin @Provides method that uses a Builder does not show a warning`() { + TODO("Not yet implemented") + } + + @Test + fun `java @Provides method that uses a Builder does not show a warning`() { + TODO("Not yet implemented") + } + + @Test + fun `kotlin @Provides method that uses a Factory does not show a warning`() { + TODO("Not yet implemented") + } + + @Test + fun `java @Provides method that uses a Factory does not show a warning`() { + TODO("Not yet implemented") + } +}