appkode/detekt-rules-compose

A collection of Detekt rules for Jetpack Compose

detekt-rules-compose's Issues

New rule: `Modifier` Should have default value

Enforce that modifier always have a default value
non compilant:
modifier: Modifier

compilant:
modifier: Modifier = Modifier
modifier: Modifier = Modifier.padding(4.dp)

False positive on `ModifierParameterPosition` and `ComposableParametersOrdering`

@Composable
fun MyComposable(
    modifier: Modifier = Modifier,
    content: LazyListScope.() -> Unit
) = LazyColumn(modifier = modifier, content = content)

ModifierParameterPosition and ComposableParametersOrdering raise an issue. The content parameter should be the last parameter.

`TopLevelComposableFunctions` and `inteface`s

I like the idea of TopLevelComposableFunctions. The composable function shouldn't be declared inside an Activity or anything like that.

But some times I have a Module were I want to display a Component that can't be defined in my module. For example I have a ProductDetail and I want to show Offers. Offers is a different feature so what I do is create an interface like this:

interface OffersProvider {
  @Composable
  fun Offers(id: String, modifier: Modifier = Modifier)
}

Then with the magic of Dagger everything works. The problem is that TopLevelComposableFunctions complains about that @Composable inside an interface.

I see different ways to fix this:

interface OffersProvider {
  fun offers(): @Composable (id: String, modifier: Modifier = Modifier) -> Unit
}

Or don't use OffersProvider and just inject the lambda @Composable (id: String, modifier: Modifier = Modifier) -> Unit.

But I'm not 100% sure if any of those solutions are better than the original one.

What do you think? Should TopLevelComposableFunctions have a flag to allow interfaces? Am I missing other solution?

Sorry if this issue is too philosophical.

New rule: Compose functions should be top-level functions

Non compilant

class A : Activity() {

  @Component
  fun Asdf() {
    // ...
  }
}

Compilant

class A : Activity() {
  // ...
}

@Component
fun Asdf() {
  // ...
}

Context

A composable function shouldn't have access to mutables states that a Activity or Fragment store. For that reason those functions should be declared as top-level functions

[Question] No Compose in the report

My detekt.gradle.kts in buildSrc:

import io.gitlab.arturbosch.detekt.Detekt
import io.gitlab.arturbosch.detekt.DetektCreateBaselineTask
import io.gitlab.arturbosch.detekt.extensions.DetektExtension

plugins {
    id("io.gitlab.arturbosch.detekt")
}

val analysisDir = file(projectDir)
val baselineFile = file("$rootDir/config/detekt/baseline.xml")
val configFile = file("$rootDir/config/detekt/detekt.yml")
//val statisticsConfigFile = file("$rootDir/config/detekt/statistics.yml")

val kotlinFiles = "**/*.kt"
val kotlinScriptFiles = "**/*.kts"
val resourceFiles = "**/resources/**"
val buildFiles = "**/build/**"

detekt {
    toolVersion = Versions.detektVersion
    buildUponDefaultConfig = true
    baseline = baselineFile
    config = files("config/detekt/detekt.yml")
    source = objects.fileCollection().from(
        DetektExtension.DEFAULT_SRC_DIR_JAVA,
        "src/test/java",
        DetektExtension.DEFAULT_SRC_DIR_KOTLIN,
        "src/test/kotlin"
    )
    reports {
        html.enabled = true
        html.destination = file("$projectDir/build/detekt/report.html")
        xml.enabled = true
        xml.destination = file("$projectDir/build/detekt/report.xml")
        txt.enabled = true
        txt.destination = file("$projectDir/build/detekt/report.txt")
    }
}

        const val detektVersion = "1.19.0"
        const val detektTwitterComposeRulesVersion = "0.0.4-SNAPSHOT"
        const val detektKodeComposeRulesVersions = "1.2.1"

dependencies {
    detektPlugins("io.gitlab.arturbosch.detekt:detekt-gradle-plugin:${Versions.detektVersion}")
//    detektPlugins("com.twitter.compose.rules:detekt:${Versions.detektTwitterComposeRulesVersion}")
    detektPlugins("ru.kode:detekt-rules-compose:${Versions.detektKodeComposeRulesVersions}")
}

val detektFormat by tasks.registering(Detekt::class) {
    description = "Formats whole project."
    parallel = true
    disableDefaultRuleSets = true
    buildUponDefaultConfig = true
    autoCorrect = true
    setSource(analysisDir)
//    config.setFrom(listOf(statisticsConfigFile, configFile))
    include(kotlinFiles)
    include(kotlinScriptFiles)
    exclude(resourceFiles)
    exclude(buildFiles)
    baseline.set(baselineFile)
    reports {
        xml.enabled = true
        html.enabled = true
        txt.enabled = true
    }
}

val detektAll by tasks.registering(Detekt::class) {
    description = "Runs the whole project at once."
    parallel = true
    buildUponDefaultConfig = true
    setSource(analysisDir)
//    config.setFrom(listOf(statisticsConfigFile, configFile))
    include(kotlinFiles)
    include(kotlinScriptFiles)
    exclude(resourceFiles)
    exclude(buildFiles)
    baseline.set(baselineFile)
    reports {
        xml.enabled = true
        html.enabled = true
        txt.enabled = true
    }
}

val detektProjectBaseline by tasks.registering(DetektCreateBaselineTask::class) {
    description = "Overrides current baseline."
    buildUponDefaultConfig.set(true)
    ignoreFailures.set(true)
    parallel.set(true)
    setSource(analysisDir)
//    config.setFrom(listOf(statisticsConfigFile, configFile))
    include(kotlinFiles)
    include(kotlinScriptFiles)
    exclude(resourceFiles)
    exclude(buildFiles)
    baseline.set(baselineFile)
}

and in the end of detekt.yml:

...
compose:
  ReusedModifierInstance: #finds usages of modifier parameter on non-top-level children of a composable function. This tends to happen during refactorings and often leads to incorrect rendering of a composable
    active: true
  UnnecessaryEventHandlerParameter: #suggests hoisting event argument passing to the upper level which often simplifies individual composable components
    active: true
  ComposableEventParameterNaming: #ensures that all event handler parameters of composable functions are named in the same Compose-like style, i.e. they have on prefix and do not use past tense
    active: true
  ComposableParametersOrdering: #suggests separating required an optional parameters of the composable function into groups
    active: true
  ModifierHeightWithText: #suggests using Modifier.heightIn() instead of Modifier.height() on a layouts which have Text children, so that if the text turns out to be long and would wrap, layout will not cut it off
    active: true
  ModifierParameterPosition: #ensures that modifier is declared as a first parameter
    active: true
  ModifierDefaultValue: #ensures that modifier parameter has a correct default value
    active: true
  MissingModifierDefaultValue: #checks if modifier default value is specified
    active: true
  PublicComposablePreview: #finds and reports composable previews which are not marked as private
    active: true
  TopLevelComposableFunctions: #ensures that all composable functions are top-level functions (disabled by default)
    active: true

And then this: gradle detektAll.
The result doesn't have anything related to compose!

The project can't be compiled/tested out of the box

When I run ./gradlew I get this error:

* Where:
Build file '/Users/brais/projects/detekt-rules-compose/build.gradle.kts' line: 119

* What went wrong:
Could not get unknown property 'NEXUS_USERNAME' for root project 'detekt-rules-compose' of type org.gradle.api.Project.

If you don't have NEXUS_USERNAME defined the gradle configuration fails.

False positive in ModifierHeightWithText rule

This piece of code

Row(
  modifier = Modifier
    .height(IntrinsicSize.Min),
) {
  Text(
    modifier = Modifier
      .fillMaxHeight(),
    text = "Hello world",
  )
}

results in an error:

Composable uses "height" modifier and contains a Text child. Use heightIn(min = N.dp) instead [ModifierHeightWithText]

which is incorrect as this doesn't make sense for IntrinsicSize overload of height.

Exception is needed for this specific usage.

ComposableEventParameterNaming false positive

Expected Behavior

I have a Composable function that receive an extension of LazyListScope lambda as content, these lambda cannot be @composable because LazyColumn requires that it isn't.

The false positive is that detekt says that the name of the parameter should be like "onClick".

For example, I have a scaffold like:

fun Scaffold(
    topBar: @Composable () -> Unit,
    logo: @Composable BoxScope.() -> Unit,
    content: LazyListScope.(minContentHeight: Dp) -> Unit,
    bottomContent: @Composable ColumnScope.() -> Unit,
    modifier: Modifier = Modifier,
    state: GamificationScaffoldState = rememberGamificationScaffoldState(),
    snackbarHost: @Composable (SnackbarHostState) -> Unit = { SnackbarHost(it) },
) {
    ....
}

Observed Behavior

Detekt says that the content parameter should be named like "onClick" or "onEvent"

Steps to Reproduce

Create a @composable Scaffold with a non composable content, like LazyListScope.() -> Unit

Context

Your Environment

• Version of detekt used: 1.20.0
• Operating System and version: Mac OS 12.4

Improve message on `ComposableEventParameterNaming`

I'm getting legit errors like this:

/the/path/QuantityRow.kt:27:5: Invalid event parameter name. Use names like "onClick", "onValueChange" etc [ComposableEventParameterNaming]

It would be great to know what's the current parameter name from this message. Something like:

Invalid event parameter name `doSomething`. Use names like "onClick", "onValueChange", etc

If you agree I can open a PR changing it.

False positive on ReusedModifierInstance with theme as direct child

Hello!

The following causes ReusedModifierInstance to fire:

@Composable
fun MyComposable(
    modifier: Modifier = Modifier
) {
    MaterialTheme {
        Row(
            modifier = modifier,
        ) { }
    }
}

This seems like a false positive given that the direct child is a theme.

False positive for "ConditionCouldBeLifted": lambda with parameter

This code reports ConditionCouldBeLifted while it shouldn't: technically it's legal to have pager with an "empty" page.

HorizontalPager(...) { pageIndex ->
  val branchEntry = entries.elementAtOrNull(pageIndex)
  if (branchEntry != null) { // <- reported here
    BranchDetailsWithAction(...)
  }
}

False positive on ComposableFunctionName when using single expression functions

I get a false positive on this rule when using the following syntax:

// Part of theme file
@Composable
fun AppTheme(content: @Composable () -> Unit) {
    // Theme setup stuff goes here
}

// False positive is on this function
@Preview
@Composable
private fun PreviewSomeComposable() = AppTheme {
    SomeComposable()
}

Seems like the rule doesn't detect that its still only a Unit return type so wants me to start the function with a lowercase letter.

False positive on `UnnecessaryEventHandlerParameter`

Given this code:

internal sealed class State {
    object Loading : State()
    data class Data(val id: String) : State()
}

@Composable
internal fun MyButton(
    state: State,
    onClick: (String) -> Unit,
) {
    when (state) {
        State.Loading -> Text("Loading")
        is State.Data -> Button(onClick = { onClick(state.id) }) { Text("Click here") }
    }
}

UnnecessaryEventHandlerParameter raises an issue. But in this case the parameter is necessary because state.id is accesible because of the smart cast that you don't have in the function that call this component.

Extend `UnnecessaryEventHandlerParameter` for constants

I think that code like this should be also flagged:

@Composable
private fun Foo(
    onSomething: (Int) -> Unit,
) {
    Button(onClick = { onSomething(5) }) { /*...*/ }
}

When 5 could be any kind of constant value/object/data class with contant values on it... Probably it's impossible to handle ALL the cases but probably the more easy ones.

Context

In my project we use MVI so we start with a lambda like this: onUserIntent: (SealedClassIntent) -> Unit. And we propagate that or something similar. I'm not 100% sure that I like that but it's kind of OK. But what I think it's not Ok is when it ends up to a Component like this:

@Composable
private fun Foo(
    onUserIntent: (SealedClassIntent) -> Unit,
) {
    Button(onClick = { onUserIntent(SealedClassIntent.Back) }) { /*...*/ }
}

I think it should be something like this:

@Composable
private fun Foo(
    onBackClick: () -> Unit,
) {
    Button(onClick = onBackClick) { /*...*/ }
}

And it's the caller job to decide which SealedClassIntent should this Composable emit.

Compose ReusedModifierInstance false positive

Expected Behavior

I have a @composable that receives a modifier, and the first child is ProvideWindowInsets that has no modifier parameter, and I put the received modifier on the first child of ProvideWindowInsets, then Detekt says that the received modifier instance should be passed only to de first child.

Here is an example:

@Composable
fun MyComposable(
    ....
    modifier: Modifier = Modifier,
    .....
    ) {
          ProvideWindowInsets {
               Surface(
                      modifier = modifier,
                      .....
               ) {
                    .....
               }
          }
    }

Observed Behavior

Detekt says that the received modifier instance should be passed only to de first child.

Steps to Reproduce

Create a composable that receive a modifier and its first child has no modifier parameter (ProvideWindowInsets) so pass the modifier to the first child of ProvideWindowInsets.

Context

Your Environment

• Version of detekt used: 1.20.0
• Operating System and version: Mac OS 12.4

Add "UnusedModifierParameter" rule

@ExperimentalAnimationGraphicsApi
@ExperimentalCoilApi
@ExperimentalComposeUiApi
@Composable
fun ProductView(
    product: Product,
    modifier: Modifier = Modifier, //unused
) {
    val ...

    Card(
        modifier = Modifier
            .fillMaxWidth()

Private Composable Previews and UnusedPrivateMember conflict

When I enable PublicComposablePreview and set all my previews to private this causes UnusedPrivateMember to be triggered.
How to avoid that?

ComposableParametersOrdering: Ignore trailing lambda parameters

Currently this rule does not take in consideration the trailing lambda syntax like, for example, in this composable:

@Composable
fun DatePicker(
    show: Boolean,
    selection: Long? = null,
    calendarConstraints: CalendarConstraints? = null,
    title: String? = null,
    @MaterialDatePicker.InputMode inputMode: Int = MaterialDatePicker.INPUT_MODE_CALENDAR,
    fragmentTag: String = "MaterialDatePicker",
    onNegativeButtonClick: (() -> Unit)? = null,
    onCancel: (() -> Unit)? = null,
    onDismiss: (() -> Unit)? = null,
    onPositiveButtonClick: (Long) -> Unit,
) {}

The parameter onPositiveButtonClick not optional but it's the final parameter of the function to be able to pass the lambda outside of the function call parentheses.

False positive for `UnnecessaryEventHandlerParameter`?

Following code flags B composable with UnnecessaryEventHandlerParameter, is that OK?

@Composable
private fun Smth(
    state: State,
    onClick: (String) -> Unit,
) {
    Column {
        A(onClick = onClick)
        B(onClick = { onClick(state.string) } )
    }
}

If I would avoid this lint error, I would need to have 2 callbacks, which I think is maybe less readable?

@Composable
private fun Smth(
    state: State,
    onClickA: (String) -> Unit,
    onClickB: () -> Unit,
) {
    Column {
        A(onClick = onClick)
        B(onClick = onClickB)
    }
}

Version: 1.3.0

False positive on `ReusedModifierInstance` when shadowing modifier

1.3.0 fixed a lot a false positives of ReusedModifierInstance. But there is one that it wasn't fixed yet. I created a test case to make it easy to reproduce it:

@Composable
fun Extra(a: @Composable (Modifier) -> Unit) {}
@Composable
fun Test(
  modifier: Modifier = Modifier, 
) {
  Column(modifier) {
    Extra { modifier ->
      Text("Hi", modifier)
    }
  }
}

The modifier that I'm using on Text is not the same that the one defined on Test.

Update ModifierParameterPosition to skip non-optional parameters

Currently the ModifierParameterPosition rule requires modifier to be placed as a first parameter, but Compose actually has a convention for it to be first optional parameter:

Adam Powell wrote

The official convention is that the modifier param is the first optional parameter in the list. The idea is that you never have to type modifier = in the parameter list, it can always be specified positionally if the only other parameters you're using are required

ModifierHeightWithText should not apply when Modifier.height(Intrinsic.Min)

If you specify Modifier.height(Intrinsic.Min) then this rule should not apply, because you allow the views to be drawn out fully and then wrap the content. This is useful when you want a child to expand to the max size of your minimum sized parent.

New rule: naming

The @Composable functions that returns Unit should start with upper case but the @Composable functions that return any other type should start with lower case.

Outdated wiki?

Wiki seems to be missing some rule documentation

Add ignoreOverridden support for MissingModifierDefaultValue rule

Since An overriding function is not allowed to specify default values ​​for its parameters I would like to be able to disable this rule for overriding functions.

`UnnecessaryEventHandlerParameter` raises an exception

I wanted to Suppress an issue raised by UnnecessaryEventHandlerParameter because it is a false positive (more about this in #13)

But when I add the @Suppress to the parameter like this:

@Composable
internal fun MyButton(
    state: State,
    @Suppress("UnnecessaryEventHandlerParameter") onClick: (String) -> Unit,
) {
    when (state) {
        State.Loading -> Text("Loading")
        is State.Data -> Button(onClick = { onClick(state.id) }) { Text("Click here") }
    }
}

I'm getting this exception:

> Analyzing /Users/brais.gabin/Workspace/lidl/features/selfscanning/src/main/java/es/lidlplus/features/selfscanning/checkout/AA.kt led to an exception.
  Location: ru.kode.detekt.rule.compose.UnnecessaryEventHandlerParameter$UnnecessaryHandlerArgumentsVisitor.reportError(UnnecessaryEventHandlerParameter.kt:114)
  The original exception message was: class org.jetbrains.kotlin.psi.KtDeclarationModifierList cannot be cast to class org.jetbrains.kotlin.psi.KtTypeReference (org.jetbrains.kotlin.psi.KtDeclarationModifierList and org.jetbrains.kotlin.psi.KtTypeReference are in unnamed module of loader java.net.URLClassLoader @54724ee9)
  Running detekt '1.21.0-RC2' on Java '11.0.11+9' on OS 'Mac OS X'

The interesting part of the stack trace is this:

Caused by: java.lang.ClassCastException: class org.jetbrains.kotlin.psi.KtDeclarationModifierList cannot be cast to class org.jetbrains.kotlin.psi.KtTypeReference (org.jetbrains.kotlin.psi.KtDeclarationModifierList and org.jetbrains.kotlin.psi.KtTypeReference are in unnamed module of loader java.net.URLClassLoader @54724ee9)
        at ru.kode.detekt.rule.compose.UnnecessaryEventHandlerParameter$UnnecessaryHandlerArgumentsVisitor.reportError(UnnecessaryEventHandlerParameter.kt:114)
        at ru.kode.detekt.rule.compose.UnnecessaryEventHandlerParameter$UnnecessaryHandlerArgumentsVisitor.visitCallExpression(UnnecessaryEventHandlerParameter.kt:107)

If I set the suppress at the function level all works as expected.

ComposableParametersOrdering should not consider one last function parameter

It is a good practice to pass function as last parameter to use it as lambda:

fun someFunction(
    modifier: Modifier = Modifier,
    onClick: () -> Unit
)

someFunction(
    modifier = Modifier.width(1.dp)
) {
  doSomething()
}

But when last function parameter has no default value ComposableParametersOrdering considers it as error. It is good to allow one mandatory function as last parameter in this rule.

Not detekt on CompositionLocalProvider due to Modifier wrong level

Composable uses "modifier" on the wrong level, non-direct children should use "Modifier"

fun Button(modifier: Modifier, ...) {
    CompositionLocalProvider(LocalRippleTheme provides TextButtonRippleTheme) {
        MaterialTextButton(
            modifier = modifier,
            ...
        )
    }
}

Fat jar

Hello!

It would be nice to have a fat-jar to be used with detekt CLI like in twitter rules repository