golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.24k stars 17.7k forks source link

proposal: Go 2: simplify error handling with || err suffix #21161

Closed ianlancetaylor closed 6 years ago

ianlancetaylor commented 7 years ago

There have been many proposals for how to simplify error handling in Go, all based on the general complaint that too much Go code contains the lines

if err != nil {
    return err
}

I'm not sure that there is a problem here to be solved, but since it keeps coming up, I'm going to put out this idea.

One of the core problems with most suggestions for simplifying error handling is that they only simplify two ways of handling errors, but there are actually three:

  1. ignore the error
  2. return the error unmodified
  3. return the error with additional contextual information

It is already easy (perhaps too easy) to ignore the error (see #20803). Many existing proposals for error handling make it easier to return the error unmodified (e.g., #16225, #18721, #21146, #21155). Few make it easier to return the error with additional information.

This proposal is loosely based on the Perl and Bourne shell languages, fertile sources of language ideas. We introduce a new kind of statement, similar to an expression statement: a call expression followed by ||. The grammar is:

PrimaryExpr Arguments "||" Expression

Similarly we introduce a new kind of assignment statement:

ExpressionList assign_op PrimaryExpr Arguments "||" Expression

Although the grammar accepts any type after the || in the non-assignment case, the only permitted type is the predeclared type error. The expression following || must have a type assignable to error. It may not be a boolean type, not even a named boolean type assignable to error. (This latter restriction is required to make this proposal backward compatible with the existing language.)

These new kinds of statement is only permitted in the body of a function that has at least one result parameter, and the type of the last result parameter must be the predeclared type error. The function being called must similarly have at least one result parameter, and the type of the last result parameter must be the predeclared type error.

When executing these statements, the call expression is evaluated as usual. If it is an assignment statement, the call results are assigned to the left-hand side operands as usual. Then the last call result, which as described above must be of type error, is compared to nil. If the last call result is not nil, a return statement is implicitly executed. If the calling function has multiple results, the zero value is returned for all result but the last one. The expression following the || is returned as the last result. As described above, the last result of the calling function must have type error, and the expression must be assignable to type error.

In the non-assignment case, the expression is evaluated in a scope in which a new variable err is introduced and set to the value of the last result of the function call. This permits the expression to easily refer to the error returned by the call. In the assignment case, the expression is evaluated in the scope of the results of the call, and thus can refer to the error directly.

That is the complete proposal.

For example, the os.Chdir function is currently

func Chdir(dir string) error {
    if e := syscall.Chdir(dir); e != nil {
        return &PathError{"chdir", dir, e}
    }
    return nil
}

Under this proposal, it could be written as

func Chdir(dir string) error {
    syscall.Chdir(dir) || &PathError{"chdir", dir, err}
    return nil
}

I'm writing this proposal mainly to encourage people who want to simplify Go error handling to think about ways to make it easy to wrap context around errors, not just to return the error unmodified.

bradfitz commented 7 years ago
    syscall.Chdir(dir) || &PathError{"chdir", dir, e}

Where does e come from there? Typo?

Or did you mean:

func Chdir(dir string) (e error) {
    syscall.Chdir(dir) || &PathError{"chdir", dir, e}
    return nil
}

(That is, does the implicit err != nil check first assign error to the result parameter, which can be named to modify it again before the implicit return?)

ianlancetaylor commented 7 years ago

Sigh, messed up my own example. Now fixed: the e should be err. The proposal puts err in scope to hold the error value of the function call when not in an assignment statement.

mvdan commented 7 years ago

While I'm not sure if I agree with the idea or the syntax, I have to give you credit for giving attention to adding context to errors before returning them.

This might be of interest to @davecheney, who wrote https://github.com/pkg/errors.

object88 commented 7 years ago

What happens in this code:

if foo, err := thing.Nope() || &PathError{"chdir", dir, err}; err == nil || ignoreError {
}

(My apologies if this isn't even possible w/o the || &PathError{"chdir", dir, e} part; I'm trying to express that this feels like a confusing override of existing behavior, and implicit returns are... sneaky?)

ianlancetaylor commented 7 years ago

@object88 I would be fine with not permitting this new case in a SimpleStmt as used in if and for and switch statements. That would probably be best though it would complicate the grammar slightly.

But if we don't do that, then what happens is that if thing.Nope() returns a non-nil error, then the calling function returns with &PathError{"chdir", dir, err} (where err is the variable set by the call to thing.Nope()). If thing.Nope() returns a nil error, then we know for sure that err == nil is true in the condition of the if statement, and so the body of the if statement is executed. The ignoreError variable is never read. There is no ambiguity or override of existing behavior here; the handling of || introduced here is only accepted when the expression after || is not a boolean value, which means that it would not compile currently.

I do agree that implicit returns are sneaky.

object88 commented 7 years ago

Yeah, my example is pretty poor. But not permitting the operation inside an if, for, or switch would resolve a lot of potential confusion.

jimmyfrasche commented 7 years ago

Since the bar for consideration is generally something that is hard to do in the language as is, I decided to see how hard this variant was to encode in the language. Not much harder than the others: https://play.golang.org/p/9B3Sr7kj39

I really dislike all of these proposals for making one type of value and one position in the return arguments special. This one is in some ways actually worse because it also makes err a special name in this specific context.

Though I certainly agree that people (including me!) should be wearier of returning errors without extra context.

When there are other return values, like

if err != nil {
  return 0, nil, "", Struct{}, wrap(err)
}

it can definitely get tiresome to read. I somewhat liked @nigeltao's suggestion for return ..., err in https://github.com/golang/go/issues/19642#issuecomment-288559297

ghost commented 7 years ago

If I understand correctly, to build the syntax tree the parser would need to know the types of the variables to distinguish between

boolean := BoolFunc() || BoolExpr

and

err := FuncReturningError() || Expr

It doesn't look good.

dup2X commented 7 years ago

less is more...

mattn commented 7 years ago

When the return ExpressionList contains two ore more elements, how it works?

BTW, I want panicIf instead.

err := doSomeThing()
panicIf(err)

err = doAnotherThing()
panicIf(err)
tandr commented 7 years ago

@ianlancetaylor In your proposal's example err is still not declared explicitly, and pulled in as 'magical' (language predefined), right?

Or will it be something like

func Chdir(dir string) error {
    return (err := syscall.Chdir(dir)) || &PathError{"chdir", dir, err}
}

?

On the other hand (since it is marked as a "language change" already...) Introduce a new operator (!! or ??) that does the shortcut on error != nil (or any nullable?)

func DirCh(dir string) (string, error) {
    return dir, (err := syscall.Chdir(dir)) !! &PathError{"chdir", dir, err}
}

Sorry if this is too far off :)

henryas commented 7 years ago

I agree that error handling in Go can be repetitive. I don't mind the repetition, but too many of them affects readability. There is a reason why "Cyclomatic Complexity" (whether you believe in it or not) uses control flows as a measure for complexity. The "if" statement adds extra noise.

However, the proposed syntax "||" is not very intuitive to read, especially since the symbol is commonly known as an OR operator. In addition, how do you deal with functions that return multiple values and error?

I am just tossing some ideas here. How about instead of using error as output, we use error as input? Example: https://play.golang.org/p/rtfoCIMGAb

ianlancetaylor commented 7 years ago

Thanks for all the comments.

@opennota Good point. It could still work but I agree that aspect is awkward.

@mattn I don't think there is a return ExpressionList, so I'm not sure what you are asking. If the calling function has multiple results, all but the last are returned as the zero value of the type.

@mattn panicif does not address one the key elements of this proposal, which is an easy way to return an error with additional context. And, of course, one can write panicif today easily enough.

@tandr Yes, err is defined magically, which is pretty awful. Another possibility would be to permit the error-expression to use error to refer to the error, which is awful in a different way.

@tandr We could use a different operator but I don't see any big advantage. It doesn't seem to make the result any more readable.

@henryas I think the proposal explains how it handles multiple results.

@henryas Thanks for the example. The thing I dislike about that kind of approach is that it makes the error handling the most prominent aspect of the code. I want the error handling to be present and visible but I don't want it to be the first thing on the line. That is true today, with the if err != nil idiom and the indentation of the error handling code, and it should remain true if any new features are added for error handling.

Thanks again.

jimmyfrasche commented 7 years ago

@ianlancetaylor I don't know if you checked out my playground link but it had a "panicIf" that let you add extra context.

I'll reproduce a somewhat simplified version here:

func panicIf(err error, transforms ...func(error) error) {
  if err == nil {
    return
  }
  for _, transform := range transforms {
    err = transform(err)
  }
  panic(err)
}
jba commented 7 years ago

Coincidentally, I just gave a lightning talk at GopherCon where I used (but didn't seriously propose) a bit of syntax to aid in visualizing error-handling code. The idea is to put that code to the side to get it out of the way of the main flow, without resorting to any magic tricks to shorten the code. The result looks like

func DirCh(dir string) (string, error) {
    dir := syscall.Chdir(dir)        =: err; if err != nil { return "", err }
}

where =: is the new bit of syntax, a mirror of := that assigns in the other direction. Obviously we'd also need something for = as well, which is admittedly problematic. But the general idea is to make it easier for the reader to understand the happy path, without losing information.

henryas commented 7 years ago

On the other hand, the current way of error handling does have some merits in that it serves as a glaring reminder that you may be doing too many things in a single function, and some refactoring may be overdue.

rodcorsi commented 7 years ago

I really like the the syntax proposed by @billyh here

func Chdir(dir string) error {
    e := syscall.Chdir(dir) catch: &PathError{"chdir", dir, e}
    return nil
}

or more complex example using https://github.com/pkg/errors

func parse(input io.Reader) (*point, error) {
    var p point

    err := read(&p.Longitude) catch: nil, errors.Wrap(err, "Failed to read longitude")
    err = read(&p.Latitude) catch: nil, errors.Wrap(err, "Failed to read Latitude")
    err = read(&p.Distance) catch: nil, errors.Wrap(err, "Failed to read Distance")
    err = read(&p.ElevationGain) catch: nil, errors.Wrap(err, "Failed to read ElevationGain")
    err = read(&p.ElevationLoss) catch: nil, errors.Wrap(err, "Failed to read ElevationLoss")

    return &p, nil
}
ALTree commented 7 years ago

A plain idea, with support for error decoration, but requiring a more drastic language change (obviously not for go1.10) is the introduction of a new check keyword.

It would have two forms: check A and check A, B.

Both A and B need to be error. The second form would only be used when error-decorating; people that do not need or wish to decorate their errors will use the simpler form.

1st form (check A)

check A evaluates A. If nil, it does nothing. If not nil, check acts like a return {<zero>}*, A.

Examples

err := UpdateDB()    // signature: func UpdateDb() error
if err != nil {
    return err
}

becomes

check UpdateDB()
a, b, err := Foo()    // signature: func Foo() (string, string, error)
if err != nil {
    return "", "", err
}

// use a and b

becomes

a, b, err := Foo()
check err

// use a and b

2nd form (check A, B)

check A, B evaluates A. If nil, it does nothing. If not nil, check acts like a return {<zero>}*, B.

This is for error-decorating needs. We still check on A, but is B that is used in the implicit return.

Example

a, err := Bar()    // signature: func Bar() (string, error)
if err != nil {
    return "", &BarError{"Bar", err}
}

becomes

a, err := Foo()
check err, &BarError{"Bar", err}

Notes

It's a compilation error to

The two-expr form check A, B is short-circuited. B is not evaluated if A is nil.

Notes on practicality

There's support for decorating errors, but you pay for the clunkier check A, B syntax only when you actually need to decorate errors.

For the if err != nil { return nil, nil, err } boilerplate (which is very common) check err is as brief as it could be without sacrificing clarity (see note on the syntax below).

Notes on syntax

I'd argue that this kind of syntax (check .., at the beginning of the line, similar to a return) is a good way to eliminate error checking boilerplate without hiding the control flow disruption that implicit returns introduce.

A downside of ideas like the <do-stuff> || <handle-err> and <do-stuff> catch <handle-err> above, or the a, b = foo()? proposed in another thread, is that they hide the control flow modification in a way that makes the flow harder to follow; the former with || <handle-err> machinery appended at the end of an otherwise plain-looking line, the latter with a little symbol that can appear everywhere, including in the middle and at the end of a plain-looking line of code, possibly multiple times.

A check statement will always be top-level in the current block, having the same prominence of other statements that modify the control flow (for example, an early return).

bhinners commented 7 years ago

@ALTree, I didn't understand how your example:

a, b, err := Foo()
check err

Achieves the three valued return from the original:

return "", "", err

Is it just returning zero values for all the declared returns except the final error? What about cases where you'd like to return a valid value along with an error, e.g. number of bytes written when a Write() fails?

Whatever solution we go with should minimally restrict the generality of error handling.

Regarding the value of having check at the start of the line, my personal preference is to see the primary control flow at the start of each line and have the error handling interfere with the readability of that primary control flow as little as possible. Also, if the error handling is set apart by a reserved word like check or catch, then pretty much any modern editor is going to highlight the reserved word syntax in some way and make it noticeable even if it's on the right hand side.

ALTree commented 7 years ago

@billyh this is explained above, on the line that says:

If not nil, check acts like a return {<zero>}*, A

check will return the zero value of any return value, except the error (in last position).

What about cases where you'd like to return a valid value along with an error

Then you'll use the if err != nil { idiom.

There are many cases where you'll need a more sophisticate error-recovering procedure. For example you may need, after catching an error, to roll back something, or to write something on a log file. In all these cases, you'll still have the usual if err idiom in your toolbox, and you can use it to start a new block, where any kind of error-handling related operation, no matter how articulated, can be carried out.

Whatever solution we go with should minimally restrict the generality of error handling.

See my answer above. You'll still have if and anything else the language gives you now.

pretty much any modern editor is going to highlight the reserved word

Maybe. But introducing opaque syntax, that requires syntax highlighting to be readable, is not ideal.

go-li commented 7 years ago

this particular bug can be fixed by introducing a double return facility to the language. in this case the function a() returns 123:

func a() int { b() return 456 } func b() { return return int(123) }

This facility can be used to simplify error handling as follows:

func handle(var foo, err error )(var foo, err error ) { if err != nil { return return nil, err } return var, nil }

func client_code() (*client_object, error) { var obj, err =handle(something_that_can_fail()) // this is only reached if something not failed // otherwise the client_code function would propagate the error up the stack assert(err == nil) }

This allows people to write an error handler functions that can propagate the errors up the stack such error handling functions can be separate from main code

rodcorsi commented 7 years ago

Sorry if I got it wrong, but I want to clarify a point, the function below will produce an error, vet warning or will it be accepted?

func Chdir(dir string) (err error) {
    syscall.Chdir(dir) || err
    return nil
}
ianlancetaylor commented 7 years ago

@rodcorsi Under this proposal your example would be accepted with no vet warning. It would be equivalent to

if err := syscall.Chdir(dir); err != nil {
    return err
}
henryas commented 7 years ago

How about expanding the use of Context to handle error? For instance, given the following definition:

type ErrorContext interface {
     HasError() bool
     SetError(msg string)
     Error() string 
}

Now in the error-prone function ...

func MyFunction(number int, ctx ErrorContext) int {
     if ctx.HasError() {
           return 0
     }
     return number + 1
}

In the intermediate function ...

func MyIntermediateFunction(ctx ErrorContext) int {
     if ctx.HasError() {
           return 0
     }
     number := 0
     number = MyFunction(number, ctx)
     number = MyFunction(number, ctx)
     number = MyFunction(number, ctx)
     return number
}

And in the upper level function

func main() {
     ctx := context.New()
     no := MyIntermediateFunction(ctx)
     if ctx.HasError() {
           log.Fatalf("Error: %s", ctx.Error())
           return
     }
     fmt.Printf("%d\n", no)
}

There are several benefits using this approach. First, it doesn't distract the reader from the main execution path. There is minimal "if" statements to indicate deviation from the main execution path.

Second, it doesn't hide error. It is clear from the method signature that if it accepts ErrorContext, then the function may have errors. Inside the function, it uses the normal branching statements (eg. "if") which shows how error is handled using normal Go code.

Third, error is automatically bubbled up to the interested party, which in this case is the context owner. Should there be an additional error processing, it will be clearly shown. For instance, let's make some changes to the intermediate function to wrap any existing error:

func MyIntermediateFunction(ctx ErrorContext) int {
     if ctx.HasError() {
           return 0
     }
     number := 0
     number = MyFunction(number, ctx)
     number = MyFunction(number, ctx)
     number = MyFunction(number, ctx)
     if ctx.HasError() {
          ctx.SetError(fmt.Sprintf("wrap msg: %s", ctx.Error())
          return
     }
     number *= 20
     number = MyFunction(number, ctx)
     return number
}

Basically, you just write the error-handling code as needed. You don't need to manually bubble them up.

Lastly, you as the function writer have a say whether the error should be handled. Using the current Go approach, it is easy to do this ...

//given the following definition
func MyFunction(number int) error

//then do this
MyFunction(8) //without checking the error

With the ErrorContext, you as the function owner can make the error checking optional with this:

func MyFunction(ctx ErrorContext) {
     if ctx != nil && ctx.HasError() {
           return
     }
     //... 
}

Or make it compulsory with this:

func MyFunction(ctx ErrorContext) {
    if ctx.HasError() {  //will panic if ctx is nil
         return
    }
    //...
}

If you make error handling compulsory and yet the user insists on ignoring error, they can still do that. However, they have to be very explicit about it (to prevent accidental ignore). For instance:

func UpperFunction(ctx ErrorContext) {
     ignored := context.New()
     MyFunction(ignored)  //this one is ignored

     MyFunction(ctx) //this one is handled
}

This approach changes nothing to the existing language.

tandr commented 7 years ago

@ALTree Alberto, what about mixing your check and what @ianlancetaylor proposed?

so

func F() (int, string, error) {
   i, s, err := OhNo()
   if err != nil {
      return i, s, &BadStuffHappened(err, "oopsie-daisy")
   }
   // all is good
   return i+1, s+" ok", nil
}

becomes

func F() (int, string, error) {
   i, s, err := OhNo()
   check i, s, err || &BadStuffHappened(err, "oopsie-daisy")
   // all is good
   return i+1, s+" ok", nil
}

Also, we can limit check to only deal with error types, so if you need multiple return values, they need to be named and assigned, so it assigns "in-place" somehow and behaves like simple "return"

func F() (a int, s string, err error) {
   i, s, err = OhNo()
   check err |=  &BadStuffHappened(err, "oopsy-daisy")  // assigns in place and behaves like simple "return"
   // all is good
   return i+1, s+" ok", nil
}

If return would become acceptable in expression one day, then check is not needed, or becomes a standard function

func check(e error) bool {
   return e != nil
}

func F() (a int, s string, err error) {
   i, s, err = OhNo()
   check(err) || return &BadStuffHappened(err, "oopsy-daisy")
   // all is good
   return i+1, s+" ok", nil
}

the last solution feels like Perl though 😄

nigeltao commented 7 years ago

I can't remember who originally proposed it, but here's another syntax idea (everybody's favorite bikeshed :-). I'm not saying it's a good one, but if we're throwing ideas into the pot...

x, y := try foo()

would be equivalent to:

x, y, err := foo()
if err != nil {
    return (an appropriate number of zero values), err
}

and

x, y := try foo() catch &FooErr{E:$, S:"bad"}

would be equivalent to:

x, y, err := foo()
if err != nil {
    return (an appropriate number of zero values), &FooErr{E:err, S:"bad"}
}

The try form has certainly been proposed before, a number of times, modulo superficial syntax differences. The try ... catch form is less often proposed, but it is clearly similar to @ALTree's check A, B construct and @tandr's follow-up suggestion. One difference is that this is an expression, not a statement, so that you can say:

z(try foo() catch &FooErr{E:$, S:"bad"})

You could have multiple try/catches in a single statement:

p = try q(0) + try q(1)
a = try b(c, d() + try e(), f, try g() catch &GErr{E:$}, h()) catch $BErr{E:$}

although I don't think we want to encourage that. You'd also need to be careful here about order of evaluation. For example, whether h() is evaluated for side-effects if e() returns a non-nil error.

Obviously, new keywords like try and catch would break Go 1.x compatibility.

mattn commented 7 years ago

I suggest that we should squeeze the target of this proporsal. What issue will fixes by this proporsal? Reduce following three lines into two or one ? This might be change of language of return/if.

if err != nil {
    return err
}

Or reduce the number of times to check err? It may be try/catch solution for this.

jba commented 7 years ago

I'd like to suggest that any reasonable shortcut syntax for error-handling have three properties:

  1. It shouldn't appear before the code it is checking, so that the non-error path is prominent.
  2. It shouldn't introduce implicit variables into the scope, so that readers don't get confused when there is an explicit variable with the same name.
  3. It shouldn't make one recovery action (for example, return err) easier than another. Sometimes an entirely different action might be preferable (like calling t.Fatal). We also don't want to discourage people from adding additional context.

Given those constraints, it seems that one nearly minimal syntax would be something like

STMT SEPARATOR_TOKEN VAR BLOCK

For example,

syscall.Chdir(dir) :: err { return err }

which is equivalent to

if err := syscall.Chdir(dir); err != nil {
    return err
}

Even though it's not much shorter, the new syntax moves the error path out of the way. Part of the change would be to modify gofmt so it doesn't line-break one-line error-handling blocks, and it indents multi-line error-handling blocks past the opening }.

We could make it a bit shorter by declaring the error variable in place with a special marker, like

syscall.Chdir(dir) :: { return @err }
mattn commented 7 years ago

I wonder how this behave as for non-zero-value and error both returned. For example, bufio.Peek possibly return non-zero value and ErrBufferFull both in same time.

nigeltao commented 7 years ago

@mattn you can still use the old syntax.

mattn commented 7 years ago

@nigeltao Yes, I understand. I suspect this behavior possibly make a bug on user's code since bufio.Peek also return non-zero and nil. The code must not implicit values and error both. So the value and error both should be returned to caller (in this case).

ret, err := doSomething() :: err { return err }
return ret, err
bcmills commented 7 years ago

@jba What you're describing looks a bit like a transposed function-composition operator:

syscall.Chdir(dir) ⫱ func (err error) { return &PathError{"chdir", dir, err} }

But the fact that we're writing mostly-imperative code requires that we not use a function in the second position, because part of the point is to be able to return early.

So now I'm thinking about three observations that are all kind of related:

  1. Error handling is like function composition, but the way we do things in Go is sort of the opposite of Haskell's Error monad: because we're mostly writing imperative instead of sequential code, we want to transform the error (to add context) rather than the non-error value (which we'd rather just bind to a variable).

  2. Go functions that return (x, y, error) usually really mean something more like a union (#19412) of (x, y) | error.

  3. In languages that unpack or pattern-match unions, the cases are separate scopes, and a lot of the trouble that we have with errors in Go is due to unexpected shadowing of redeclared variables that might be improved by separating those scopes (#21114).

So maybe what we want really is like the =: operator, but with a sort of union-matching conditional:

syscall.Chdir(dir) =? err { return &PathError{"chdir", dir, err} }
n := io.WriteString(w, s) =? err { return err }

and perhaps a boolean version for , ok index expressions and type assertions:

y := m[x] =! { return ErrNotFound }

Except for the scoping, that's not much different from just changing gofmt to be more amenable to one-liners:

err := syscall.Chdir(dir); if err != nil { return &PathError{"chdir", dir, err} }
n, err := io.WriteString(w, s); if err != nil { return err }
y, ok := m[x]; if !ok { return ErrNotFound }

But the scoping is a big deal! The scoping issues are where this sort of code crosses the line from "somewhat ugly" to "subtle bugs".

urandom commented 7 years ago

@ianlancetaylor While I'm a fan of the overall idea, I'm not a huge supporter of the cryptic perl-like syntax for it. Perhaps a more wordy syntax would be less confusing, like:

syscall.Chdir(dir) or dump(err): errors.Wrap(err, "chdir failed")

syscall.Chdir(dir) or dump

Also, I didn't understand whether the last argument is popped in case of assignment, E.g.:

resp := http.Get("https://example.com") or dump
romainmenke commented 7 years ago

Let's not forget that errors are values in go and not some special type. There is nothing we can do to other structs that we can't do to errors and the other way around. This means that if you understand structs in general, you understand errors and how they are handled (even If you think it is verbose)

This syntax would require new and old developers to learn a new bit of information before they can start to understand code that uses it.

That alone makes this proposal not worth it IMHO.

Personally I prefer this syntax

err := syscall.Chdir(dir)
if err != nil {
    return err
}
return nil

over

if err := syscall.Chdir(dir); err != nil {
    return err
}
return nil

It is one line more but it separates the intended action from the error handling. This form is the most readable for me.

jba commented 7 years ago

@bcmills:

Except for the scoping, that's not much different from just changing gofmt to be more amenable to one-liners

Not just the scoping; there is also the left edge. I think that really affects readability. I think

syscall.Chdir(dir) =: err; if err != nil { return &PathError{"chdir", dir, err} } 

is much clearer than

err := syscall.Chdir(dir); if err != nil { return &PathError{"chdir", dir, err} } 

especially when it occurs on multiple consecutive lines, because your eye can scan the left edge to ignore error handling.

rodcorsi commented 7 years ago

Mixing the idea @bcmills we can introduce conditional pipe forwarding operator.

The F2 function will be executed if the last value is not nil.

func F1() (foo, bar){}

first := F1() ?> last: F2(first, last)

A special case of pipe forwarding with return statement

func Chdir(dir string) error {
    syscall.Chdir(dir) ?> err: return &PathError{"chdir", dir, err}
    return nil
}

Real example brought by @urandom in another issue For me much more readable with focus in primary flow

func configureCloudinit(icfg *instancecfg.InstanceConfig, cloudcfg cloudinit.CloudConfig) (cloudconfig.UserdataConfig, error) {
    // When bootstrapping, we only want to apt-get update/upgrade
    // and setup the SSH keys. The rest we leave to cloudinit/sshinit.
    udata := cloudconfig.NewUserdataConfig(icfg, cloudcfg) ?> err: return nil, err
    if icfg.Bootstrap != nil {
        udata.ConfigureBasic() ?> err: return nil, err
        return udata, nil
    }
    udata.Configure() ?> err: return nil, err
    return udata, nil
}

func ComposeUserData(icfg *instancecfg.InstanceConfig, cloudcfg cloudinit.CloudConfig, renderer renderers.ProviderRenderer) ([]byte, error) {
    if cloudcfg == nil {
        cloudcfg = cloudinit.New(icfg.Series) ?> err: return nil, errors.Trace(err)
    }
    _ = configureCloudinit(icfg, cloudcfg) ?> err: return nil, errors.Trace(err)
    operatingSystem := series.GetOSFromSeries(icfg.Series) ?> err: return nil, errors.Trace(err)
    udata := renderer.Render(cloudcfg, operatingSystem) ?> err: return nil, errors.Trace(err)
    logger.Tracef("Generated cloud init:\n%s", string(udata))
    return udata, nil
}
ldynia commented 7 years ago

I agree error handling is not ergonomic. Namely, when you read below code you have to vocalize it to if error not nil then -which translates to if there is an error then.

if err != nil {
    // handle error
}

I would like to have abbility of expresing above code in such way -which in my opinion is more readable.

if err {
    // handle error
}

Just my humble suggestion :)

leifungi commented 7 years ago

It does look like perl, it even has the magic variable For reference, in perl you would do

open (FILE, $file) or die("cannot open $file: $!");

IMHO, it's not worth it, one point I like about go is that error handling is explicit and 'in your face'

If we do stick with it, I'd like to have no magic variables, we should be able to name the error variable

e := syscall.Chdir(dir) ?> e: &PathError{"chdir", dir, e}

And we might as well use a symbol different from || specific for this task, I guess text symbols like 'or' are not possible due to backwards compatibility

n, , err, = somecall(...) ?> err: &PathError{"somecall", n, err}

On Tue, Aug 1, 2017 at 2:47 PM, Rodrigo notifications@github.com wrote:

Mixing the idea @bcmills https://github.com/bcmills we can introduce conditional pipe forwarding operator.

The F2 function will be executed if the last value is not nil.

func F1() (foo, bar){} first := F1() ?> last: F2(first, last)

A special case of pipe forwarding with return statement

func Chdir(dir string) error { syscall.Chdir(dir) ?> err: return &PathError{"chdir", dir, err} return nil }

Real example https://github.com/juju/juju/blob/01b24551ecdf20921cf620b844ef6c2948fcc9f8/cloudconfig/providerinit/providerinit.go brought by @urandom https://github.com/urandom in another issue For me much more readable with focus in primary flow

func configureCloudinit(icfg instancecfg.InstanceConfig, cloudcfg cloudinit.CloudConfig) (cloudconfig.UserdataConfig, error) { // When bootstrapping, we only want to apt-get update/upgrade // and setup the SSH keys. The rest we leave to cloudinit/sshinit. udata := cloudconfig.NewUserdataConfig(icfg, cloudcfg) ?> err: return nil, err if icfg.Bootstrap != nil { udata.ConfigureBasic() ?> err: return nil, err return udata, nil } udata.Configure() ?> err: return nil, err return udata, nil } func ComposeUserData(icfg instancecfg.InstanceConfig, cloudcfg cloudinit.CloudConfig, renderer renderers.ProviderRenderer) ([]byte, error) { if cloudcfg == nil { cloudcfg = cloudinit.New(icfg.Series) ?> err: return nil, errors.Trace(err) } configureCloudinit(icfg, cloudcfg) ?> err: return nil, errors.Trace(err) operatingSystem := series.GetOSFromSeries(icfg.Series) ?> err: return nil, errors.Trace(err) udata := renderer.Render(cloudcfg, operatingSystem) ?> err: return nil, errors.Trace(err) logger.Tracef("Generated cloud init:\n%s", string(udata)) return udata, nil }

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/21161#issuecomment-319359614, or mute the thread https://github.com/notifications/unsubscribe-auth/AbwRO_J0h2dQHqfysf2roA866vFN4_1Jks5sTx5hgaJpZM4Oi1c- .

romainmenke commented 7 years ago

Am I the only one that thinks all these proposed changes would be more complicated than the current form.

I think simplicity and brevity are not equal or interchangeable. Yes all these changes would be one or more lines shorter but would introduce operators or keywords which a user of the language would have to learn.

bcmills commented 7 years ago

@rodcorsi I know it seems minor, but I think it's important for the second part to be a Block: the existing if and for statements both use blocks, and select and switch both use curly-brace-delimited syntax, so it seems awkward to omit the braces for this one particular control-flow operation.

It's also much easier to ensure that the parse tree is unambiguous if you don't have to worry about arbitrary expressions following the new symbols.

The syntax and semantics I had in mind for my sketch are:


NonZeroGuardStmt = ( Expression | IdentifierList ":=" Expression |
                     ExpressionList assign_op Expression ) "=?" [ identifier ] Block .
ZeroGuardStmt = ( Expression | IdentifierList ":=" Expression |
                  ExpressionList assign_op Expression ) "=!" Block .

A NonZeroGuardStmt executes Block if the last value of an Expression is not equal to the zero value of its type. If an identifier is present, it is bound to that value within Block. A ZeroGuardStmt executes Block if the last value of an Expression is equal to the zero value of its type.

For the := form, the other (leading) values of the Expression are bound to the IdentifierList as in a ShortVarDecl. The identifiers are declared in the containing scope, which implies that they are also visible within the Block.

For the assign_op form, each left-hand side operand must be addressable, a map index expression, or (for = assignments only) the blank identifier. Operands may be parenthesized. The other (leading) values of the right-hand side Expression are evaluated as in an Assignment. The assignment occurs prior to execution of the Block and regardless of whether the Block is then executed.


I believe the proposed grammar here is compatible with Go 1: ? is not a valid identifier and there are no existing Go operators using that character, and although ! is a valid operator, there is no existing production in which it may be followed by a {.

jba commented 7 years ago

@bcmills LGTM, with concomitant changes to gofmt.

I would have thought you would make =? and =! each a token in its own right, which would make the grammar trivially compatible.

bcmills commented 7 years ago

I would have thought you would make =? and =! each a token in its own right, which would make the grammar trivially compatible.

We can do that in the grammar, but not in the lexer: the sequence "=!" can appear in valid Go 1 code (https://play.golang.org/p/pMTtUWgBN9).

The curly-brace is what makes the parse unambiguous in my proposal: =! can currently only appear in a declaration of or assignment to a boolean variable, and declarations and assignments cannot currently appear immediately before a curly-brace (https://play.golang.org/p/ncJyg-GMuL) unless separated by an implicit semicolon (https://play.golang.org/p/lhcqBhr7Te).

henryas commented 7 years ago

@romainmenke Nope. You're not the only one. I fail to see the value of a one-liner error handling. You may save one line but add much more complexity. The problem is that in many of these proposals, the error handling part becomes hidden. The idea is not to make them less noticeable because error handling is important, but to make the code easier to read. Brevity doesn't equal to easy readability. If you have to make changes to the existing error handling system, I find the conventional try-catch-finally is much more appealing than many ideas purposes here.

earthboundkid commented 7 years ago

I like the check proposal because you can also extend it to handle

f, err := os.Open(myfile)
check err
defer check f.Close()

Other proposals don't seem like they can mix with defer as well. check is also very readable, and simple to Google if you don't know it. I don't think it needs to be limited to the error type. Anything that's a return parameter in the last position could use it. So, a iterator might have a check for a Next() bool.

I once wrote a Scanner that looks like

func (s *Scanner) Next() bool {
    if s.Error != nil || s.pos >= s.RecordCount {
        return false
    }
    s.pos++

    var rt uint8
    if !s.read(&rt) {
        return false
    }
...

That last bit could be check s.read(&rt) instead.

bcmills commented 7 years ago

@carlmjohnson

Other proposals don't seem like they can mix with defer as well.

If you're assuming that we'll expand defer to allow returning from the outer function using the new syntax, you can apply that assumption equally well to other proposals.

defer f.Close() =? err { return err }

Since @ALTree's check proposal introduces a separate statement, I don't see how you could mix that with a defer that does anything other than simply returning the error.

defer func() {
  err := f.Close()
  check err, fmt.Errorf(…, err) // But this func() doesn't return an error!
}()

Contrast:

defer f.Close() =? err { return fmt.Errorf(…, err) }
jimmyfrasche commented 7 years ago

The justification for many of these proposals is better "ergonomics" but I don't really see how any of these are better other than making it so there's slightly less to type. How do these increase the maintainability of the code? The composability? The readability? The ease of understanding the control flow?

bcmills commented 7 years ago

@jimmyfrasche

How do these increase the maintainability of the code? The composability? The readability? The ease of understanding the control flow?

As I noted earlier, the major advantage of any of these proposals would probably need to come from clearer scoping of assignments and err variables: see #19727, #20148, #5634, #21114, and probably others for various ways that people have encountering scoping issues in relation to error-handling.

jimmyfrasche commented 7 years ago

@bcmills thanks for providing a motivation and sorry I missed it in your earlier post.

Given that premise, however, wouldn't it be better to provide a more general facility for "clearer scoping of assignments" that could be used by all variables? I've unintentionally shadowed my share of non-error variables as well, certainly.

I remember when the current behavior of := was introduced—a lot of that thread on go nuts† was clamor for a way to explicitly annotate which names to be reused instead of the implicit "reuse only if that variable exists in exactly the current scope" which is where all the subtle hard-to-see problems manifest, in my experience.

† I cannot find that thread does anyone have a link?

There are lots of things that I think could be improved about Go but the behavior of := always struck me as the one serious mistake. Maybe revisiting the behavior of := is the way to solve the root problem or at least to reduce the need for other more extreme changes?

bcmills commented 7 years ago

@jimmyfrasche

Given that premise, however, wouldn't it be better to provide a more general facility for "clearer scoping of assignments" that could be used by all variables?

Yes. That's one of the things I like about the =? or :: operator that @jba and I have proposed: it also extends nicely to (an admittedly limited subset of) non-errors.

Personally, I suspect I would be happier in the long term with a more explicit tagged-union/varint/algebraic datatype feature (see also #19412), but that's a much bigger change to the language: it's difficult to see how we would retrofit that onto existing APIs in a mixed Go 1 / Go 2 environment.

jba commented 7 years ago

The ease of understanding the control flow?

In my and @bcmills's proposals, your eye can scan down the left side and easily take in the non-error control flow.