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: "trap" keyword for func-wise error handling #56258

Closed
xrfang opened this issue Oct 16, 2022 · 20 comments
Closed

proposal: Go 2: "trap" keyword for func-wise error handling #56258

xrfang opened this issue Oct 16, 2022 · 20 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

@xrfang
Copy link

xrfang commented Oct 16, 2022

Author background

I am an experienced Go programmer who programmed in Go for over 10 years. I have also used many other languages such as PHP, JavaScript, ObjectPascal (Delphi), as well as loads of others which I am not so proficient or have not been using them for decades.

Related proposals

This is a proposal about Go's Error Handling. I have seen many proposals on this topic, for example: #56165 (try statement), #37141 (pass statement), most of them focus on simplifying the infamous go boilerplate:

if err != nil {
    return err //or do something else, the "handler"
}

This proposal is different in that it is a schema about default error handling, which I think may not only simplify coding, but also improve code quality in the long run.

Proposal

A new keyword trap should be added in order to define a "default" error handler for all error-generating statements, inside a func. It could be used in any of the following forms:

  1. trap &error
  2. trap func(error)
  3. trap
func Func1() (err error) {
    trap &err
    f, _ := os.Open("some file")
    defer f.Close()
    ...
}

func Func2() (ok bool) {
    trap func(err error) {
        fmt.Println("ERROR:", err)
        ok = false
    }
    ok = true
    taskWhichReturnsErr()
}

func Func3() {
    trap //or, trap nil
    f, _ := os.Create()
    defer f.Close()
    //...
}

The essence of this statement is that it automatically catches error, in the following two cases:

  1. if any statement below it generated an unhandled error (i.e. panics)
  2. if any statement below it returns an error, as the only or last return value, which is deliberately ignored by "_", i.e. f, _ := os.Open(...), or implicitly, by not assigning to any variable, i.e. f.Close().

In the Func1() example, trapped error is assigned to the err variable, in Func2(), passed to the handler, and in Func3() panic-up to the caller. It is worth pointing out that the last case, direct panic-up is especially useful, because of the trap facility is designed to automatically check for errors that is ignored by the programmer, e.g. does not check the return of f.Close(), os.Rename(), db.Exec()...

Compatibility

This feature is supposed to be 100% compatible with Go1, because it ONLY works in case the programmer does NOT handle error. Consider the following case:

func Func1() {
    trap
    f, err := os.Open()
    _ = err //even if there are errors, it will NOT be trapped!
    defer f.Close() //if Close() generated error, it WILL be trapped,
                    //unless the error is explicitly assigned to variable other than "_"
}

Orthogonality

This feature may be implemented by the compiler using mechanism similar to "defer". It may be used multiple times, and be mixed with "defer".

Further Improvement

Although not mandatory, I think the trapped err should by default include a stacktrace to make it useful, just like errors.WithStack() in the errors package. I propose the following interface:

TracedError interface{
    Err() error //the underlying/direct error
    Error() string
    Stack() []string //returns stack information of the error
}

Another reason that the error should be naturally stack-traced is that the panic/recover mechanism accept/returns an interface{}, not error. The trap mechanism may be implemented as:

func Func() {
    defer func(v interface{}) {
        switch e := recover().(type) {
        case nil:
        case *error:
            *e = trace(fmt.Errorf("%v", v)) //change "any" into error, add trace info
        case func(error):
              e(trace(fmt.Errorf("%v", v)))
        default: //compiler report error
        }
    }
}

The above sample shows one difference/incompatibility between defer and trap: trap accept an argument, while defer not.

Alternatively, another keyword might be used: trace to indicated that the trapped error should have stack information, like so:

func Func1() {
    trap func(err) {...} //this err does not have stack information
    ...
}

func Func2() (err error) {
    trace &err //this err has stack information
}

Personally, I think trapped error should always include stack information.

Costs

This proposal may not make Go easier or hearder to learn, and the cost is minimal, for the sake of the language specification.

As to the impact of the compiler or the tool chain, I think it indeed may have some work to do, especially on the tool chain, sorry I don't know which tool should be modified to add such feature, but I would expect VS Code (the language server?) should warn the programmer if it sees code such as f.Close() without assigning
its return value:

error returned by f.Close() is silently ignored, consider add "trap", or check error values explicitly.
@gopherbot gopherbot added this to the Proposal milestone Oct 16, 2022
@seankhliao seankhliao added LanguageChange v2 A language change or incompatible library change error-handling Language & library change proposals that are about error handling. labels Oct 16, 2022
@ianlancetaylor
Copy link
Contributor

I think it's fairly surprising that assigning a value to _ can cause the flow of control to change. Nothing else in Go works like that. Flow of control is always indicated by a keyword or a statement that calls panic.

@xrfang
Copy link
Author

xrfang commented Oct 18, 2022

I must say that I am purely an application level developer, with focus on cloud API. i.e., I am biased and am not proficient at performance, low level things or language design e.g. generics. After I submitted this proposal, I read carefully about the "try" proposal, and to be honest, I mostly agree with that proposal. It is a pity that the "try" proposal is rejected.

I don't know what are the concerns of the community, for me, the most important language level concern is that it should be
succinct as well as expressive. Lots of the proposals on error handling (including the "try" one) focus on simplify code writing by introducing syntactic sugar, but my point is that it should provide "default" behavior so that programmer do NOT have to write error handling again and again. Although ugly and inconvenient, I think the try-catch-finally wrapper in other language does exactly that -- the programmer needs only write once (per function) all exceptions are trapped.

I must emphasis again that I am not sensitive about application performance, I trust that to the go team to improve the compiler and architecture of Go ecosystem. However, I found myself write this kind of code again and again:

func deleteNetAssign(na netArgs) {
	cli := http.Client{Timeout: 10 * time.Second}
	addr := fmt.Sprintf("%s/api/cfg/net/%s?name=%s", cf.ServerAddr, na.Ident, na.Name)
	req, err := http.NewRequest("DELETE", addr, nil)
	assert(err)
	resp, err := cli.Do(req)
	assert(err)
	defer resp.Body.Close()
	var bs bytes.Buffer
	_, err = io.Copy(&bs, resp.Body)
	assert(err)
	if resp.StatusCode != http.StatusOK {
		ll2.Emit(http.StatusInternalServerError, bs.String(), resp.Status)
	}
	var hr ll2.HttpReply
	assert(json.Unmarshal(bs.Bytes(), &hr))
	if hr.Code != http.StatusOK {
		panic(hr)
	}
}

You can see how many "assert" I have used in this short piece of code. In essence, when writing code you literally have to check errors wherever there could be one. BTW, long time ago I have an informal proposal which is informally rejected, that is adding the assert(any) built-in function, which simply means if e != nil { panic(e) }.

Returning to your comment, @ianlancetaylor , I don't think assigning value to _ causing flow control change is unacceptable. Actually, the try function and many other proposals does exactly that. If this is one reason that these proposals are rejected, then I think it is an opinion of the core team that you try to avoid another mechanism that changes flow-control "implicitly"?

One final comment on this: I think certain level of implicit behavior is good for code quality, as stated in my proposal, the most important benefit of my proposal is to catch errors that are ignored by inexperienced programmers, for example, many of them do not have the habit to check errors when calling os.Rename, io.Copy etc.

@ianlancetaylor
Copy link
Contributor

The fact that try led to unexpected change of flow of control was indeed one of the main reasons that it was rejected. See #40432.

Being succinct is generally good, but it's not the only goal. In general Go aims to optimize for the person reading the code, because most code is read more often than it is written. We don't optimize for the person writing the code. So all else being equal, writing less code is good. But it's more important that the resulting code be clear and easy to understand for a reader who is not familiar with it. You want to write fewer assert calls, but the assert calls are clear to me, even though I know nothing about the code.

@xrfang
Copy link
Author

xrfang commented Oct 18, 2022

@ianlancetaylor ok, I understand your concern, but I want to emphasize situations that I and my colleagues encounter countless times are, for example:

  • forget to check error on i/o or db related operations
  • invalid type assertion on JSON decode result (esp. map[string]any or expected string but is actually nil etc.)

What is your opinion on how the language design itself should help programmers on making less mistakes, or, relief cognitive overload?

As to bias toward reader or writer of the code, I 100% agree that go should be "plain", by avoid using symbols everywhere (although I believe the C ternary operator is loved by many), I do think that help programmer write bug free code is also important, just like the design of Go and my other languages to have automatic memory management.

@gregwebs
Copy link

@xrfang you should consider using my try library. It implements close to what trap &err is here as try.Handle* in the library. There is no magical underscore: you still do have to throw the error with try.Try or try.Check. But that is quite similar to the assert you are putting in your code now, but with the library will get to return the error as a value instead of panicking it. I think you might find that a magical underscore isn't that necessary for succinct code. The problem with the magical underscore for me is that it doesn't scale to providing an error annotation (fmt.Errorf(": %w", err)), so it ends up discouraging annotation. This would also be true of a special statement for assignment like try x := f().

A nice benefit of the try library is things mentioned/implied as benefits of this proposal:

  • errors get stack traces added
  • panics get annotated as well

I like this proposal but unfortuantely it is dead on arrivale because the Go maintainers have decided (based on community feedback) that an early return should be done by something quite obvious like a keyword statement.

@xrfang
Copy link
Author

xrfang commented Oct 20, 2022

@gregwebs thank you for try library, I will take it a look, as well as the original library yours is forked from. To be honest, what you'v written is exactly same in concept with my assert mechanism. I have not published a library, because I personally think it is a bit heavy to rely on another package to do error handling that is so frequently required.

As to annotation, I did something like if e := recover(); e != nil { err = trace("this is an error: %v", e) }, where the trace function will add the stack info I require. In later version of my function, I simplified it to trace(e), because, I think stack tracing is much much more important than annotation.

I don't know what is the criticism on try-catch (as in Java or C++) by the Go designer and the community. But I think the most headache Go's error handling caused me is lack of stack trace, even if you do assert(err) after _, err := io.Copy(...), you will not know exactly where inside io.Copy() does the error occur. This is critical for non-library functions.

If anybody is reading this, please kindly point me to any literature about why Go does not like try-catch -- I personally think the defer function in Go is very nice, much better than try-catch, If Go changes from treating error as value, but always panics, and let the programmer "recover" when required, it would be better for me.

@atdiar
Copy link

atdiar commented Oct 20, 2022

@xrfang

https://go.dev/doc/faq#exceptions

Also note that you can use the runtime library to add the stack trace information to an error, the line it occured at and so on if you want.

Go handles errors in a more fine grained way. Try-catch does not allow to reason about domain errors easily: it treats everything as a program error which is too coarse and thus confusing since the handling occurs out of natural program flow.

@gregwebs
Copy link

@xrfang my try library uses the errors library underneath, which is a fork of the original pkg/errors library whose goal was really just to add stack traces via wrapping. There is proposal work done by the Go team to add stack traces to errors. It is a shame that it hasn't been followed through on. Although the wrapping standard means we can do a decent job solving the issue in the library space.

You might put your code out on github, not for others to depend on as a library, but just so that people like myself can look at it.

I am very much in agreement with Go on avoiding exceptions, but I think the Go FAQ on exceptions does not actually state the negative effect of exceptions.
The problem is that exceptions say: unwind the call stack until you find a spot where this can be caught. The thrower now has an arbitrary effect of stack unwinding on the program. This is okay for errors that should terminate the program, but not okay for normal errors that we want to handle and recover from. Code can now come crashing down if a library adds a new exception type that is not caught. The effect can only be made non-arbitrary with checked exceptions. Using checked exceptions ends up being a heavy-weight way of just returning an error as Go and Rust do. Rust does the same thing but has more affordances: a Result type and an early return mechanism (the ? suffix operator). Rust programmers seem to be pretty happy with this. A bigger issue has been figuring out how to reduce the overhead of returning concrete error types. Go handles this by almost always returning the error interface and mostly just annotating with strings, and there are some Rust libraries like anyhow that do the same.

@xrfang
Copy link
Author

xrfang commented Oct 21, 2022

@gregwebs here is my errors.go I didn't release it simply because I think using a library for error handling is too heavy, and that's why I am reluctant to adopt pkg/errors, or your try library. Long before this file is created, I created several error handling helpers for example, aux.Assert() which did the same thing. However I don't like any package name to prefix it, I want my code to be just assert() or Assert()... I even wrote a Makefile to copy errors.go from up-directory to any packages and rename the package name accordingly...

In my errors.go, the TracedError interface has a Describe() function, which does the work of "adding domain specific information". Also, the assert() function accept usage like this: assert(cnt > 0, "count must be positive, it is now %d', cnt)
These are all good, however, in my view, does not tackle my pain point -- automatic stack tracing.

I personally do NOT agree with the criticism that try-catch makes flow control a mess, or it should only be used on exceptions that are not handle-able, and should terminate the program. In lots of my applications, especially those doing background data processing in a standalone goroutine, it is absolutely vital to catch all exceptions and handle them nicely, even for things like network outage, I want my program to keep trying, and automatically resume after error condition vanishes.

Anyway, throw-try-catch is not meant to be the only way to handle errors, especially in Go context. In fact panic is 100% inevitable, typical non-environmental cause is to use invalid index on a slice. Why the Go people are so afraid of try-catch? :-) @ianlancetaylor

@gregwebs
Copy link

here is my errors.go

Thanks, it is interesting for me to see these implementations. It is also interesting to see how different Go programmer place some kind of hard constraint on error handling. In your case the contraint seems that panics should be handled very precisely.

However I don't like any package name to prefix

I guess that is why my library is named just try- only 3 characters :)
But can't you just do an import as _ to avoid any prefix?

the TracedError interface has a Describe() function, which does the work of "adding domain specific information"

That is interesting, but since it requires a TracedError that comes from assert are you doing this in a defer block?

even for things like network outage, I want my program to keep trying, and automatically resume after error condition vanishes.

I am able to do that with the existing error system. Panics make this more difficult for me because the program can now potentially be unwound first before the retry.

Why the Go people are so afraid of try-catch

This proposal is dead on arrival. Give my try library a try- it seems to have everything you want from this proposal or your library (just missing a boolean assert, we could figure out how to add that). And I don't think there is any downside for you since you are already expecting panics (the main downside is forgetting a try.Handle handler and accidentally panicking instead of returning an error. I think the only reason you would need the Go language to do something is because you don't want to bother with using named return variables. But _ works there so you really just need to name err.

@xrfang
Copy link
Author

xrfang commented Oct 21, 2022

@gregwebs I do use defer to handle errors and I love it. This is the most important, correct thing in Go's error handling. Actually try-catch is not required, because defer is exactly the Go version of try-catch. What I wanted but Go didn't have is exactly stated in this proposal -- automatic panic, not programatic panic, because if the programmer does panic(err) he/she cannot trace down to the exact point where error occurs! Also, by analyzing the stack trace, I can know high level cause (route) of the error, so, augments are not so necessary!

Here I invite you to take a look at my minimalistic logging package with a Catch() method does similar to your try.Handle() :-)

@gregwebs
Copy link

arguments are not so necessary

It is quite useful for example to add a database ID. Then you can lookup the db state that caused the panic and can reproduce more quickly. Strictly speaking not necessary since you can deduce all possible edge cases, but sometimes it makes things go a lot faster.

my minimalistic logging package with a Catch() method does similar to your try.Handle()

Yes, it is the same except that I re-throw normal panics after annotating them. Your package returns them as errors. I had a version of Handle that did exactly that, but the problem is that a program using the try library might end up not printing out the stack trace of the error (by using "%v" instead of "%+v"). Whereas if I rethrow the panic the program of the library is still going to crash and print out the stack trace.

It is interesting to see different library authors independently coming up with the same creative solutions.

@kiqi007
Copy link

kiqi007 commented Oct 24, 2022

@gregwebs I'm sorry, I may not understand the community culture very well, this is the first time I participate in the discussion, I take the liberty to ask a question:
All discussions can actually be achieved by adding a logical branch to lexical analysis or syntax analysis.
Has the go community discussed adding an entry point for lexical analysis and parsing that allows users to add extra processing to their own go?

@gregwebs
Copy link

@kiqi007 not that I know of. There are hacks to have macros in Go. There is of course the Go generate tool and tools for analyzing and modifying go source code (look into Go fix and linters). The failpoints tool alters Go code and then reverts it when testing is completed.

@kiqi007
Copy link

kiqi007 commented Oct 25, 2022

@gregwebs
I know that tools like go-linters are used for pre-compilation and therefore will change the code forever.

Just if possible, add user entry points for lexical analysis or syntax analysis at compile time.
This would allow the user to change some token to another token, or, alternatively, to enter a sequence of unparseable tokens into the user's registered plugin, thus returning a parseable syntax tree by the user or nil for unparseable.

This allows the user to implement the syntactic sugar they expect

@xrfang
Copy link
Author

xrfang commented Oct 25, 2022

@kiqi007 I guess your idea is not compatible with go culture. As @ianlancetaylor pointed out, Go language (and compiler?) is aiming at optimize for reader, not writer.

I have no idea at all about compiler design, however, I think the idea of macro and compile time switches are very useful and some of these already exists in Go. If your idea could be used to add "built-in" functions (which are actually user-defined functions), for example, my assert() function, which can be used just like the print() built-in (without explicitly declaring it in every go project), it will be very nice.

@kiqi007
Copy link

kiqi007 commented Oct 25, 2022

@gregwebs
My idea is just that go provides users with the ability to intervene in lexical and syntactic analysis by way of plugins, so that they can change/modify the syntactic tree and thus influence the final execution of the program.

For example, when the syntax analysis reads:
try hander(args)
the go compiler does not recognize this syntax and passes it to the user plugin, which returns a new syntax tree (or token list) representing the statement:
if err ! = nil { return hander(args) }
This will allow users to implement their own syntactic sugar, or even macros.

This is similar to using go-linter-tools to change the code before go compile: replace all try hander() in the code with if err ! = nil {return hander()}, but without permanently changing the code file.

Therefore,for the user, it reads as if the go language had the try/trap keyword

@kiqi007
Copy link

kiqi007 commented Oct 25, 2022

@xrfang
Just as you would expect from the built-in function assert(args), when the go compiler reads.
do assert(args)
There is no such built-in method, so do assert() will be passed to the user plugin, which returns a new syntax tree:
func(args) { // your assert impl }
Finally the go language executes as if it had a built-in function assert().

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Nov 2, 2022

This proposal changes the meaning of _ significantly if there is a trap keyword. It also changes control flow in a surprising way, merely by the absence of an error variable, or using _ as an error variable. Currently change in control flow is always indicated by a keyword or a call to the builtin function panic; it doesn't happen implicitly.

Similar ideas have been proposed like the try proposal and met with considerable resistance (the try proposal didn't have trap, but you could use defer; it did have unexpected control flow).

Therefore, this is a likely decline. Leaving open for four weeks for final comments.

@ianlancetaylor
Copy link
Contributor

No further comments.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Dec 7, 2022
@golang golang locked and limited conversation to collaborators Dec 7, 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

7 participants