mrmans0n / compose-rules

Lint rules for ktlint/detekt aimed to contribute to a healthier usage of Compose. Actively maintained and evolved fork of the Twitter Compose rules.
https://mrmans0n.github.io/compose-rules
Other
542 stars 20 forks source link

Ktlint compose rules not working in ktlint-intellij-plugin #176

Closed paul-dingemans closed 7 months ago

paul-dingemans commented 7 months ago

In https://github.com/nbadal/ktlint-intellij-plugin/issues/425 it was reported that the compose rules are not working in the new ktlint-intellij-plugin while they do work in Ktlint CLI.

Post.kt:

import androidx.compose.material3.Text
import androidx.compose.runtime.Composable

@Composable
fun Post() {
    Text("foo")
}

.editorconfig:

root = true
[*.{kt,kts}]
ij_kotlin_imports_layout = *,java.**,javax.**,kotlin.**,^
ij_kotlin_allow_trailing_comma = false
ij_kotlin_allow_trailing_comma_on_call_site = false
ktlint_function_naming_ignore_when_annotated_with=Composable

Output with ktlint CLI 1.1.0:

/opt/homebrew/bin/ktlint -F -R <path>/scripts/ktlint-compose-0.3.8-all.jar .../Post.kt
<path>/Post.kt:11:5: This @Composable function emits content but doesn't have a modifier parameter.

See https://mrmans0n.github.io/compose-rules/rules/#when-should-i-expose-modifier-parameters for more information. (compose:modifier-missing-check)

Summary error count (descending) by rule:
  compose:modifier-missing-check: 1

Process finished with exit code 1

The lint violation is however not reported by the ktlint-intellij-plugin.

After investigation, I have found that the usages of KtStubElementTypes in KtlintRule is the most likely cause of the problem.

        when (node.elementType) {
            KtStubElementTypes.FILE -> {
                psi.attach(config)
                visitFile(psi as KtFile, autoCorrect, emit.toEmitter())
            }

            KtStubElementTypes.CLASS -> visitClass(psi as KtClass, autoCorrect, emit.toEmitter())
            KtStubElementTypes.FUNCTION -> {
                val function = psi as KtFunction
                val emitter = emit.toEmitter()
                visitFunction(function, autoCorrect, emitter)
                if (function.isComposable) {
                    visitComposable(function, autoCorrect, emitter)
                }
            }
        }

Here the node.elementType is checked against KtStubElementTypes instead of using ElementType. This might also be used in other classes.

To proof that above assumption is the cause of the problem, I have build a new simple rule in a new custom ruleset in ktlint:

class NoFooFun :
    Rule(
        ruleId = RuleId("$CUSTOM_RULE_SET_ID:no-foo-fun"),
        about =
            About(
                maintainer = "Your name",
                repositoryUrl = "https://github.com/your/project/",
                issueTrackerUrl = "https://github.com/your/project/issues",
            ),
    ) {
    override fun beforeVisitChildNodes(
        node: ASTNode,
        autoCorrect: Boolean,
        emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
    ) {
        // Compose ruleset uses KtStubElementTypes in KtlintRule class to match element types. This works fine when rules are used with
        // ktlint CLI as the embeddable kotlin compiler is used.
        if (node.elementType == KtStubElementTypes.FUNCTION) {
            node
                .findChildByType(ElementType.IDENTIFIER)
                ?.takeIf { it.text == "foo" }
                ?.let {
                    emit(node.startOffset, "Unexpected fun foo (via KtStubElementTypes.FUNCTION)", false)
                }
        }

        // However, when the ruleset is used in combination with the ktlint-intelli-plugin, the KtStubElementTypes are no longer recognized.
        // Most likely this is caused by interoperability problems between the embedded kotlin compiler in the KtLintRuleEngine versus the
        // Kotlin compiler in IntelliJ IDEA.
        if (node.elementType == ElementType.FUN) {
            node
                .findChildByType(ElementType.IDENTIFIER)
                ?.takeIf { it.text == "foo" }
                ?.let {
                    emit(node.startOffset, "Unexpected fun foo (via elementType.FUN)", false)
                }
        }
    }
}

Given code sample below:

fun foo() {
    // do something
}

and ktlint CLI 1.1.0 this produces:

$ ktlint-1.1.0 --relative -R ~/Downloads/ktlint-custom-ruleset-example-1.1.0.jar
15:21:12.813 [main] INFO com.pinterest.ktlint.cli.internal.KtlintCommandLine -- Enable default patterns [**/*.kt, **/*.kts]
src/main/kotlin/Foo.kt:1:1: Unexpected fun foo (via KtStubElementTypes.FUNCTION) (custom-rule-set-id:no-foo-fun)
src/main/kotlin/Foo.kt:1:1: Unexpected fun foo (via elementType.FUN) (custom-rule-set-id:no-foo-fun)

Summary error count (descending) by rule:
  custom-rule-set-id:no-foo-fun: 2

The problem is reported twice, as was to be expected. The embeddable kotlin compiler in Ktlint CLI can match the element type of the node both to 'ElementType.FUNandKtStubElementTypes.FUNCTION`.

When using the same ruleset jar in ktlint-intellij-plugin, following problems are reported: Screenshot 2023-12-29 at 15 25 06 As you can see, the problem is now only reported as it matches with ElementType.FUN. I am not 100% sure about the reason, but I believe that the KtStubElementTypes are not compatible with the IDEA kotlin compiler.

Can you provide a snapshot version of the compose ruleset that uses ElementType instead of KtStubElementTypes so that we can verify whether this resolves the problem?

mrmans0n commented 7 months ago

Thanks for the thorough writeup!

I am going to update the ktlint version in this library 1.1.0 if I can, just in case it helps.

FWIW I am currently AFK for a few days, can't do much coding with just the phone. If this can't wait, feel free to submit the changes, and I can approve + the snapshot will be generated automatically + the unit tests would run. If it ends up not being that, I can always easily revert from the phone 🥲

paul-dingemans commented 7 months ago

There is no harm in waiting. It is just an inconvenience I would say. Enjoy your time off.

mrmans0n commented 7 months ago

I think I'm going to try to use the type of the PsiElement for the when instead (I'm doing a cast immediately after in all code paths so it should be fine), that should be a harmless change I hope (and might fix this?).

mrmans0n commented 7 months ago

Snapshot should be already up with this patch in, if KtNodeStubElementTypes were the problem they shouldn't be anymore. https://github.com/mrmans0n/compose-rules/actions/runs/7450793445/job/20270545604

If the ktlint IJ plugin requires the fat jars, you'll need to download the current master and compile it manually, something like: ./gradlew clean :rules:ktlint:shadowJar -PuberJar

paul-dingemans commented 7 months ago

Coool, will have a look soon. Can you point to the locations of the published snapshot?

mrmans0n commented 7 months ago

https://oss.sonatype.org/service/local/repositories/snapshots/content/io/nlopez/compose/rules/ktlint/maven-metadata.xml this is in the base directory, it should be the 0.3.9-SNAPSHOT one.

paul-dingemans commented 7 months ago

I have tested the fat-jar of 0.3.9.-SNAPSHOT in ktlint-intellij-plugin.

If .editorconfig contains setting below:

ktlint_function_naming_ignore_when_annotated_with=Composable

then the Post function is not being flagged as having an improper name as it is annotated with @Composable. Function Text is still flagged as it is not annotated. Screenshot 2024-01-09 at 11 33 21

Without the configuration setting above, both functions are flagged: Screenshot 2024-01-09 at 11 33 05

As far as I am concerned, this issue can be closed, provided that it will be released ;-)

chrimaeon commented 7 months ago

But there is still the issue, that the compose ktlint rule for the modifier does not show up.

in the screenshot above, Post is a public composable that should show an lint issue that a modifier should be provided as mentioned in the start of the thread.

the function naming issue was actually solved with the ktlint-inellij-plugin update

paul-dingemans commented 7 months ago

in the screenshot above, Post is a public composable that should show an lint issue that a modifier should be provided as mentioned in the start of the thread.

This screenshot is a kind of faked and may not present what is happening with a real code example. So, I have not used the actual Composable imports but just a custom annotation class. Same for the functions Post and Text. Please validate with an actual example of a Composable.

chrimaeon commented 7 months ago

I validated in my original code and it's not working.

chrimaeon commented 7 months ago

so this

import androidx.compose.material3.Text
import androidx.compose.runtime.Composable

@Composable
fun Post() {
    Text("foo")
}

does not show

Post.kt:11:5: This @Composable function emits content but doesn't have a modifier parameter.

See https://mrmans0n.github.io/compose-rules/rules/#when-should-i-expose-modifier-parameters for more information. (compose:modifier-missing-check)

Summary error count (descending) by rule:
  compose:modifier-missing-check: 1

Process finished with exit code 1

with ktlint-compose 0.3.9 and intellij-ktlint-plugin 0.20.0

mrmans0n commented 7 months ago

Feel free to open an issue, and if you can provide a sample project it would be useful 😁

But being transparent here, given that it seems that detekt cli, detekt gradle, detekt intellij, and ktlint cli and ktlint gradle pick the rule it up correctly, I guess the issue might lie elsewhere. I hope it can be solved somehow 🤞