Suppressed rules should not execute autoCorrect

[TWiStErRob]

Environment

Detekt: 1.14.0 and 1.15.0 (can't upgrade further)
Gradle: 6.7 and 6.9.1
OS: Windows 10

Expected Behavior

Suppressed rules should not execute autoCorrect.

Current Behavior

Suppressed rules execute autoCorrect.

Context

I wrote a custom rule ("CompanyLogging") that changes logging calls that are for the wrong class. For example:

private val LOG = logger<D>()
class C { }

where D is wrong, it should be a logger for C, so the autoCorrect changes it to

private val LOG = logger<C>()
class C { }

Now, what happens if I want to keep it D, because it makes sense? For example

@Suppress("CompanyLogging") // Using the central class of said library.
private val LOG = logger<RandomImageLoaderInAndroid>()
class RandomImageLoaderDependencyInjectionModule { }

The problem is that, while suppressed, the code is changed to

@Suppress("CompanyLogging") // Using the central class of said library.
private val LOG = logger<RandomImageLoaderDependencyInjectionModule>()
class RandomImageLoaderDependencyInjectionModule { }

so every time someone would run autoCorrect they would have to revert this change.

This prevents automated autoCorrect as well, because it would be committed every time a PR branch is autoCorrected.

[BraisGabin]

The detekt code related with auto correction is really minimum. It's basically this:

fun withAutoCorrect(block: () -> Unit) {
    if (autoCorrect) {
        block()
    }
}

That function can know if the issue that it is fixing is suppressed or not. Even more, that function doesn't know if an issue exist or not. It just execure some code if autoCorrect is enabled. We could create a "more complex" extension function that foce you to provide a Finding. That way that function could use isSuppressedBy to know if the autoCorrection should be done or not.

Do you have the time to do it? do you want to give it a try?

[TWiStErRob]

No, sorry, I don't have time for this now.

Does this mean that it's up to the author of the autoCorrect code to decide (as they have all the info available either by domain knowledge or by functions available):

• fix always (regardless of autoCorrect flag)
• withAutoCorrect
• withAutoCorrect + check isSuppressedBy

As a "quick fix" we could add the existence of this isSuppressedBy to the docs?

[BraisGabin]

Yes, right now is the rule author who should decide this.

As a "quick fix" we could add the existence of this isSuppressedBy to the docs?

Sure, please go ahead! The documentation about auto correctable rules is really minimum right now.

Note: I don't recommend you to ignore the autoCorrect flag because it have some implication in the core. By default detekt runs all the autocorrectable rules first so the other checks are runned directly over the fixed code. If you don't honor it you could end up with issues pointing missleading lines.

[BraisGabin]

Thinking a bit more about this. Do we want to implement this? I think that it shouldn't be implemented on the core of detekt. It should be the rule author taking this decission.

So, the only action we could do on this issue is to decide if we want to support it as authors of formatting.

[chao2zhang]

This is probably worth consideration when we evolve detekt-compiler-plugin (And eventually move to IR). All auto-correction is limited to ktlint rules as of now.

[TWiStErRob]

What do you mean by it's limited to ktlint only? And also it's public API so any third party too, right?