ktlint 0.48.1 doesn't use experimental rules if .editorconfig is empty

[mateuszkwiecinski]

We're trying to upgrade kotlinter to use the latest version and our tests caught the plugin doesn't respect experimental rules.

Here's the smallest repro I was able to come up with:

    @Test
    fun `ktlint 0-48-1`() {
        val standard = StandardRuleSetProvider()
        val experimental = ExperimentalRuleSetProvider()
        val ruleProviders = listOf(standard, experimental).flatMap { it.getRuleProviders() }.toSet()

        val ktLintEngine = KtLintRuleEngine(
            ruleProviders = ruleProviders,
            editorConfigOverride = EditorConfigOverride.EMPTY_EDITOR_CONFIG_OVERRIDE,
        )
        val errors = mutableListOf<LintError>()
        ktLintEngine.lint(
            code = Code.CodeSnippet(
                """
                val variable = "should not contain'()'".count() { it == 'x' }
    
                """.trimIndent(),
            ),
            callback = errors::add,
        )

        val failedRules = errors.map { it.ruleId }
        check(failedRules.contains("experimental:unnecessary-parentheses-before-trailing-lambda"))
        check(!failedRules.contains("filename"))
    }

Expected Behavior

1. ktlint should respect experimental rules
2. filename rule shouldn't fail for Code.CodeSnippet

Observed Behavior

If the editorconfig is empty, the test fails because ktlint doesn't run experimental checks against the code snippet. If the editorconfig contains any overrides the test passes. It seems like the VisitorProvider#isEnabled doesn't work well if the consumer has empty editorconfig.

Also, please note the check(!failedRules.contains("filename")) check, which I believe is a separate issue, the code currently emits LintError(line=1, col=1, ruleId=filename, detail=File name 'file.kt' should conform PascalCase) which I believe comes from here. Linting a Code.CodeSnipped shouldn't probably result in an filename error.

Steps to Reproduce

☝️
In the repro, while debugging the code here, the repro fails only when ruleExecutionContext.editorConfigProperties is empty here. Otherwise, experimental rulesets work as expected

Your Environment

• Version of ktlint used: 0.48.1
• Relevant parts of the .editorconfig settings - it has to be empty
• Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task): the repro uses raw kotlin dependency
• Version of Gradle used (if applicable): all
• Operating System and version: all

[lwasyl]

I'll join with a related question: how should API consumers configure the rules? The API looks like passing ruleProviders will run all the rules provided, unless some of them were explicitly disabled in the .editorconfig somewhere. But that's not happening and as far as I see, the engine will:

• run all the custom rules unless they're explicitly disabled
• run all the standard rules unless they're explicitly disabled
• not run any of the experimental rules unless they're explicitly enabled

This inconsistency seems confusing from the API usage perspective, IMO. Perhaps it'd make sense to keep the API straightforward, and make it responsibility of the wrappers (like CLI or Kotlinter) to properly filter out experimental rules based on the tool configuration?

[paul-dingemans]

I will come back on this issue next week earliest. I am having holidays and do not have to my laptop.

[paul-dingemans]

If the editorconfig is empty, the test fails because ktlint doesn't run experimental checks against the code snippet. If the editorconfig contains any overrides the test passes. It seems like the VisitorProvider#isEnabled doesn't work well if the consumer has empty editorconfig.

This indeed is not correct. It will be fixed with #1772. It is not strictly related to the editorconfig being empty but whether property ktlint_experimental is not set.

Also, please note the check(!failedRules.contains("filename")) check, which I believe is a separate issue, the code currently emits LintError(line=1, col=1, ruleId=filename, detail=File name 'file.kt' should conform PascalCase) which I believe comes from here. Linting a Code.CodeSnipped shouldn't probably result in a filename error.

Indeed, the file name used for code snippets should not cause a violation by the filename rule. Will be fixed with #1778.

ktlint should respect experimental rules

This is the most difficult part of the issue to answer. I do understand that both of you advocate that all rules provided by the API Consumer have to be respected (e.g. to be run automatically). However, from the viewpoint of Ktlint, the end user (e.g. the developer who uses ktlint, kotlinter, or any other KtLint API consumer) should be in control on enabling or disabling experimental rules. This can best be done by providing the EditorConfigDefault value as shown below:

    @Test
    fun `ktlint 0-48-1`() {
        val standard = StandardRuleSetProvider()
        val experimental = ExperimentalRuleSetProvider()
        val ruleProviders = listOf(standard, experimental).flatMap { it.getRuleProviders() }.toSet()

        val ktLintEngine = KtLintRuleEngine(
            ruleProviders = ruleProviders,
            editorConfigDefaults = EditorConfigDefaults(
                EditorConfig
                    .builder()
                    .section(
                        Section
                            .builder()
                            .glob(Glob("*.kt"))
                            .properties(
                                Property
                                    .builder()
                                    .name("ktlint_experimental")
                                    .value("enabled"),
                            ),
                    )
                    .build()
            )
        )
        val errors = mutableListOf<LintError>()
        ktLintEngine.lint(
            code = Code.CodeSnippet(
                """
                val variable = "should not contain'()'".count() { it == 'x' }

                """.trimIndent(),
            ),
            callback = errors::add,
        )

        val failedRules = errors.map { it.ruleId }
        check(failedRules.contains("experimental:unnecessary-parentheses-before-trailing-lambda"))
        check(!failedRules.contains("filename"))
    }

If the user has set property ktlint_experimental explicitly than that value will be used. If the value is not defined, the value provided via editorConfigDefaults will be used.

If you do want to ignore the value of ktlint_experimental as set by the end user, than you can set the EditorConfigOverride property. But as said before that is discouraged as the end user will not understand why the .editorconfig property is being ignored (provided that the value set is not equal to the value provided by the API Consumer).

This inconsistency seems confusing from the API usage perspective, IMO. Perhaps it'd make sense to keep the API straightforward, and make it responsibility of the wrappers (like CLI or Kotlinter) to properly filter out experimental rules based on the tool configuration?

I have been thinking about this as well but disregarded it in the end. Some projects use multiple .editorconfig files for different parts of the project directory hierarchy or use different setting based on globs in a single .editorconfig file. The API Consumer has to provide all rules that possibly can be executed. Ideally, the end user should have the final say about what is executed.

Above explanation has also been added to the changelog via #1778 for other API Consumers.

[paul-dingemans]

@mateuszkwiecinski @lwasyl Can you please try the latest snapshot of ktlint (https://pinterest.github.io/ktlint/install/snapshot-build/) to check if this helps?

[mateuszkwiecinski]

@paul-dingemans I tested latest snapshot locally, and it seems to behave in an expected way 👍
Also, thanks for the clarification as it helps to understand the context. Especially the last part:

If you do want to ignore the value of ktlint_experimental as set by the user, than you can set the EditorConfigOverride property. But as said before that is discouraged as the user might not understand why the .editorconfig property is being ignored

🙏
I guess we (people involved in the PR) will have a discussion, and either do as you suggest, or since ktlint has now that extensive configuration via .editorconfig we probably could consider dropping additional config 👀

[paul-dingemans]

Tnx for confirmation. I will do some more tests and prepare for the 0.48.2 release.