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: aggregate error handling & defer syntax sugar #34403

Closed
latitov opened this issue Sep 19, 2019 · 9 comments
Closed

proposal: Go 2: aggregate error handling & defer syntax sugar #34403

latitov opened this issue Sep 19, 2019 · 9 comments

Comments

@latitov
Copy link

@latitov latitov commented Sep 19, 2019

Aggregate error handling

I re-open an issue #34140 I myself closed a week ago because it seemingly stuck in a dead-end; Now, I have a solution.

A lot of people already tried to raise a question to improve close-to-perfect error handling in Go in one way or another, so I apologize twice that I do it not just once again, but a second time in a month. And still. I am quite new to Go, started using it very intensely just recently, and there is this thing that so annoys me, that I literally sleep and dream what can be done to that.

Context and The Problem

I totally agree to the consensus that it's already good as it is now. However, there's this quite typical case, where the same code is typed all over again, polluting the screen, and making what otherwise would fit in half the screen, to span two screens. Here it is:

f, err := excelize.OpenFile("./BestRestaurantsInLA.xlsx")
if err != nil {
	fmt.Println(err)
	return
}
//
cellVal, err := f.GetCellValue("Japanese", "A2")
if err != nil {
	fmt.Println(err)
	return
}

more example:

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

w, err := os.Create(dst)
if err != nil {
	fmt.Println(err)
	return
}
defer w.Close()

if _, err := io.Copy(w, r); err != nil {
	fmt.Println(err)
	return
}

As you can see, the same thing is repeatedly typed again and again. And it's not just typing, it also consumes valuable space on the screen, and disperses the focus of attention.

More example:

dbinfo := fmt.Sprintf("user=%s password=%s dbname=%s sslmode=disable",
	DB_USER, DB_PASSWORD, DB_NAME)
db, err := sql.Open("postgres", dbinfo)
if err != nil {
	panic(err)
}
defer db.Close()

fmt.Println("# Inserting values")

var lastInsertId int
err = db.QueryRow("INSERT INTO userinfo(username,departname,created) VALUES($1,$2,$3) returning uid;", "astaxie", "研发部门", "2012-12-09").Scan(&lastInsertId)
if err != nil {
	panic(err)
}
fmt.Println("last inserted id =", lastInsertId)

fmt.Println("# Updating")
stmt, err := db.Prepare("update userinfo set username=$1 where uid=$2")
if err != nil {
	panic(err)
}
res, err := stmt.Exec("astaxieupdate", lastInsertId)
if err != nil {
	panic(err)
}
affect, err := res.RowsAffected()
if err != nil {
	panic(err)
}
fmt.Println(affect, "rows changed")

fmt.Println("# Querying")
rows, err := db.Query("SELECT * FROM userinfo")
if err != nil {
	panic(err)
}
...

Now, don't you see the pattern? Hope yes.

Analysis

  1. Truth be told, it's not always this way. A lot of times we really need to be able to handle errors individually. So, the ability to do so must not be affected.
  2. Still, it'd bee good to have some way to handle errors in a fall-down section, in aggregate way.
  3. There's a question how to best express this syntactically.
  4. And, technical challenges to do so, in existing language infrastructure.

In my previous proposal #34140 I suggested this syntax idiom to solve this:

	...
	f, err := excelize.OpenFile("./BestRestaurantsInLA.xlsx") !!!
	cellVal, err := f.GetCellValue("Japanese", "A2") !!!
	return
fail:
	fmt.Printf("myf1() failed: %v\n", _e)
}

And suggested to implemented it with goto. Sadly, it appeared that it's much complex than I thought at first. You can see the details in the #34140 proposal thread. I closed that, because seemingly it did not worth an effort. Today, however, I have a solution, which works, and solves all the problems outlined there.

The proposal:

  1. Aggregate error handling code at the end of the function. The function body would also serve as a block of scope for this (similar as it is done in Ada).
  2. Use the "!!!" keyword at the end of every statement where you may get an error, which you want to be handled in aggregated way. (Or otherwise handle it individually in classic way, if you wish so).

The above example of opening and reading BestRestaurantsInLA.xlsx file, can be implemented in existing Go this way:

package main

import (
	"fmt"
	"github.com/360EntSecGroup-Skylar/excelize"
)

func myf1() {
	var _e error
E:
	for {
		f, err := excelize.OpenFile("./BestRestaurantsInLA.xlsx")
		if err != nil {
			_e = err
			break E
		}
		err = f.SaveAs("./Book1.xlsx")
		if err != nil {
			_e = err
			break E
		}
		return
	}
	fmt.Printf("myf1() failed: %v\n", _e)
}

func main() {
	myf1()
}

Note, this actually compiles and works. And it solves all the problems outlined previously, including the scope shadowing, and limitations of goto, and absence of hoisting in Go.

The E explicit label is needed in case there are nested loops in a function, which also use break,

So what is left now, is to add some syntactical sugar to it.

Let's re-format it a little bit:

func myf1() { var _e error; E: for {
	f, err := excelize.OpenFile("./BestRestaurantsInLA.xlsx") ; if err != nil { _e = err; break E; }
	err = f.SaveAs("./Book1.xlsx") ; if err != nil { _e = err; break E; }
	return
}
	fmt.Printf("myf1() failed: %v\n", _e)
}

It's still essentially the same code, it compiles and works.

Now, let's make these substitutions "by preprocessor":

	"{e"	->	"{ var _e error; E: for {"
	"fail:"	->	"}"
	"!!!"	->	"; if err != nil { _e = err; break E; }"

Now we can write:

func myf1() {e
	f, err := excelize.OpenFile("./BestRestaurantsInLA.xlsx") !!!
	cellVal, err := f.GetCellValue("Japanese", "A2") !!!
	return
fail:
	fmt.Printf("myf1() failed: %v\n", _e)
}

That is it.

And it doesn't require any changes to the language inner workings, only this syntactic sugar.

Side note: someone raised a (weird, IMHO) issue-proposal to allow continue to restart a function. I have no idea what that can be good for, but here it makes possible: simply continue E.

This all is possible at these expenses:

  1. {e is a new keyword (token);
  2. fail: is not a label, but a keyword, looking like a label;
  3. _e is a "reserved variable". You can use it on your own, but better not;
  4. !!! is a new keyword too (which will break !!!bool, btw);
  5. E: is a reserved label, you can't use it for your own needs, if you're in a "{e"-function;

Side note: instead of fail:, we can instead use exception:, as it is in the Ada language. One might argue that abundance of "fail" words in a code isn't a good-looking thing.

Examples of how it might look like

I will basically take the (anti-)examples from the beginning of this proposal, and re-write it in a new syntax, one-by-one:

func somefunc() {e
	f, err := excelize.OpenFile("./BestRestaurantsInLA.xlsx") !!!
	cellVal, err := f.GetCellValue("Japanese", "A2") !!!
	...
	return
fail:
	fmt.Println(_e)
}

and

func somefunc() {e
	r, err := os.Open(src) !!!
	defer r.Close()
	
	w, err := os.Create(dst) !!!
	defer w.Close()
	
	_, err := io.Copy(w, r) !!!

	return
fail:
	fmt.Println(_e)
}

and

func somefunc() {e
	dbinfo := fmt.Sprintf("user=%s password=%s dbname=%s sslmode=disable", DB_USER, DB_PASSWORD, DB_NAME)
	db, err := sql.Open("postgres", dbinfo) !!!
	defer db.Close()
	
	fmt.Println("# Inserting values")
	
	var lastInsertId int
	err = db.QueryRow("INSERT INTO userinfo(username,departname,created) VALUES($1,$2,$3) returning uid;", "astaxie", "研发部门", "2012-12-09").Scan(&lastInsertId) !!!
	fmt.Println("last inserted id =", lastInsertId)
	
	fmt.Println("# Updating")
	stmt, err := db.Prepare("update userinfo set username=$1 where uid=$2") !!!

	res, err := stmt.Exec("astaxieupdate", lastInsertId) !!!

	affect, err := res.RowsAffected() !!!
	fmt.Println(affect, "rows changed")
	
	fmt.Println("# Querying")
	rows, err := db.Query("SELECT * FROM userinfo") !!!

	...
	return
fail:
	panic(_e)
}

Imagine how it would affect the code, if there are not two, but (as it is typically) 4..10 of them. You don't need to imagine, you can see it here, how much shorter and more readable the code becomes.

And one more thing.

One might argue, that "all this is unnecessary, we love old good ways to do things". And I will answer by pointing him/her at the fact that Go already has quite a lot of various syntactic sugar, long present in Go, iota for example. Really, how many people use iota? And still, it's there. Probably the aggregate error handling sugar isn't any worse than that.

So please vote it up.

@gopherbot gopherbot added this to the Proposal milestone Sep 19, 2019
@gopherbot gopherbot added the Proposal label Sep 19, 2019
@latitov latitov changed the title proposal: Go 2: aggregate error handling proposal: Go 2: aggregate error handling & defer syntax sugar Sep 19, 2019
@latitov

This comment has been minimized.

Copy link
Author

@latitov latitov commented Sep 19, 2019

IF ALSO REPLACE THIS:

	defer func() {
		...
	}()

WITH THIS:

	defer {
		...
	}

(By "replace" I mean making it syntactically equivalent)

THEN the code example from Stackoverflow - How to read/write from/to file using Go? can be rewritten this way, please look at the picture:

Screenshot 2019-09-19 17 16 10

Clarification:

Not just a syntactically equivalent, but actually apply defer to a... chunk of code (possibly a block), without introducing actual function and function call. So that break E; inside !!! would break what it should break.

Id est this:

func main() {e
	fi, err := os.Open("input.txt") !!!
	defer { err := fi.Close() !!! }

	...
	return
fail:
	panic(err)
}

would unfold into this:

func main() { var _e error; E: for {
	fi, err := os.Open("input.txt") ; if err != nil { _e = err; break E; }
	defer { err := fi.Close() ; if err != nil { _e = err; break E; } }
	...
	return
}
	panic(err)
}

and then defer must defer that block of code at the end of the E: for scope... which is not how it is now in Go, as defer defers at the end of a function. Ghhmmmmm... Maybe that should be improved too? The picture looks good, isn't it?

But that should be a different proposal, then.

p.s.
To avoid breaking code with change of semantics of defer, we can leave it alone, and introduce new keyword, e.g. defer_bs:

func main() { var _e error; E: for {
	fi, err := os.Open("input.txt") ; if err != nil { _e = err; break E; }
	defer_bs { err := fi.Close() ; if err != nil { _e = err; break E; } }
	...
	return
}
	panic(err)
}

Where "bs" stands for "block-scope", not what might one think else.

@latitov

This comment has been minimized.

Copy link
Author

@latitov latitov commented Sep 29, 2019

There's yet another way to solve this in existing Go. This code:

package main

import (
	"fmt"
)

func myf1() (err_res error) {
	defer func() {
		if err := recover(); err != nil {
			err_res = fmt.Errorf("The myf1 failed: %v", err)
			fmt.Println(err_res)
		}
	}()
	//--
	fmt.Println("Ln1")
	fmt.Println("Ln2"); panic("Ln2 panicked.")
	fmt.Println("Ln3")
	return nil
}

func main() {
	defer func() {
		if err := recover(); err != nil {
			err_res := fmt.Errorf("The main failed: %v", err)
			fmt.Println(err_res)
		}
	}()
	//--
	fmt.Println("m1")
	fmt.Println("m2")
	err := myf1(); checkErr(err)
	fmt.Println("m3")
}

func checkErr(err error) {
	if err != nil {
		panic(err)
	}
}

It will print:

m1
m2
Ln1
Ln2
The myf1 failed: Ln2 panicked.
The main failed: The myf1 failed: Ln2 panicked.

In this, there's no problem with goto jumping over var definitions, no problem with scope, no nothing.... Except, it's ugly. It reminds me of Perl way of handling exceptions, which is:

eval {
     # code that might throw exception, e.g.
     #      die("something happened");
     1;  # always return true to indicate success
}
or do {
    # this block executes if eval did not return true (bacuse of an exception)
 
    # save the exception and use 'Unknown failure' if the content of $@ was already
    # removed
    my $error = $@ || 'Unknown failure';
 
    # report the exception and do something about it
};

The point is, it is already in the language, but its very existence is not obvious nor it is concise and clear in code what it is.

The above solution has these advantages (relative to the original solution of this proposal):

  • It can catch things like division by zero, not only explicit err returns; it catches everything;
  • It will enforce an exceptional situation; for example, with checking err only, it's possible to omit/forget the check somewhere, and it'll (possibly but not necessarily) cause other errors indirectly which will be handled; on the contrary, here any error, is required to be handled, probably few levels up the call stack, or else the program will crash.

And these disadvantages:

  • The exception handling code is counter-intuitively located at the top of the function;
  • Without syntax sugar, its existence is not obvious (unless documented somewhere), and it's ugly;
  • Intermittent calls checkErr() to just check the simple condition doesn't look efficient;
  • The code in main() is required to be slightly different than in any other function;

It's possible to add syntax sugar to it. Let's re-format the myf1() and main() functions:

func myf1() (err_res error) { defer func() { if err := recover(); err != nil {
			err_res = fmt.Errorf("The myf1 failed: %v", err)
			fmt.Println(err_res)
	}}()
	fmt.Println("Ln1")
	fmt.Println("Ln2")
	panic("Ln2 panicked.")
	fmt.Println("Ln3")
	return nil
}
func main() { defer func() { if err := recover(); err != nil {
			err_res := fmt.Errorf("The main failed: %v", err)
			fmt.Println(err_res)
	}}()
	fmt.Println("m1")
	fmt.Println("m2")
	err := myf1(); checkErr(err)
	fmt.Println("m3")
}

Now, if replace these (just like in original proposal):

	"{e"	->	"{ defer func() { if err := recover(); err != nil {"
	"b:"	->	"}}()"
	"!!!"	->	"; if err != nil { panic(err) }"

The "b" stands either for 'body' (as in Ada) or 'begin', as who likes how.

Then there'd be no longer checkErr() calls, and we can re-write the code this way:

func myf1() (err_res error) {e
	// error handling code here
	err_res = fmt.Errorf("The myf1 failed: %v", err)
	fmt.Println(err_res)
b:
	// normal function code here
	fmt.Println("Ln1")
	fmt.Println("Ln2")
	panic("Ln2 panicked.")
	fmt.Println("Ln3")
	return nil
}
func main() {e
	// error handling code here
	err_res := fmt.Errorf("The main failed: %v", err)
	fmt.Println(err_res)
b:
	// normal function code here
	fmt.Println("m1")
	fmt.Println("m2")
	err := myf1() !!!
	fmt.Println("m3")
}

In the end, it looks like in Go there are two alternative mechanisms to handle errors, panic()/recover()/defer(), and if err != nil {. Interesting.

P.S. few hours later.
Personally for my own needs, I am going to adopt and stick to this latter variant. Though the fail section is counter-intuitively on the top, generally it's better, because catches all kinds of faults, both returned errors, and panics. And I am going to implement it in this form:

func somefunc() (errres error) {panic:
	errres = fmt.Errorf("somefunc() failed: %v", err)
	fmt.Println(errres)
} {body:
	r, err := os.Open(src) !!!
	defer r.Close()
	
	w, err := os.Create(dst) !!!
	defer w.Close()
	
	_, err := io.Copy(w, r) !!!
	
	return nil
}

The translation will happen as part of a build process.

@latitov

This comment has been minimized.

Copy link
Author

@latitov latitov commented Sep 29, 2019

But there's more to it. It's not about handling errors, but is directly related to that.

What if I want to re-try an operation several times, probably with a delay, until finally failing? What if I want to ask an operator (read: user) if it should be re-tried once more?

There are certain fields, e.g. a distributed industrial IO (or distributed anything, when a connection can be spotty, but the operation to be performed is VERY IMPORTANT). For example, there is a fire on the floor plant, and some communications are broke, some are not, and mostly the field network work at 75% of data loss, and the safety logic requires that in such a situation that particular pump should be tried to be switched on no matter what. Or, if that's not possible, then an alternative action must be performed.

This can be performed with this idiom:

for ec := 0;; ec++ {
	err = switch_that_on(arg1, arg2)
	if err != nil {
		if ec > 15 {
			panic(err)
		}
		time.Sleep(100 * time.Millisecond)
	} else {
		break
	}
}

However, it could've been written this way, much clearer, with the expense of introducing a retry keyword:

err = switch_that_on(arg1, arg2) retry {
	if retry > 15 { panic(err) }
	time.Sleep(100 * time.Millisecond)
}

P.S.
It could've been possible to wrap this logic in a function, and pass the code to be executed as a function. Like this:

retry(switch_that_on(...), function () {
	if retry > 15 { panic(err) }
	time.Sleep(100 * time.Millisecond)
})

However, it looks not that nice, and isn't possible, because there's no way to re-invoke passed in function several times, with the same arguments pre-evaluated beforehand. Just like defer does, it first evaluates the parameters, and puts a ready-to-be-called record of a function call on the stack. There's no generally usable mechanism for that, and I am not sure if it's needed. Well, if it was present, then there would be possible (probably) to implement user-defined defer, maybe few, with different semantics, or whatever else.

Let's see what it would look like, hypothetically. Let's create a special new type, 'immutable, input parameters-evaluated, function call", "iipefc", and then:

var fc iipefc
fc = switch_that_on(arg1, arg2)

Now we could pass fc to any function, which will try to invoke it any number of times:

retry(fc, function () {
	if retry > 15 { panic(err) }
	time.Sleep(100 * time.Millisecond)
})

However, it's not clear how retry() will pass back return values, and called function will be called not in the original scope.

The original idea of:

err = switch_that_on(arg1, arg2) retry {
	if retry > 15 { panic(err) }
	time.Sleep(100 * time.Millisecond)
}

doesn't suffer from either of these problems.

@latitov

This comment has been minimized.

Copy link
Author

@latitov latitov commented Sep 29, 2019

Ironically, none of this could ever go into Golang for the very same reasons why the error handling in Go is still sane and simple. Namely, because of conservativeness of its designers (if I am right), and their drive to simplicity and "old-good ways" of doing things (C) and granularity of control.

That's good.

But, the existing error handling in Go isn't perfect, and here's why. I did some research on the Internet, and there are plenty of attempts over the years to offer some kind of improvement to it, and very few to improve, say, channels. That's an indicator, I think. An indicator that channels in Go are designed perfectly.

@latitov

This comment has been minimized.

Copy link
Author

@latitov latitov commented Oct 10, 2019

Hi, this is an "error-handling guy" again, with a little feedback from myself.

I actually tried to implement it in an actual production project, and it worked and works just nicely.

However, I stumbled upon the following observation / wanted feature: I got too many points of possible issuance of an error, with !!! syntax, or wrapping an error passed up from beneath, and it'd be good to have meaningful messages added at each of these points.

One way to create messages (or wrap passed up errors with additional message) was in panic block, this way:

	func Transact(db *sql.DB, txFunc func(*sql.Tx)) (err_res error) {panic:
		err_res = fmt.Errorf("Transact failed: %v", err)
		//      ^^^-------- the wrapping of an error passed from inside the body block
	} {body:
		...
		return nil
	}

And it works. But I also wanted that these also do the wrapping, for example on the calling side of Transact():

	err = Transact(db, func (tx *sql.Tx) {
		rows, err := tx.Query(`
			SELECT	* FROM sometable
		`, prc_id) !!!
		...
	}) !!!

So here it is - I have two !!!, and I want them to also wrap errors with meaningful info, not just pass an error up.

So, here's what I did:

	my $s = shift;
	$s =~ s/\{panic\:/\{ defer func\(\) \{ if err \:= recover\(\)\; err \!= nil \{/g;
	$s =~ s/\}\s*\{body\:/\}\}\(\)/g;
	$s =~ s/\!\!\!\s*(.*?)\s*$/; if err \!= nil \{ panic\(fmt\.Errorf\(\"$1: \%v\"\, err\)\) \}/g;  <--- HERE
	return "$s\n";

This is my "pre-processor", written in Perl, which does the transformation on each line of source code of Go program. The line I marked, basically transforms this, e.g.:

`, prc_id) !!! Querying tbl list failed

to this:

`, prc_id) ; if err != nil { panic(fmt.Errorf("Querying tbl list failed: %v", err)) }

thus automatically wrapping an error.

In short, here's an example of production code that is possible with these improvements:

err = Transact(db, func (tx *sql.Tx) {
	rows, err := tx.Query(`
		SQL QUERY HERE...
	`, prc_id) !!! Querying tbl list failed
	...
}) !!! Step 2 failed
...
db, err := sql.Open("postgres", psqlInfo) !!! Can't connect to DB
err = db.Ping() !!! DB ping failed
...
tmpl, err := template.ParseFiles(templfile) !!! Can't load template file
printer, err := prt.Open(printername) !!! Fail opening a printer
...
err = printer.StartPage() !!! Fail starting print page (label)
_, err := printer.Write(text.Bytes()) !!! Fail printing data

and so on and on. It's like a stack trace, but differently formatted, and as convenient, probably more.

This is an example a real-life error reported, as seen on the terminal:

The main failed: Querying tbl_id_list failed: pq: column pd.points_to_id does not exist

Which means that PostgreSQL erred with "pq: column pd.points_to_id does not exist", at "Querying tbl_id_list failed", at main. Debugging becomes a no brainer. The code looks clean and tidy.

The main failed: Step 1 failed: Transact failed: : Rollback because of: PARCEL NOT FOUND?: sql: no rows in result set
@latitov

This comment has been minimized.

Copy link
Author

@latitov latitov commented Oct 16, 2019

As long as this is not part of the Go language, it has this limitation:

for rows.Next() {
	row, err := rows.Columns() !!! Can't get row columns of Sheet1 of xlsx file
	rows_cnt ++

This works. But, unfortunately, there's no way I can add rows_cnt into the error message, like so:

for rows.Next() {
	row, err := rows.Columns() !!! Can't get row %(rows_cnt)v columns of Sheet1 of xlsx file
	rows_cnt ++

Simply because:

  1. The message is transformed before compilation... WELL, I AM WRONG, it's not a problem, because in the end it'll as is go into the ""-string inside fmt.Errorf().
  2. The real reason this won't work, is because Go doesn't have (as of now) way of interpolate vars into the string inline (e.g. #34174). Another reason to have that, though.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 29, 2019

Similar proposals, such as #33002, have been discussed at length before. See those issues for more discussion. Any new proposal in this area should consider those earlier issues and offer something new and distinctively better. Also, this specific proposal does not seem to have aroused much interest in the community. For these reasons, this is a likely decline. Leaving open for four weeks for further comments.

@latitov

This comment has been minimized.

Copy link
Author

@latitov latitov commented Oct 30, 2019

I understand you will close it after 4 weeks.

However, I would note, that it's not just another "error handling proposal", like the one you mentioned, the #33002. I actually went a long way comparing what was proposed before.

The fundamental difference of this particular proposal from the one you mentioned, the #33002, is that that is a syntax just-a-proposal, and this one offers a solution how to achieve that, and also this one really works, at least in one project. I demonstrated that to make it work all is needed is a per-line regex translation of source code, even with zero modifications to the Go compiler.

It works.

It is convenient.

You are free to close it. Because you all are tired of steady flow of proposals to improve error handling, and because no one cared to support this one.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 26, 2019

No change in consensus.

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