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: A built-in go error check function, "catch" #32811

Closed
natefinch opened this issue Jun 27, 2019 · 89 comments

Comments

@natefinch
Copy link
Contributor

@natefinch natefinch commented Jun 27, 2019

This is a counter-proposal to #32437

Proposal: A built-in Go error check function, catch

catch would function much like the proposed try with a few specific differences:

1.) catch would not return any values, meaning it must be on a line by itself, like panic()
2.) catch takes 1..N arguments
2a.) the first argument must be of type error
2b.) The remainder of the arguments are optional and wrap the error using the given format string and args, as if sent through the new fmt.Errorf which does error wrapping similar to github.com/pkg/errors.

e.g.

func getConfig(config string) (*Config, error)
    f, err := os.Open(config)
    catch(err)
    defer f.Close()
    // use f to make a c *Config...
    return c, nil
}

In this code, catch is the equivalent of

if err != nil {
    return nil, err
}

If err is non-nil, it will return zero values for all other return values, just like try. The difference being that since catch doesn't return values, you can't "hide" it on the right hand side. You also can't nest catch inside another function, and the only function you can nest inside of catch is one that just returns an error. This is to ensure readability of the code.

This makes catch just as easy to see in the flow of the code as if err != nil is now. It means you can't magically exit from a function in the middle of a line if something fails. It removes nesting of functions which is otherwise usually rare and discouraged in go code, for readability reasons.

This almost makes catch like a keyword, except it's backwards compatible with existing code in case someone already has the name catch defined in their code (though I think others have done homework saying try and catch are both rarely used names in Go code).

Optionally, you can add more data to an error in the same line as catch:

func getConfig(user, config string) (*Config, error)
    f, err := os.Open(config)
    catch(err, "can't open config for user %s", user)
    defer f.Close()
    // use f to make a c * Config...
    return c, nil
}

In this configuration, catch is equivalent to

if err != nil {
    return nil, fmt.Errorf("can't open config for user %s: %v", user, err)
}

And would utilize the new implicit error wrapping.

This proposal accomplishes 3 things:

1.) It reduces if err != nil boilerplate
2.) It maintains current legibility of exit points of a function having to be indicated by the first word on the line
3.) It makes it easier to annotate errors than either if err != nil or the try proposal.

@gopherbot gopherbot added this to the Proposal milestone Jun 27, 2019
@gopherbot gopherbot added the Proposal label Jun 27, 2019
@natefinch

This comment has been minimized.

Copy link
Contributor Author

@natefinch natefinch commented Jun 27, 2019

For the record, I don't mind if err != nil, and all things being equal, I would rather not remove that boilerplate, since errors are just values and all that jazz. I know many, many long-time gophers that agree.

However, if we're going to change the language, this is a much better change than the proposed try in my opinion. This maintains legibility of code, maintains ease of finding exit points in the code, still reduces boilerplate, and encourages people to add context to an error.

IMO it removes the major faults of try, which are the high likelihood of missing a call to try embedded in the right hand side somewhere when reading code, and the problem of encouraging nesting of functions which is distinctly bad for code readability.

@thepudds

This comment has been minimized.

Copy link

@thepudds thepudds commented Jun 27, 2019

One variation would be:

  • to start, catch only takes one argument
  • don't allow catch to take additional arguments for format string and args
  • after experience or experimentation, consider including a handler function as an optional second argument, such as:
  decorate := func(err error) error { return fmt.Errorf("foo failed: %v", err) }

  ...

  f, err := os.Open(config)
  catch(err, decorate)          // decorate called in error case

or using helper functions that could live somewhere in the standard library:

  catch(err, fmt.Annotatef("foo failed for %v", arg1))      

That would be more parallel to the current proposal for try as a builtin.

edit: part of the rationale for a handler function is it means a builtin does not directly rely on fmt semantics, which is the concern @Merovius expressed better in the next comment.

@Merovius

This comment has been minimized.

Copy link

@Merovius Merovius commented Jun 27, 2019

My main reason for disagreeing with that is that it couples the fmt-package into the language. i.e. to actually add this to the spec, you'd also need to define what fmt does in the spec - and IMO, fmt is far too large (conceptually) to be part of the language proper. So, IMO, the multiple-argument form you are suggesting should be removed.

@natefinch

This comment has been minimized.

Copy link
Contributor Author

@natefinch natefinch commented Jun 27, 2019

That's a good point, and actually why I made 2a and 2b. I think we could remove 2b pretty easily. It could easily be replaced by a helper function in fmt (or user code):

// Wrap returns nil if err is nil, otherwise it returns a wrapped 
// version of err with the given fmt string as per fmt.Errorf.
func Wrap(err error, msg string, args ...interface{})

then you could do

catch(fmt.Wrap(err, "error opening config for user %s", user))

It's not quite as nice, though.

And for the record, I'd be ok with making this part of fmt be part of the language if it encourages people annotate their errors.

@rof20004

This comment has been minimized.

Copy link

@rof20004 rof20004 commented Jun 27, 2019

@natefinch What about if i have more than two returns?

func getConfig(user, config string) (*Config, error, result)
    f, err := os.Open(config)
    catch(err)
    defer f.Close()
    // use f to make a c * Config...
    return c, nil, nil
}
@natefinch

This comment has been minimized.

Copy link
Contributor Author

@natefinch natefinch commented Jun 27, 2019

It works like try from the linked issue, where using it requires that it's in a function where the last return value is of type error, and it returns zero values for everything except the error.

so like if you had

func getConfig(user, config string) (*Config, result, error)

catch(err) would return nil, result{}, err

@bakul

This comment has been minimized.

Copy link

@bakul bakul commented Jun 27, 2019

You can simplify this a bit. Since it expects an error value, you can give it any error value. So then it can be catch err or catch fmt.Errorf(“can’t open config for %s”, user) — no need to implicitly use this error generating function and you can return any kind of error that makes sense in your situation. Though I don’t like the use of catch. It would be perfectly legal in your scheme to say something like this but it looks weird!

    If !inrange(low, value, high) {
        catch OutOfRangeError
    }

Except for the name, this would be a perfectly legitimate use at the where error conditions are first detected and even better than having to say

    If !inrange(low, value, high) {
        return nil, OutOfRangeError
    }

On IBM systems on can use a macro ABEND to abnormally terminate a task. It was a bit like panic but the name was suggestive of its intended use. catch doesn’t have that. Neither did try. I thought of returnOnError or errorReturn or just ereturn or abreturn or may be even raise as in C. Any of these work for me better than catch or try but none seem ideal.

@natefinch

This comment has been minimized.

Copy link
Contributor Author

@natefinch natefinch commented Jun 27, 2019

So, it can't be a keyword, because that's not backwards compatible. So it would need to be catch(OutOfRangeError) ....

I do agree that the name might need some tweaking. Naming is hard, and I welcome suggestions for another name that might be more appropriate.

The problem is really that it is most accurately called maybeReturn

How about yield? Where it's keep going, or stop if something is in the way? Also yield is to produce, like it produces the error.

f, err := os.Open(config)
yield(err) 
defer f.Close()

and then it could be

yield(OutOfRangeError)

which looks better to me

@bakul

This comment has been minimized.

Copy link

@bakul bakul commented Jun 27, 2019

(Ignoring its use as giving up the processor to another coroutine or thread) yield is better than catch!
returnOnError or returnIfError exactly captures the semantics but it is a bit long.

returnIf can also work provided error values have err or error in their name.

returnIf(OutOfRangeError)
@provPaulBrousseau

This comment has been minimized.

Copy link

@provPaulBrousseau provPaulBrousseau commented Jun 27, 2019

Both yield and catch have very strong connotations in other languages, and I wouldn't suggest them for your proposal. As you say, naming is hard.

But I have to disagree with this if for no other reason than its rigidity; if

catch(err, "can't open config for user %s", user)

is really just

if err != nil {
  return fmt.Errorf("can't open config for user %s: %v", user, err)
}

... it's really not that valuable. Most of my error handling is done with github.com/pkg/errors, although I have yet to evaluate the new proposals around wrapped errors. And others perhaps like dedicated typed errors, while others may use constants. This solves a narrow set circumstances.

But to think this through further... what happens in this case:

func DoYourThing(a string) (x string, err error) {
  x = fmt.Sprintf("My thing is '%a'", a)
  // always returns an err
  err = DoSomethingBad() 
  catch(err)

  // process some other stuff.

  return
}

func DoAThing() {
  x, _ := DoYourThing("cooking")
  fmt.Printf("%s!!\n", x)
}
@bakul

This comment has been minimized.

Copy link

@bakul bakul commented Jun 27, 2019

@provPaulBrousseau, IMHO catch or its equivalent shouldn’t be used if you are returning an error as well as a legit value. [This is another reason I bemoan the lack of a sum type in Go. With sum type you’d use it in most cases where an error or a legit value is returned but not both. For examples such as yours, a tuple or a product type would be used. In such a language catch would only work if the parent function returned a sum type]

But if you do use catch, it would return “”, err.

@natefinch

This comment has been minimized.

Copy link
Contributor Author

@natefinch natefinch commented Jun 27, 2019

@provPaulBrousseau note that my intent is that this use the new fmt.Errorf which actually does wrapping like github.com/pkg/errors (which is what we use at work, as well). So that fmt.Errorf looks like the oldschool "just reuse the string", but it's actually wrapping under the hood. I'll update the original post to make that clear.

Certainly, if you want something more complicated, like turning one error into some other type of error, you'd instead use the methods you used before, with an if statement. This is just trying to remove a little boilerplate and remove some friction from adding context to errors.

@natefinch

This comment has been minimized.

Copy link
Contributor Author

@natefinch natefinch commented Jun 27, 2019

oh.... and yes, just like try, catch, and yield have meanings in other languages.... but this isn't other languages, it's Go. And if the name is a good one, reusing it to mean something different seems ok, IMO... especially since it's not a keyword.

@freeformz

This comment has been minimized.

Copy link
Contributor

@freeformz freeformz commented Jun 27, 2019

Option 2c?

catch takes an error value. When the error value is nil processing continues. When it's !nil it returns from the enclosing function with zero values for each return parameter except the last error value, for which it uses the supplied error.

If named return parameters are used, then the function returns with those values and the supplied error when the error is not nil.

It's a compile time error to use catch in a function that does not have an error value as it's last return.
It's a compile time error to provide more than one argument to catch.

Examples:

func Foo() (int, int, error) {
  err := ....
  catch err // nil processing continues, !nil return 0,0, err

  user := "sally"
  err := ....
  catch fmt.Errorf("can't open config for user %s: %w", user, err) // note new %w flag, so it's an `error`  and an `errors.Wrapper`, nil processing continue, !nil return 0,0, <formatted error>

I like this for all the reasons you pointed out. And there is less "magic" wrt formatting, and if after a release or two it's felt that formatting should be built in it can still be added later by allowing the catch statement to take multiple arguments.

catch <error> makes it look more like a statement than a function, which IMO helps with it standing out visually and is less likely to lead newcomers to think it's a function.

@freeformz

This comment has been minimized.

Copy link
Contributor

@freeformz freeformz commented Jun 27, 2019

PS: I realize this would require a tweak to fmt.Errorf to make it return nil if the provided error is nil. So what you said with the Wrap function instead:

func Foo() (int, int, error) {
  user := "sally"
  err := ...
  catch errors.Wrap("can't open config for user: %s: %w", user, err)
@apghero

This comment has been minimized.

Copy link

@apghero apghero commented Jun 27, 2019

catch is a statement, i.e. it must be on a line by itself, like panic()

This seems like a non-starter. Can it just be a func catch(err error, args ...interface{}) instead? That'd be the same result, and backwards compatible. That makes the biggest problem the fact that you need some static analysis to ensure that the return type of the enclosing function returns an error... (not sure how difficult that'd be to achieve in Go's compiler)

@freeformz

This comment has been minimized.

Copy link
Contributor

@freeformz freeformz commented Jun 27, 2019

WRT Backwards compatibility (catch <error> isn't backwards compatible)..... I'd be more than happy to wait for something like this until modules becomes the default build mode ala https://blog.golang.org/go2-next-steps

@daved

This comment has been minimized.

Copy link

@daved daved commented Jun 27, 2019

At first glance, this is generally what try and check should have been. check is a great name (and way to avoid the baggage of catch), whether meaning "verify" or "halt/slow progress".

@natefinch

This comment has been minimized.

Copy link
Contributor Author

@natefinch natefinch commented Jun 27, 2019

@apghero why is it a nonstarter? That's basically the whole difference between this and the try proposal. I do not want a function on the right hand side of a complicated statement to be able to exit. I want it to have to be the only thing on the line so it is hard to miss.

@freeformz

This comment has been minimized.

Copy link
Contributor

@freeformz freeformz commented Jun 27, 2019

@daved check SGTM, just not all the other bits.

@ubikenobi

This comment has been minimized.

Copy link

@ubikenobi ubikenobi commented Jun 27, 2019

This is many ways worse than try() function proposal in my opinion because it doesn't really solve the underlying goal of reducing boilerplate code.

A function with multiple error check looks like this:

func Foo() (err error) {
	var file1, err1 = open("file1.txt")
	catch(err1)
	defer file1.Close()
	var file2, err2 = open("file2.txt")
	catch(err2)
	defer file1.Close()
}

Vs. originally proposed try() function:


func Foo() (err error) {
	var file1 = try(open("file1"))
	defer file1.Close()
	var file2 = try(open("file1"))
	defer file2.Close()
}


Later looks more concise and elegant in my opinion.

@daved

This comment has been minimized.

Copy link

@daved daved commented Jun 27, 2019

@ubikenobi It's always trade-offs. In my opinion, the reduction provided by try costs far too much in readability. More so when the extra "flexibility" try permits is weighed. The clarity that comes from that single newline (and possibly two columns of text) is worthwhile. The surety that comes from reduced flexibility? Even more critically so.

@apghero

This comment has been minimized.

Copy link

@apghero apghero commented Jun 27, 2019

@apghero why is it a nonstarter? That's basically the whole difference between this and the try proposal. I do not want a function on the right hand side of a complicated statement to be able to exit. I want it to have to be the only thing on the line so it is hard to miss.

Me either. But that can be accomplished without syntax. The syntax part is the thing that breaks backwards compatibility, so why not just do what panic does and return nothing? You can't use a function that returns nothing on the right hand side of a statement, or in an expression. Therefore it's limited to the left most column of a block, which is what we're looking for.

@mathieudevos

This comment has been minimized.

Copy link

@mathieudevos mathieudevos commented Jun 28, 2019

This still would take up a keyword: catch, but only barely improve on the try variant.
While I do think this is a step forward, it still only allows "lazily" error handling.

What for users that use custom errors that use wrapping functions?
What for users that want to log for developers & operators and return human-understandable errors to the users?

if err != nil {
    log.Println("Hey operating, big error, here's the stack trace: ...")
    return myerrorpackage.WrappedErrorConstructor(http.StatusInternalServerError, "Some readable message")
}

If we plan on taking up a whole keyword, can we at least attach some function to it?
Counter proposal to the whole try or catch: do try/catch with check & handle (better keywords imho), scope specific.

Suggestion is part of the whole mix of error handling proposals: scoped check/handle proposal

@daved

This comment has been minimized.

Copy link

@daved daved commented Jun 28, 2019

This still would take up a keyword: catch, but only barely improve on the try variant.

The point is to reduce the footgun rating of a solution, not to offer an "improved" novelty.

What for users that use custom errors that use wrapping functions?

This was covered in comments.

If we plan on taking up a whole keyword, can we at least attach some function to it?

Now I'm not sure if this is a troll post. Most keywords serve exactly one function. return, if, etc.

@ngrilly

This comment has been minimized.

Copy link

@ngrilly ngrilly commented Jun 28, 2019

This proposal states several times that catch(...) is a statement, like panic(...). But panic is not a statement, it's a built-in function with no return value.

@erizocosmico

This comment has been minimized.

Copy link

@erizocosmico erizocosmico commented Jul 4, 2019

@provPaulBrousseau Go does support optional arguments in certain built-in functions, and this would be a built-in function. You can make(map[int]int), make([]int, 5) and make([]int, 0, 5).

WRT @kennygrant's example: isn't that actually more verbose than just using an iferrnil?

@nikolay

This comment has been minimized.

Copy link

@nikolay nikolay commented Jul 4, 2019

I think this is not much better than the try proposal. All we need is to be able to write if err { ... } without having to put != nil.

@Daniel1984

This comment has been minimized.

Copy link

@Daniel1984 Daniel1984 commented Jul 4, 2019

I guess you should close this proposal as the one to keep it simple and keep it as it is got way more positive attention... 💩

@Dragomir-Ivanov

This comment has been minimized.

Copy link

@Dragomir-Ivanov Dragomir-Ivanov commented Jul 4, 2019

I think this is not much better than the try proposal. All we need is to be able to write if err { ... } without having to put != nil.

Thank you sir, I do think Go should allow implicit casting from nil interface to bool.

@kennygrant

This comment has been minimized.

Copy link
Contributor

@kennygrant kennygrant commented Jul 5, 2019

I think this is not much better than the try proposal. All we need is to be able to write if err { ... } without having to put != nil.

Everyone seems to have a different opinion on error handling ;)

I'd be fairly happy with that solution too - allow if err {} on one line, and simple error handling would be a lot more succinct. It would perhaps have a more wide-ranging impact though (allowing implicit conversion of non-nil to bool in all code). Not sure all the heat and noise around this issue is warranted, but I would prefer a solution that shortens error handling and doesn't insert itself into the happy path instead (as try does).

@Chillance

This comment has been minimized.

Copy link

@Chillance Chillance commented Jul 5, 2019

I'm just gonna throw this out there at current stage. I will think about it some more, but I thought I post here to see what you think. Maybe I should open a new issue for this?

So, what about doing some kind of generic C macro kind of thing instead to open up for more flexibility?

Like this:

define returnIf(err error, desc string, args ...interface{}) {
	if (err != nil) {
		return fmt.Errorf("%s: %s: %+v", desc, err, args)
	}
}

func CopyFile(src, dst string) error {
	r, err := os.Open(src)
	:returnIf(err, "Error opening src", src)
	defer r.Close()

	w, err := os.Create(dst)
	:returnIf(err, "Error Creating dst", dst)
	defer w.Close()

	...
}

Essentially returnIf will be replaced/inlined by that defined above. The flexibility there is that it's up to you what it does. Debugging this might be abit odd, unless the editor replaces it in the editor in some nice way. This also make it less magical, as you can clearly read the define. And also, this enables you to have one line that could potentially return on error. And able to have different error messages depending on where it happened.

Edit: Also added colon in front of the macro to suggest that maybe that can be done to clarify it's a macro and not a function call.

@ngrilly

This comment has been minimized.

Copy link

@ngrilly ngrilly commented Jul 5, 2019

@nikolay

All we need is to be able to write if err { ... } without having to put != nil.

It's a much larger change than try. It would imply to change the language spec to accept implicit casting from nil to bool, not just in if, but everywhere (an exception for if would be really weird). The implicit casting could introduce hard to track bugs. And we would still need to change gofmt to format this on 1 line instead on 3.

@ngrilly

This comment has been minimized.

Copy link

@ngrilly ngrilly commented Jul 5, 2019

@Chillance You're essentially proposing to introduce an hygienic macro system in Go. This opens another can of worms.

An issue was created proposing this: #32620.

Here is what it looks like in Rust: https://doc.rust-lang.org/1.7.0/book/macros.html.

@Chillance

This comment has been minimized.

Copy link

@Chillance Chillance commented Jul 5, 2019

@ngrilly Yeah, I'm sure there other things to consider for the macro way. I just wanted to throw it out there before things are added too quickly. And thanks for the link issue. Great to see others brought it up too. Naturally, if macros would be overused it could get complicated, but then you have to blame yourself writing the code like that I suppose... :)

@ngrilly

This comment has been minimized.

Copy link

@ngrilly ngrilly commented Jul 5, 2019

@Chillance I'm personally opposed to adding hygienic macros to Go. People are complaining about try making the code harder to read (I disagree and think it makes it easier to read, but that's another discussion), but macros would be strictly worse from this point of view. Another major issue with macros is that it makes refactoring, and especially automated refactoring, so much harder, because you usually have to expand the macro to refactor. I repeat myself, but this is a whole can of worms. try is 1 year old toy compared to this.

@Chillance

This comment has been minimized.

Copy link

@Chillance Chillance commented Jul 5, 2019

@ngrilly My initial thought with Go macros is that they would be simple, shorter tidbits. And you would use those in the function to make it easier to read. That is, not overuse macros. And, use for simpler things that you don't want to repeat, such as error handling. It is not suppose to be very large, because then just use a function I suppose. Also, having a macro, doesn't that make refactoring easier as you have one place to change things instead of maybe many in the function?

@ngrilly

This comment has been minimized.

Copy link

@ngrilly ngrilly commented Jul 5, 2019

@Chillance The problem with macros is that tools (especially refactoring tools) have to expand the code to understand the semantics of the code, but must produce code with the macros unexpanded. It's hard. Look at this for example in C++: http://scottmeyers.blogspot.com/2015/11/the-brick-wall-of-c-source-code.html. I guess it's a little bit less hard in Rust than in C++ because they have hygienic macros. If the complexity issue can be overcomed, I could change my mind. Must be noted it changes nothing to the discussion about try, since Rust which has macros, has implemented a strict equivalent of try as a built-in macro ;-)

@ungerik

This comment has been minimized.

Copy link

@ungerik ungerik commented Jul 6, 2019

Why not use existing keywords like:

func ReadFile(filename string) ([]byte, error) {
    f, err := os.Open(filename)
    return if err != nil
    defer f.Close()
    return ioutil.ReadAll(f)
}
  1. No new language rule for implicit bool(err != nil)
  2. So other conditions than err != nil are also possible:
    f, err := os.Open(filename)
    return if errors.Is(err, os.ErrNotExist)
  1. return without named values is already in the language
  2. Only implicit magic is returning err when used in the if statement, but explicit return values could be used like:
    return x, y, err if err != nil

A more complicated example doesn't seem to have advantages in terms of readability compared to the traditional if clause, though:

    f, err := os.Open(filename)
    return nil, errors.Wrap(err, "my extra info for %q", filename) if errors.Is(err, os.ErrNotExist)
@fbnz156

This comment has been minimized.

Copy link

@fbnz156 fbnz156 commented Jul 7, 2019

This seems to be nothing more than an equivalent to a macro such as the following:

#define catch(a) if a != nil { return }

Why not just make macros a feature and let users implement their own shorthands? Could even shorthand the decoration

#define catch(a, msg) if a != nil { return fmt.Errorf("%s: %w", msg, a) }

@ohir

This comment has been minimized.

Copy link

@ohir ohir commented Jul 7, 2019

@Fabian-F

Why not just make macros a feature and let users implement their own shorthands?

Because we have no time to learn every other library's own flavor of Go just to be able to comprehend what is it doing. We've been there, done that @ C, we've been suffocated by it in C++. RiP Rust was such a wünderkid until got the feature-fever and passed away.
I wish not the same with Go.

Could even shorthand the decoration
#define catch(a, msg) if a != nil { return fmt.Errorf("%s: %w", msg, a) }

...and possibly will send user keys to the moon in yet other decoration you'll skim over.

@ohir

This comment has been minimized.

Copy link

@ohir ohir commented Jul 8, 2019

@natefinch
I humbly inform you that I reused the catch word for my #32968 check proposal.
I would be really glad to see your opinion there, too.
The check abstract:

func check(Condition bool) {} // built-in signature

check(err != nil)
{
    ucred, err := getUserCredentials(user)
    remote, err := connectToApi(remoteUri)
    err, session, usertoken := remote.Auth(user, ucred)
    udata, err := session.getCalendar(usertoken)

  catch:               // sad path
    ucred.Clear()      // cleanup passwords
    remote.Close()     // do not leak sockets
    return nil, 0,     // dress before leaving
      fmt.Errorf("Can not get user's Calendar because of: %v", err)
}
// happy path
check(x < 4) // implicit: last statement is of the sad path 
  {
    x, y = transformA(x, z)
    y, z = transformB(x, y)
    x, y = transformC(y, z)
    break // if x was < 4 after any of above
  }

Last one already compiles at playground.


@Merovius [about print/println]

If you look at the spec, it seems clear to me that that wouldn't be a good idea. Both in that it's implementation-defined what it does and that it is limited in what arguments it can receive.

You're right. I was about reusing print short implementations for rudimentary sprint - its too cumbersome though.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 9, 2019

I agree with @Merovius that we can't put fmt.Errorf into the language proper. I think that reduces this proposal to the single argument case. It then seems to me that this is isomorphic to the try proposal (#32437) except that try (here called catch) does not return any values. Does that seems correct?

Of course, then the new built-in doesn't help with error annotation, which is one of the objections that people raise about try.

The name catch doesn't seem right to me for this. It doesn't catch anything. Seems like check would be a better choice.

@thepudds

This comment has been minimized.

Copy link

@thepudds thepudds commented Jul 9, 2019

@ianlancetaylor

Of course, then the new built-in doesn't help with error annotation, which is one of the objections that people raise about try.

As a fallback position regarding the fmt concern, the proposal author had suggested passing in a handler to check (using your suggested term for catch) in #32811 (comment) above.

So under that alternative, a wrapping example could be:

f, err := os.Open(config)
check(fmt.ErrorHandlerf(err, "error opening config for user %s", user))

A similar-in-spirit solution would be to have an error handler as an optional second argument to check for in-place error annotation (e.g., as suggested by @eihigh in #32437 (comment)):

f, err := os.Open(config)
check(err, fmt.ErrorHandlerf("error opening config for user %s", user))

Some of the concerns that have been raised in the discussion about the try proposal (#32437) include:

  1. concern about in-place error annotation or wrapping.
  2. concern about control flow visibility, including the risk of not noticing an exit point.
  3. concern about the amount of complexity per line.

As far I understood the discussion, I think this proposal targets all three of those.

One final question / comment: if a builtin such as check was to be introduced without returning any values, it seems there would be at least a possibility to allow it to return values in the future in a backwards compatible way? In other words, if this particular proposal was to be pursued, it seems that check could be introduced without the ability to nest check, but the door would still be open to allow nesting of check as a follow-on change if experience suggests it is needed?

@balasanjay

This comment has been minimized.

Copy link
Contributor

@balasanjay balasanjay commented Jul 10, 2019

@thepudds

f, err := os.Open(config)
check(fmt.ErrorHandlerf(err, "error opening config for user %s", user))

FWIW, this code would also work with try. The changes between the other proposal and this one would seem to be: a) rename try to check and b) remove the return value.

The former seems fairly minor.

The latter seems more significant and will essentially require the builtin to be used in a statement context. Rolling out try (or check) in two separate phases as you suggest actually sounds pretty good to me.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 30, 2019

As I understand it, the proposal as it stands is:

  1. Add a new builtin function check. This function takes a single argument of type error. It may only be called in a function whose last result parameter is of type error. If the argument to check is not nil, then it immediately returns from the calling function, returning its argument as the last result. Other results remain as they are; if unnamed, they are the zero value of their type.

  2. Add a new function fmt.ErrorHandlerf. This takes a value of type error, a formatting string, and other arguments. If the first argument, of type error, is nil, then fmt.ErrorHandlerf returns nil. Otherwise it returns the result of calling fmt.Errorf on the arguments. Maybe the first argument is also passed to fmt.Errorf, though I'm not sure quite how that part works.

The advantage is that the block if err != nil { return ... } turns into a single statement, and there is some support for wrapping errors using fmt.ErrorHandlerf. We could also add new similar functions as needed.

A disadvantage is that the fmt.ErrorHandlerf function is always called, although it would return very quickly after not doing anything.

The proposed check function is similar to the earlier try proposal (#32437) (which could also have used the suggested fmt.ErrorHandlerf) except that check, unlike try, cannot be used in an expression.

Does this seem like a correct summary of the current state? Thanks.

@natefinch

This comment has been minimized.

Copy link
Contributor Author

@natefinch natefinch commented Jul 31, 2019

That is my understanding.

However, this was a counterproposal to try, which has been discarded. I don't think this proposal actually solves that much, I mostly did it as a compromise between people that wanted something like try, and those that thought try would hurt readability too much.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 31, 2019

Do you want to withdraw the proposal? The votes on it aren't all that great.

@natefinch

This comment has been minimized.

Copy link
Contributor Author

@natefinch natefinch commented Jul 31, 2019

I'm happy to withdraw it given the votes.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 31, 2019

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.