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

Kotlin should use the builtin Result type #697

Open
Manishearth opened this issue Sep 25, 2024 · 7 comments
Open

Kotlin should use the builtin Result type #697

Manishearth opened this issue Sep 25, 2024 · 7 comments

Comments

@Manishearth
Copy link
Contributor

Manishearth commented Sep 25, 2024

cc @jcrist1 @emarteca

Kotlin has a Result<T> (it's not a Result<T, E>, but it still encapsulates a typed Throwable error)

Is there a reason we can't use it? We should use idiomatic stuff where possible. Is it idiomatic?

https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/-result/

@Manishearth
Copy link
Contributor Author

If the issue is things not implementing Throwable, you can for now hack around it by having an FFIError<E> that implements Throwable, and in the long term we want to have #446

@jcrist1
Copy link
Contributor

jcrist1 commented Sep 26, 2024

There's no reason not to use it other than the difficulty of making it throwable. I don't know how common Result usage is but I think it's nice to offer the user an idiomatic solution that doesn't require throwing. I tried introducing some types that made the return more rusty, but it seems antithetical to diplomat's philosophy.

I'm going on a two week vacation tomorrow with intermittent rail travel so will be mostly away from the computer except during the longish train rides when I'll probably want to look at this to pass the time. @emarteca if you want to handle this faster, let me know, otherwise I'll see how far I can get on the trains.

@Manishearth
Copy link
Contributor Author

Manishearth commented Sep 26, 2024

@jcrist1 do you mean the difficulty of making the error types throwable?

because as I understand it Res doesn't need to be thrown, but I could be wrong

@jcrist1
Copy link
Contributor

jcrist1 commented Sep 26, 2024

@Manishearth Res was a custom type I created just to be rustier in kotlin, and the Err variant didn't need to be throwable. I added the wrapErrAndThrow method to stringly type the error and throw a runtime exception, but it would be nice do something a bit more ergonomic, hopefully leveraging the error trait in rust. Does FFIError leverage the Error trait so we can get access to the debug or display?

@Manishearth
Copy link
Contributor Author

@jcrist1 FFIError doesn't exist, but it could; it would be a type FFIError<E> that wraps arbitrary errors and is throwable.

However we could also just add #[diplomat::attr(error)] and have kotlin/JS complain when you use an error type that's not marked as such.

@jcrist1
Copy link
Contributor

jcrist1 commented Sep 30, 2024

class FFIError<E>(val er: E): Throwable

Yields

...somelib/src/main/kotlin/dev/diplomattest/somelib/Lib.kt:336:15 Subclass of 'Throwable' may not have type parameters

which is a bummer. But we could instead return a runtime exception with some runtime type info, e.g.

data class FFIError(val errStr: String, val errType: String?) : Throwable("Received error $errStr of type $errType") {
    companion object {
        fun <E>fromE(err:E): FFIError {
            return FFIError(err.toString(), err!!::class.simpleName)
        }
    }
}

@Manishearth
Copy link
Contributor Author

Ah, okay. Yeah let's just do that for now. I'll try and make the #[diplomat::attr(auto, error)] thing work eventually and I'll let you know when I do

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

No branches or pull requests

2 participants