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: error handling with functions and an error return? #27567

Closed
gregwebs opened this issue Sep 8, 2018 · 55 comments
Closed

proposal: Go 2: error handling with functions and an error return? #27567

gregwebs opened this issue Sep 8, 2018 · 55 comments
Labels
error-handling Language & library change proposals that are about error handling. Proposal
Milestone

Comments

@gregwebs
Copy link

gregwebs commented Sep 8, 2018

Background (go 2 Process)

go 2 has laid out the problem of error handling (Please read before reading this proposal).

I am told that alternative proposals should be raised as git issues here. Please add anyone else you think would like to join the discussion.

Introduction

It is amazing to see the error handling problem being properly addressed. The existing draft proposal is good, but I think we can iterate to make it better.
To avoid confusion by comparison to the existing proposal, I will avoid mentioning it in this one.
However, if you are a supporter of the existing proposal, please separately read my critique of it.

It's useful to take a step back and clearly state what we are trying to do with our implementation:

  • provide an abstraction that allows for the insertion of a return statement for errors.
  • compose handler functions together before they are used with the error return

In the existing go language, I cannot write a handler function which will create an early return from the function.
There are a few approaches that use existing languages features for this control flow:

  • Macros (e.g. Rust originally used a try! macro).
  • Ruby supports anonymous functions that return from the enclosing function (method)
  • exceptions
  • Sequencing code with short-circuits. Some usage of monads in Haskell are a particularly good example of this.

For sequences with short-circuits, see the errors are values post for how this can be done in go. However, this severely alters how one would write go code.

Proposal: handlers as functions, just a special check

Lets repeat our goals again:

  • provide an abstraction that allows for the insertion of a return statement for errors.
  • compose handler functions together before they are used with the error return

Composition can be handled with ordinary functions that take and return an error.

That means we just need a mechanism to insert a return.
For early return in my proposal, I will use a question mark operator ? rather than a check keyword. This is for two reasons

  • the operator can be used postfix, which has readability advantages
  • the original draft proposal used check, but it functions differently, so this may help avoid confusion.

See "Appendix: Operator versus check function" for a discussion on using ? or a check keyword.

Implementation as syntactic expansion

v := f() ? handler

expands to

v, err := f()
if err != nil {
    return Zero, handler(err)
}

Where handler is a function that takes an error and returns one. Zero is the zero value for the (success) value returned before the error, assuming the function returns a single success value. A function that returns 4 values, the last one being an error, would have.

    return Zero, Zero, Zero, handler(err)

This is a simple, easy to understand transformation. It is easy to underestimate the value from being able to understand the usage site without searching for context. I am trying to avoid comparisons to other proposals, but I want to say that none of the others I have seen can be described this simply.

All of the transformation is performed entirely by ?. It inserts the nil check, the return, and creates the needed zero values. The handler is just a normal function and an argument to ?.

For some small convenience in writing cleanup handlers, the return section would actually expand to this:

    return Zero, handler.ToModifyError()(err)

See the section on handler types and the appendix section on ThenErr and ToModifyError.

Basic example from the original proposal, re-written

Putting this together, lets re-write SortContents, which wants different handlers in different places.

func SortContents(w io.Writer, files []string) error {
    handlerAny := func(err error) error {
	return fmt.Errorf("process: %v", err)
    }

    lines := []strings{}
    for _, file := range files {
	handlerFiles := func(err error) error {
	    return fmt.Errorf("read %s: %v ", file, err)
	}
	scan := bufio.NewScanner(os.Open(file) ? handlerFiles)
	for scan.Scan() {
	    lines = append(lines, scan.Text())
	}
	scan.Err() ? handlerFiles
    }
    sort.Strings(lines)
    for _, line := range lines {
	io.WriteString(w, line) ? handlerAny
    }
}

Let's show another example from the proposal (slightly simplified) that has handler composition:

func process(user string, files chan string) (n int, err error) {
    ahandler := func(err error) error { return fmt.Errorf("process: %v", err) }
    for i := 0; i < 3; i++ {
	bhandler := func(err error) error { return fmt.Errorf("attempt %d: %v", i, err) }
	do(something()) ? ahandler.ThenErr(bhandler)
    }
    do(somethingElse()) ? ahandler
}

It is possible to combine handlers in the same way one would combine functions:

do(something()) ? ahandler.ThenErr(func(err error) error {
	return fmt.Errorf("attempt %d: %v", i, err) }
)

Or

do(something()) ? func(err error) { return ahandler(bhandler(err)) }

The example uses a .ThenErr method (see appendix) as a way to compose error handler functions together.

Results

  • This alternative proposal introduces just one special construct, ?
  • The programmer has control and flexibility in the error handling.
  • Handlers can be naturally composed as functions
  • The code is much more succinct and organized than current go error handling code.
  • errors can be returned from defer.

Checking error returns from deferred calls

This alternative proposal can support returning errors from defer:

defer w.Close() ? closeHandler

Notes on error handler function types

To respond to errors we want to do one of two things:

  • cleanup (side-effecting): (error) -> nil or () -> nil
  • modify the error: (error) -> error

An error handler function must always have the type of the modifier, but we may not want the extra noise when writing a purely cleanup handler. The question mark operator can accept all forms. A cleanup function can be automatically converted to return the original error that would have been passed to it.

This is also true of helpers that compose error functions such as ThenErr.
See the Appendix section on ThenErr to see how this is implemented.

Appendix

Appendix: Handle and anonymous function syntax

This proposal is slightly more verbose than others that introduce a special anonymous function syntax that is lighter-weight and infers types.

handle err { return fmt.Errorf("process: %v", err) }

Without this syntax, the proposal would read:

handle func(err error) error { return fmt.Errorf("process: %v", err) }

I think it is worthwhile to explore having anonymous functions that are lighter-weight.
However, I think this should be usable anywhere rather than just with a single keyword.

But please leave this for another proposal rather than throw it in the mix with error handlers!

Appendix: unary and binary.

The question mark operator can be used as a unary to just return the exception without any handlers running.

something()?

This is equivalent to

something() ? func(err error) error { return err }

I am favoring writing the unary form without any spaces in this case (more similar to Rust), but we should use whatever syntax the community finds best.

Appendix: Handling errors within the handler itself

A cleanup handler may generate a new error that should be propagated in addition to the current error.
I believe this should just be handled by a multi-error technique, e.g. multierr.

Appendix: custom error types

The existing proposal seems like it would cast a concrete error type to the error interface when it is passed to a handler.
I don't think this proposal is fundamentally different.
I think this issue should be solved by the generics proposal.

Appendix: ThenErr and ToModifyErr

An implementation of ThenErr and ToModifyErr. See the syntax expansion section for how the ? operator uses ToModifyError.

type Cleanup func(error)
type CleanupNoError func()
type ModifyError func(error) error

type ToModifyError interface {
	ToModifyError() ModifyError
}

func (fn1 ModifyError) ToModifyError() ModifyError {
	return fn1
}

func (fn1 CleanupNoError) ToModifyError() ModifyError {
	return func(err error) error {
		fn1()
		return err
	}
}

func (fn1 Cleanup) ToModifyError() ModifyError {
	return func(err error) error {
		fn1(err)
		return err
	}
}

// Its easier to write this once as a function
func CombineErrs(funcs ...ToModifyError) ModifyError {
	return func(err error) error {
		for _, fn := range funcs {
			err = fn.ToModifyError()(err)
		}
		return err
	}
}

// But method syntax is convenient
type ErrorHandlerChain interface {
	ThenErr(ToModifyError) ModifyError
}

func (fn1 ModifyError) ThenErr(fn2 ToModifyError) ModifyError {
	return func(err error) error {
		return fn1(fn2.ToModifyError()(err))
	}
}

func (fn1 Cleanup) ThenErr(fn2 ToModifyError) ModifyError {
	return func(err error) error {
		fn1(err)
		return fn2.ToModifyError()(err)
	}
}

func (fn1 CleanupNoError) ThenErr(fn2 ToModifyError) ModifyError {
	return func(err error) error {
		fn1()
		return fn2.ToModifyError()(err)
	}
}

Appendix: Operator versus check function

The original proposal rejected the question mark and gave some reasons why.
Some of those points are still valid with this proposal, and others are not.

Here is another proposal that I believe advocates the same solution proposed in this alternative, but with a check function. I would be happy with that as a solution, but below I give my preference for ?.

The original proposal had just one argument given to check. This alternative favors the question mark in large part because there are now 2 arguments.
The original proposal states that there is a large readability difference in these two variants:

check io.Copy(w, check newReader(foo))
io.Copy(w, newReader(foo)?)?

However, I think this is a matter of personal preference. Once there is a left-hand-side assignment, the readability opinion may also change.

copied := check io.Copy(w, check newReader(foo))
copied := io.Copy(w, newReader(foo)?)?

Now lets add in a handlers and check our preference again.

copied := check(io.Copy(w, check(newReader(foo), ahandler), bhandler)
copied := io.Copy(w, newReader(foo) ? ahandler) ? bhandler

I believe ? will be slightly nicer to use due to

  • fewer parantheses
  • putting error handling solely on the right-hand-side rather than both the left and right.

Note that it is also possible to put all the error handling on the left-hand-side of the error source.

copied := check(bhandler, io.Copy(w, check(ahandler, newReader(foo)))

But I prefer keeping error handling on the right-hand-side for two reasons

  • a success result is still transferred to the left
  • it is possible to write an anonymous handler rather than being forced to declare it ahead of time

Appendix: built-in result type

A go programmer that has used Rust, Swift, Haskell, etc will be missing a real result type.
I would like to see a go 2 proposal for discriminated unions which includes a result type.
However, I think both the original proposal and this alternative proposal would work fine with the addition of a result type.
This is because go effectively already has a result type when dealing with errors. It is a tuple where the last member is of type error.

A future version of go with discriminated unions should be able to use ? for dealing with a discriminated union result type.

Appendix: intermediate bindings for readability

Error handling on the right-hand-side may increase line length undesirably or seem to be easy to miss. Its always possible to use an intermediate binding.

v, err := f(...) // could be a million lines long
err ? handler

Appendix: left-hand-side

It is possible to support placing the handler on the left-hand-side.

v := handler ? f(...)

This could make more sense for check. One of the ideas behind this would be to emphasize the handler, for example in the case where f(...) is an enormous expression (see above section on intermediate bindings which is another way to handle this).

Appendix: returning the zero value

This proposal does not allow for the defensive practice of returning -1 as the success value, along with the error. Where -1 is useful because zero or a positive number are an allowed value in the problem domain, so someone may notice a -1 propagating. I don't think we need to support this use case for a few reasons:

  • It is not generally applicable anyways (consider a uint).
  • The contract of using the function is already that errors must be checked before looking at success values.
  • There are standard linters (errcheck) that will warn people about ignoring errors: we should instead ship this ability with go vet.

Appendix: all proposal examples re-written

Below are the rest of the code snippets shown in the original proposal, transformed to this alternative proposal.

func TestFoo(t *testing.T) {
	handlerFatal := func(err error) { t.Fatal(err) }
	for _, tc := range testCases {
		x := Foo(tc.a) ? handlerFatal
		y := Foo(tc.b) ? handlerFatal
		if x != y {
			t.Errorf("Foo(%v) != Foo(%v)", tc.a, tc.b)
		}
	}
}

func printSum(a, b string) error {
	handler := func(err error) error { fmt.Errorf("printSum(%q + %q): %v", a, b, err) }
	x := strconv.Atoi(a) ? handler
	y := strconv.Atoi(b) ? handler
	fmt.Println("result:", x + y)
	return nil
}

func printSum(a, b string) error {
	fmt.Println("result:", strconv.Atoi(x)? + strconv.Atoi(y)?)
	return nil
}

func CopyFile(src, dst string) error {
	handlerBase := func(err error) error {
		return fmt.Errorf("copy %s %s: %v", src, dst, err)
	}

	r := os.Open(src) ? handlerBase
	defer r.Close()

	w := os.Create(dst) ? handlerbase
	handlerWithCleanup := handlerBase.ThenErr(func(err error) {
		w.Close()
		os.Remove(dst) // (only if a check fails)
	})

	check io.Copy(w, r) ? handlerWithCleanup
	check w.Close() ? handlerWithCleanup
	return nil
}


func main() {
	handlerAll := func(err error) error {
		log.Fatal(err)
	}

	hex := check ioutil.ReadAll(os.Stdin) ? handlerAll
	data := check parseHexdump(string(hex)) ? handlerAll
	os.Stdout.Write(data)
}
@gopherbot gopherbot added this to the Proposal milestone Sep 8, 2018
@PeterRK
Copy link

PeterRK commented Sep 8, 2018

Operator ? looks less noticeable than "check".

@gregwebs
Copy link
Author

gregwebs commented Sep 9, 2018

@PeterRK you might want to state whether that is a good or a bad thing! I am assuming it is a critique.

One advantage of this proposal is that it is not breaking any new ground, but instead following the lead of Rust (but adding a handler component). So we could survey Rust users to see if noticeably of ? is a problem.

Although I have a preference for ?, I want to note that I would be perfectly happy with this proposal being accepted but modified to use check instead.

@PeterRK
Copy link

PeterRK commented Sep 9, 2018

@gregwebs You are right. Less noticeable may be an advantage.

@gregwebs
Copy link
Author

gregwebs commented Sep 9, 2018

@networkimprov you are right I should explicitly talk about how the question mark was mentioned in "considered ideas". In that, the case is made for check rather than ?.
Some of those points are still valid with this proposal, and others are not. This is reviewed in the section "Appendix: Operator versus check function".

I hope we can move the conversation from comparing ? to check (either of which are acceptable to me) to the bigger picture of using regular functions instead of special stacking.

@networkimprov
Copy link

There is no doubt in my mind that the next proposal draft from the Go team will add named handlers (or a func type) and drop implicit handler chains, given the feedback to date. It might drop check altogether.

However, that isn't enough IMO. My detailed critique: Golang, how dare you handle my checks!

@deanveloper
Copy link

I'll say this a million times - I hate exiting from a function without a return. This should never happen unless there is a catastrophic error (panic). Also, returning an error without adding on to the context should be discouraged. The unary ? is just a bad idea for Go in general. (Both returning implicitly AND not adding context to the error)

The rest of the proposal is interesting, but I'm not sure how much I like the idea of the ?. I think it means too many things in too many different languages, and it would add to the confusion. (Conditional operator (C), null-safe operation (Kotlin), Coallessing (C#), etc).

I also feel like the built-in check function approach feels more like Go. I like that better than the ?. You guys discussed it being "less noticable" which is "good", but I'd say the opposite. It's the potential exit to a function, it needs to be noticable to be maintainable.

Using check(...) instead of ? also resolves your "should we allow break and continue as the RHS? The answer: no.

Either way, this shouldn't be about syntax, syntax can be rethought. Let's discuss the idea behind it.

I think having different function signatures doing different things is an interesting idea, but I don't like it. I think it makes reading code confusing, especially at the call site ("check site"?) of the handler function. If I'm reading someone else's code, I don't want to have to scroll back up to the top of the function to see if my code continues or not. The nice thing about the handle/check construct is that you know that if the error is not nil, the function exits.

I do like this idea though. Those are my critiques, I like the rest of the proposal. The use of anonymous functions rather than handler blocks is a good idea in my book.

@deanveloper
Copy link

To expand on the whole function signature thing, here's what I mean:

func errorProne() error {
    handler := func(err error) {
        fmt.Println("unimportant error occurred:", err)
    }

    // 50 lines later

    // My internal monologue:
    // "Does this function exit if there's an error,
    // or does it continue execution?"
    f := check(os.Create("Foo"), handler)
}

Again, in the check/handle made it clear: if err != nil, the function will exit

@PeterRK
Copy link

PeterRK commented Sep 9, 2018

Error handling is a potential control flow changing point, so we care about it.

The design in original draft introduces a new control flow changing rule, what we call "special stacking" or "chained handler". That brings confusion more than convenience.

Some guys, include me, suggest to use a new control flow changing mark with normal function as error handler. However, how to implement this mark is controversial.

@networkimprov
Copy link

A named catch block after check/?/etc does the trick nicely :-)

?outer f1() // or more Go-like: #outer = f()
for ... {
   ?inner f2()
   catch inner { ?outer fmt.Errorf("loop %v", inner) } // no return
}
catch outer { return fmt.Errorf("context %v", outer) }

@PeterRK
Copy link

PeterRK commented Sep 9, 2018

@deanveloper I agree with that if err != nil, the function will exit. I believe exit on error is the common case. We should focus on the common case. If needing continue, just handle it with old style.

@networkimprov @gregwebs I know you guys want to figure out a solution covering all cases. I hope it will be a lightweight one. I think heavy solution is against the philosophy of GO. And the design in original draft is already too heavy to me.

@PeterRK
Copy link

PeterRK commented Sep 9, 2018

Error handling consists of handler and trigger.
Let me ask some questions.

  1. Should trigger be bound with one or more handlers explicitly?
    In the original draft, "check" is the trigger. It cannot be bound with any handler explicitly. So a matching rule is needed.

  2. Should handler be special or just a normal function?

  3. Should trigger be a filter or just a consumer?
    In the original draft, "check" is the filter. It take return values from child function, and filters out the error. But in code _, #err := function(), the trigger #err is just a consumer.

@networkimprov
Copy link

In #27519 (#id/catch model), #err = f() filters for non-zero, as stated.

@PeterRK
Copy link

PeterRK commented Sep 9, 2018

@networkimprov I mean that filter can work with pipe.

@networkimprov
Copy link

See link I posted above re "critique" for perspective on nesting calls that return error. (A "pipe" is an IPC or stream mechanism btw.)

@PeterRK
Copy link

PeterRK commented Sep 9, 2018

Good luck! @networkimprov

@gregwebs
Copy link
Author

gregwebs commented Sep 9, 2018

@deanveloper it seems you have misread the proposal. Perhaps I wrote too much, let me know how I can make the section on handler function types more clear. Currently it does state:
"A cleanup function will automatically be converted to return the original error that would have been passed to it."

If you write

    handler := func(err error) {
        fmt.Println("unimportant error occurred:", err)
    }

When used as a handler, the error will still be passed along (see the section on ThenErr to show how this can be accomplished).

In this proposal, the usage of check or ? always means that the function returns immediately if the error is not nil.

@gregwebs
Copy link
Author

gregwebs commented Sep 9, 2018

@deanveloper thanks for critiquing adding break/continue. I removed that section now because I don't like the idea either and it seems to distract from the proposal rather than to clarify.

@deanveloper
Copy link

Ah, I see. You are correct I did misread it, although now there's no explicit exit to the function. (See how that can get confusing to a reader? The function doesn't mark an exit, so I didn't think there was one)

Anyway, there shouldn't be a special case to allow a developer to return the error which occurred without additional context. If they want to do that, they should do it explicitly with return err

Also, assuming you want to return the zero value for the other numbers is a dangerous game. For instance, let's say I wanted to write the following function:

// Returns how many occurrences of find exist in the UTF8 encoded reader r
func CountOccurences(r io.Reader, find string) (n int, err error)

If an error occurs, I don't want to return n=0 because 0 is a valid return value of my function, I'd want to return n=-1.

check/handle does this well because the return in it's system actually returns to the function, so there's no assumptions about what you're trying to return.

Perhaps the handler should always be in the form (error) -> (parent function's return values). This kind-of destroys the idea of reusable handler generators (ThenErr), though.

@gregwebs
Copy link
Author

gregwebs commented Sep 9, 2018

@bmeh (or anyone else that comes along), it would be great if you left your reason for the thumbs down. The proposal has received a lot of useful critiques around the edge cases of language interaction.
But I actually have not yet seen a single critique of the core idea of this proposal, including outside this go 2 process where I have shown it to others.

@networkimprov similarly, it would be great to see critical comments of the core idea here and leave promotions of your proposal on the github issue that is already open for that.

@networkimprov
Copy link

I posted a link to a pure critique of check/handle, which largely applies to this proposal. It does not mention #id/catch. I urge you to read it.

I mentioned a catch block here as a solution to the control flow issue raised above, and used a prefix variation of your ? handler syntax with it.

@gregwebs
Copy link
Author

gregwebs commented Sep 9, 2018

@deanveloper zero values: thanks for bringing that up. This proposal is essentially for discriminated unions. That is, the non-error value should not exist. I know that use cases do exist for actually returning a tuple of two meaningful values. However, I believe they are rare enough (I have seen thousands of lines of go code that never do this) that it is a mistake to place them as a design constraint on an error handling system. One can still use one of two approaches:

  • use the existing style of error handling
  • use an error type that gives back the value you want

The latter looks something like this:

type CountOccurencesError struct {
    Count int
    Err error
}
func (e CountOccurencesError) Error() string { return e.Err.Error() }

// Returns how many occurrences of find exist in the UTF8 encoded reader r
func CountOccurences(r io.Reader, find string) (n int, err CountOccurencesError)

I believe you do need generics to be able to return the concrete type through an error check.

@gregwebs
Copy link
Author

gregwebs commented Sep 9, 2018

@deanveloper no unary form of the check: I would be okay with always requiring an error handler that adds context. But I thought always requiring a handler was probably too heavy-handed for a go community that is not already consistently doing that.

If you define a function identity, then you just have to write ? identity if you don't want to add anything to an error. So keep in mind it is easy to subvert the intent.

An additional consideration is that some users may be satisfied enough by using stack traces that they don't feel the need to add context in every passing of an error.

@deanveloper
Copy link

That's not what I'm trying to say here - what I'm saying is that the zero-value of int is meaningful in the CountOccurences function, so I would much rather return a -1 to make it clear that the function doesn't return meaningful information if an error occurs.

I want to be clear. I don't want the caller to see 0, err, as it could be mistaken for "zero occurrences were found before finding the following error", I want them to see values from the function indicating that the function does not return useful information (other than the error) if an error occurs, which can be done by returning -1, err.

Most of the time 0, err works, but in my experience, returning -1, err is not an uncommon case

@gregwebs
Copy link
Author

gregwebs commented Sep 9, 2018

@deanveloper sorry for missing your actual concern. I think your level of programming defensiveness is probably appropriate given the lack of discriminated unions in go and the prevalence of zero values. However, it seems not generally applicable (what if I have a uint?) and unnecessary. The contract is always that the caller must check the error value before looking at the success value. We shouldn't weaken this proposal because someone is going to ignore errors. There are linters that check for this (errcheck): it would be much more powerful to add that capability to go vet or otherwise have this statically checked.

@Azareal
Copy link

Azareal commented Sep 10, 2018

I personally think that handle is sugar for goto rather than an anonymous function.

It seems to be doing this:

func something() {
    var __reservedVar error
    {
    errHandle:
        return __reservedVar
    }
    blah, err := x()
    if err != nil {
        __reservedVar = err
        goto errHandle
        return
    }
}

aka

func something() {
    handle err {
        return err
    }
    blah := check x()
}

If you read it like that, the return makes perfect sense. Simplified example.

@deanveloper
Copy link

deanveloper commented Sep 10, 2018

The contract is always that the caller must check the error value before looking at the success value

This is a very fair point. Although I think that -1 is still a pretty common thing to return when an error occurs.

I've said this before, I really like the proposal. It feels very Go-like (at least when using a check built-in function), which is hard to come by for proposals not from the Go team themselves.

I added a +1. Sorry if it seems like I'm nitpicking it pretty hard, just want to make sure everything is considered, this is a really good proposal

I personally think that handle is sugar for goto rather than an anonymous function.

Yeah I was the same way. I saw handle as more of a goto than a function.

Although both views work and I can see it going both ways. I think it personally makes more sense as a goto (it's how it probably works under the hood, AND just works better in general when it comes to how it returns).

@gregwebs
Copy link
Author

@deanveloper thanks for the 👍 thorough review, and the good questions!
Also, please help me promote usage of errcheck/gosec so that we don't have to bend over backwards with defensive coding practices!

@ianlancetaylor is there a process to moving this proposal forward with more reviews?

@ianlancetaylor
Copy link
Contributor

There is a relatively slow moving Go2 proposal review process.

@ianlancetaylor
Copy link
Contributor

Let me expand on that to say that nothing is going to happen in a hurry. We're going to take the time required to make changes that seem good.

@ianlancetaylor
Copy link
Contributor

It sounds like this proposal is suggesting that the ? operator take a function as the right hand argument. That function is treated in an unusual manner: if that function returns, then the calling function, the one in which ? appears, returns. But orthogonality suggests that if ? can be used with an ordinary function literal, then it can be used with any function. What happens then? That seems very confusing, not to mention hard to implement.

func ahandler(err error) { return fmt.Errorf("process: %v", err) }

func process(user string, files chan string) (n int, err error) {
    for i := 0; i < 3; i++ {
	bhandler := func(err error) error { return fmt.Errorf("attempt %d: %v", i, err) }
	do(something()) ? ahandler.ThenErr(bhandler)
    }
    do(somethingElse()) ? ahandler
}

It's not so far fetched to think that if this capability is available, people will naturally want to consolidate their error handlers in the form of a function. When using handler from the design draft, this happens by writing something like

handle err { return unifiedHandler(err) }

With this one it seems unnecessary to do that at all. But does it work?

@gregwebs
Copy link
Author

@ianlancetaylor I understand that the go team will be thorough and take time with proposals as they should. I am just trying to understand if I am following the proposal process properly. As to your comments on the proposal:

I seems my proposal is hard to make it through parts without mis-reading. I think I can fix this by showing how it would actually be implemented in terms of syntax expansion. I will try that out, along with some re-wording, let me know how else I can make things more clear.

The ? operator always returns the error from the function if one is present, a handler cannot alter this fact, it can only run side-effects and/or alter the error that gets returned.
The right-hand-side function is treated the same way any function is treated in go. The special functionality comes only from the ? operator.

We could require that a handler always return an error. As a convenience (and I am open to the possibility that this is a bad idea), we can allow a handler function to not return the error. I call this type of handler a cleanup handler. That just means that we transparently upgrade the cleanup handler to return its error (see the ThenErr appendix section for how this can be done).

Yes, ? can be used with any function. The function must, however type check to avoid a compilation error. I agree user's will consolidate some handlers, but you actually lost me at the end. With this proposal one would write

f() ? unifiedHandler

@ianlancetaylor
Copy link
Contributor

I see, so the handler is passed the error, and can return a modified error, and that error is returned by the function using the ? operator?

(For a language change proposal I find that it's normally best to present the suggested changes by themselves as a standalone document, rather than by comparison to a different change.)

@gregwebs
Copy link
Author

@ianlancetaylor yes, I can see that the comparison is causing problems for everyone. I will remove it to a gist. I did now write a section "Implementation as syntactic expansion" that should hopefully make things very clear.

@gregwebs
Copy link
Author

@ianlancetaylor I think you got it now (should be easy to understand with the syntactic expansion section). I cleaned up the proposal by moving out critiques to a separate gist, let me know what else I can do to clarify things.

@PeterRK
Copy link

PeterRK commented Sep 14, 2018

I think our key goal is making the consequence of error handling more clear.
When I read a check or ?, I want to know two things:

  1. Which handler will be invoked.
  2. What will happen to current function after error handling.
    This proposal let me understand them without searching in code context.

@gregwebs @ianlancetaylor

@lonng
Copy link

lonng commented Sep 14, 2018

This is the most natural way for programmers who use the Rust language. There are some risks in introducing the 'check/handle' keywords.

@gomarcus
Copy link

I would hereby like to follow @freman's line of reasoning and would like to express my dislike of the new error handler proposals. Personally, I still consider the central principles of GO to be extremely precious and fear that the demands for syntactic sugar only lead to a dilution of the previous clarity and purity. The manual and explicit error checking/handling is in my opinion one of GO's core strengths.

Currently, type error is an interface like any other. A special syntax for handling errors would be a fundamental change.

Interesting observation
Many who start learning GO complain about the repetitive explicit error checking, but most get used to it and soon appreciate it in the vast majority of cases.

cases:

  1. Should the repeated use of if err != nil {…} be a visually disturbance for some users… a different color scheme in the editor could easily solved this problem for them…

  2. Should the introduction of error handler be focused on enrichment of error information… It may be better to think about why the received error messages are incomplete. It might be beneficial to improve the code of the error returning function, instead of creating new syntax to iron out the initial fault.

  3. @leafbebop argued that error handler may improve chaining. And even though this may be true in some cases, I would like to question the premise here. In my opinion, chaining does not necessarily result in less writing effort, more comprehensible structures nor easy to maintain program code.
    In addition, chaining is only possible if all functions involved have a exact argument order. This would result in chaining being used in some cases and the conventional way in others, creating two parallel paradigms.

  4. The proposal of @gregwebs suggests that one of the goals is reducing the program code and/or the writing effort for the programmer, as well as a new syntactic expression for reformatting a received error.
    It is particularly difficult for me to follow this reasoning, because it is already optional to use a formatting function and also the amount / readability of the program code is not improved.
    The proposal introduces a new function type type errorHandler func(error)error. However the proposal seems to disregard that such a formatting function has only a limited selection of information, which is a bad prerequisite as a formatting function (only e). Unless such a function would be specially incorporated in the calling function and get access to all variables (with all disadvantages).
    The use of errorHandler's not only changes the language appearance, but also the reading flow. This effect is reinforced by errorHandler's defined outside of the calling function body.

Current

func A() error {
   x, err := f()
   if err != nil {
      return fmt.Errorf("…%v…", err.Error())
   }
}

proposal of @gregwebs

func B() error {
   errorHandler := func(e error) error {
      return fmt.Errorf("…%v…", e.Error())
   }
   x := f() ? errorHandler
}

The confusion becomes more evident when different formatting functions are called.
Current

func A() error {
   x, err := f1()
   if err != nil {
      return fmt.Errorf("…%v…", err.Error())
   }
  y, err := f2(x)
   if err != nil {
      return fmt.Errorf("…%v…", err.Error())
   }
}

proposal of @gregwebs

func C() error {
   errorHandler1 := func(e error) error {
      return fmt.Errorf("…%v…", e.Error())
   }
   errorHandler2 := func(e error) error {
      return fmt.Errorf("…%v…", e.Error())
   }
   x := f1() ? errorHandler1
   y := f2(x) ? errorHandler2
}

@freman
Copy link

freman commented Sep 18, 2018

This is the most natural way for programmers who use the Rust language. There are some risks in introducing the 'check/handle' keywords.

But I want to code in the Go language, not the Rust language - or I'd be coding in the Rust language :)

Are our goals to be like other languages? Error handling Go results in the same questions and issues every time a new one of our devs picks it up. Over time, so long as they actually care to improve their craft they usually come to find it refreshing, especially as they learn of better ways of dealing with the errors.

Sure it's not entry level easy, and it can be repetitive... but visual basic has on error resume next which has to be the easiest least repetitive way to ignore errors create unreliable code beyond underscoring them in Go.

If I want more sugar, I can go back to perl where half of what I write in a maintainable way in Go can be squeezed into one line of code.

@deanveloper
Copy link

Should the repeated use of if err != nil {…} be a visually disturbance for some users… a different color scheme in the editor could easily solved this problem for them…

Syntax highlighting is never a solution to a problem and a programming language should NOT rely on syntax highlighting to make it readable.

Here are a few lines of code peeled from the draft:

func CopyFile(src, dst string) error {
	r, err := os.Open(src)
	if err != nil {
		return fmt.Errorf("copy %s %s: %v", src, dst, err)
	}
	defer r.Close()

	w, err := os.Create(dst)
	if err != nil {
		return fmt.Errorf("copy %s %s: %v", src, dst, err)
	}

	if _, err := io.Copy(w, r); err != nil {
		w.Close()
		os.Remove(dst)
		return fmt.Errorf("copy %s %s: %v", src, dst, err)
	}

	if err := w.Close(); err != nil {
		os.Remove(dst)
		return fmt.Errorf("copy %s %s: %v", src, dst, err)
	}
}

15 lines of error handling code, 5 lines of "what we want to do" code. That's why it's visually unappealing - there is way too much boilerplate and code repetition.

Go is all about writing programs that do what we want them to do, and it gets a lot harder to do that when we need at least 3 lines of code on every function call that even has a chance of returning an error.

@gomarcus
Copy link

gomarcus commented Sep 18, 2018

@deanveloper

Syntax highlighting is never a solution to a problem […]

If so, please tell me, why do so many programmers use syntax highlighting? Because they want to paint beautiful letters pictures?

15 lines of error handling code, 5 lines of "what we want to do" code. That's why it's visually unappealing - there is way too much boilerplate and code repetition.

It seems we do not agree on this point. I think it's an obsession and a relic from other programming languages to think error's are something out of the ordinary (special cases) and not part of the actual code.

Go is all about writing programs that do what we want them to do, and it gets a lot harder to do that when we need at least 3 lines of code on every function call that even has a chance of returning an error.

It seem to me that your use case or expectation for GO may be inappropriate. We dont start to adapt Go for the styling of web pages, as a replacement of CSS.

@deanveloper
Copy link

If so, please tell me, why do so many programmers use syntax highlighting? Because they want to paint beautiful letters pictures?

Because it's helpful. But that doesn't mean that a language should require syntax highlighting to be readable.

It seems we do not agree on this point. I think it's an obsession and a relic from other programming languages to think error's are something out of the ordinary (special cases) and not part of the actual code.

I agree that errors really shouldn't have special treatment in Go. It's nice that in Go errors are just values, but having all of the code repetition that we have is not okay and needs to be addressed. Code repetition leads to many issues, especially when it comes to refactoring. There is no current solution to the code repetition problem for handling error values, and that's what the check/handle construct is trying to aide.

Again, we only cared about 5 lines of code for that example I showed. But because we wanted to handle errors, we ended up needing to write 15 lines of code specifically to handle if something goes wrong.

It seem to me that your use case or expectation for GO may be inappropriate. We dont start to adapt Go for the styling of web pages, as a replacement of CSS.

When did I say anything like this? CSS isn't programming, and it shouldn't be considered programming. I never said anything about Go doing things outside of programming. What I am saying is that Go is meant to be a language where what we write is what happens when it's run, contrasted to lots of OOP languages which have extremely confusing type systems and constructs. (Also, stop spelling it GO, it's spelled Go.)

@gomarcus
Copy link

Because it's helpful. But that doesn't mean that a language should require syntax highlighting to be readable.

Go is absolutely readable in its current form. I just said, in case it's not for a minority group of people... they could use it... Let's drop this, it does not help the actual Diskursion...

It's nice that in Go errors are just values, but having all of the code repetition that we have is not okay and needs to be addressed. Code repetition leads to many issues, especially when it comes to refactoring.

Especially when refactoring functions, I can not grip your problem. Because using your example function CopyFile, would only require a single test. done!

err := CopyFile(a,b)
if err != nil {
   …
}

@gomarcus
Copy link

gomarcus commented Sep 18, 2018

[...] What I am saying is that Go is meant to be a language where what we write is what happens when it's run, contrasted to lots of OOP languages which have extremely confusing type systems and constructs. [...]

Well, if this is your opinion, I wonder why you want to change this! That's the whole point, let GO as it is!
Don't put a square peg in a round hole…

If something needs to be changed, add something useful ... like add generics… but dont rush it…

(Also, stop spelling it GO, it's spelled Go.)

miiiiaaaaauuuu 😄

@deanveloper
Copy link

Especially when refactoring functions, I can not grip your problem. Because using your example function CopyFile, would only require a single test. done!

I meant refactoring the CopyFile function. If I wanted to change the error message, I'd need to remember to change it in all 4 places.

Well, if this is your opinion, I wonder why this should change! That's the whole point, let GO GO be!
Don't put a square peg in a round hole…

Again, as I said, it really is nice for errors to just be treated as a value, but when we have an issue as bad as bad as how it is now (referencing code repetition), we need to weigh what matters more, and I believe that fixing the code repetition problem is more important than errors "not being special"

@gomarcus
Copy link

gomarcus commented Sep 18, 2018

…, but when we have an issue as bad as bad as how it is now (referencing code repetition), we need to weigh what matters more, and I believe that fixing the code repetition problem is more important than errors "not being special"

I completely disagree

question
If you do not care enough about the current error philosophy (if err != nil…) , why not use the defer version? This would also solve your 'refactoring' objection... No changes needed.

@deanveloper
Copy link

deanveloper commented Sep 18, 2018

If you do not care enough about the current error philosophy (if err != nil…) , why not use the defer version? This would also solve your refactoring objection... No changes needed.

Because now we've got 20 lines of error handling code, while still maintaining the 5 lines of code that we really care about. Sure it helps with error handling, but it doesn't help with the boilerplate issue.

Also, it completely falls apart when you want a handler for a specific scope (especially loops!) or if you want to return a different error halfway through the function.

Examples of how the defer solution fails:

// A solution which loops through.
func Loop(arr []string) (err error) {
    defer func() {
        if err != nil {
            return fmt.Errorf("loop arr(%q) error(%v)", arr, err)
        }
    }()
    for _, str := range arr {
        defer func(str string) { // this could be improved by moving outside of the loop, but that doesn't fix the scoping issue
            if err != nil {
                err = fmt.Errorf("inloop str(%q) err(%v)", str, err)
            }
        }(str) 
        err = errorProne(str)
        if err != nil {
            return err
        }
    }
    return anotherErrorProneFunc()
}

If we call Loop([]string{ "str0", "str1" }), and the last error prone function encountered an error, the output would look something like...
inloop str("str1") err(inloop str("str0") err(loop arr("str0" "str1") err("my source error")))

Under the handler structure, it would instead look like

func LoopHandle(arr []string) (err error) {
    handle err {
        return fmt.Errorf("loop arr(%q) error(%v)", arr, err)
    }

    for _, str := range arr {
        handle err {
            err = fmt.Errorf("inloop str(%q) err(%v)", str, err)
        }
        check errorProne(str)
    }
    check anotherErrorProneFunc()
    return nil
}

For the same input and same error location we get a more expected result:
loop arr("str0" "str1") err("my source error")

@gomarcus
Copy link

gomarcus commented Sep 18, 2018

Examples of how the defer solution fails:

I also read the proposals, but all examples do little change to my opinion. Both examples you put forward are specifically designed to provoke a constructed problem, knowing that both cases are far-fetched.

Moreover, the current standard way is still shorter and more elegant, then your second example.

func LoopHandle(arr []string) (err error) {
    for _, str := range arr {
        if err := f(str); err != nil {
           return fmt.Errorf("inloop str(%q) err(%v)", str, err)
        }
    }
    if err := f(); err != nil {
       return fmt.Errorf("loop arr(%q) error(%v)", arr, err)
    }
    return nil
}

@networkimprov
Copy link

@gomarcus, I encourage you to paste your first comment here into a gist linked from the feedback wiki, as it's mostly a critique of the error handlers concept, not the substance of this proposal :-)

@deanveloper, this issue isn't a forum to debate a commenter who took issue with handlers :-)

Regarding @gomarcus' critique of handlers, I have tried to document all the possible requirements for a new errors idiom (and thus benefits) on golang-dev.

@gregwebs
Copy link
Author

gregwebs commented Sep 18, 2018

Thanks @networkimprov for trying to keep the discussion on track: I would like to keep it focused on the proposal at hand. I see one point that is quite relevant to address: Is it bad to introduce this error handler concept if it doesn't scale down to simple usage? That is, if I don't need a shared handler, would I use this "improved" error handling? This is the benefit of the handler being a second argument to the check: you can write your handler inline without the overhead of naming it or registering it. So my version given in the above examples would still be:

func C() error {
   x := f1() ? func(e error) error {
      return fmt.Errorf("…%v…", e)
   }
   y := f2(x) ? func(e error) error {
      return fmt.Errorf("…%v…", e)
   }
}

I don't think this is inherently better than doing the traditional err != nil, but it is something you could do if you wanted to consistently do error handling the same way. Note that if you are not just side-effecting but also return error values, the ? will generate zero values for you also.

But this example is quite contrived as is: the real way you would write it is

func C() error {
   x := f1() ? prependToError("…")
   y := f2(x) ? prependToError("…")
}

func prependToError(format string, v ...interface{}) func(error) error {
    return func(e error) error {
        v = append(v, e)
        return fmt.Errorf(format + ": %v", ...v)
    }
}

This is why I believe it is important to use normal functions as handlers: function composition is such a powerful tool.

It is probably possible to come up with an example more like the first that doesn't benefit from handler helper functions. If go lang had a good anonymous function syntax (type inference and no return), I would still prefer ?.

func C() error {
   x := f1() ? fn(e) { fmt.Errorf("…%v…", e) }
   y := f2(x) ? fn(e) { fmt.Errorf("…%v…", e) }
}

@gomarcus
Copy link

Although I can understand the hint of @networkimprov, I refuse the accusation by @gregwebs that my objections had little to do with the proposal being discussed. The basic problems of the proposal remain.

@gregwebs
Copy link
Author

@gomarcus your objections are real. I only meant that some of them are not specific to this proposal (e.g. suggestions for adding special syntax highlighting, stating that the current verbosity is a non-problem, etc) but apply to essentially any proposal including this one. In that case we would prefer to keep it as general feedback on the wiki and just point to that on new error handling proposals instead of re-hashing the same conversations on every error handling proposal.

I did respond to the given code sample critique that was specific to the proposal. I know you did have some other specific points, but unfortunately I wasn't able to make sense of them (code is the clearest expression, nobody else was understanding my proposal until I wrote it in code).

@golang golang locked as too heated and limited conversation to collaborators Sep 18, 2018
@davecheney
Copy link
Contributor

It’s time for everyone to have a time out and cool off.

@rsc
Copy link
Contributor

rsc commented Sep 19, 2018

The right way to submit feedback and alternate designs like this is to post it somewhere else (a blog post, Medium, a Gist, etc) and then link it from the wiki page. See https://go.googlesource.com/proposal/+/master/design/go2draft.md. Thanks.

@rsc rsc closed this as completed Sep 19, 2018
@rsc
Copy link
Contributor

rsc commented Sep 19, 2018

To everyone else, please note that detailed discussion like this does not belong on the issue tracker.

@bradfitz bradfitz added the error-handling Language & library change proposals that are about error handling. label Oct 29, 2019
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. Proposal
Projects
None yet
Development

No branches or pull requests