golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.02k stars 17.68k forks source link

proposal: Go 2: "onerr return" #32848

Closed kstenerud closed 5 years ago

kstenerud commented 5 years ago

Proposal: "onerr return"

Edit: Originally this was or return, but I like onerr return better since it's more explicit about what we're doing and why.

Current syntax:

func (this *ReaderThing) readBytesFromFile(filename string, maxBytes int) (bytesRead int, err error) {
    f, err := os.Open(filename)
    if err != nil {
        return 0, err
    }
    ...
}

Proposed syntax:

func (this *ReaderThing) readBytesFromFile(filename string, maxBytes int) (bytesRead int, err error) {
    f, err := os.Open(filename) onerr return 0, err
    ...
}

Basically, if the last thing returned by a function is an error type, onerr checks if it's null, and if not, runs the statement return 0, err.

Specifically:

Edit:

As an alternative (but only if we can be sure this won't break things or induce too much cognitive load) Allow block statements:

err := DoSomething() onerr {
    fmt.PrintF("We got error %v\n", err)
    do other stuff...
}
the commented 5 years ago

I like how readable the error handling becomes with this proposal, especially by re-using return. I think using “otherwise” instead of “or” might fit even better, albeit be a bit on the long side for a keyword. Is the idea to only allow return or other statements, too (calling another function, panic, ...)? It reminds me of Perl, is that where the idea came from?

kstenerud commented 5 years ago

Hmm, I'd originally thought of it as orelse but then shortened it to or.

Actually, I suppose technically it could be used as a general error handling keyword:

err := DoSomething() or {
    fmt.PrintF("We got error %n",err)
    do other stuff...
}

But I won't push that far in this proposal if there's resistance to it. It's enough to get rid of the if err != nil ... return boilerplate for now.

andreygrehov commented 5 years ago

I am not a fan of this syntax. The fact that or return block knows the result of os.Open puts me off. It doesn't feel like a natural flow.

kstenerud commented 5 years ago

err exists in the outside block. The or clause gets executed if it was not nil, and follows normal scoping rules, which makes err visible to it (and f and filename and maxBytes for that matter).

kstenerud commented 5 years ago

We could also call it onerr:

func (this *ReaderThing) readBytesFromFile(filename string, maxBytes int) (bytesRead int, err error) {
    f, err := os.Open(filename) onerr return 0, err
    ...
}
andrewrothman commented 5 years ago

I like the idea a lot, but I don't think or is the best keyword. What about reusing else?

andreygrehov commented 5 years ago

I understand the flow, it just doesn't look natural. Will the or clause allow you to do anything other than return? It may open a can of worms like:

func (this *ReaderThing) readBytesFromFile(filename string, maxBytes int) (bytesRead int, err error) {
    f, err := os.Open(filename) or doSomethingElse() && return 0, err
    ...
}

Which imho adds cognitive load.

kstenerud commented 5 years ago

No, this isn't supposed to allow chaining. It just means:

func (this *ReaderThing) readBytesFromFile(filename string, maxBytes int) (bytesRead int, err error) {
    f, err := os.Open(filename) [but if err != nil] [do this]
    ...
}

In the strictest version of this proposal, it only accepts return after the or. In the more relaxed version (if we should decide it's worthwhile), it allows a statement, but not an assignment, meaning that you couldn't chain doSomethingElse() and return 0, err because doSomethingElse() has nothing to the left of it to assign to. (the f, err at the beginning are from os.Open() and not usable for assignment by doSomethingElse()).

owlwalks commented 5 years ago

Very good proposal indeed, tackle exactly what we try to achieve - syntactic sugar for if err != nil { ... }, this is the only proposal that don't make me think how to use it because it’s dead simple, also very minimal impact or might be none at all to current go codebase

sirkon commented 5 years ago

Well, this is much better than flawed try they seriously considered to adapt:

stat := try(try(os.Open(fileName)).Stat())

File descriptor leak here — the example taken from comment in a popular widely supported issue where someone proposed to leave everything as is. There’s no RAII in Go to handle this.

sirkon commented 5 years ago

err exists in the outside block. The or clause gets executed if it was not nil, and follows normal scoping rules, which makes err visible to it (and f and filename and maxBytes for that matter).

+1

I mean yet another person who barely uses Go is actively “improving” it. Not surprised.

DisposaBoy commented 5 years ago

Well, this is much better than flawed try they seriously considered to adapt:

stat := try(try(os.Open(fileName)).Stat())

File descriptor leak here — the example taken from comment in a popular widely supported issue where someone proposed to leave everything as is. There’s no RAII in Go to handle this.

No-one (except maybe complete newbies) writes code like this.

sirkon commented 5 years ago

Well, this is much better than flawed try they seriously considered to adapt:

stat := try(try(os.Open(fileName)).Stat())

File descriptor leak here — the example taken from comment in a popular widely supported issue where someone proposed to leave everything as is. There’s no RAII in Go to handle this.

No-one (except maybe complete newbies) writes code like this.

LOL, another “smart”. Boy, the sooner you understand we humans are naturally apes the better. Please remember Murphy’s law. It actually works.

PS if we weren’t idiots something like Marx described as communism would rule the world.

ianlancetaylor commented 5 years ago

Please keep the conversion polite and on topic. Thanks.

as commented 5 years ago

stat := try(try(os.Open(fileName)).Stat()) @sirkon

This isn't an optimal example because the file descriptor actually will get cleaned up in the current implementation of Go with this indulgent finalizer:

https://github.com/golang/go/blob/master/src/os/file_windows.go#L59

This undocumented behavior (which certainly shouldn't be part of any compatibility agreement) presents somewhat of a false sense of security to me (not sure about what other people think). However, it would be of benefit to think of other counter-examples for this reason.

sirkon commented 5 years ago

@as it can easily be some custom resource handler that has no finalizers

fkarakas commented 5 years ago

would you like to talk english this way too ? "eat apple i like"

ianlancetaylor commented 5 years ago

I kind of like the way that this pushes error handling over to the right, where it is easier to skip when skimming the code.

But I think we need more clarity on what can follow onerr. If the onerr does not return, I guess we just keep execution? But that seems potentially confusing.

Also it's a bit odd to have to write the err over on the left, so that it gets, and then execution jumps over to the right.

ianlancetaylor commented 5 years ago

The return statement executed with the onerr clause follows standard scoping rules, which means that err is visible to it (as are f and filename and maxBytes).

This does not seem true to me. With standard scoping rules the variables declared in an assignment are only visible after the assignment is complete.

    x := 1
    {
        x := x + 1
        fmt.Println(x)
    }

This will print 2: the x in the expression x + 1 is not the variable x being declared in the statement.

I think you are suggesting that the onerr clause introduces a new scope point that occurs after the variables on the left-hand-side have been defined.

ianlancetaylor commented 5 years ago

This proposal does not have strong supported based on votes for the initial comment.

The scoping issue is not resolved. In a case like err := f() onerr return err the second err is not in the scope of the first err with current scoping rules. Any change to those rules to make this work would be quite subtle.

Based on these comments, this looks like a likely decline. Leaving open for a month for final comments.

ianlancetaylor commented 5 years ago

There were no further comments.