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

Functions that return single error #17

Open
arslanarm opened this issue May 2, 2024 · 12 comments
Open

Functions that return single error #17

arslanarm opened this issue May 2, 2024 · 12 comments

Comments

@arslanarm
Copy link

I was writing definitions for os/exec and stumbled upon functions that return single error.
Example:
https://pkg.go.dev/os/exec#Cmd.Run

func (c *Cmd) Run() error

Run() returns error if error happened, otherwise returns nil.
I tried wrapping error in Option. But that assumes that there is second bool return argument.
Then I tried using Result<()>, but still it assumed that there are two return arguments.

What should I use for this use case?

@AbstractiveNord
Copy link

Consider nil error as Ok(()) option of Result enum. In Rust return of None/nil/0 or -1 instead of error message is bad practice, because it breaks compiler checks down. Since you perform a fallible action, you have only 2 options: success or fail. Result<()> is exact what you need.

@arslanarm
Copy link
Author

Here is the exec.brg that I added to std/os/exec

use io
use os
use syscall
use time

fn LookPath(file: string) -> Result<string> { EXT }

fn Command(name: string, arg: VarArgs<string>) -> *Cmd { EXT }

struct Cmd {
   ...
}

impl (c: *Cmd) {
    fn Start() -> Result<()> { EXT }
    fn Run() -> Result<()> { EXT }
    fn StderrPipe() -> Result<io.ReadCloser> { EXT }
    fn StdinPipe() -> Result<io.WriteCloser> { EXT }
    fn StdoutPipe() -> Result<io.ReadCloser> { EXT }
    fn String() -> string { EXT }
    fn Wait() -> Result<()> { EXT }
}

and tried to call the functions

    match exec.Command("borgo", "build").Run() {
        Ok(_) => fmt.Println("Build successful")
        Err(e) => {
            fmt.Println("Build failed:", e)
            return
        }
    }

    exec.Command("go", "run", ".").Run()

The problem is that exec.Command().Run() generates this code

var3_result := func() Result[struct{}, error] {
   var1_check, var2_err := exec.Command("borgo", "build").Run()
   if var2_err != nil {
       return make_Result_Err[struct{}, error](var2_err)
   }
   return make_Result_Ok[struct{}, error](var1_check)
}()

which results to compilation error:
.\main.go:26:29: assignment mismatch: 2 variables but exec.Command("borgo", "build").Run returns 1 value

@alpacaaa
Copy link
Contributor

alpacaaa commented May 5, 2024

Hey @arslanarm thanks for looking into this without any docs whatsoever!

This is a case I hadn't considered and I can confirm the existing translation to Option/Result by the compiler won't cut it. I haven't thought this through yet but my gut reaction would be to add another built-in type Nilable<T> (or Nullable?) which has a single method toOption() -> Option<T>. This can probably be inlined by the compiler, although initially I would just implement it as a standalone function (my hope is that the go compiler is already doing much of the heavy lifting in inlining calls like this).

So to answer your question, the definition would be:

impl (c: *Cmd) {
    fn Run() -> Nilable<error> { EXT }
}

and you would use it something like this:

    match exec.Command("borgo", "build").Run().toOption() {
        None => fmt.Println("Build successful")
        Some(e) => {
            fmt.Println("Build failed:", e)
            return
        }
    }

This is a pattern that will come in handy in other places as well so we need a way to support nil interfaces. What do you think?

@arslanarm
Copy link
Author

Hi, adding Nilable<T> would help with the seamless compatibility with go's ecosystem. But for this case specifically, in my opinion, Result<()> is more aligned with the rust convention. Currently, it is an equivalent of (struct{}, error), and I don't think that any sane person would design the library with these kind of semantics using (struct{}, error). But if there would be a time when (struct{}, error) is necessary, Nilable<T> will come in handy.

So I propose adding Nilable<T> and changing the codegen for Result<()> as a corner case.

@alpacaaa
Copy link
Contributor

alpacaaa commented May 5, 2024

I agree in this specific case Result<()> is semantically more correct. I like the idea of special casing Result<()> in return position for external definitions during codegen so that the error gets wrapped (it won't be zero cost but that's fine). I'd still add Nilable<T> to handle those cases where functions return an interface which can be nil (any examples in the stdlib off the top of your head to add a test?).

@arslanarm
Copy link
Author

Didn't recall any stdlib functions that return nilable interface. But after looking up docs, found this https://pkg.go.dev/[email protected]#Lookup

@alpacaaa
Copy link
Contributor

alpacaaa commented May 6, 2024

Awesome, thanks for looking it up!

@arslanarm
Copy link
Author

Also, how would you define os.exec.Cmd struct.

My take on this struct:

struct Cmd {
    Path: string,
    Args: [string],
    Env: Nilable<[string]>,
    Dir: string,
    Stdin: Nilable<io.Reader>,
    Stdout: Nilable<io.Writer>,
    Stderr: Nilable<io.Writer>,
    ExtraFiles: Nilable<[*os.File]>,
    SysProcAttr: *syscall.SysProcAttr,
    Process: Nilable<*os.Process>,
    ProcessState: Nilable<*os.ProcessState>,
    Err: Nilable<error>,
    Cancel: fn() -> Result<()>,
    WaitDelay: time.Duration
}

NOTE: it is a go struct, so some of the fields are nilable. Option<T> doesn't work for this case. Because Option<T> expects tag for deciding if value is Some or None. Same goes for Result<()> in case of Err error

@alpacaaa
Copy link
Contributor

alpacaaa commented May 7, 2024

I'm thinking Nilable may be too heavy handed – any interface value could be nil after all. Perhaps a simpler alternative that can be shipped faster is to add a isNil(x: T) -> Option<T> function, which then wraps the value in an option as you would expect.

So to go back to the original example, the return type stays true to the Go definition:

impl (c: *Cmd) {
    fn Run() -> error { EXT }
}

And then you'd call isNil on the return value:

let err = exec.Command("borgo", "build").Run()

match isNil(err) {
  Some(err) => ...,
  None => ...,
}

In the specific case of an error value, we can have another function Result.fromError(x: error) -> Result<()>:

let err = exec.Command("borgo", "build").Run()

match Result.fromError(err) {
  Ok(()) => ..,
  Err(err) => // handle error
}

I prefer this design better as it doesn't add complexity to the compiler and doesn't try to hide the underlying semantics of existing Go code. This will also come in handy for nil pointers. What do you think?

@arslanarm
Copy link
Author

TLDR: I would like to stick with the idea of Nilable<T> or creating the annotation for Option<T> to be treated as "value that can be nil" rather than tagged union.

Actually, not a totally bad idea. But it affects nil-safety :) in a way I, personally, don't like. Because it diverges with the idea that if a variable is of type T, then it contains a valid value of T. Option<T> describes that the variable may not be a valid value of T, I would prefer if language enforces nil-safety.

Note: Actually, I am very biased toward null-safety in languages, because I am a kotlin developer, and I really like what language designers have done to preserve null-safety, and be fully compatible with java.

Some knowledge about kotlin's null-safety that can be helpful(I hope it will be helpful):
Every non-primitive type in Java can be null.
Every type in kotlin that is not marked with ? is not nullable.
Functions in java libraries are marked with !, a special type that means "might be null, or might not be null, who knows, do your thing". You cannot mark a variable in kotlin with type of T!.

Example:

fun main() {
    val `some java integer` = JavaClass.staticFunctionThatReturnsInteger()
    val myInt: Int = `value I assume is not null`
    println(myInt + 1)
}

In this example some java integer has type of Int! which means that the nullability of it is unknown. At the point when I assign some java integer to myInt which has a type of Int, that cannot be null, kotlin adds an intrinsic assert for value of some java integer not being null.

This way we don't compromise on null-safety, but that leads to runtime errors, if interactions with java don't go as we expected. I like it, because it not only preserves the full null-safety in kotlin code, also it helps debug the code, because it throws a NullPointerException at the very beginning when that value come from java side to kotlin side. Not when we try to actually use it. Because sometimes, usage is 7 layers after getting the value.

P.S. you don't need to write kotlin "definitions" to interact with java libs. So T! types can be created only by the compiler when it looks at java code.

@alpacaaa
Copy link
Contributor

alpacaaa commented May 9, 2024

That's great, Kotlin is actually a good reference to take inspiration from.
I'm looking at this page https://kotlinlang.org/docs/java-interop.html#notation-for-platform-types

T! means "T or T?"

If I read this correctly, this would be equivalent to Nilable<Option<T>>. So we could keep Nilable<T> and add a Nil.toOption() -> Option<T> method as proposed earlier.

At this point I think this is the best way forward, I'm just not convinced about special casing Result<()> when an error that may be nil is returned. I'd rather introduce an extra type NilResult with a NilResult.toResult() -> Result<()> method which is then special cased by the compiler (so that we don't overload Result with multiple meanings and (pun) exceptions).

Final take:

impl (c: *Cmd) {
    fn Run() -> NilResult { EXT }
}

let err = exec.Command("borgo", "build").Run()

match err.toResult() {
  Ok(()) => ...,
  Err(err) => ...,
}

The Run() method could be typed just as well as Nilable<error> but, as you noted, that would be a little bit weird semantically. Do you think adding both Nilable<T> and NilResult makes sense?

@arslanarm
Copy link
Author

T! means that "nullability is unknown", and the behavior will be like using variables in java: you can access everything without checking for null, but if it is null, it will NPE. Thats more of the "special compiler behavior" for compatibility. For example you know that SomeClass.someMethod() won't return null, compiler won't restrict you in that case. But if it happens that you were wrong, thats your fault.

About NilResult, I am not against it :). It preserves all of the type safety. The only thing I am concerned about is the ergonomics of ? operator. In ideal case scenario I would like to write code like this:

fn runBorgoBuild() -> Result<()> {
    let borgoBuildCommand = exec.Command("borgo", "build")
    borgoBuildCommand.Run()? // Result<()>
    borgoBuildCommand.Wait()? // Result<()>
    match borgoBuildCommand.Err { // Option<error>
        Some(e) => Err(e)
        None => Ok(())
    }
}

But I guess I am fine with:

fn runBorgoBuild() -> Result<()> {
    let borgoBuildCommand = exec.Command("borgo", "build")
    borgoBuildCommand.Run().toResult()? // NilResult
    borgoBuildCommand.Wait().toResult()? // NilResult
    match borgoBuildCommand.Err.toOption() { // Nilable<error>
        Some(e) => Err(e)
        None => Ok(())
    }
}

Of course it would be fire if we could declare Nilable<error>.toResult() specifically to the Nilable<error>, like implementing a trait for specific generic type. But I am not sure, is it even possible to make:

impl (err: Nilable<error>) {
    fn toResult() -> Result<()> {
        match err.toOption() {
            Some(e) => Err(e)
            None => Ok(())
        }
    }
}

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

3 participants