Refactor Analyzer so that RuleSetProvider.instance is only called once

[chao2zhang]

Background

I WTF'ed when debugging #3515. Also, my colleague @kkoser WTF'ed when working on a custom detekt rule.

Solution

The problem here is that we are not only calling RuleSetProvider.instance() twice for each rule, but the first time to generate IdMapping is even wrong because the top-level config instead of config.subConfig(it.ruleSetId) is passed to each ruleSet.

To fix this, I have refactored the code to share Sequence<> to make sure RuleSetProvider.instance().
(Technically, RuleSetProvider.instance() is called again in :detekt-output-sarif/RuleDescriptors.kt, but I am going to address this in #3554)

[codecov]

Codecov Report

Merging #3555 (8baecad) into master (b5aac52) will decrease coverage by 0.01%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master    #3555      +/-   ##
============================================
- Coverage     77.53%   77.52%   -0.02%     
  Complexity     2829     2829              
============================================
  Files           464      464              
  Lines          8769     8769              
  Branches       1713     1713              
============================================
- Hits           6799     6798       -1     
- Misses         1046     1047       +1     
  Partials        924      924              

Impacted Files	Coverage Δ	Complexity Δ
...otlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt	80.26% <71.42%> (-0.82%)	9.00 <0.00> (ø)
...io/gitlab/arturbosch/detekt/core/rules/RuleSets.kt	100.00% <100.00%> (ø)	0.00 <0.00> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5aac52...bc0eaf2. Read the comment docs.

[BraisGabin]

The problem here is that we are not only calling RuleSetProvider.instance() twice for each rule, but the first time to generate IdMapping is even wrong because the top-level config instead of config.subConfig(it.ruleSetId) is passed to each ruleSet.

Twice for each rule? We call it once per file and one more time when we instantiate the analyzer, right? And in this last one we don't care about the configuration because we just want to extract the ids. I imagine that this last part is not true for what you want to do with SARIF, right?

All those calls to RuleSetProvider.instance() are gone in the new analyzer. But the fact that you need to instantiate a rule just to get it's id is something that I don't like too much. Luckly the rules are light to instantiate and in the new analyzer they are instantiated just once.

[chao2zhang]

Agreed.

All those calls to RuleSetProvider.instance() are gone in the new analyzer.
Could we push the new analyzer to a separate branch on master and start collaboration?

[chao2zhang]

Twice for each rule? We call it once per file and one more time when we instantiate the analyzer, right? And in this last one we don't care about the configuration because we just want to extract the ids. I imagine that this last part is not true for what you want to do with SARIF, right?

To make sure the idea is communicated properly:

• The first call is wrong https://github.com/detekt/detekt/pull/3555/files#diff-6149700bc8437bfd660774c9282c1051c9de2e5f7ea07711393cc17ca5944edeL38
• The second call is correct https://github.com/detekt/detekt/pull/3555/files#diff-6149700bc8437bfd660774c9282c1051c9de2e5f7ea07711393cc17ca5944edeL109
• The third call is inside SARIF report but that's out of the scope for this PR

[chao2zhang]

@schalkms I am reverting this since this is for 1.17 release. We are probably waiting for Kotlin 1.4.32 so that we can release 1.16.1

[schalkms]

@schalkms sorry again, my bad. Can you please reopen this changeset and tag it with a blocked label. Then we can prevent this things from happening in the future.