Hello, I don't understand why the compiler compla...

# announcements

[Julien Cayzac]

Hello,

I don't understand why the compiler complains here about the missing branch in when. The require clause implies element cannot be a JsonNull :

fun unbox(element: JsonElement): Any {
    require(element !is JsonNull) { "The token contains null values" }
    return when (element) { // error: JsonNull case missing
        is JsonLiteral -> element.body
        is JsonArray -> element.filter { it != JsonNull }.map { unbox(it) }
        is JsonObject -> element.content.filterValues { it != JsonNull }.mapValues { unbox(it.value) }
    }
}

[Mike]

The compiler doesn’t do that much work, and I don’t believe contracts can help with something like this. This would likely have a noticeable performance hit on compilation speed…

I think the easiest is to add branch for JsonNull and throw an IllegalStateException as the branch action.

[Julien Cayzac]

Yes I reverted to JsonNull -> require(false) { "The token contains null values" } but I was curious

[Julien Cayzac]

I think I don't understand how implies really works.

[Mike]

You don’t need require(false) {"message"}. Just use throw IllegalArgumentException("The token contains null values"). It’s the same thing with less indirection.

[Mike]

I’m not real familiar with implies either. Looking at documentation in this specific case, it just says if the function returns (i.e. require), then it guarantees that the passed value is ‘true’

[Julien Cayzac]

It’s the same thing with less indirection.

It's longer to type.

[Mike]

Concision should not be the main reason to choose something. Readability should be.

Imo, your version requires more thought to figure out what it's doing. Mine is obvious.

Writing happens once. Reading happens many times.

Something to think about...

[Julien Cayzac]

Thanks. I think standard contracts enhance readability. It's easier to understand require than if (condition) throw LegacyJDKExceptionClassFrom20YearsAgo() as a failed requirement, especially since the actual exception class doesn't matter since it's not part of our public API and should be let to crash.

[Mike]

We agree that your original is definitely the cleanest/most readable. But unfortunately, contracts don’t support something that deep just yet.

My comment wasn’t to the code in your original post.

My comment was your change to have it work. i.e. removing the require(element !is JsonNull).... line and instead being forced to explicitly state it in the when statement.

So you put your when statement as JsonNull -> require(false) { "The token contains null values"} and I’m suggesting that is harder, or at least unexpected, to read (require(false) just doesn’t feel right to me).

So I’m suggesting it should be explicit, and just throw the exception.
JsonNull -> throw IllegalArgumentException("The token contains null values").

[Mike]

and then hope contracts can/do expand to support your original code.