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

proposal: Go 2: block scoped error handling #33161

Closed
mvndaai opened this issue Jul 17, 2019 · 22 comments
Closed

proposal: Go 2: block scoped error handling #33161

mvndaai opened this issue Jul 17, 2019 · 22 comments
Labels
error-handling Language & library change proposals that are about error handling. FrozenDueToAge LanguageChange Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Milestone

Comments

@mvndaai
Copy link

mvndaai commented Jul 17, 2019

After reading through the problem overview the try() proposal and every comment on the try() issue #32437 I like the idea of try(), thank you everyone for such hard work! The implementations so far have not solved what I view as the issue of error boilerplate, namely the scope of the err var.

Problem

Go scopes errors using inline if statements, but does not scope errors when a variable needs to be used after.

Scoped:

if err := foo(); err != nil {
if _, err := bar(); err != nil {

Not Scoped:

r, err := bar()
if err != nil {

When the error is not scoped it is has multiple problems. The ones that come to mind are:

  • Shadowing
  • All errs need to be the same type
  • The variable existing past when handling makes sense

Proposal

I propose the language add try/handle keywords similar to what was proposed by @james-lawrence.

r := try bar() handle(err error) {
    return err
}

The try keyword would return everything but the final value. The handle block would only run if the final value was non-zero.

This proposal also solves many common complaints from the try proposal #32437:

  • Requiring a defer function
  • Requiring a named returned which some developers try to avoid
  • Implicit error handling
  • Obscured return
  • Adding complexity to error decoration

Examples

CopyFile

The CopyFile func found in the overview becomes

func CopyFile(src, dst string) error {
	r := try os.Open(src) handle(err error) {
		return err
	}
	defer try r.Close() handle(err error) {
		//handle
	}

	w := try os.Create(dst) handle(err error) {
		return err
	}
	defer try w.Close() handle(err error) {
		//handle
	}

	try io.Copy(w, r); handle(err error) {
		return err
	}
	try  w.Close(); handle(err error) {
		return err
	}
}

The main difference between current error boilerplate and using block scoped error handling are:

  • The err from os.Open(src) doesn't live for the whole scope of the function
  • The defer functions can handle their errors in the same manner as other functions

Hex

The hex example in the overview becomes:

func main() {
	hex := try ioutil.ReadAll(os.Stdin) handle(err error) {
		log.Fatal(err)
	}

	data := try parseHexdump(string(hex)) handle(err error) {
		log.Fatal(err)
	}

	os.Stdout.Write(data)
}

This is very similar to the current go except the err returned from ioutil.ReadAll isn't overwritten by the err from parseHexdump.

Inline/Curried functions

The try proposal comments included examples of nested/curried functions that caused some worry such as this AsCommit.

func AsCommit() error {
    return try(try(try(tail()).find()).auth())
}

Wrapping a try/handle in () would mean a variable never needs to be saved if it is not wanted. This case is similar to using an anonymous func func()int { ... }() and handling the error, but would allow use of other keywords like break, continue, and return.

Current Go

func AsCommit() error {
	t, err := tail()
	if err != nil {
		return err
	}
	f, err := t.find()
	if err != nil {
		return err
	}
	if _, err := f.auth(); err != nil {
		return err
	}
}

With block scoped error handling (please don't do this)

func AsCommit() error {
	(try tail() handle(err error) {
		return err
	}).(try find() handle(err error) {
		return err
	}).(try auth() handle(err error) {
		return err
	})
}

Struct Init

type foo struct {
	Value int
}

Current go

func styleA(s string) error {
	f := foo{}
	var err error
	f.Value, err = strconv.Atoi(s)
	if err != nil {
		return errors.Wrap(err, "value could not be converted")
	}
func styleB(s string) error {
	n, err := strconv.Atoi(s)
	if err != nil {
		return errors.Wrap(err, "value could not be converted")
	}
	f := foo{
		Value: n,
	}

With block scoped error handling

func styleB(s string) error {
	f := foo{}
	f.Value: try strconv.Atoi(s) handle(err error) {
		return errors.Wrap(err, "value could not be converted")
	}
func styleA(s string) error {
	f := foo{
		Value: (try strconv.Atoi(s) handle(err error) {
			return errors.Wrap(err, "value could not be converted")
		}),
	}
@mvndaai mvndaai changed the title Block scoped error handling Proposal: Block scoped error handling Jul 17, 2019
@gopherbot gopherbot added this to the Proposal milestone Jul 17, 2019
@ianlancetaylor ianlancetaylor added v2 A language change or incompatible library change LanguageChange labels Jul 17, 2019
@ianlancetaylor ianlancetaylor changed the title Proposal: Block scoped error handling proposal: Go 2: block scoped error handling Jul 17, 2019
@urandom
Copy link

urandom commented Jul 18, 2019

There seems to be a magic variable "err" in the handle block. Where was it defined?

@mvndaai
Copy link
Author

mvndaai commented Jul 18, 2019

@urandom that is on purpose. The proposal says:

handle blocks would have an implicit err variable

Another way to do this would define handle blocks as handle (err error) {}, but I thought since this could be used in nearly every third line it would be nice to be more concise.

If handle blocks define variables it cold probably be overloaded to handle ok functions as well.

Edit: Proposal changed to no longer have an implicit err

@earthboundkid
Copy link
Contributor

I proposed this under the name guard, based on Swift's guard keyword. That might have been in the old issue before try().

guard f, err := os.Open("hello.txt") {
    // err is in scope here
    // block must exit
}
// f is in scope here

Some thoughts:

  • I think if something like this is done, it should be called guard because that's a name used by another language.
  • Probably it's not enough better than if to be adopted.

@mvndaai
Copy link
Author

mvndaai commented Jul 18, 2019

@carlmjohnson although this looks similar to guard it is different because of the two flow comments you made.

block must exit

The block should not control the flow of your program. Here are some examples why:

Go functions

go try superSlowFunctionThatCanWorkInTheBackground() handle {
    log.Println("logging error because function has already returned", err)
}

Loops

for _, v := range foo() {
    try validate(v) handle {
        log.Println("value in range was invalid", err)
        continue //or break
    }
}

f is in scope here.

If you do not wait for f to be in scope you can do inline defaults.

n := try strconv.Atoi(userInput) handle {
    n = 10
    log.Println("user input could not be converted to a int, using fallback ", n, err)
}

@DeedleFake
Copy link

DeedleFake commented Jul 18, 2019

Is try f() handle {} an expression, or is v := try f() handle {} the only legal usage? It looks like an expression from some of the ways that it's used, but is

fmt.Printf(
  "Result: %v\n",
  try strconv.ParseInt(term1, 10, 0) handle { return err } + try strconv.ParseInt(term2, 10, 0) handle { return err }),
)

legal?

I think I prefer this with the explicit variable declaration. It does make it a bit more verbose, but by making it explicit it removes the hidden nature of it and also makes it, as you mentioned, capable of handling other usages such as map retrievals and whatnot.

The biggest issue I have with this is that it puts the actual function call so far down into the line, surrounded by what is essentially boilerplate. It's one of the reasons that I prefer separate ifs for error handling. I find

v, err := strconv.ParseInt(str, 10, 0)
if err != nil {
  // ...
}

to be a lot more readable, and easier to edit later, than

if v, err := strconv.ParseInt(str, 10, 0); err != nil {
  // ...
}

Might just be my personal preference, though.

@mvndaai
Copy link
Author

mvndaai commented Jul 18, 2019

@DeedleFake, yes try f() handle {} is an expression.

I would expect gofmt to put handle blocks on a new line so you can still see the indentations like you would with if err != nil {} and you would need to wrap it in () to make it work inline.

Your function would probably look like

fmt.Printf(
    "Result: %v\n", 
    (try strconv.ParseInt(term1, 10, 0) handle(err error) { 
        return err
    }) + (try strconv.ParseInt(term2, 10, 0) handle(err error) {
        return err
    }),
)

@mvndaai
Copy link
Author

mvndaai commented Jul 18, 2019

@DeedleFake to respond to

I think I prefer this with the explicit variable declaration. It does make it a bit more verbose, but by making it explicit it removes the hidden nature of it and also makes it, as you mentioned, capable of handling other usages such as map retrievals and whatnot.

I haven't put enough thought into the implications, so take this with a bag of salt.

If this isn't implemented until Go2 maybe the return from map retrievals and type assertions should change to return an error rather than an ok bool so new users do not have to understand two flows for handling errors.

@donaldnevermore
Copy link

I think implicit err is confusing. And I don't like too much keywords to remember.

@mvndaai
Copy link
Author

mvndaai commented Jul 20, 2019

@wdc-python-king, I can understand not wanting more keywords there are already a few I don't use like goto. Does making err explicit make it less confusing?

try foo() handle (err error) {

@donaldnevermore
Copy link

donaldnevermore commented Jul 20, 2019

@mvndaai Explicit err helps. And I'm thinking of a type switch on errors.

result := os.Open(filename) on err NotFound {
        // handle case there isn't a file
} on err PermissionDenied {
        // handle case you don't have the permission
}
// happy path
// result can be used here if there's no error

The () in handle (err error) is unnecessary because the if statement doesn't need that too.
The on could be something else.

And I've got some rules.
Don't add more than one keyword.
Solve error handling problem, not err != nil.
Keep consistency.
Backward compatibility.
etc.

I want error handling be more powerful and enable us to write more robust code.
Graceful error handling is just harder than anyone thinks.

@mvndaai
Copy link
Author

mvndaai commented Jul 20, 2019

@wdc-python-king since one of the main goals of changing errors is to reduce boilerplate, being explicit on just err variable name doesn't make sense unless you plan on nesting and want different names. Which I don't really like.

try foo() handle err (
    try bar() handle err2 {
        // ...
    }
)

The reason I would be okay with handle(name type) is to make this work for any kind of non-Zero handling of the final parameter returned. It would give extra flexibility to handle things that aren't just error.

try foo() handle (err myCustomeError) {
try bar() handle (ok bool) {
try baz() handle (i int) {

@mvndaai
Copy link
Author

mvndaai commented Jul 20, 2019

@wdc-python-king it seems like you would prefer a type switch on errors, something this is equivalent to this:

if err != nil {
    switch v := err.(type) {
        case NotFound:
            // ...
        case PermissionDenied:
            // ...
        }
        return err // don't forget to handle exepected cases
    }
}

Coming in 1.13, or currently using xerrors, will be errors.Is which gives the ability that you want except not as a type switch.

Is follows the idea of wrapping an error and uses an Unwrap() error interface. An error could be wrapped with types like this:
error -> NotFound -> DBError -> error -> ...

Here is an example of how you could do what you want:

if err != nil {
    if errors.Is(NotFound) {
        //...
    }
    if errors.Is(PermissionDenied) {
        // ...
    }
    return err
}

@donaldnevermore
Copy link

donaldnevermore commented Jul 21, 2019

@mvndaai Yes. With errors.Is, the boilerplate is still there but somehow part of the problem is solved.

The Go team solved it without adding any language feature, which I would appreciate.

The reason I would be ok with explicit err is to follow the declare before use convention.

handle err looks like you're giving the error a name, and then you can do anything with the err.

IMHO, I would just make handle do one thing and do it well.

@mvndaai
Copy link
Author

mvndaai commented Jul 29, 2019

I updated the proposal to make err explicit. The changed was based on feedback and this quote from the Contracts/generics proposal.

In a language like Go, we expect every identifier to be declared in some way.

@mikeschinkel
Copy link

@mvndaai Although I think the Go team is unlikely to revisit error handling anytime soon, I do like your proposal and unless there are side-effects I do not yet see I would be happy to see it included in a future version Go. #fwiw

@griesemer
Copy link
Contributor

Except for the scoping, there's really not much difference between

r := try os.Open(src) handle(err error) {
        return err
}

and the current approach:

r, err := os.Open(src)
if err != nil {
        return err
}

In fact, the try-handle approach is more verbose by a few characters, requires declaration of the error type, and understanding yet another major language mechanism/syntax (rather than a helper function such as try) solely for error handling. The use of two keywords is not very economical either given that this is a very specialized statement.

It is very easy to come up with new control flow structures for any language - but it is hard to come up with control flow statements that are universally useful and add significant power. This construct does not add power over what we already have, nor is it universally useful. I don't believe we should add two new keywords and a whole new statement for something we can already write in Go.

@mvndaai
Copy link
Author

mvndaai commented Jul 31, 2019

@griesemer that is very fair to say that the only thing gained by this is the scoping of the error, but that was kind of the point. I consider an err variable existing past when it should be handled as an issue with the language.

@griesemer
Copy link
Contributor

@mvndaai The point you are making about err existing past when it should is a point one could make about any value that is returned by a function and only needed in a subsequent test and perhaps for a return - it's not unique to err or error handling for that matter. There are plenty of functions that return a bool to indicate success. I can imagine plenty of scenarios where a function returns multiple values, with one of those values tested to decide whether to continue or return (and where that value is not used anymore even of the function continues).

In other words, the mechanism you are introducing would be far more interesting if it could be used in a variety of scenarios, and if it "just happens" to also work well for errors; especially if the mechanism would also be dead simple. Such a mechanism would amortize the cost and complexity it adds to the language by being more universally useful.

That more universal construct seems to be a plain if statement. Together with redeclaration in Go, there is really no issue with err living past its first use, so it can be re-used again and again with other if err != nil checks. While it would be "nice" to restrict the scope of err, it doesn't seem to solve an urgent issue.

@mvndaai
Copy link
Author

mvndaai commented Jul 31, 2019

@griesemer the try/handle mechanism could be used in other scenarios since I defined it as

The handle block would only run if the final value was non-zero.

Meaning that you could handle a bool or any other type. The main issue with the bool handling is that Go standard is to return an ok and we use if !ok { which is the opposite of non-zero. Using this would mean reversing the bool from ok to something like failure.

try foo() handle(failure bool) {

Thinking about this more, a handle(a *b) wouldn't make much sense because handle would handle the success case instead of the failure. Thanks for pointing that out.

@griesemer
Copy link
Contributor

@mvndaai Exactly. That's also the reason why try couldn't be trivially generalized to non-error types. It's hard to improve over the simplicity and conciseness of an if statement for general use.

@ianlancetaylor
Copy link
Contributor

For the reasons given above in #33161 (comment) and subsequent discussion, this is a likely decline.

Leaving open for four week for final comments.

@bradfitz bradfitz added the error-handling Language & library change proposals that are about error handling. label Oct 29, 2019
@ianlancetaylor
Copy link
Contributor

No final comments. Closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
error-handling Language & library change proposals that are about error handling. FrozenDueToAge LanguageChange Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

10 participants