From 256c1e9512259927ba340d19464be28ff7229741 Mon Sep 17 00:00:00 2001 From: Nacho Lopez Date: Thu, 16 Mar 2023 11:13:18 +0100 Subject: [PATCH] Add visibility-threshold to ComposeModifierMissing rule --- .../lint/compose/ModifierMissingDetector.kt | 30 ++++- .../compose/ModifierMissingDetectorTest.kt | 111 +++++++++++++++++- docs/rules.md | 14 +++ 3 files changed, 150 insertions(+), 5 deletions(-) diff --git a/compose-lint-checks/src/main/java/slack/lint/compose/ModifierMissingDetector.kt b/compose-lint-checks/src/main/java/slack/lint/compose/ModifierMissingDetector.kt index 5cc67cdd..f9ab8f6d 100644 --- a/compose-lint-checks/src/main/java/slack/lint/compose/ModifierMissingDetector.kt +++ b/compose-lint-checks/src/main/java/slack/lint/compose/ModifierMissingDetector.kt @@ -8,12 +8,14 @@ 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 com.android.tools.lint.detector.api.TextFormat import org.jetbrains.kotlin.psi.KtFunction import org.jetbrains.kotlin.psi.psiUtil.isPublic import slack.lint.compose.util.Priorities import slack.lint.compose.util.definedInInterface import slack.lint.compose.util.emitsContent +import slack.lint.compose.util.isInternal import slack.lint.compose.util.isOverride import slack.lint.compose.util.isPreview import slack.lint.compose.util.modifierParameter @@ -30,6 +32,14 @@ constructor( companion object { val CONTENT_EMITTER_OPTION = ContentEmitterLintOption.newOption() + internal val VISIBILITY_THRESHOLD = + StringOption( + name = "visibility-threshold", + description = "Visibility threshold to check for Modifiers", + defaultValue = "only_public", + explanation = + "You can control the visibility of which composables to check for Modifiers. Possible values are: `only_public` (default), `public_and_internal` and `all`" + ) val ISSUE = Issue.create( @@ -45,18 +55,16 @@ constructor( severity = Severity.ERROR, implementation = sourceImplementation() ) - .setOptions(listOf(CONTENT_EMITTER_OPTION)) + .setOptions(listOf(CONTENT_EMITTER_OPTION, VISIBILITY_THRESHOLD)) } override fun visitComposable(context: JavaContext, function: KtFunction) { // We want to find all composable functions that: - // - are public // - emit content // - are not overridden or part of an interface // - are not a @Preview composable if ( - !function.isPublic || - function.returnsValue || + function.returnsValue || function.isOverride || function.definedInInterface || function.isPreview @@ -64,6 +72,20 @@ constructor( return } + // We want to check now the visibility to see whether it's allowed by the configuration + // Possible values: + // - only_public: will check for modifiers only on public composables + // - public_and_internal: will check for public and internal composables + // - all: will check all composables (public, internal, protected, private + val shouldCheck = + when (VISIBILITY_THRESHOLD.getValue(context.configuration)) { + "only_public" -> function.isPublic + "public_and_internal" -> function.isPublic || function.isInternal + "all" -> true + else -> function.isPublic + } + if (!shouldCheck) return + // If there is a modifier param, we bail if (function.modifierParameter != null) return diff --git a/compose-lint-checks/src/test/java/slack/lint/compose/ModifierMissingDetectorTest.kt b/compose-lint-checks/src/test/java/slack/lint/compose/ModifierMissingDetectorTest.kt index a9213cac..769d0f5e 100644 --- a/compose-lint-checks/src/test/java/slack/lint/compose/ModifierMissingDetectorTest.kt +++ b/compose-lint-checks/src/test/java/slack/lint/compose/ModifierMissingDetectorTest.kt @@ -114,7 +114,7 @@ class ModifierMissingDetectorTest : BaseSlackLintTest() { } @Test - fun `non-public visibility Composables are ignored`() { + fun `non-public visibility Composables are ignored (by default)`() { @Language("kotlin") val code = """ @@ -145,6 +145,115 @@ class ModifierMissingDetectorTest : BaseSlackLintTest() { lint().files(kotlin(code)).allowCompilationErrors().run().expectClean() } + @Test + fun `public and internal visibility Composables are checked for 'public_and_internal' configuration`() { + @Language("kotlin") + val code = + """ + @Composable + fun Something() { + Row { + } + } + @Composable + protected fun Something() { + Column(modifier = Modifier.fillMaxSize()) { + } + } + @Composable + internal fun Something() { + SomethingElse { + Box(modifier = Modifier.fillMaxSize()) { + } + } + } + @Composable + private fun Something() { + Whatever(modifier = Modifier.fillMaxSize()) { + } + } + """ + .trimIndent() + lint() + .files(kotlin(code)) + .allowCompilationErrors() + .configureOption(ModifierMissingDetector.VISIBILITY_THRESHOLD, "public_and_internal") + .run() + .expect( + """ + src/test.kt:2: Error: This @Composable function emits content but doesn't have a modifier parameter. + See https://slackhq.github.io/compose-lints/rules/#when-should-i-expose-modifier-parameters for more information. [ComposeModifierMissing] + fun Something() { + ~~~~~~~~~ + src/test.kt:12: Error: This @Composable function emits content but doesn't have a modifier parameter. + See https://slackhq.github.io/compose-lints/rules/#when-should-i-expose-modifier-parameters for more information. [ComposeModifierMissing] + internal fun Something() { + ~~~~~~~~~ + 2 errors, 0 warnings + """ + .trimIndent() + ) + } + + @Test + fun `all Composables are checked for 'all' configuration`() { + @Language("kotlin") + val code = + """ + @Composable + fun Something() { + Row { + } + } + @Composable + protected fun Something() { + Column(modifier = Modifier.fillMaxSize()) { + } + } + @Composable + internal fun Something() { + SomethingElse { + Box(modifier = Modifier.fillMaxSize()) { + } + } + } + @Composable + private fun Something() { + Whatever(modifier = Modifier.fillMaxSize()) { + } + } + """ + .trimIndent() + + lint() + .files(kotlin(code)) + .allowCompilationErrors() + .configureOption(ModifierMissingDetector.VISIBILITY_THRESHOLD, "all") + .run() + .expect( + """ + src/test.kt:2: Error: This @Composable function emits content but doesn't have a modifier parameter. + See https://slackhq.github.io/compose-lints/rules/#when-should-i-expose-modifier-parameters for more information. [ComposeModifierMissing] + fun Something() { + ~~~~~~~~~ + src/test.kt:7: Error: This @Composable function emits content but doesn't have a modifier parameter. + See https://slackhq.github.io/compose-lints/rules/#when-should-i-expose-modifier-parameters for more information. [ComposeModifierMissing] + protected fun Something() { + ~~~~~~~~~ + src/test.kt:12: Error: This @Composable function emits content but doesn't have a modifier parameter. + See https://slackhq.github.io/compose-lints/rules/#when-should-i-expose-modifier-parameters for more information. [ComposeModifierMissing] + internal fun Something() { + ~~~~~~~~~ + src/test.kt:19: Error: This @Composable function emits content but doesn't have a modifier parameter. + See https://slackhq.github.io/compose-lints/rules/#when-should-i-expose-modifier-parameters for more information. [ComposeModifierMissing] + private fun Something() { + ~~~~~~~~~ + 4 errors, 0 warnings + """ + .trimIndent() + ) + } + @Test fun `interface Composables are ignored`() { @Language("kotlin") diff --git a/docs/rules.md b/docs/rules.md index 54ce7ea1..19980f5f 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -246,6 +246,20 @@ More info: [Always provide a Modifier parameter](https://chris.banes.dev/posts/a Related rule: [`ComposeModifierMissing`](https://github.com/slackhq/compose-lints/blob/main/compose-lint-checks/src/main/java/slack/lint/compose/ModifierMissingDetector.kt) +!!! note "Configuration" +By default, this rule will only check for modifiers in public methods. However, you can configure the threshold via using `visibility-threshold` option in `lint.xml`. + + ```xml + + + ``` + +Possible values are: +* `only_public`: (default) Will check for missing modifiers only for public composables. +* `public_and_internal`: Will check for missing modifiers in both public and internal composables. +* `all`: Will check for missing modifiers in all composables. + ### Don't re-use modifiers Modifiers which are passed in are designed so that they should be used by a single layout node in the composable function. If the provided modifier is used by multiple composables at different levels, unwanted behaviour can happen.