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: ErrWrap expressions #39451

Closed
xushiwei opened this issue Jun 7, 2020 · 30 comments
Closed

proposal: Go 2: ErrWrap expressions #39451

xushiwei opened this issue Jun 7, 2020 · 30 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

@xushiwei
Copy link

xushiwei commented Jun 7, 2020

I suggest to reinvent error handling specification in Go. I call them ErrWrap expressions:

expr! // panic if err
expr? // return if err
expr?:defval // use defval if err

How to use them? Here is an example:

import (
	"fmt"
	"strconv"
)

func add(x, y string) (int, error) {
	return strconv.Atoi(x)? + strconv.Atoi(y)?, nil
}

func addSafe(x, y string) int {
	return strconv.Atoi(x)?:0 + strconv.Atoi(y)?:0
}

func main() {
	fmt.Println(`add("100", "23"):`, add("100", "23")!)

	sum, err := add("10", "abc")
	fmt.Println(`add("10", "abc"):`, sum, err)

	fmt.Println(`addSafe("10", "abc"):`, addSafe("10", "abc"))
}

The output of this example is:

add("100", "23"): 123
add("10", "abc"): 0 strconv.Atoi: parsing "abc": invalid syntax

===> errors stack:
main.add("10", "abc")
	/Users/xsw/goplus/tutorial/15-ErrWrap/err_wrap.gop:6 strconv.Atoi(y)?

addSafe("10", "abc"): 10

Compared to corresponding normal Go code, It is clear and more readable.

And the most interesting thing is, the return error contains the full error stack. When we got an error, it is very easy to position what the root cause is.

How these ErrWrap expressions work? I have implemented them in the Go+ language:

See Error Handling for more information.

@smasher164 smasher164 added error-handling Language & library change proposals that are about error handling. v2 A language change or incompatible library change LanguageChange labels Jun 7, 2020
@smasher164 smasher164 changed the title Reinvent error handling specification proposal: ErrWrap expressions Jun 7, 2020
@gopherbot gopherbot added this to the Proposal milestone Jun 7, 2020
@mvdan
Copy link
Member

mvdan commented Jun 7, 2020

Please note that you should fill https://github.com/golang/proposal/blob/master/go2-language-changes.md when proposing a language change.

@mvdan mvdan added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 8, 2020
@xushiwei
Copy link
Author

xushiwei commented Jun 8, 2020

Would you consider yourself a novice, intermediate, or experienced Go programmer?

Yes.

What other languages do you have experience with?

C/C++, Java, Javascript, Objective-C, Python, erlang, etc.

Would this change make Go easier or harder to learn, and why?

Easy to learn. Because we give a more stable error handling specification. It not only reduces code size. The most important thing is it reduce many time to location root case of an error.

Has this idea, or one like it, been proposed before?
If so, how does this proposal differ?

There are already many error handling proposal. Most of them are care about code size. But I think the important thing is how to find root cause quickly.

Who does this proposal help, and why?

It's a basic language feature, and all of Gopher will be helped.

What is the proposed change? Please describe as precisely as possible the change to the language.
What would change in the language spec? Please also describe the change informally, as in a class teaching Go.
Show example code before and after the change.

We reinvent error handling specification in Go. I call them ErrWrap expressions:

expr! // panic if err
expr? // return if err
expr?:defval // use defval if err

All these expr should have multiple values (val1, val2, ..., valN1, valN), and the last value valN should implements interface error.

if valN is not nil, It means an error happens. In an error case, expr! wraps the error and panic. expr? wraps the error and return. expr?:defval uses defval as result of expr.

val1, val2, ..., valN1 := expr!
// equal to:
val1, val2, ..., valN1, valN := expr
if valN != nil {
    panic(errors.NewFrame(valN, ...)) // save error stack frame information and panic
}

val1, val2, ..., valN1 := expr?
// equal to:
val1, val2, ..., valN1, valN := expr
if valN != nil {
    _ret_err = errors.NewFrame(valN, ...)) // _ret_err is the last output parameter
    return
}

val1 := expr?:defval
// equal to:
val1, val2 := expr
if val2 != nil {
    val1 = defval
}

Is this change backward compatible?

Yes. It doesn't affect existing Go code.

What is the cost of this proposal? (Every language change has a cost).

I think ErrWrap expressions is easy to remember, to learn.

How many tools (such as vet, gopls, gofmt, goimports, etc.) would be affected?

We add new form expressions named ErrWrapExpr, so go parser will need to be updated, but most of go tools don't need to change their code. Of course, gofmt need to support ErrWrapExpr.

What is the compile time cost?

parsing ErrWrapExpr and translating them into normal go code.

What is the run time cost?

No extra cost.

Can you describe a possible implementation?
Do you have a prototype? (This is not required.)

Yes. See https://github.com/qiniu/goplus/blob/v0.6.20/exec/golang/stmt.go#L170

How would the language spec change?
Orthogonality: how does this change interact or overlap with existing features?

No overlap with existing features.

Is the goal of this change a performance improvement?
If so, what quantifiable improvement should we expect?
How would we measure it?

No. But it changes the performance of location root cause of an error.

The following is output of the above example I given.

strconv.Atoi: parsing "abc": invalid syntax

===> errors stack:
main.add("10", "abc")
	/Users/xsw/goplus/tutorial/15-ErrWrap/err_wrap.gop:6 strconv.Atoi(y)?

It not only points filename and line, but also what expr? code is, and the values of function call parameters. This error information recording will be performed automatically, as an error handling specification.

Does this affect error handling?
If so, how does this differ from previous error handling proposals?

There are already many error handling proposal. Most of them are care about code size. But I think the important thing is how to find root cause quickly.

Is this about generics?
If so, how does this differ from the the current design draft and the previous generics proposals?

No.

@andig
Copy link
Contributor

andig commented Jun 8, 2020

The most important thing is it reduce many time to location root case of an error.

I don‘t feel it does. It seems that the syntactical sugar serves as an excuse to spell out the problem handling even if it is more verbose.

Did you consider how your proposal would work with more than 2 return parameters with regards to default values?

@ianlancetaylor ianlancetaylor changed the title proposal: ErrWrap expressions proposal: Go 2: ErrWrap expressions Jun 8, 2020
@ianlancetaylor ianlancetaylor removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 8, 2020
@ianlancetaylor
Copy link
Contributor

Has some similarities to #32845, #33067, #33074, #33152.

I don't understand why it is a good idea to combine the syntax changes with error wrapping. Sometimes it is appropriate to return a wrapped error. Sometimes it is not. Some of this distinction was examined in https://go.googlesource.com/proposal/+/refs/heads/master/design/go2draft-error-printing.md, although that design draft has not been accepted into the language.

@xushiwei
Copy link
Author

xushiwei commented Jun 9, 2020

The most important thing is it reduce many time to location root case of an error.

I don‘t feel it does. It seems that the syntactical sugar serves as an excuse to spell out the problem handling even if it is more verbose.

Did you consider how your proposal would work with more than 2 return parameters with regards to default values?

I didn't.

@xushiwei
Copy link
Author

xushiwei commented Jun 9, 2020

Has some similarities to #32845, #33067, #33074, #33152.

I don't think It is similar. In these issues, err? means err != nil. They only do less-typing things. My proposal is care of error stack tracking. For example:

var ErrInvalidInteger = errors.New("invalid integer")

func atoi(v string) (int, error) {
	n, err := strconv.Atoi(v)
	if err != nil {
		ErrInvalidInteger? // use ErrWrap expression to track error stack
	}
	return n, nil
}

func add(x, y string) (int, error) {
	return atoi(x)? + atoi(y)?, nil
}

func main() {
	sum, err := add("10", "abc")
	if errors.Is(err, ErrInvalidInteger) {
		// do something error handling ...
	}
}

I don't understand why it is a good idea to combine the syntax changes with error wrapping. Sometimes it is appropriate to return a wrapped error. Sometimes it is not. Some of this distinction was examined in https://go.googlesource.com/proposal/+/refs/heads/master/design/go2draft-error-printing.md, although that design draft has not been accepted into the language.

@davecheney
Copy link
Contributor

var ErrInvalidInteger = errors.New("invalid integer")

func atoi(v string) (int, error) {
	n, err := strconv.Atoi(v)
	if err != nil {
		ErrInvalidInteger? // use ErrWrap expression to track error stack
	}
	return n, nil
}

In this example you're using ? as shorthand for something like

return errors.Wrap(ErrInvalidInteger)`

I believe the problem is not the need for a shorthand for errors.Wrap, it is the use of the named error value. Consider this replacement.

func atoi(v string) (int, error) {
	n, err := strconv.Atoi(v)
	if err != nil {
		return 0, fmt.Errorf("invalid integer: %w", err)
	}
	return n, nil
}

But I also note that strconf.Atoi already returned a perfectly good error message, so the value of the atoi function is adding in this scenario appears is low.

@xushiwei
Copy link
Author

xushiwei commented Jun 9, 2020

var ErrInvalidInteger = errors.New("invalid integer")

func atoi(v string) (int, error) {
	n, err := strconv.Atoi(v)
	if err != nil {
		ErrInvalidInteger? // use ErrWrap expression to track error stack
	}
	return n, nil
}

In this example you're using ? as shorthand for something like

return errors.Wrap(ErrInvalidInteger)`

ErrInvalidInteger? is also an example of expr?. It is an ErrWrap expression and it means:

val1 := ErrInvalidInteger
if val1 != nil {
	_ret_err = errors.NewFrame(val1, ...) // _ret_err is the last output parameter
	return
}

I believe the problem is not the need for a shorthand for errors.Wrap, it is the use of the named error value. Consider this replacement.

func atoi(v string) (int, error) {
	n, err := strconv.Atoi(v)
	if err != nil {
		return 0, fmt.Errorf("invalid integer: %w", err)
	}
	return n, nil
}

But I also note that strconf.Atoi already returned a perfectly good error message, so the value of the atoi function is adding in this scenario appears is low.

Here ErrInvalidInteger is only to show an error wrapping example.

@davecheney
Copy link
Contributor

Thank you for your reply. If I'm following correctly, you're proposing combining wrapping and control flow changes. This has been discussed in the try proposal, #32437, and rejected.As a meta comment, proposals that combine more than one thing are rarely successful. I suggest that you pare back your idea to a single suggestion.

@xushiwei
Copy link
Author

xushiwei commented Jun 9, 2020

Thank you for your reply. If I'm following correctly, you're proposing combining wrapping and control flow changes. This has been discussed in the try proposal, #32437, and rejected.As a meta comment, proposals that combine more than one thing are rarely successful. I suggest that you pare back your idea to a single suggestion.

I don't think I combine two things together. Gophers will use their error wrapping themself, as above example. Of course, I also wrap the errors, but it is for error control flow. It is required by error stack tracking. They are different things.

@xushiwei
Copy link
Author

xushiwei commented Jun 9, 2020

What is the goal of error handling? I think it is origin of our discussion.

@beoran
Copy link

beoran commented Jun 9, 2020

Since this idea comes from your Go+ programming language, it would be interesting to see large open source projects that have been written in Go+ to compare how easy they are to read versus plain Go code. Do you know of any examples of Go+ projects apart from the compiler itself?

@xushiwei
Copy link
Author

xushiwei commented Jun 9, 2020

Since this idea comes from your Go+ programming language, it would be interesting to see large open source projects that have been written in Go+ to compare how easy they are to read versus plain Go code. Do you know of any examples of Go+ projects apart from the compiler itself?

It have been implemented only for two days. See https://github.com/qiniu/goplus/releases/tag/v0.6.20

@seeruk
Copy link

seeruk commented Jun 9, 2020

Personally, I don't find this very clear. Right now error handling is verbose, yes - but it's extremely explicit. I strongly believe that this is one of the reasons that Go software can be so robust.

func add(x, y string) (int, error) {
	return strconv.Atoi(x)? + strconv.Atoi(y)?, nil
}

It's really not obvious what this does. You've described it as "return if err", but you're already in a return statement there? What happens if both error? Where is it returning? Can you add any context to the error yourself? The current situation is verbose here, that's for sure, but it is extremely clear and easy to understand what's happening (i.e. you wouldn't need to know Go to understand what was going on even).

func addSafe(x, y string) int {
	return strconv.Atoi(x)?:0 + strconv.Atoi(y)?:0
}

This one is quite interesting, but you can accomplish this already. It'd just mean defining your own function to handle it. As mentioned somewhere above, you'd have problems with functions that return multiple values with a more general approach like this if it were built into the language.

I think this kind of thing would be easier to tackle if Go had generics and could implement types like Maybe or Either to deal with errors; and if we had Tuple types it'd be much easier to build something that handles multiple return values too (in your own code at leasy, I think this is the difference really - all Go code wouldn't swap over to using containers like this... probably ever!)

I do really like the idea of automatically adding stack information, but I'd like to see that implemented in a way that means that stack frames were just added automatically any time an error is returned from something; I don't think new syntax would need to be introduced for that.

Overall, not really a fan of this, the Go+ README says "Less is exponentially more", I wouldn't disagree in this case, but I'd say the "more" that you're getting is just "more confusion". IMO you do actually get more out of the current error handling because it's so vanilla and easy to grok.

@hantmac
Copy link

hantmac commented Jun 9, 2020

@seeruk Agree with you

@xushiwei
Copy link
Author

xushiwei commented Jun 9, 2020

https://github.com/golang/proposal/blob/master/design/go2draft-error-handling.md

Compared to check..handle err, in fact expr? is very similar to check expr (including operator priority), except that expr? records error frame.

Of course, handle err can use something like github.com/pkg/errors to track caller stack, but in fact it still lose some useful information to location root cause. And the most important thing is most of Gopher didn't call errors.Wrap before they had to location an error. This is why I think recording error frame should be done by the compiler.

So, expr? is same as check expr.

What will happen when we use check expr, but doesn't use handle err? For example:

func add(a, b string) (int, error) {
	return check strconv.Atoi(a) + check strconv.Atoi(b), nil
}

I think it should be equal to:

func add(a, b string) (int, error) {
	handle err { return 0, err }
	return check strconv.Atoi(a) + check strconv.Atoi(b), nil
}

And expr! is equal to use handle err { panic(err) }. For example:

func add(a, b string) int {
	handle err { panic(err) }
	return check strconv.Atoi(a) + check strconv.Atoi(b)
}

@davecheney
Copy link
Contributor

This is why I think recording error frame should be done by the compiler.

No. The frame should be recorded by errors.New or fmt.Errorf. That is the thing to address for Go2.

@xushiwei
Copy link
Author

xushiwei commented Jun 10, 2020

This is why I think recording error frame should be done by the compiler.

No. The frame should be recorded by errors.New or fmt.Errorf. That is the thing to address for Go2.

If check doesn't respond to record error frame, we have to do it in handle err. So every function that have errors have to write in the following form:

func foo(...) (..., _ret_err error) {
    handle err {
         _ret_err = errors.Frame(err, ...)
         return
    }
    ...
}

And then, handle err should not be omitted, just because of error frame tracking. I think it's boring for Gophers.

If we think check responds to record error frame, Things become graceful:

func foo(...) (..., _ret_err error) {
    handle errWithFrame {
         return ..., errWithFrame
    }
    ...
}

@davecheney
Copy link
Contributor

davecheney commented Jun 10, 2020

I’m not sure where your going with check, it’s not a thing. The check / handle proposal mutated into try then was withdrawn.

@xushiwei
Copy link
Author

I’m not sure where your going with check, it’s not a thing. The check / handle proposal mutated into try then was withdrawn.

I think error frame tracking is required by error control flow, not a user stuff. This is the essence.

@davecheney
Copy link
Contributor

I’m not sure I agree. My thesis is if errors.New and fmt.Errorf captured the stack where they were called then most of the use case for wrapping would not be needed. I see this as independent of control flow syntactic sugar.

@xushiwei
Copy link
Author

xushiwei commented Jun 10, 2020

I’m not sure I agree. My thesis is if errors.New and fmt.Errorf captured the stack where they were called then most of the use case for wrapping would not be needed. I see this as independent of control flow syntactic sugar.

Let's consider error processing style from an example:

func foo(...) (..., error) {
     if somthing error {
         return fmt.Errorf(...) // also for errors.New("...")
     }
     ...
}

..., err := foo(...)
if ??? { // how to check error kind?
    // do error processing
}

It's difficult to check error kind when we using fmt.Errorf(...). Why? I think It's because fmt.Errorf is a various kind of error frame recording.

In fact, I always use the following error processing style:

var ErrSomething = errors.New("...")

func foo(...) (..., error) {
     if somthing error {
         return errors.NewFrame(ErrSomething, ...)
     }
     ...
}

..., err := foo(...)
if errors.Is(err, ErrSomething) {
// or: if errors.Cause(err) == ErrSomething, here errors.Cause is provided by `github.com/pkg/errors`
    // do error processing
}

That is, errors.New should be used for a global error variable initialization, and don't call it to create a dynamic error object. Don't use fmt.Errorf forever, use errors.NewFrame instead.

So, if we standardize error frame recording and do it automatically, then it becomes the following:

var ErrSomething = errors.New("...")

func foo(...) (..., error) {
     if somthing error {
         return ErrSomething
     }
     ...
}

..., err := foo(...)
if errors.Is(err, ErrSomething) {
    // do error processing
}

@ianlancetaylor
Copy link
Contributor

Don't forget that fmt.Errorf using %w allows for errors.Is. https://golang.org/pkg/fmt/#Errorf

@xushiwei
Copy link
Author

Don't forget that fmt.Errorf using %w allows for errors.Is. https://golang.org/pkg/fmt/#Errorf

Thanks. And this proves fmt.Errorf is a various kind of error frame recording. So it becomes if we should standardize error frame recording or not.

@Splizard
Copy link

@davecheney

No. The frame should be recorded by errors.New or fmt.Errorf. That is the thing to address for Go2.

I think it would be neat to have access to compile-time trace values to avoid stack-trace overhead.
IE. line number, function name, package as a builtin.
These values could be passed to fmt.Errorf or errors.New

@xushiwei
I don't see any advantages this proposal has over #39372
Errors are hidden, control flow is hidden, the syntax is tied to specific function signatures.
Having shorthand for panic encourages developers to panic on errors which really should be avoided if possible. I don't understand why there is a default value shorthand in your example when the returned value from strconv.Atoi is already zero.

For example:

func add(x, y string) (int, error) {
	return strconv.Atoi(x)? + strconv.Atoi(y)?, nil
}

func addSafe(x, y string) int {
	return strconv.Atoi(x)?:0 + strconv.Atoi(y)?:0
}

(with #39372) could be written as:

func add(x, y string) (int, error) {
	xi, err := strconv.Atoi(x); err.return
	yi, err := strconv.Atoi(y); err.return
	return xi + yi, nil
}

func addSafe(x, y string) int {
	xi, _ := strconv.Atoi(x)
	yi, _ := strconv.Atoi(y)
	return xi + yi
}

Granted, you won't get a trace of the error but as discussed above, that issue can be addressed without syntax changes.

@davecheney
Copy link
Contributor

I think it would be neat to have access to compile-time trace values to avoid stack-trace overhead, IE. line number, function name, package as a builtin.

Why? Errors shouldn't be performance sensitive, the cost of the error is often eclipsed by the cost of recovering from the error; undoing actions, removing temporary files, rolling back transitions, shutting down processes, restarting pods, etc. In that context, the generation of the error itself should be negligible.

Addendum, in the Go 1.12 ish timeframe @mpvl suggested that rather than the stack trace leading up to the error, we could capture the program counter at that point; this is extremely cheap, its one word to store and the pc is right there in a register. This isn't the stack trace leading up to the error, but it is the point at which the error occurred. Granted, this isn't as good as capturing the full stack trace, but I argue its much better than no caller information at all and ameliorates the concerns about expensive object construction in the error path.

@ianlancetaylor
Copy link
Contributor

The suggested '?' operator is very similar to the try proposal (#32437). Instead of

f := try(os.Open(filename))

with this proposal one would write

f := os.Open(filename)?

In both cases an expression can cause an immediate return from a function if it returns a non-nil error. One of the main reasons that the try proposal did not catch on was the hidden control flow: the appearance of try in a complex expression could cause it to return from the function. The same argument seems to apply here. In fact, ? seems even smaller and easier to miss than try.

The ! could be handled by a ? using a defer statement in the function. Also Go doesn't really encourage using exceptions to handle errors (https://golang.org/doc/faq#exceptions), but the suggested ! seems very close to that.

If this proposal has strong support, should we go back to the try proposal?

@carnott-snap
Copy link

One can implement something similar to @ianlancetaylor's suggestion today in go: errors.Check.

@ianlancetaylor
Copy link
Contributor

Based on the discussion above, including the terse syntax and some similarities to the rejected try proposal, this is a likely decline. Leaving open for four weeks for final comments.

@ianlancetaylor
Copy link
Contributor

No further comments.

@golang golang locked and limited conversation to collaborators Apr 6, 2023
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