Algebraic data types are not exhaustive

http://try.kotlinlang.org/#/UserProjects/817cu2gf3lhui8kslloffunpdu/vds59e7n0dkhcoqspp9ge6j0ql

1 Like

If when expression result is not used, Kotlin doesn’t check it for exhaustiveness (I think, Idea emits a warning, but not an error). You could use the following code to force a compilation error:

    val unused = when (p) {
        is Parent.First -> println("first")
    }

Actually I don’t understand the reasoning for this behaviour. IMO, when (obj) must always be exhaustive.

1 Like

Thanks.

IMO, when (obj) must always be exhaustive

+1

This really is a Kotlin pitfall!

There are plenty of cases where you need to handle just a few options out of all the possibilities and ignore the rest. We decided against requiring you to write something like else -> /* do nothing */ in such statements.

We’ve discussed the possibility of adding explicit syntax for “exhaustive when” but didn’t see any option that we liked.

3 Likes

I think that current situation is good enough. There’s a warning for those who think that when should be exhaustive (warning or error, it doesn’t matter that much in practice) and it’s possible to disable a warning for those, who prefer not to add else -> {}, so everyone can tweak this setting to his preference.

This is a missed opportunity IMHO.

There are plenty of cases where you need to handle just a few options out of all the possibilities and ignore the rest. We decided against requiring you to write something like else → /* do nothing */ in such statements.

Exhaustive matches make code safer, refactoring easier and speed up code reviews. Implicitly non-exhaustive matches only save one line of code. A line that I’d argue actually has value as it signals to other developers that the author has consciously chosen not to handle every possibility.

Having worked with languages on both sides of the divide my opinion is that exhaustive matches are better in 100% of use cases.

All that leads to is reflexive adding of the else clause. You can lead a horse to water but you can’t make him think.

A warning is appropriate, an error is not.

I respectfully disagree, particularly because you’re only addressing the least valuable of the three benefits I mentioned:

  • Makes refactoring safer and simpler by making the compiler work for you.
  • Makes code reviewing when blocks simpler by eliminating the need to verify that all possible branches are handled in the cases where matching is exhaustive and no else block exists. (should be the majority)
  • Makes developer intent more clear as far as whether or not a block is meant to be exhaustive.

It’s usage becomes a automatic only in an dysfunctional dev environment that does not enforce best practices in code reviews, just like converting a nullable to a non nullable using !! would. In this case the best practice would be “Don’t use an else clause without a very good reason.”

As an example:

when(animal) {
  Dog -> alpha()
  Duck -> beta()
}

You cant tell by looking at just this code whether it could or should handle more Animal types. If you’re doing a code review you then have to go find the definition of Animal to see if any other Animals exist, and then from there you can get on with determining whether or not the actual logic is correct.

Granted, the following is functionally equivalent:

when(animal) {
  Dog -> alpha()
  Duck -> beta()
  else -> /* do nothing*/
}

The difference is that in an exhaustive environment, it’s a red flag to verify that else is justified, just like using !! operator should, since as a best practice, it’s usage would be an exception, not the rule.

2 Likes

You’re ignoring or misunderstanding what I’m saying. The benefits you claim aren’t accrued becuase when a programmer is forced to write this, the else and non-else forms become equivalent. Nothing can be inferred from the else since most programmers are adding it becuase they know they have to avoid an error. Thus my comment about making the horse think. You can’t force the programmer to think about the statement and they’re more likely just to reflexively add the else. Just like those “Are you sure?” dialogs don’t stop people deleting files they want to keep.

I agree that providing more information about intent is good, but forcing people to write code is not the answer, that’s why I think a warning is appropriate. I think it’s just the right amount of nagging and generally programmers who care about warnings care enough to make sure all cases are arefully considered.

You’re ignoring or misunderstanding what I’m saying.

I don’t think so. You’ve said that because it would be possible to get around exhaustive matching by including an else clause that lazy programmers will always do so and is therefore not worthwhile, which is silly. By that reasoning we shouldn’t bother distinguishing between nullable and non nullable or var vs val because lazy programmers can (and usually do) force unwrap nullables using !! and use var where val would be more idiomatic.

The benefits you claim aren’t accrued becuase when a programmer is forced to write this, the else and non-else forms become equivalent.

Using else isn’t compulsoryand as I've said the best practice would be to provide an exhaustive set of matches to eliminate the else whenever possible: usingelse` is the exception not the rule. Just like it is with using guard let to safely use nullables or any of a hundred other non mandatory best practices, the team that leverages language features will benefit while teams that don’t, won’t. It’s not about making a language construct that forces people to be good developers but to give the good developers tools that make them even better.

However we should find a better alternative to

when {
...
}.let { }

like

when (b) {
    Boolean.TRUE -> wow()
    Boolean.FALSE -> damn()
    !else
}
2 Likes

What about

when (b, exaustive = true) { 
  Boolean.TRUE  -> wow() 
  Boolean.FALSE -> damn() 
}

or even

exaustiveWhen (b) { 
  Boolean.TRUE  -> wow() 
  Boolean.FALSE -> damn() 
}
2 Likes

I found on some blog a really nice solution for the non-exhaustive when statement.

val Any.exhaust : Unit
  get() = Unit

fun test() {
when {
  true -> println()
}.exhaust
}

So if you make it a rule in your project to use this for all side-effect when statements, the problem goes away.

1 Like

would really love to see this as a language feature - so i opened a KEEP issue for it to maybe push it a bit: unreachable() · Issue #204 · Kotlin/KEEP · GitHub