Mixing autocorrectable and non correctable rules results in obsolete issue locations for reports

[omainegra]

I found an error in the html report:

• Rule: TooGenericExceptionCaught
• Detetk version: 1.5.1
• Stacktrace:

java.lang.StringIndexOutOfBoundsException: String index out of range: 19
	at java.lang.String.substring(String.java:1963)
	at io.gitlab.arturbosch.detekt.cli.out.HtmlUtilsKt.writeErrorLine(HtmlUtils.kt:51)
	at io.gitlab.arturbosch.detekt.cli.out.HtmlUtilsKt.access$writeErrorLine(HtmlUtils.kt:1)
	at io.gitlab.arturbosch.detekt.cli.out.HtmlUtilsKt$snippetCode$1$1.invoke(HtmlUtils.kt:35)
	at io.gitlab.arturbosch.detekt.cli.out.HtmlUtilsKt$snippetCode$1$1.invoke(HtmlUtils.kt)

• How to reproduce it:

[BraisGabin]

Could you provide a sample code to reproduce this issue?

[omainegra]

🤦‍♂ I fix the code and now is working fine, let me check the history

[omainegra]

I was able to reproduce it with the following snippet

enum class SomeEnum {
    VALUE1, VALUE2, VALUE3, UNKNOWN;

    companion object {
        fun from(string: String): SomeEnum =
            try { valueOf(string) }
            catch (e: Exception) { UNKNOWN }
    }
}

[BraisGabin]

WOW Really strange behaviour. The problem is not related with the Exception but with the code itself. If I request the psiFile.text I get this:

enum class SomeEnum {
    VALUE1, VALUE2, VALUE3, UNKNOWN;

    companion object {
        fun from(string: String): SomeEnum =
            try { valueOf(string) } catch (e: Exception) { UNKNOWN }
    }
}

the catch is moved to the same line as the try {}.

Any idea about why this is happening? It seems an error in PSI.

[schalkms]

@BraisGabin even if the Psi had a problem the HtmlReport shouldn’t crash.

[BraisGabin]

It's not crashing. We fixed that already. But it shows this:

We thought that this erros were related with our rules, but it seems they that they are not. So, any idea?

[schalkms]

It's not crashing. We fixed that already.

Good! This wasn't clear to me just by looking at this issue description.

We thought that this erros were related with our rules, but it seems they that they are not. So, any idea?

That's really strange. In my opinion this is an issue with the PSI itself.
@BraisGabin do you mind reporting this to the compiler team with all the details you found out while investigating this issue?

[BraisGabin]

I'm researching this issue. It's on our side.

The extension function KtFile.analyze inside Detektor.kt changes the KtFile. I don't know how and I don't know why, yet.

[BraisGabin]

Found! SpacingAroundKeyword is changing the KtFile!

[BraisGabin]

Ok! I know what's going on:

• TooGenericExceptionCaught runs and find an issue.
• SpacingAroundKeyword has autoCorrect enabled by default so it edits the KtFile. This happen evenwhen you don't enable the autoCorrect (the KtFile is editted but it's never saved in the file system)
• When the HtmlReport wants to read the KtFile it reads the editted one so the lines doesn't mach with the ones reported by TooGenericExceptionCaught and 💥 here we have the observed behaviour.

How to fix it? I think that we need to do two different tasks:

• If the user doesn't want to actually correct the code we shouldn't set the autoCorrect flag to true. This way ktlint will no edit the KtFile content (and we can even save some time).
• We need to run the correctable rules before the no correctable ones. This way if a rule fixes or changes the position of an issue the report will be correct.

What do you think?

[schalkms]

@BraisGabin your proposal sounds good to me.

[arturbosch]

Autsch, good finding!

Ok! I know what's going on:

* `TooGenericExceptionCaught` runs and find an issue.

* `SpacingAroundKeyword` has `autoCorrect` enabled by default so it edits the `KtFile`. This happen evenwhen you don't enable the autoCorrect (the `KtFile` is editted but it's never saved in the file system)

* When the HtmlReport wants to read the `KtFile` it reads the editted one so the lines doesn't mach with the ones reported by `TooGenericExceptionCaught` and boom here we have the observed behaviour.

How to fix it? I think that we need to do two different tasks:

* If the user doesn't want to actually correct the code we shouldn't set the `autoCorrect` flag to true. This way ktlint will no edit the `KtFile` content (and we can even save some time).

https://github.com/arturbosch/detekt/blob/f98ccac9aa352805621dd6a19888bb472d776cef/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/ConfigAware.kt#L51-L53

If I understand you correctly, we already do this here?
However autoCorrect has three levels. Top config level, which we do not pass to the rule so an autoCorrect can happen, the rule set and rule level which we take care of with above code location.

Changing the default autoCorect from true to false would only be possible in a 2.0 release.

* We need to run the correctable rules before the no correctable ones. This way if a rule fixes or changes the position of an issue the report will be correct.

True that, we could introduce basic rule configuration options like supportsAutoCorrect to support this. Further we could introduce needsBindingContext to get rid of if (BindingContext == Empty) checks etc.
I would also deprecate the RuleSet methods as they expose detekt-core's implementation details.

What do you think?

[BraisGabin]

If I understand you correctly, we already do this here?
However autoCorrect has three levels. Top config level, which we do not pass to the rule so an autoCorrect can happen, the rule set and rule level which we take care of with above code location.
That's the problem, we only check two of the three levels. We need to pass the third level to the rules. I tryed to find a quick fix but it seems that we need a refactor here.

Changing the default autoCorect from true to false would only be possible in a 2.0 release.

I think that we don't need this change to fix this problem.

True that, we could introduce basic rule configuration options like supportsAutoCorrect to support this. Further we could introduce needsBindingContext to get rid of if (BindingContext == Empty) checks etc.

I think tha, we don't need supportsAutoCorrect to fix this (but maybe it could be handy for other use cases). For this issue we just need to sort the rules that has the autoCorrect enabled. The problem that I found with this is that I can't get a list of Rules. I just get a list of RuleSets and those RuleSets doesn't allow me to sort the rules inside them. This need a refactor in the RuleSet to change the api so it would create a BC.

[arturbosch]

Yes, you are right.

We can sort the rules here:
https://github.com/arturbosch/detekt/blob/b6f8ecb894b2af9044e38b884841ab3d08abe3f8/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/RuleSet.kt#L32-L40

What I additionally had in mind but didn't wrote down is that sorting via the autoCorrect property, always triggers this:
https://github.com/arturbosch/detekt/blob/f98ccac9aa352805621dd6a19888bb472d776cef/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/ConfigAware.kt#L33-L34

In fact every valueOrXXX triggers creating a new sub config. After a detekt run we have X millions of Config objects that are not garbage collected until the top most initial Config gets garbage collected. This is due to the fact that Config keeps a reference to the parent when calling Config#subconfig.

I will open a PR to change

private val ruleConfig: Config
        get() = ruleSetConfig.subConfig(ruleId)

to

private val ruleConfig: Config by lazy(NoSynch) { ruleSetConfig.subConfig(ruleId) }

and apply your suggestion to sort by autoCorrect.
Additionally I will open an issue to handle the potential OOM on extremly large projects when the top most Config is garbage collected on the end of the detekt run.

Edit: regarding needsBindingContext. Such a property could help extending the visitCondition so each Rule does not need to check for if (bindingContext != BindingContext.Empty).

[BraisGabin]

We can sort the rules here:

https://github.com/arturbosch/detekt/blob/b6f8ecb894b2af9044e38b884841ab3d08abe3f8/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/RuleSet.kt#L32-L40

We can only sort the rules that are inside a RuleSet. But we can have multiple RuleSets with some rules that are autocorrectables and others that aren't. This is the tricky part.

[BraisGabin]

I think that we should open this issue again or open a new one because we need to do this too:

If the user doesn't want to actually correct the code we shouldn't set the autoCorrect flag to true. This way ktlint will no edit the KtFile content (and we can even save some time).

This is important because, if the user don't enable the auto correct we are going to show incorrect locations for the issues (the html report will work but those snippets are not going to be the same as the user code). Or we can even not detect the issue at all.

[arturbosch]

I think that we should open this issue again or open a new one because we need to do this too:

If the user doesn't want to actually correct the code we shouldn't set the autoCorrect flag to true. This way ktlint will no edit the KtFile content (and we can even save some time).

This is important because, if the user don't enable the auto correct we are going to show incorrect locations for the issues (the html report will work but those snippets are not going to be the same as the user code). Or we can even not detect the issue at all.

This can be fixed by wrapping the original config by a NoAutoCorrectConfig when --auto-correct=false ?

[BraisGabin]

I don't get exactly how do you want to do that. But yes. The point is that if --auto-correct=false we should get false in all the config rules so they don't change the KtFile