Added rule UnnecessaryInnerClass

[severn-everett]

This addresses issue #875.

[codecov]

Codecov Report

Merging #4394 (1ce0a2d) into main (21a751a) will increase coverage by 0.05%.
The diff coverage is 92.10%.

@@             Coverage Diff              @@
##               main    #4394      +/-   ##
============================================
+ Coverage     84.34%   84.39%   +0.05%     
- Complexity     3299     3324      +25     
============================================
  Files           473      474       +1     
  Lines         10532    10605      +73     
  Branches       1885     1899      +14     
============================================
+ Hits           8883     8950      +67     
- Misses          671      672       +1     
- Partials        978      983       +5     

Impacted Files	Coverage Δ
...rbosch/detekt/rules/style/UnnecessaryInnerClass.kt	91.89% <91.89%> (ø)
...in/io/github/detekt/metrics/CognitiveComplexity.kt	88.23% <100.00%> (-0.28%)	⬇️
...rturbosch/detekt/rules/style/StyleGuideProvider.kt	100.00% <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 21a751a...1ce0a2d. Read the comment docs.

[schalkms]

Thanks for tackling this very old issue! I think this rule fits nicely into detekt.
As mentioned in the issue description, there's a high risk for false positives though. Hence, this rule should be tested thoroughly on sample projects. Have you run this rule on other codebases? If not, the following script in detekt might help to achieve that.

https://github.com/detekt/detekt/blob/main/scripts/get_analysis_projects.kts

[schalkms]

The Assertions. reference can be imported and thus removed from the assert statements for a cleaner test structure.
Other rules show examples on how to use it.

[severn-everett]

Thanks for tackling this very old issue! I think this rule fits nicely into detekt. As mentioned in the issue description, there's a high risk for false positives though. Hence, this rule should be tested thoroughly on sample projects. Have you run this rule on other codebases? If not, the following script in detekt might help to achieve that.

https://github.com/detekt/detekt/blob/main/scripts/get_analysis_projects.kts

The rule requires the binding context to be present, which would mean configuring the classpath option when running Detekt on each of the sample projects. Is there a way to determine the classpaths for these projects programatically? Otherwise, as they are both non-trivial and externally-managed, a manually-maintained list of classpaths for each project would be rather fragile. Given that this is not going to be an active rule by default - and thus won't be breaking workflows automatically when users upgrade Detekt - I would say to publish the rule in the next release and address the issues as they come.

[schalkms]

I'm still hesitant to merge this, since I feel like the rule could take a few more test runs. Testing it in production is not the route, which I'd like to go here.

[severn-everett]

Why not? Releasing experimental and beta code is not an unusual workflow, e.g. the pattern matching preview code that's been released in JDK 16 and 17; and given that the rule will be disabled by default, no user's going to have their code broken by any issues with this rule unless they actively go out of their way to enable it. It'll be a lot more straightforward to be able to test the new rule when it can be integrated via the Gradle plug-in; otherwise, it's going to be a very arduous task to hand-build the classpath for all of these projects to test out the rule manually.

[chao2zhang]

Alternatively, we could enable this new rule for the detekt project itself to help us evaluate. I agree with @severn-everett that since this is a new rule disabled by default, we can roll it out to production and gather feedback.

A counterpart is UnusedPrivateMember, this is a rule that seems to require constant maintenance. I don't think UnnecessaryInnerClass would require as much load as UnusedPrivateMember.

[cortinico]

Can we replace it now? #4277

[severn-everett]

Yeah, I'll swing back and fix this tomorrow.

[severn-everett]

It doesn't look like it's available until the API version is at 1.6; I'll update the note to reflect that