Add ElseCaseInsteadOfExhaustiveWhen rule #4632

G00fY2 · 2022-03-14T10:12:42Z

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 · 2022-03-14T17:50:41Z

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 · 2022-03-14T17:55:03Z

...orprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/ElseCaseInEnumOrSealedWhen.kt

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


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.

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

G00fY2 · 2022-03-14T20:15:51Z

@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 · 2022-03-14T23:34:22Z

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 · 2022-03-15T08:14:29Z

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 · 2022-03-15T17:20:02Z

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 · 2022-03-19T12:49:46Z

...ion/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/OutdatedDocumentation.kt

-            else -> emptyList()
+            KDocKnownTag.AUTHOR, KDocKnownTag.THROWS, KDocKnownTag.EXCEPTION, KDocKnownTag.RECEIVER,
+            KDocKnownTag.RETURN, KDocKnownTag.SEE, KDocKnownTag.SINCE, KDocKnownTag.CONSTRUCTOR, KDocKnownTag.SAMPLE,
+            KDocKnownTag.SUPPRESS -> emptyList()


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 · 2022-03-20T13:42:02Z

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 · 2022-03-20T16:22:45Z

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 · 2022-03-23T01:06:54Z

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 · 2022-03-23T07:34:22Z

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 · 2022-04-02T16:21:22Z

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. 😄

* Add ElseCaseInEnumOrSealedWhen rule * Adapt to new rule by replacing else cases * Change rule name, support booleans, improve docs * Change variable name of boolean condition * Rename rule to ElseCaseInsteadOfExhaustiveWhen * Improve rule descriptions * Suppress rule for outdated documentation method * Not report sealed class subjects that have an expect modifier * Fix sample code syntax and not compile mulitplatform code

julioromano · 2024-06-14T14:30:06Z

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

Kind of a late answer but just wanted to point out that it looks like in Kotlin lingo (see: https://kotlinlang.org/docs/whatsnew16.html#stable-exhaustive-when-statements-for-enum-sealed-and-boolean-subjects ) an "exhaustive when" is defined as:

An exhaustive when statement contains branches for all possible types or values of its subject, or for some types plus an else branch. It covers all possible cases, making your code safer.

So perhaps to avoid confusion this rule should have been named ElseInEnumOrSealedWhen ?

BraisGabin reviewed Mar 14, 2022

View reviewed changes

Add ElseCaseInEnumOrSealedWhen rule

c14306c

G00fY2 force-pushed the else_in_enum_or_sealed_when branch from 01ea611 to 922d2c5 Compare March 14, 2022 18:51

Adapt to new rule by replacing else cases

09a04ef

G00fY2 force-pushed the else_in_enum_or_sealed_when branch from 922d2c5 to 09a04ef Compare March 14, 2022 19:15

Change rule name, support booleans, improve docs

b93f631

G00fY2 changed the title ~~Add ElseCaseInEnumOrSealedWhen rule~~ Add ElseCaseInLimitedWhen rule Mar 14, 2022

Change variable name of boolean condition

5023869

Rename rule to ElseCaseInsteadOfExhaustiveWhen

0ab40c6

G00fY2 changed the title ~~Add ElseCaseInLimitedWhen rule~~ Add ElseCaseInsteadOfExhaustiveWhen rule Mar 15, 2022

Improve rule descriptions

f9ee78d

BraisGabin approved these changes Mar 19, 2022

View reviewed changes

Suppress rule for outdated documentation method

14a7bbf

BraisGabin approved these changes Mar 20, 2022

View reviewed changes

BraisGabin added this to the 1.20.0 milestone Mar 20, 2022

Not report sealed class subjects that have an expect modifier

b4a756e

cortinico approved these changes Mar 23, 2022

View reviewed changes

cortinico mentioned this pull request Mar 23, 2022

Random failure in treeCopyHandler when running Detekt 1.20.0-RC1 Gradle tasks #4629

Closed

Fix sample code syntax and not compile mulitplatform code

3e42301

BraisGabin merged commit 93d1f43 into detekt:main Mar 23, 2022

cortinico mentioned this pull request Apr 2, 2022

Update changelog to mention ElseCaseInsteadOfExhaustiveWhen in the list of added rules #4667

Merged

cortinico added rules notable changes Marker for notable changes in the changelog labels Apr 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ElseCaseInsteadOfExhaustiveWhen rule #4632

Add ElseCaseInsteadOfExhaustiveWhen rule #4632

G00fY2 commented Mar 14, 2022 •

edited

Loading

BraisGabin commented Mar 14, 2022

BraisGabin left a comment

BraisGabin Mar 14, 2022

G00fY2 Mar 14, 2022

G00fY2 commented Mar 14, 2022 •

edited

Loading

codecov bot commented Mar 14, 2022 •

edited

Loading

BraisGabin commented Mar 15, 2022

G00fY2 commented Mar 15, 2022

BraisGabin left a comment •

edited

Loading

BraisGabin Mar 19, 2022

G00fY2 commented Mar 20, 2022 •

edited

Loading

BraisGabin commented Mar 20, 2022

cortinico commented Mar 23, 2022

G00fY2 commented Mar 23, 2022 •

edited

Loading

G00fY2 commented Apr 2, 2022

julioromano commented Jun 14, 2024

Add ElseCaseInsteadOfExhaustiveWhen rule #4632

Add ElseCaseInsteadOfExhaustiveWhen rule #4632

Conversation

G00fY2 commented Mar 14, 2022 • edited Loading

BraisGabin commented Mar 14, 2022

BraisGabin left a comment

Choose a reason for hiding this comment

BraisGabin Mar 14, 2022

Choose a reason for hiding this comment

G00fY2 Mar 14, 2022

Choose a reason for hiding this comment

G00fY2 commented Mar 14, 2022 • edited Loading

codecov bot commented Mar 14, 2022 • edited Loading

Codecov Report

BraisGabin commented Mar 15, 2022

G00fY2 commented Mar 15, 2022

BraisGabin left a comment • edited Loading

Choose a reason for hiding this comment

BraisGabin Mar 19, 2022

Choose a reason for hiding this comment

G00fY2 commented Mar 20, 2022 • edited Loading

BraisGabin commented Mar 20, 2022

cortinico commented Mar 23, 2022

G00fY2 commented Mar 23, 2022 • edited Loading

G00fY2 commented Apr 2, 2022

julioromano commented Jun 14, 2024

G00fY2 commented Mar 14, 2022 •

edited

Loading

G00fY2 commented Mar 14, 2022 •

edited

Loading

codecov bot commented Mar 14, 2022 •

edited

Loading

BraisGabin left a comment •

edited

Loading

G00fY2 commented Mar 20, 2022 •

edited

Loading

G00fY2 commented Mar 23, 2022 •

edited

Loading