-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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: local-only throw-catch error handling #48896
Comments
If we wanted to make the syntax both more succinct and more unique to Go, would could call the exceptions "traps" and use the trigger "or" to "trigger" traps. Here without chaining: func CopyFile(src, dst string) error {
r, !err := os.Open(src) or failedPrep
defer r.Close()
w, !err := os.Create(dst) or failedPrep
_, !err := io.Copy(w, r) or failedCopy
!err := w.Close() or failedClose
trap failedPrep {
return fmt.Errorf("copy %s %s: %v", src, dst, err)
}
trap failedCopy {
w.Close()
os.Remove(dst)
return fmt.Errorf("copy %s %s: %v", src, dst, err)
}
trap failedClose {
os.Remove(dst)
return fmt.Errorf("copy %s %s: %v", src, dst, err)
}
} And if we wanted to support chaining, we might complement the "or" trigger with an "and" trigger: func CopyFile(src, dst string) error {
r, !err := os.Open(src) or fail
defer r.Close()
w, !err := os.Create(dst) or fail
_, !err := io.Copy(w, r) or closeDst
!err := w.Close() or removeDst
trap closeDst {
w.Close()
and removeDst
}
trap removeDst {
os.Remove(dst)
and fail
}
trap fail {
return fmt.Errorf("copy %s %s: %v", src, dst, err)
}
} Note that these keywords better supports the use of verbs to name exceptions (traps). We should probably avoid using the keywords "handle" and "handler", because these are likely common identifiers in existing code bases. |
Heck, the catch (or trap) code blocks could actually be macro-expanded where exceptions are thrown (or traps are triggered), eliminating the need to treat them as closures. |
To me the examples in the proposal are overwhelmingly more confusing than the traditional alternative. |
Back when the discussions for error handling were lively, there was a proposal to improve goto, so it could handle cases like this. (At present, goto won't let you goto a section of code where new variables have been declared.) This is very similar to those proposals. The effect ends up looking like Ruby, in which methods end with a rescue block. Having read through all those debates, the conclusion I came to is that there's not really any way to improve Go's error handling besides just adding |
@carlmjohnson, thanks for the thoughtful reply. I'm thinking that the more complicated the error handling, the more obscure the behavior for which the function was created. I agree that the benefit is minimal in this short example. The more often that later code depends on prior variables, the more you'll either be nesting the primary code path within It just seems to me that mixing error handling with the main path is at odds with writing "self-documenting" code. Sure, code precisely documents what it's intended to do even with error handling mixed in, but when we write a paragraph explaining what a function does, we describe the non-exceptional path first and then enumerate how exceptions are handled. It helps to see what we're trying to do before seeing how we handle everything that can go wrong. |
Also, dealing with complex error handling by moving it out to separate functions just moves error handing farther from the code to which it pertains, violating the principle of keeping error handling near the error it handles. Without a separate mechanism like this, we might stick with this Go principle only for simple cases and violate this Go principle for lengthy cases, akin to dealing with exceptions that unwind the stack and put error handling far from where it belongs. A mechanism like this can help us better stick to principles overall. |
The current proposed syntax somehow feels a little bit off however in regard to the quoted comment, one of the benefits of having all error handling not inline is better readability of the happy path. Often there are functions where the actual work is lost in a sea of inline error handling. |
Playing around with some demo client code I wrote, I think I prefer the following:
Here's some code illustrating how we can use exception names to clearly name the errors inline while still moving lengthy, distracting error descriptions to the end of the function. It should suffice for the compile to just do macro-expansion. const (
loginURL = "https://some.source.com/context/login/"
refererURL = "https://some.source.com/"
apiURL = "https://some.source.com/api/specify/"
)
type collectionsJSON struct {
Collections map[string]int `json:"collections"`
}
type loginJSON struct {
UserName string `json:"username"`
Password string `json:"password"`
Collection int `json:"collection"`
}
type Connection struct {
CsrfToken string
Referer string // (sic)
Cookies []*http.Cookie
}
func Connect(collectionName, username, password string) (*Connection, error) {
// Visit login page to get the CSRF token and collection ID
req, !err := http.NewRequest("GET", loginURL, nil) or noInitialRequest
res, !err := http.DefaultClient.Do(req) or noInitialPage
var csrfToken string
cookies := res.Cookies()
for _, c := range cookies {
if c.Name == "csrftoken" {
csrfToken = c.Value
break
}
}
if csrfToken == "" {
throw noCsrfToken
}
body, !err := ioutil.ReadAll(res.Body) or noInitialPageBody
defer res.Body.Close()
var result collectionsJSON
!err = json.Unmarshal(body, &result) or failedParsingInitialBody
collections := result.Collections
var collectionID int
foundCollection := false
for name, id := range collections {
if name == collectionName {
collectionID = id
foundCollection = true
break
}
}
if !foundCollection {
throw collectionNotFound
}
// Login
loginInfo := loginJSON{
UserName: username,
Password: password,
Collection: collectionID,
}
loginInfoJSON, !err := json.Marshal(loginInfo) or failedParsingLogin
req, !err = http.NewRequest("PUT", loginURL, bytes.NewBuffer(loginInfoJSON))
or noLoginRequest
req.Header.Set("Content-Type", "application/json")
req.Header.Set("X-CSRFToken", csrfToken)
req.Header.Set("Referer", refererURL)
for _, cookie := range cookies {
req.AddCookie(cookie)
}
res, !err = http.DefaultClient.Do(req) or noLoginResponse
if res.StatusCode < 200 || res.StatusCode >= 300 {
throw failedLogin
}
body, !err = ioutil.ReadAll(res.Body) or failedParsingLoginBody
defer res.Body.Close()
cookies = res.Cookies()
return &Connection{
CsrfToken: csrfToken,
Referer: refererURL,
Cookies: cookies,
}, nil
// Handle exceptions
catch noInitialRequest {
return nil, fmt.Errorf("failed to create login page request: %v", err)
}
catch noInitialPage {
return nil, fmt.Errorf("failed to receive login page response: %v", err)
}
catch noCsrfToken {
return nil, fmt.Errorf("did not receive a CSRF token")
}
catch noInitialPageBody {
return nil, fmt.Errorf("failed to read login page body: %v", err)
}
catch failedParsingInitialBody {
return nil, fmt.Errorf("failed to unmarshall [%s]: %v", body, err)
}
catch collectionNotFound {
return nil, fmt.Errorf("failed to find collection \"%s\" in %v",
collectionName, collections)
}
catch failedParsingLogin {
return nil, fmt.Errorf("failed to marshal login info: %v", err)
}
catch noLoginRequest {
return nil, fmt.Errorf("failed to create auth request: %v", err)
}
catch noLoginResponse {
return nil, fmt.Errorf("failed to receive auth response: %v", err)
}
catch throwFailedLogin {
return nil, fmt.Errorf("login failed with status code %d", res.StatusCode)
}
catch failedParsingLoginBody {
return nil, fmt.Errorf("failed to read auth body: %v", err)
}
} |
IMO, mixing error handling with the primary code path makes the intention of code harder to understand. I have expend some effort to filter for what the code is supposed to do, when I could otherwise have determined this at a glance. I've become used to this benefit in other programming languages. I feel like I'm backsliding. I guess the design team needs to decide whether or not to add a feature that might bring more programmers in the Go camp, despite most Go programmers apparently saying they are not interested in the feature. As far as I can tell, looking through the history of responses to error handling proposals, most of the community is against separating error handling from the main code path, regardless of the details of the error handling proposal. Should mixed error handling be mandatory coding philosophy across all projects? Or should it be optional per project? |
I don't know that I've seen a strong preference for keeping error handling with the main code path as such. I've seen a strong preference for simplicity and clarity. With this proposal, I'm inclined toward @carlmjohnson 's comment at #48896 (comment). You've written |
Based on the discussion above and the emoji voting, this is a likely decline. Leaving open for four weeks for final comments. |
No further comments. |
For the record, it wouldn't be a "goto" due to closure requirements. As explained above, this would best be implemented as macro expansion. In any case, I find several aspects of Go at odds with writing self-documenting code:
My experience with this thread suggests that the community in general does not value self-documenting code. |
Or perhaps we disagree as to what self-documenting code looks like. |
I'll take ownership of my statement: I, a veteran programmer of 40 years, find myself unable to write code as clear in Go as I can in many other languages. Maybe it's my shortcoming, but this inclines me to avoid Go for future work. |
It's definitely true that Go is an opinionated language. Not everybody is going to agree with the choices that it makes. I apologize for any suggestion that this might somehow be your shortcoming. I only meant to say that people can disagree about these points. |
Oh no need for apology. Your point was fair, even if I don't understand how any of these things are beneficial to code clarity or even neutral in regard to clarity. For example, I am quite fond of the flexibility that left-side targeting provides, and I hadn't complained about its readability before, but I did feel inclined to identify the trend that pushes me away from Go. |
My suggestion. func CopyFile(src, dst string) error {
r := os.Open(src) catch
defer r.Close()
w := os.Create(dst) catch
_ := io.Copy(w, r) catch { w.Close(); os.Remove(dst) }
w.Close() catch { os.Remove(dst) }
catcher (err error) {
return fmt.Errorf("copy %s %s: %v", src, dst, err)
}
} Could even pass arguments to the catcher: func CopyFile(src, dst string) error {
r := os.Open(src) catch ("open src")
defer r.Close()
w := os.Create(dst) catch ("create dst")
_ := io.Copy(w, r) catch ("copy src to dst") { w.Close(); os.Remove(dst) }
w.Close() catch ("close dst") { os.Remove(dst) }
catcher (msg string, err error) {
return fmt.Errorf("%s error: %v", msg, err)
}
} I removed Edit:Another option: func CopyFile(src, dst string) error {
r := os.Open(src) catch
defer r.Close()
w := os.Create(dst) catch
io.Copy(w, r) catch { w.Close(); os.Remove(dst) }
w.Close() catch { os.Remove(dst) }
return nil
} catch (err error) {
return fmt.Errorf("copy %s %s: %v", src, dst, err)
} Or yet: func CopyFile(src, dst string) error {
r := os.Open(src) catch
defer r.Close()
w := os.Create(dst) catch
io.Copy(w, r) catch { w.Close(); os.Remove(dst) }
w.Close() catch { os.Remove(dst) }
return nil
catch (err error):
return fmt.Errorf("copy %s %s: %v", src, dst, err)
} |
@ofabricio I love it! But I suspect the compiler is going to need a clue to look ahead for the This thread is dead, so if you want people to consider the proposal, you'll need to start a new thread. Unfortunately, I think the community is dead set on rejecting alternative error handling, no matter the proposal. |
@jtlapp I think I'll only link this issue in other issues so people can reopen this one if they like this idea. Im not very excited about proposing this myself. Anyway, I took a http handler I have and converted it using this catch solution to see how it goes. And this is the result: With
|
With a proper IDE, I could live with this. |
This is a response to the 2018 request for error handling proposals.
My objective is to make the primary/expected code path clear at a glance, so that we can see what a function normally does without having to mentally filter out the exceptional processing. This is also the only thing I'm uncomfortable with about Go, as I'm absolutely in love with Go's implementation of OOP. However, an extension of this solution supports chaining, so we could also address the issue of redundant error handling code.
Under this proposal, we would implement a local-only throw-catch feature that does not unwind the stack. The feature has no knowledge of the
error
type, relying instead on '!' to indicate which variable to test for nil. When that variable is not nil, the local-only "exception" named for the assignment is thrown.catch
statements at the end of the function handle the exceptions. The body of eachcatch
statement is a closure scoped to the assignment that threw the associated exception, so that the variables declared at the point of the assignment are available to the exception handler.Here is how the example from the original RFP would look. Mind you, the code would fail to compile if a thrown exception is not caught within the same function or if a caught exception is not thrown within the same function.
For reference, here is the original code from the RFP:
If we wanted to support chaining in order to reduce redundant error handling, we might allow something like this:
If we did allow chaining, then to keep the error handling code clear, we could require that a throw within a catch must precede the subsequent catch, so that execution only cascades downward through the listing. We might also restrict standalone
throw
statements to the bodies ofcatch
statements. I'm thinking that the presence of the '!' in the assignment statement should be sufficient to distinguish trailing throw clauses from standalone throw statements, should an assignment line be long and incline us to indent its throw clause on the next line.We can think of the '!' as an assertion that we might read as "not" or "no", consistent with its usage in boolean expressions, asserting that the code only proceeds if a value is not provided (
nil
is suggestive of not having a value). This allows us to read the following statement as "w and not error equals..." or "w and no error equals...":A nice side-benefit of this approach is that, once one understands what '!' is asserting, it's obvious to anyone who has ever used exceptions how this code works.
Of course, the solution isn't wedded to '!' nor to the keywords
throw
orcatch
, in case others can think of something better. We might also decide to call what is thrown and caught something other than an "exception".UPDATE: I replaced the switch-like
case
syntax with separatecatch
statements and showed the possible addition of a chaining feature.UPDATE: I didn't see it at the time I wrote this, but the proposal #27519 has some similarities.
The text was updated successfully, but these errors were encountered: