Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise Result type for Kotlin 1.5, remove restrictions #244

Merged
merged 4 commits into from
May 9, 2021
Merged

Conversation

elizarov
Copy link
Contributor

@elizarov elizarov commented Mar 19, 2021

  • Kotlin 1.5
    • Allow returning the Result type from functions.
    • Allow Kotlin null-safety operators ?., ?: and !! on both nullable and non-null Result types.
    • Text updated to replace inline class with @JvmInline value class.

@elizarov elizarov requested a review from ilya-g March 19, 2021 09:11
proposals/stdlib/result.md Outdated Show resolved Hide resolved
proposals/stdlib/result.md Outdated Show resolved Hide resolved
Copy link
Member

@ilya-g ilya-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to @udalov suggestions.

elizarov and others added 2 commits March 19, 2021 16:22
Co-authored-by: Alexander Udalov <[email protected]>
Co-authored-by: Alexander Udalov <[email protected]>
@arhont375
Copy link

arhont375 commented Mar 19, 2021

Just read this KEEP as updated in this PR and there still places where:

  1. Said that However, writing functions that return Result is not allowed in Kotlin in the Appendix seciton.
  2. sealed inline class used in the future improvements section, should it be sealed value class?

I'm not sure if it's intended or not, but such inconsistency confused me during read

@elizarov
Copy link
Contributor Author

@arhont375

  1. Said that However, writing functions that return Result is not allowed in Kotlin in the Appendix seciton.
  2. sealed inline class used in the future improvements section, should it be sealed value class?

Good catch. Thanks. Fixed.

@LouisCAD
Copy link
Contributor

runCatching would catch any Throwable, including CancellationException, which becomes a problem when your code rely on them to be thrown to be cancellable.

To alleviate that significant problem, I'm suggesting to make the lambda of all the runCatching functions crossinline, so one cannot run suspending, and likely cancellable code into a construct that would eat what powers the cancellation mechanism.

That safety measure would limit usage of runCatching, but with the benefit of making it impossible to misuse, ans encouraging people to define their own, domain specific error types, until possible new facilities come into the language or libraries to make it less effort to implement.

@ilmirus
Copy link
Member

ilmirus commented Mar 22, 2021

@LouisCAD fixing runCatching does not fix other problems with CancellationException. To fix the issue, we should make structured concurrency a language feature instead of library one. I.e. treat cancellation as we treat suspension. We do not have SuspendedException, after all.

@LouisCAD
Copy link
Contributor

Sure @ilmirus , but in the meantime, it'd avoid a lot of bugs, and that crossinline can always be dropped later without affecting anyone.

Is structured concurrency as a language feature something we can expect in the not too distant future?

@gmk57
Copy link

gmk57 commented May 6, 2021

runCatching would catch any Throwable, including CancellationException, which becomes a problem when your code rely on them to be thrown to be cancellable.

To alleviate that significant problem, I'm suggesting to make the lambda of all the runCatching functions crossinline, so one cannot run suspending, and likely cancellable code into a construct that would eat what powers the cancellation mechanism.

I've hit this too, but current behavior makes sense if you regard runCatching just as a fancy try/catch.

Personally, I often use runCatching as a top-level wrapper around a whole bunch of suspendable work. There is no heavy lifting after it, so non-propagating cancellation is not a real issue. Making lambda crossinline would break this use case.

Now that Result is non-restricted we can freely make our own variants of myRunCatching, e.g. with
if (e is CancellationException) throw e

And in some (rare) cases it is preferrable to treat CancellationException exactly like any other exception, e.g. in some Repository:
.onFailure { dataLoading = false }

@elizarov elizarov merged commit 497c861 into master May 9, 2021
@elizarov elizarov deleted the result-1-5 branch May 9, 2021 11:05
@camhashemi
Copy link

camhashemi commented Feb 20, 2022

is there a discussion or proposal we can link to for why these restrictions were lifted?

I've just been catching up on the original keep and discussion, but I don't see anything around this restriction being removed. So there's a gap for me between "Result is only allowed for restricted use cases" and "Result is only encouraged for restricted use cases".

@elizarov
Copy link
Contributor Author

@camhashemi is there a discussion or proposal we can link to for why these restrictions were lifted?

The original keep mentioned some potential future enhancements to support null-related operators like ?. and ?: to have a special effect for result types. We've tried to design this approach in more detail and did not find any feasible way forward. With this potential enhancement off the table, there was no need to restrict usages of the result type anymore.

sschuberth added a commit to oss-review-toolkit/ort that referenced this pull request Dec 6, 2022
This is not needed anymore as of Kotlin 1.5, see [1].

[1]: Kotlin/KEEP#244

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit to oss-review-toolkit/ort that referenced this pull request Dec 7, 2022
This is not needed anymore as of Kotlin 1.5, see [1].

[1]: Kotlin/KEEP#244

Signed-off-by: Sebastian Schuberth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants