From de606d8ca7af5516fcee51f66fb3b7cba8ea8132 Mon Sep 17 00:00:00 2001 From: nicholasdoglio Date: Fri, 10 Feb 2023 17:12:05 -0500 Subject: [PATCH 1/5] Update ViewModelInjectionDetector to support a lint option --- .../compose/ViewModelInjectionDetector.kt | 37 ++++++++++++++----- .../compose/ViewModelInjectionDetectorTest.kt | 8 +++- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt b/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt index 0c8eb6dd..6e169601 100644 --- a/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt +++ b/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt @@ -8,15 +8,28 @@ import com.android.tools.lint.detector.api.Issue import com.android.tools.lint.detector.api.JavaContext import com.android.tools.lint.detector.api.Severity import com.android.tools.lint.detector.api.SourceCodeScanner +import com.android.tools.lint.detector.api.StringOption import org.jetbrains.kotlin.psi.KtCallExpression import org.jetbrains.kotlin.psi.KtFunction import org.jetbrains.kotlin.psi.KtProperty import slack.lint.compose.util.* import slack.lint.compose.util.sourceImplementation -class ViewModelInjectionDetector : ComposableFunctionDetector(), SourceCodeScanner { +class ViewModelInjectionDetector +@JvmOverloads +constructor(private val allowedNames: StringSetLintOption = StringSetLintOption(ALLOW_LIST)) : + ComposableFunctionDetector(allowedNames), SourceCodeScanner { companion object { + + internal val ALLOW_LIST = + StringOption( + "allowed-viewmodel-injection", + "A comma-separated list of viewModel factories.", + null, + "This property should define comma-separated list of allowed viewModel factory names." + ) + private fun errorMessage(factoryName: String): String = """ Implicit dependencies of composables should be made explicit. @@ -27,14 +40,15 @@ class ViewModelInjectionDetector : ComposableFunctionDetector(), SourceCodeScann val ISSUE = Issue.create( - id = "ComposeViewModelInjection", - briefDescription = "Implicit dependencies of composables should be made explicit", - explanation = "Replaced when reporting", - category = Category.CORRECTNESS, - priority = Priorities.NORMAL, - severity = Severity.ERROR, - implementation = sourceImplementation() - ) + id = "ComposeViewModelInjection", + briefDescription = "Implicit dependencies of composables should be made explicit", + explanation = "Replaced when reporting", + category = Category.CORRECTNESS, + priority = Priorities.NORMAL, + severity = Severity.ERROR, + implementation = sourceImplementation() + ) + .setOptions(listOf(ALLOW_LIST)) private val KnownViewModelFactories by lazy { setOf( @@ -57,7 +71,10 @@ class ViewModelInjectionDetector : ComposableFunctionDetector(), SourceCodeScann .flatMap { property -> property .findDirectChildrenByClass() - .filter { KnownViewModelFactories.contains(it.calleeExpression?.text) } + .filter { + KnownViewModelFactories.contains(it.calleeExpression?.text) || + allowedNames.value.contains(it.calleeExpression?.text) + } .map { property to it.calleeExpression!!.text } } .forEach { (property, viewModelFactoryName) -> diff --git a/compose-lint-checks/src/test/java/slack/lint/compose/ViewModelInjectionDetectorTest.kt b/compose-lint-checks/src/test/java/slack/lint/compose/ViewModelInjectionDetectorTest.kt index 2dae692f..6b0fbc49 100644 --- a/compose-lint-checks/src/test/java/slack/lint/compose/ViewModelInjectionDetectorTest.kt +++ b/compose-lint-checks/src/test/java/slack/lint/compose/ViewModelInjectionDetectorTest.kt @@ -3,6 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 package slack.lint.compose +import com.android.tools.lint.checks.infrastructure.TestLintTask import com.android.tools.lint.checks.infrastructure.TestMode import com.android.tools.lint.detector.api.Detector import com.android.tools.lint.detector.api.Issue @@ -23,7 +24,8 @@ class ViewModelInjectionDetectorTest(private val viewModel: String) : BaseSlackL arrayOf("weaverViewModel"), arrayOf("hiltViewModel"), arrayOf("injectedViewModel"), - arrayOf("mavericksViewModel") + arrayOf("mavericksViewModel"), + arrayOf("tangleViewModel"), ) } } @@ -34,6 +36,10 @@ class ViewModelInjectionDetectorTest(private val viewModel: String) : BaseSlackL // This mode is irrelevant to our test and totally untestable with stringy outputs override val skipTestModes: Array = arrayOf(TestMode.SUPPRESSIBLE, TestMode.TYPE_ALIAS) + override fun lint(): TestLintTask { + return super.lint().configureOption(ViewModelInjectionDetector.ALLOW_LIST, "tangleViewModel") + } + @Test fun `passes when a weaverViewModel is used as a default param`() { @Language("kotlin") From 29f851b79f1cf8d5711cc023c1ab00c79817663e Mon Sep 17 00:00:00 2001 From: Nicholas Doglio Date: Wed, 15 Feb 2023 12:46:26 -0500 Subject: [PATCH 2/5] Update compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt Co-authored-by: Zac Sweers --- .../main/java/slack/lint/compose/ViewModelInjectionDetector.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt b/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt index 6e169601..97d92816 100644 --- a/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt +++ b/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt @@ -27,7 +27,7 @@ constructor(private val allowedNames: StringSetLintOption = StringSetLintOption( "allowed-viewmodel-injection", "A comma-separated list of viewModel factories.", null, - "This property should define comma-separated list of allowed viewModel factory names." + "This property should define comma-separated list of allowed viewModel factory function names." ) private fun errorMessage(factoryName: String): String = From a86a5d89e35ba452b7e34bd1629ffb94c50426ec Mon Sep 17 00:00:00 2001 From: nicholasdoglio Date: Wed, 15 Feb 2023 12:48:46 -0500 Subject: [PATCH 3/5] Simplify filtering allowed and known ViewModel factory functions --- .../java/slack/lint/compose/ViewModelInjectionDetector.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt b/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt index 97d92816..0d3b8755 100644 --- a/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt +++ b/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt @@ -72,8 +72,8 @@ constructor(private val allowedNames: StringSetLintOption = StringSetLintOption( property .findDirectChildrenByClass() .filter { - KnownViewModelFactories.contains(it.calleeExpression?.text) || - allowedNames.value.contains(it.calleeExpression?.text) + val allFactoryNames = KnownViewModelFactories + allowedNames.value + it.calleeExpression?.text in allFactoryNames } .map { property to it.calleeExpression!!.text } } From bf8422e0b5cfbb439c5d21b8debe19768842a52d Mon Sep 17 00:00:00 2001 From: nicholasdoglio Date: Wed, 15 Feb 2023 16:20:50 -0500 Subject: [PATCH 4/5] Avoid unnecessarily creating a Set on each property checked --- .../java/slack/lint/compose/ViewModelInjectionDetector.kt | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt b/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt index 0d3b8755..2c43f1c5 100644 --- a/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt +++ b/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt @@ -65,16 +65,14 @@ constructor(private val allowedNames: StringSetLintOption = StringSetLintOption( if (function.isOverride || function.definedInInterface) return val bodyBlock = function.bodyBlockExpression ?: return + val allFactoryNames = KnownViewModelFactories + allowedNames.value bodyBlock .findChildrenByClass() .flatMap { property -> property .findDirectChildrenByClass() - .filter { - val allFactoryNames = KnownViewModelFactories + allowedNames.value - it.calleeExpression?.text in allFactoryNames - } + .filter { it.calleeExpression?.text in allFactoryNames } .map { property to it.calleeExpression!!.text } } .forEach { (property, viewModelFactoryName) -> From d39522accc202e1ecd2dc5c89558607b1560f716 Mon Sep 17 00:00:00 2001 From: nicholasdoglio Date: Wed, 15 Feb 2023 22:42:06 -0500 Subject: [PATCH 5/5] Use more accurate name for user added viewModel factory list --- .../slack/lint/compose/ViewModelInjectionDetector.kt | 12 ++++++------ .../lint/compose/ViewModelInjectionDetectorTest.kt | 3 ++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt b/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt index 2c43f1c5..254bc7d8 100644 --- a/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt +++ b/compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt @@ -17,14 +17,14 @@ import slack.lint.compose.util.sourceImplementation class ViewModelInjectionDetector @JvmOverloads -constructor(private val allowedNames: StringSetLintOption = StringSetLintOption(ALLOW_LIST)) : - ComposableFunctionDetector(allowedNames), SourceCodeScanner { +constructor(private val userFactories: StringSetLintOption = StringSetLintOption(USER_FACTORIES)) : + ComposableFunctionDetector(userFactories), SourceCodeScanner { companion object { - internal val ALLOW_LIST = + internal val USER_FACTORIES = StringOption( - "allowed-viewmodel-injection", + "viewmodel-factories", "A comma-separated list of viewModel factories.", null, "This property should define comma-separated list of allowed viewModel factory function names." @@ -48,7 +48,7 @@ constructor(private val allowedNames: StringSetLintOption = StringSetLintOption( severity = Severity.ERROR, implementation = sourceImplementation() ) - .setOptions(listOf(ALLOW_LIST)) + .setOptions(listOf(USER_FACTORIES)) private val KnownViewModelFactories by lazy { setOf( @@ -65,7 +65,7 @@ constructor(private val allowedNames: StringSetLintOption = StringSetLintOption( if (function.isOverride || function.definedInInterface) return val bodyBlock = function.bodyBlockExpression ?: return - val allFactoryNames = KnownViewModelFactories + allowedNames.value + val allFactoryNames = KnownViewModelFactories + userFactories.value bodyBlock .findChildrenByClass() diff --git a/compose-lint-checks/src/test/java/slack/lint/compose/ViewModelInjectionDetectorTest.kt b/compose-lint-checks/src/test/java/slack/lint/compose/ViewModelInjectionDetectorTest.kt index 6b0fbc49..10fccc38 100644 --- a/compose-lint-checks/src/test/java/slack/lint/compose/ViewModelInjectionDetectorTest.kt +++ b/compose-lint-checks/src/test/java/slack/lint/compose/ViewModelInjectionDetectorTest.kt @@ -37,7 +37,8 @@ class ViewModelInjectionDetectorTest(private val viewModel: String) : BaseSlackL override val skipTestModes: Array = arrayOf(TestMode.SUPPRESSIBLE, TestMode.TYPE_ALIAS) override fun lint(): TestLintTask { - return super.lint().configureOption(ViewModelInjectionDetector.ALLOW_LIST, "tangleViewModel") + return super.lint() + .configureOption(ViewModelInjectionDetector.USER_FACTORIES, "tangleViewModel") } @Test