-F produces false-positive with experimental rules enabled

[krisgerhard]

Expected Behavior

package com.pinterest.ktlint

import com.pinterest.ktlint.enum.Enum
import com.pinterest.ktlint.enum.EnumThree
import com.pinterest.ktlint.enum.EnumTwo

data class TrailingCommaTest(
    val foo: String,
    val bar: Enum,
    val bar2: EnumTwo,
    val bar3: EnumThree
)

Automatic formatting should add trailing comma and succeed.

Observed Behavior

Trailing comma is added correctly but a false-positive error is thrown.
TrailingCommaTest.kt:4:1: Unused import

Steps to Reproduce

I have created a test case which fails at the moment.
https://github.com/krisgerhard/ktlint/tree/incorrect-experimental-formatting-showcase

Added test file: ExperimentalCLITest https://github.com/krisgerhard/ktlint/blob/incorrect-experimental-formatting-showcase/ktlint/src/test/kotlin/com/pinterest/ktlint/ExperimentalCLITest.kt

[paul-dingemans]

I can not reproduce the problem with current development version of ktlint. Can you please verify that your problem is solved with latest snapshot version?

[krisgerhard]

Interesting 🤔 I can verify it is working as expected on today's snapshot version. Only diff compared to yesterday is PR #1364 .

EDIT: No idea how it succeeded for me the first time I tried... Updated the fork to latest snaphot version and it's still failing.
I enabled workflows in my fork so you can see the test failing here as well https://github.com/krisgerhard/ktlint/actions/runs/1828500070

[paul-dingemans]

Could it be that you expectations are not correct?

The message below will be printed when running without option -F:

TrailingCommaTest.kt:4:1: Unused import

When running with option -F, ktlint only reports errors which could not be fixed. As the unused import was removed successfully, there is no reason for ktlint to print the error. As a result the assert below is incorrect:

assertFalse(output.contains(".*Unused import".toRegex()))

[krisgerhard]

Hmm. You are mislead a bit. Let me try to explain my test.

The code I've provided in "expected behaviour" does not contain unused imports. Neither does the automatically fixed result. Yet, ktlint reports that there is an unused import (as demonstrated in ExperimentalCLITest I've provided).

I found out that no unused imports rule does not parse EnumThree correctly. Under "refs" variable it contains "EnumThree," and thus reporting "EnumThree" as unused import. One possible fix would be to remove commas when cleaning up ref names. But as I am not too familiar with ktlint code I'm hesitant to create an pull request. 😊

[paul-dingemans]

We seem indeed not to be on the same page.

Yet, ktlint reports that there is an unused import (as demonstrated in ExperimentalCLITest I've provided).
I would agree if this was written as:

assertTrue(output.contains(".*Unused import".toRegex()))

Note the usage of assertTrue instead of assertFalse.

I rewrote your test case to a normal unit test as using the CLI test to run multiple rules is not required anymore. It is now possible to run a selected list of rules in a single unit test. As a result it is easier to assert results of linting and formatting.

Compared to your original example, I have added an import for EnumFour which without any doubts is unused. As you can see in the "formattedCode", the trailing comma was added and EnumThree has not been removed. I think this is the correct output to be expected. Agree?

    @Test
    fun `Trailing comma and unused imports do not affect each other`() {
        val code =
            """
            package com.pinterest.ktlint
            import com.pinterest.ktlint.enum.Enum
            import com.pinterest.ktlint.enum.EnumFour
            import com.pinterest.ktlint.enum.EnumThree
            import com.pinterest.ktlint.enum.EnumTwo
            data class TrailingCommaTest(
                val foo: String,
                val bar: Enum,
                val bar2: EnumTwo,
                val bar3: EnumThree
            )
            """.trimIndent()
        val formattedCode =
            """
            package com.pinterest.ktlint
            import com.pinterest.ktlint.enum.Enum
            import com.pinterest.ktlint.enum.EnumThree
            import com.pinterest.ktlint.enum.EnumTwo
            data class TrailingCommaTest(
                val foo: String,
                val bar: Enum,
                val bar2: EnumTwo,
                val bar3: EnumThree,
            )
            """.trimIndent()

        val editorConfigFilePath = writeEditorConfigFile(
            ALLOW_TRAILING_COMMA_ON_CALL_SITE,
            ALLOW_TRAILING_COMMA_ON_DECLARATION_SITE
        ).absolutePath
        val rules = listOf(TrailingCommaRule(), NoUnusedImportsRule())
        assertThat(rules.lint(editorConfigFilePath, code)).containsExactly(
            LintError(3, 1, "no-unused-imports", "Unused import"),
            LintError(10, 24, "trailing-comma", "Missing trailing comma before \")\""),
        )
        assertThat(rules.format(editorConfigFilePath, code)).isEqualTo(formattedCode)
    }

[krisgerhard]

@paul-dingemans

assertTrue(output.contains(".*Unused import".toRegex()))

☝️ this expects code to output "Unused import" error but it should not!
My example does not contain unused imports -> thus assertFalse.
Example once again 👇

package com.pinterest.ktlint

import com.pinterest.ktlint.enum.Enum
import com.pinterest.ktlint.enum.EnumThree
import com.pinterest.ktlint.enum.EnumTwo
data class TrailingCommaTest(
    val foo: String,
    val bar: Enum,
    val bar2: EnumTwo,
    val bar3: EnumThree
)

Your example with EnumFour is correct but you are not checking LintError output of format.
Would you agree that it should not report any errors because the output code is correct?

If we add these lines to your code

val afterFormatLintErrors = ArrayList<LintError>()
val formatResult = rules.format(editorConfigFilePath, code, cb = { e, _ -> afterFormatLintErrors.add(e) })
assertThat(afterFormatLintErrors).isEmpty()
assertThat(formatResult).isEqualTo(formattedCode)

then we can see that ktlint still reports an unused import although it is not there.
PS! This example does not need EnumFour import to expose the bug. It would be easier to understand the issue without it in my opinion.

TLDR;
If ktlint worked correctly https://github.com/krisgerhard/ktlint/commit/8d53f67bb366905d3e140b2966a23efe9f44329c#diff-082d34b31b30dda32bb9ea38ab3379ca983e80e11c632653c4e2259056f70662R92 test would pass. Instead it is reporting an unused import error for a code without any unused imports.

[krisgerhard]

Bonus material showing NoUnusedImportRule incorrectly collecting names of used references. Note the comma after "EnumThree"

[paul-dingemans]

Thanks for you persistence. Espically you last comment, the screen dump and especially the nice red arrow made finally clear to me what you meant. This is indeed very strange behavior and is related which is caused by the combination of those two rules. It might explain some other problems we have with unused imports.

As far as I now can see, the problem is cause at following lines:

                } else if ((type == REFERENCE_EXPRESSION || type == OPERATION_REFERENCE) &&
                    !vnode.isPartOf(IMPORT_DIRECTIVE)
                ) {
                    ref.add(Reference(text.removeBackticksAndWildcards(), psi.parentDotQualifiedExpression() != null))

Due to the trailing comma rule the reference EnumThree for variable bar3 is transformed to REFERENCE_EXPRESSION. So at a certain moment the vnode variable at this source line consisting of two elements:

• An identifier (EnumThree)
• A comma
  Obviously, on the identifier should have been added to the list of references.

So well done for tracking down the bug and helping me to understand it. Fixing it, should not be that hard from this point on.