Result 2.0: unsafe unwrapping of Result.value and Result.error

[kirillzh]

I noticed that the new inline value class in API 2.0 lets us access result.value directly without checking if it’s actually safe, which wasn’t the case before. Here’s what I mean:

val result: Result<Int, SomeError> = getResult()
result.value // This compiles even if the result is an error, leading to potential runtime errors.

This could lead to runtime exceptions that were harder to make before when the API required type checking and more explicit unwrapping using getOrThrow() (which is a lot easier to catch or denylist through code analysis).

It seems like API 2.0 introduces a downgrade in compile-time safety compared to the previous version. Could we consider reinstating some form of safety, perhaps through Kotlin contracts or another mechanism, to prevent potential runtime errors?

[michaelbull]

Could we consider reinstating some form of this safety, perhaps through Kotlin contracts or another mechanism, to prevent potential runtime errors?

Do you have a proposal of which compiler contract would achieve this? We effectively need user-defined guards, such that you can't call .value unless you've done an isOk/isErr check - I don't think such a thing exists in Kotlin?

[raamcosta]

If value is always available, then maybe it should be nullable. This way we can check if it is null, and get smart cast.

It would also mean that people are more likely to use better approaches such as doing .onSuccess { value -> } given that inside the lambda, "value" wouldn't be null.

But I may be missing something?

[michaelbull]

It being nullable would force it to be boxed at all call sites. See the following example from Kotlin's docs:

interface I

@JvmInline
value class Foo(val i: Int) : I

fun asNullable(i: Foo?) {}

fun main() {
    val f = Foo(42)

    asNullable(f)  // boxed: used as Foo?, which is different from Foo
}

https://kotlinlang.org/docs/inline-classes.html#representation

[raamcosta]

For us at work, in the type of apps I develop, I’d rather have safer compile time access than the optimisations. So this means we’ll keep using version 1 then.

I guess it would be interesting to know how much users of the library fall into the same camp as us.

Also, I’m not sure if version 1 would need to have some support going forward? With updates to Kotlin versions and stuff like that? Probably not new features though.

[kirillzh]

For us at work, in the type of apps I develop, I’d rather have safer compile time access than the optimisations. So this means we’ll keep using version 1 then.

I guess it would be interesting to know how much users of the library fall into the same camp as us.

Also, I’m not sure if version 1 would need to have some support going forward? With updates to Kotlin versions and stuff like that? Probably not new features though.

Sharing the sentiment here. We didn't get a chance to prioritize this but we will likely either downgrade to version 1, or fork/implement our own Result type.

I'm a bit disappointed to see this issue being closed without resolution, considering that this API change is a quite major regression (loosing compile time safety), in my opinion.

[michaelbull]

@raamcosta

For us at work, in the type of apps I develop, I’d rather have safer compile time access than the optimisations. So this means we’ll keep using version 1 then.

I don't think it's necessarily any different. In v1 you have .unwrap() which unsafely gets you the value, in v2 you have .value which unsafely gets you the value. In neither case does the compiler help you, it's just the difference of a function call vs a property accessor.

Also, I’m not sure if version 1 would need to have some support going forward? With updates to Kotlin versions and stuff like that? Probably not new features though.

I am not maintaining two versions going forward.

[michaelbull]

@kirillzh

Sharing the sentiment here.

As mentioned above, it's really no different. You always had access to unsafely getting the underlying value (by calling .unwrap()), you just don't do it with a function call now.

We didn't get a chance to prioritize this but we will likely either downgrade to version 1, or fork/implement our own Result type.

I'm a bit disappointed to see this issue being closed without resolution, considering that this API change is a quite major regression (loosing compile time safety), in my opinion.

The issue has been open for over half a year and no active discussion is happening.

As it stands you have still not replied to my message engaging with you, so to come back and express disappointment is rather rude. If this is a priority to solve I would appreciate at minimum an actual reply to messages I post in the thread.

A reply to the questions I've asked would be better than coming back after 6 months to express your disappointment with my project management.

I have asked and engaged on this subject and the conversation came to a complete halt with no solutions or improvements being proposed. If you have something that would change that, then a PR is welcome.

[kirillzh]

As mentioned above, it's really no different. You always had access to unsafely getting the underlying value (by calling .unwrap()), you just don't do it with a function call now.

@michaelbull I should have clarified - in our codebase we disallowed usage of unwrap() (a common practice in our Rust counterpart too), and instead we relied on compile time safe type checking, before:

val result: Result<Int> = ...
// result.value is not possible to call until type is checked - good!:
when (result) {
  is Ok -> result.value
  is Err -> ...

Now, I can just call result.value without checking for "type" and risk it throwing an exception at runtime.

[michaelbull]

I should have clarified - in our codebase we disallow usage of unwrap()

How did you disallow it? Can you disallow calling value the same way?

You really want the value/error accessible only in functions that have Result as a receiver, as they are likely to be the library functions that require it, which would stop accidental unwrapping in business logic.

If you can shed some light as to how you managed to arbitrarily stop people calling a function from the library in your codebase maybe a similar approach could be adopted.

[raamcosta]

I do not agree it’s the same though. Semantically it’s very different. With unwrap, it’s clear that it is a separate way to get the value without checking it first, whereas .value is the way to get the successful value both before and after checking.

We didn’t even need to disallow it, no one ever used it and most likely no one ever will. I can almost guarantee it that people will use .value though.

(Sorry for not using proper formatting and probably also not explaining it in the best way.. just doing some quick response reaction on the smartphone 😅)

[raamcosta]

Another way of putting it: what you’re saying is almost like saying that there is no point in nullable types because people can !!. It’s maybe not quite, as double bang is even more clear that you’re taking the risk, but it goes in the same direction.

[michaelbull]

what you’re saying is almost like saying that there is no point in nullable types because people can !!

No, I am not saying that.

Fundamentally, what I am saying is that you can type .unwrap() or .value and both have the exact same semantics -> they both give you access to the underlying type without checking, throwing an exception if it's not the presumed type.

If I was to write this project from scratch again, I probably wouldn't have even had the unwrap function to begin with, as accessing the value property is identical in nature.

[raamcosta]

We can agree to disagree. Like I said, people have never tried to use unwrap and they will surely use value without checking.

As a library author myself, I believe creating a meaning behind the APIs you expose and an expectation around them is very important. You can present both safe and unsafe APIs, and use names that create that expectation automatically, without even needing to open documentation.

[michaelbull]

The expectation that you described still remains. If I was writing my own business code I would use unwrap to clearly tell a reader that I am unsafely unwrapping the result, and I would only use value in library extension methods. Of course these are just stylistic choices for now and not enforced, but definitely preferred for the same reasons you outlined.

What I am keen to understand is if there's a way to enforce it, whether utilising compiler contracts or something else. It sounded earlier like it was possible to enforce it at their workplace, but I am waiting to hear how.

[kirillzh]

How did you disallow it?

We simply followed the practice to never call unwrap() (well except for in tests). We could implement a linter easily but we just didn't get to that point.

Can you disallow calling value the same way?
Before value could be called on type casted Ok value. Now, value can be called in different contexts: before and after a result type check - so it doesn't really make sense to disallow .value usage entirely. We would need to implement a much more sophisticated lint check.

[michaelbull]

We simply followed the practice to never call unwrap()

So in both v1 and v2 you identified a way that you can interact with the API in an unsafe way and agreed to not write code that uses it unsafely.

I was under the impression that there was actually something stopping you from making the same agreement for v2.

As you identified, a linting rule is the best way to solve this. Something that can analyse whether you've done an isOk check before calling .value, and similarly an isErr before calling .error.

That linting rule would be the functional equivalent of writing a rule that prevents calling .unwrap() altogether in v1.

Before value could be called on type casted Ok value. Now, value can be called in different contexts: before and after a result type check

This is the exact case same situation as the the unwrap() function: it could be called both before and after a result type check.

The only difference is that it's a function call and not a property. It's functionally equivalent. There is no increase or decrease in safety between v1 and v2 in this regard: both of them let you access the underlying value and throw an exception if the Result wasn't Ok.

[kirillzh]

This is the exact case same situation as the the unwrap() function: it could be called both before and after a result type check.

The unwrap() API – well getOrThrow() in this case – has clear naming and behavior, no issues with that.

There is no increase or decrease in safety between v1 and v2 in this regard: both of them let you access the underlying value and throw an exception if the Result wasn't Ok.

Setting getOrThrow() aside, in version 1, you had to check that result was Ok before accessing someResult.value, ensuring safety. In version 2, you can access result.value without any checks, and it's not obvious that this API is throwing at runtime, even though the code compiles.

After upgrading to version 2, we encountered bugs because we accessed result.value without proper checks. In v1, this was preventable by compiler, in v2 we need to write a custom linter for that. Desired API for us would prevent us from calling result.value until type is checked - as it was implemented in v1. This is probably the main reason why we want to use something like Result to begin with. I hope this clarifies things.

[michaelbull]

The only idea I can think of so far: moving the value and error fields to be extension properties on the Result that live in an "unsafe" package.

It would look like this:

package com.github.michaelbull.result.unsafe

import com.github.michaelbull.result.Failure
import com.github.michaelbull.result.Result

@Suppress("UNCHECKED_CAST")
public val <V, E> Result<V, E>.value: V
    get() = inlineValue as V

@Suppress("UNCHECKED_CAST")
public val <V, E> Result<V, E>.error: E
    get() = (inlineValue as Failure<E>).error

This would ensure the library, and authors of extensions to the library, can still access them as before, requiring just a new import:

import com.github.michaelbull.result.unsafe.value

public infix fun <V, E> Result<V, E>.getOr(default: V): V {
    return when {
        isOk -> value // this would not compile without the import
        else -> default
    }
}

This would break anyone that's writing their own extension functions as they'd now need to import these unsafe extension properties, as the properties would no longer be there by default in the Result class.

This doesn't help distinguish the difference of simply "accessing the value in a library function" vs "unsafely unwrapping the result in business code" that is causing the friction you have identified, but it would at least require people to physically opt-in (by importing) to access to the underlying value/error fields on a Result.

It still doesn't actually require them to check the type though.

Not sure if is any good whatsoever, but it is the only idea I've had.

[michaelbull]

Another alternative: now that Kotlin 2.1 has landed [1] (albeit in preview) that supports non-local break and continue, the building-block functions like fold and getOrElse should be a lot less limited to the point that they may facilitate the implementation of the rest of the library.

If so, the value and error fields may disappear entirely and the only mechanism for accessing either would be through functions like fold. Will experiment.

[1] https://kotlinlang.org/docs/whatsnew21.html#non-local-break-and-continue

[Shykial]

Hello @michaelbull. First of all, thanks a lot for all your hard work for the library!
It really does make life easier and is greatly appreciated!

Inline classes improvements are great, but at the same time I also find myself missing the compile time safety that the V1 had.
As I understand, the main challenge to achieve it in V2 is that we cannot easily have compiler contracts like "isOk returned true 2 lines before, so value can be smart-casted to V now".
These are great, but personally when choosing between two, I would rather favour compile time safety and not have them.

If so, the value and error fields may disappear entirely and the only mechanism for accessing either would be through functions like fold. Will experiment.

That was my idea as well. With that however, would mechanisms introduced in Kotlin 2.1 be necessary?
If Result.inlineValue was internal, the library could behave similarly to a built-in kotlin.Result: perform isOk/isErr checks and cast inlineValue to V or Failure<E> on demand.
This should retain performance improvements while bringing back V1 compile time safety.
What do you think? I can create draft PR for it in case you find the idea worth giving a try.

[michaelbull]

@Shykial Thanks for your kind words.

Preventing library consumers from accessing the underlying value prevents them from writing their own Result extension methods. For example, if you wanted to implement this method yourself in your own codebase (or wanted to alter it slightly for your project):

public fun <V, E> Result<Result<V, E>, E>.flatten(): Result<V, E> {
    return when {
        isOk -> value
        else -> this.asErr()
    }
}

If you don't have access to value, you can't write it.

This is why I think the only way we can 'restrict' access to the value is if we make the value internal, and rely on everyone using something like fold to access the underlying value. Currently this is not possible as it would break consumer code that relies on calling break/continue inside an isOk check, as you can't call continue/break inside lambas until Kotlin 2.1.

[Shykial]

This is why I think the only way we can 'restrict' access to the value is if we make the value internal, and rely on everyone using something like fold to access the underlying value.

That's essentially what I was thinking when I said "If Result.inlineValue was internal" 😄

Preventing library consumers from accessing the underlying value prevents them from writing their own Result extension methods

I don't think that's true, the value would still be accessible, just not via direct .value call.
The number of exposed APIs by the library is rather big and I think it should be all that's needed for users to come up with their own functions/extensions.
To work on your example of Result<Result<V, E>, E>.flatten(), user could easily replace when statement with getOrElse { }, where getOrElse is function from the library which uses internal inlinedValue under the hood.
At the very worst case, users can still do things like get()!! or getError()!! (which would imply boxing), but I think it would be very rarely needed given library's extensive API.

Currently this is not possible as it would break consumer code that relies on calling break/continue inside an isOk check, as you can't call continue/break inside lambas until Kotlin 2.1.

That's a valid point, didn't think about that use case,

[michaelbull]

The number of exposed APIs by the library is rather big and I think it should be all that's needed for users to come up with their own functions/extensions.

The example I gave was simply to emphasise the point that library consumers need access to the underlying value in order to provide library-extension methods. As you rightly point out in practice, the example I gave you could achieve with the get().

In most of the other library functions, you're just doing: return if (isOk) oneThing else anotherThing - so these could theoretically all be replaced with fold(success = { oneThing }, failure = { anotherThing }), at which point we can prevent users accessing the underlying value and force them to use fold in a type-safe manner to get access to the values, effectively replicating the old behaviour of if (it is Ok) oneThing else anotherThing.

This all falls down if you had code like if (isOk) { something = it.value; break; } else { somethingElse = it.value; continue; }, because as we've discussed you couldn't replace this code with fold as you can't call break or continue inside a lambda until 2.1.

I think when break/continue inside lambdas become fully supported, we should theoretically be able to force all consumers to use fold for everything and hide the underlying value.

If you have any code examples where this wouldn't be the case I am interested to hear, but I think it will solve the problem that people are eluding to in this thread.