Enable binary compatibility validator for detekt-test and detekt-test-api

[chao2zhang]

Per discussion in #4117, detekt-test and detekt-test-api are commonly used for custom extensions like custom rules.

This PR enables binary compatibility validator so that we are more conscious about the public API changes - It does not necessarily mean we need to ensure binary compatibility but does provide necessary insights.

[codecov]

Codecov Report

Merging #4157 (1c6cc32) into main (f12697d) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #4157   +/-   ##
=========================================
  Coverage     83.44%   83.44%           
  Complexity     3177     3177           
=========================================
  Files           465      465           
  Lines          9104     9104           
  Branches       1775     1775           
=========================================
  Hits           7597     7597           
  Misses          572      572           
  Partials        935      935           

Impacted Files	Coverage Δ
...kotlin/io/gitlab/arturbosch/detekt/api/BaseRule.kt	100.00% <ø> (ø)
...otlin/io/gitlab/arturbosch/detekt/api/MultiRule.kt	100.00% <ø> (ø)
...ain/kotlin/io/gitlab/arturbosch/detekt/api/Rule.kt	65.21% <ø> (ø)
.../kotlin/io/gitlab/arturbosch/detekt/api/RuleSet.kt	70.00% <ø> (ø)
...otlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt	80.95% <ø> (ø)
...io/gitlab/arturbosch/detekt/core/rules/RuleSets.kt	100.00% <ø> (ø)
...arturbosch/detekt/core/rules/SingleRuleProvider.kt	100.00% <ø> (ø)
.../arturbosch/detekt/core/suppressors/Suppressors.kt	100.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 f12697d...1c6cc32. Read the comment docs.

[3flex]

All functions in RuleExtensions are extensions on BaseRule which is in api.internal package. So we have a public test API for a class that's supposed to be used internally only (at least, that's what the package names currently suggest).

Perhaps BaseRule should be moved out of api.internal into api?

[chao2zhang]

Yeah this makes sense to me - Extension methods like BaseRule.compileAndLintWithContext defined in RuleExtensions are also used in https://github.com/BraisGabin/detekt-junit-rules/blob/main/src/test/kotlin/com/braisgabin/detekt/junit/TestFunctionsShouldReturnUnitTest.kt

[BraisGabin]

BaseRule is internal because no one should extend from it or use it directly. If someone provides to the core a BaseRule directly that is not a Rule nor MultiRule undefined behaviours could happen. So I'm against this change. I don't think that we should move BaseRule to api.

If we don't want to expose this in our public api we should duplicate all these extensions for Rule and MultiRule.

[cortinico]

BaseRule is internal because no one should extend from it or use it directly. If someone provides to the core a BaseRule directly that is not a Rule nor MultiRule undefined behaviours could happen. So I'm against this change. I don't think that we should move BaseRule to api.

I tend to disagree with this. As Rules were modelled with inheritance, and we expect users to extend from a specific class, the whole inheritance chain has to be part of the public API. We should have instead modelled this using composition or designed it differently (say MultiRule should have been a subclass of Rule which would have been the top of the hierarchy).

I think that BaseRule is effectively part of the public API and should be moved out of the .internal. At the same time I understand that the .internal package is the only warning we have to prevent users from subclassing from it directly. Perhaps BaseRule and Rule should just be refactored and merged?

[BraisGabin]

Perhaps BaseRule and Rule should just be refactored and merged?

I think that it's not worth it right now. If we want to expend time refactoring the rule api we should aim for 2.0.

And I agree, BaseRule is part of the public api in a transitive way.

[BraisGabin]

I like the idea of adding this to help us take better decissions. But, in general, I think that it's ok to break binary compatibility for the tests.