Add ElseCaseInsteadOfExhaustiveWhen rule

[G00fY2]

I added a rule to flag when expressions that have a enum or sealed class subject and contain an else case.

We try to avoid this in our code base, since this leads to automatically handled cases when you add a new enum / sealed class type to existing ones.

I am very open to suggestions regarding the naming and/or description of this rule. 😊

I also started a discussion about this here: #4619

[BraisGabin]

I have a question: Should this rule work for Boolean and Boolean?? Probably not because the values of Boolean can't be extended so it is safe to use else. But, what do you think?

[BraisGabin]

I'm not inpired right now to provide better descriptions and messages but I think that they should be reformulated following the style guide: https://github.com/detekt/detekt/blob/main/.github/CONTRIBUTING.md#rule-descriptions

[BraisGabin]

Suggested change

	* fun whenOnEnumFail(c: Color) {
	* when(c) {
	* Color.BLUE -> {}
	* Color.GREEN -> {}
	* else -> {}
	* }
	* }
	* when(c) {
	* Color.BLUE -> {}
	* Color.GREEN -> {}
	* else -> {}
	* }

What is it better? To keep the code "compilable" or more "focused" on the issue.

[G00fY2]

Yeah, I think it improves readability and everyone should get the idea behind this sample. I changed it to your suggested version.

[G00fY2]

@BraisGabin I forced pushed my initial commit again, to make sure I use GitHubs verified commits. I guess you have to approve the pr workflow again.

I have a question: Should this rule work for Boolean and Boolean?? Probably not because the values of Boolean can't be extended so it is safe to use else. But, what do you think?

I think this would make sense. Because changig the following code would be covered by the else case (which could lead to unintended behavior).

-fun whenOnBooleanFail(b: Boolean) {
+fun whenOnBooleanFail(b: Boolean?) {
    when (b) {
        true -> {}
        else -> {}
    }
}

I added the check for boolean types in b93f631.

But maybe there is a second opinion. We could also make the behavior about Boolean configurable.

[codecov]

Codecov Report

Merging #4632 (3e42301) into main (8bb4c85) will increase coverage by 0.02%.
The diff coverage is 89.65%.

@@             Coverage Diff              @@
##               main    #4632      +/-   ##
============================================
+ Coverage     84.52%   84.55%   +0.02%     
- Complexity     3358     3397      +39     
============================================
  Files           481      484       +3     
  Lines         11193    11270      +77     
  Branches       2042     2059      +17     
============================================
+ Hits           9461     9529      +68     
  Misses          698      698              
- Partials       1034     1043       +9     

Impacted Files	Coverage Δ
...etekt/rules/documentation/OutdatedDocumentation.kt	88.75% <ø> (ø)
...lin/io/github/detekt/tooling/api/AnalysisResult.kt	83.33% <0.00%> (-16.67%)	⬇️
...tekt/rules/bugs/ElseCaseInsteadOfExhaustiveWhen.kt	92.59% <92.59%> (ø)
...turbosch/detekt/rules/bugs/PotentialBugProvider.kt	100.00% <100.00%> (ø)
...n/kotlin/io/github/detekt/report/html/HtmlUtils.kt	98.33% <0.00%> (ø)
...b/arturbosch/detekt/rules/style/ForbiddenImport.kt	95.45% <0.00%> (ø)
...etekt/formatting/wrappers/ParameterListWrapping.kt	100.00% <0.00%> (ø)
...osch/detekt/rules/coroutines/CoroutinesProvider.kt	100.00% <0.00%> (ø)
...ekt/rules/style/RedundantVisibilityModifierRule.kt	98.24% <0.00%> (ø)
...pers/UnnecessaryParenthesesBeforeTrailingLambda.kt	100.00% <0.00%> (ø)
... and 4 more

---

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 8bb4c85...3e42301. Read the comment docs.

[BraisGabin]

If Boolean is included (even with a configuration) I would vote to rename the rule to ElseCaseInsteadOfExhaustiveWhen... Or something similar. Because we raise this only when the when can be exhaustive.

[G00fY2]

If Boolean is included (even with a configuration) I would vote to rename the rule to ElseCaseInsteadOfExhaustiveWhen... Or something similar. Because we raise this only when the when can be exhaustive.

I agree. This name fits better and makes it more clear for consumers what this rule does. Changed it.

[BraisGabin]

The descriptions are perfect now! Thank you so much for this rule :)

[BraisGabin]

I think that this case is a good place to add a @Suppress("ElseCaseInsteadOfExhaustiveWhen").

This rule is good in general (for example it have a lot of sense the change in AnalysisResult) but I think that in this case the else have sense.

[G00fY2]

I am quite happy with current state of this PR. Only thing that bothers me is the behaviour when used in an Kotlin multiplattform project because of this:

when expressions on expect sealed classes in the common code of multiplatform projects still require an else branch. This happens because subclasses of actual platform implementations aren't known in the common code.

Is there any way for us to detect these cases, or is detekt currently even properly supporting Kotlin multiplatform?

[BraisGabin]

We are working to support Kotlin multiplatform. But, right now, there are some missing pieces. We can merge this and then open an issue to check what can be done in terms of Kotlin Multiplatform.

[cortinico]

Is there any way for us to detect these cases, or is detekt currently even properly supporting Kotlin multiplatform?

As this is known in the PSI regardless of your target platform/project, you can do so in the following way:

val hasExpectModifier = TypeUtils.getClassDescriptor(subjectType!!)?.isExpect

Please update the PR with tests for this case as well 👍 Other than that looks good to me :)

[G00fY2]

Please update the PR with tests for this case as well 👍 Other than that looks good to me :)

Updated the PR. Thanks for your feedback everyone.

[G00fY2]

Hey @cortinico could you add this rule to the "new rules" section in the 1.20.0 changelog? So there are currently actually 16 new rules in this new minor release. 😄