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: introduce "throws" keyword for error handling #32852

Closed
kidlj opened this issue Jun 29, 2019 · 18 comments
Closed

Proposal: introduce "throws" keyword for error handling #32852

kidlj opened this issue Jun 29, 2019 · 18 comments
Labels
error-handling Language & library change proposals that are about error handling. FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Milestone

Comments

@kidlj
Copy link

kidlj commented Jun 29, 2019

First of all, I prefer leaving if err != nil alone, it's quite good. But if we must add something for error handling, here's a new approach that behaves like try() but can handle more scenarios, seems to be clearer, and most importantly, keeps compatibility with if err != nil ways.

The proposal suggests to add a new keyword throws for error handling.

The signature is simple:

// return the original error
func testFuncA() error {
    result, err := fooFunc() throws err
}

or

// return a new wrapped error with contexts
func testFuncB() error {
    message: = "error message"
    detail := "error detail"
    code := 500
    result, err := fooFunc() throws NewBarError(message, detail, code, err)
}

// or construct a BarError literal directly
func testFuncB() error {
    result, err := fooFunc() throws &BarError{
                                    Message: "error message",
                                    Detail: err.Error(),
                                    Code: 500,
    }
}

where NewBarError returns a new error of type BarError which implements the standard error interface.

In both cases, the last return value of enclosing function must be error interface type. if err != nil, the expression following throws will take place and the enclosing function returns with the expression evaluation result as the value of its last return parameter(error type) and its other return values are the type default values or the value of current corresponding variable(for named return parameters) .

If err == nil, the expression after throws will not execute and the program goes on.

Last, the throws keyword is totally optional for error handling, you can omit it somewhere and use it at another place in the same enclosing function.

Examples:

  1. The CopyFile example from the overview becomes:
func CopyFile(src, dst string) (err error) {
        defer func() {
                if err != nil {
                        err = fmt.Errorf("copy %s %s: %v", src, dst, err)
                }
        }()

        r, err := os.Open(src) throws err
        defer r.Close()

        w, err := os.Create(dst) throws err
        defer func() {
                w.Close()
                if err != nil {
                        os.Remove(dst) 
                }
        }()

        err = io.Copy(w, r) throws err
        err = w.Close() throws err
        return nil
}
  1. An example of returning custom error(an echo middleware example):
package main

import (
	"fmt"
	"net/http"

	"github.com/labstack/echo/v4"
)

const (
	successCode                   = 200
	serverErrorCode               = 500
)

// IngressError is the return error of manipulating ingresses.
type IngressError struct {
	httpCode int
	Code     int    `json:"code"`
	Message  string `json:"message"`
	Detail   string `json:"detail"`
}

// Error returns the formatted string of an IngressError.
func (e *IngressError) Error() string {
	return fmt.Sprintf("httpCode=%d Message=%s Detail=%s", e.httpCode, e.Message, e.Detail)
}

// NewIngressError returns a new IngressError.
func NewIngressError(httpCode, code int, message, detail string) *IngressError {
	return &IngressError{
		httpCode: httpCode,
		Code:     code,
		Message:  message,
		Detail:   detail,
	}
}


// CreateIngress creates an ingress
// POST /ingress
func CreateIngress(next echo.HandlerFunc) echo.HandlerFunc {
	return func(c echo.Context) error {
		spec := c.Get("spec").(*types.Spec)
		namespace := spec.Namespace
		name := spec.Name
		client := ingress.NewClient(namespace)
		message := fmt.Sprintf("create ingress error: %s/%s", namespace, name)
		result, err := client.CreateIngress(name, spec) throws NewIngressError(http.StatusInternalServerError, serverErrorCode, message, err.Error())

		c.Set("data", result)
		return next(c)
	}
}

I believe this approach may handle more scenarios than the try built-in proposal, especially when you need to wrap a new error for different function calls.

Drawbacks:

  1. introduce a new keyword.
  2. the code following throws may make the line longer(it depends).
@sirkon

This comment has been minimized.

@dpinela
Copy link
Contributor

dpinela commented Jun 29, 2019

This seems to be very similar to #32848.

@ianlancetaylor ianlancetaylor added v2 A language change or incompatible library change LanguageChange labels Jun 29, 2019
@ianlancetaylor

This comment has been minimized.

@sirkon
Copy link

sirkon commented Jun 29, 2019

@sirkon Please stay polite in discussions on this issue tracker. Please see Gopher Code of Conduct at https://golang.org/conduct. Thanks.

@ianlancetaylor

I hope to be rational, not polite. This recent “try” proposal allows

info := try(try(os.Open(fileName)).Stat())

I can easily imagine me, senior, making such a mistake when I’m tired. And I bet juniors and even middles will do this all the time.

@tandr
Copy link

tandr commented Jun 29, 2019

Just throwing it out there ...

and if we replace throws with ? ?

info := os.Open(fileName)?.Stat()?

(? equivalent to "if res is not nil and err from return is not nil")

@sirkon
Copy link

sirkon commented Jun 29, 2019

Just throwing it out there ...

and if we replace throws with ? ?

info := os.Open(fileName)?.Stat()?

(? equivalent to "if res is not nil and err from return is not nil")

And we will have file descriptor leak. We need to call Close on each successful Open.

@tandr
Copy link

tandr commented Jun 29, 2019

@sirkon -- so the original "try" does Close(), or it leaks too?

info := try(try(os.Open(fileName)).Stat())

@sirkon
Copy link

sirkon commented Jun 29, 2019

@sirkon -- so the original "try" does Close(), or it leaks too?

info := try(try(os.Open(fileName)).Stat())

It leaks too. We don’t have a RAII in Go, so this essentially means a leak. That’s why try proposal is evil. Go type system is far from being able to handle this. This proposal gives nothing meanwhile: less lines but denser code.

@tandr
Copy link

tandr commented Jun 29, 2019

Thank you. Means ? is no worse than try, but unfortunately no better either...

(to expand a bit, I am thinking out loud here)

func stat(filename string) (os.FileInfo, error) {
     return os.Open(filename)?.Stat()?
}

to be equivalent to

func stat(filename string) (os.FileInfo, error) {
	var info os.FileInfo
	{
		var a1 *os.FIle
		if a1, err := os.Open(filename); err != nil || a1 == nil {
			return _, err
		}
		var a2 os.FileInfo
		if a2, err := a1.Stat(); err != nil || a2 == nil {
			return _, err
		}
		info = a2
	}
	return info, nil
}

@sirkon
Copy link

sirkon commented Jun 29, 2019

Thank you. Means ? is no worse than try, but unfortunately no better either...

(to expand a bit, I am thinking out loud here)

func stat(filename string) os.FileInfo {
     info := os.Open(filename)?.Stat()?
     return info
}

to be equivalent to

func stat(filename string) (os.FileInfo, error) {
	var info os.FileInfo
	{
		var a1 *os.FIle
		if a1, err := os.Open(filename); err != nil || a1 == nil {
			return _, err
		}
		var a2 os.FileInfo
		if a2, err := a1.Stat(); err != nil || a2 == nil {
			return _, err
		}
		info = a2
	}
	return info, nil
}

Yes, exactly

@tandr
Copy link

tandr commented Jun 29, 2019

(continue TOLH)

func stat(filename string) (os.FileInfo) {
     file := os.Open(filename)?
     defer file?.Close()
     return file?.Stat()?
}

func stat2(filename string) (_ os.FileInfo, err error) {
     f, err := os.Open(filename)
     defer f?.Close()
     return f?.Stat()
}

@sirkon
Copy link

sirkon commented Jun 29, 2019

(continue TOLH)

func stat(filename string) (os.FileInfo) {
     file := os.Open(filename)?
     defer file?.Close()
     return file?.Stat()?
}

func stat2(filename string) (_ os.FileInfo, err error) {
     f, err := os.Open(filename)
     defer f?.Close()
     return f?.Stat()
}

This is complicated. Better to invent RAII

@tandr
Copy link

tandr commented Jun 29, 2019

defer is Go's RAII. I am not smart enough to see something descriptive and working at that level short of destructors from c++.

@sirkon
Copy link

sirkon commented Jun 29, 2019

@tandr Go type system cannot even warn you about lack of resource release. It is already an issue even though we actually have sane syntax and semantics what softly directs us to care about it.

@ianlancetaylor

This comment has been minimized.

@ianlancetaylor
Copy link
Contributor

As was mentioned, this is a lot like #32848, except that this variant only permits returning from the function.

It's not clear to me that we need a new keyword here; can we just use return?

This has the same odd assign-on-the-left then execute-on-the-right as in #32848.

@kidlj
Copy link
Author

kidlj commented Jul 10, 2019

As was mentioned, this is a lot like #32848, except that this variant only permits returning from the function.

That's the point, just return an error when applicable, and do nothing more to keep things simple.
This is like what try() tries to achieve and seems to be clearer. It also can handle more scenarios than try(), since we can return a new wrapped error with contexts -- that's the usual.

This has the same odd assign-on-the-left then execute-on-the-right as in #32848.

That's true, but is to reduce the if err != nil typing. And here's another advantage to try(), it keeps same number of variables on the left side of an assignment statement as the called function returns on the right side.

It's not clear to me that we need a new keyword here; can we just use return?

As of its 'assign-on-the-left then execute-on-the-right' nature, we should not confuse the return keyword use, and the keyword throws can better indicate the purpose and when to use it.

@ianlancetaylor
Copy link
Contributor

This has the same scoping problem as #32848: variables being defined in an assignment statement must come into scope at the point of the throws keyword.

In any case closing as dup of #32848. The differences here seem like minor details.

@bradfitz bradfitz added the error-handling Language & library change proposals that are about error handling. label Oct 29, 2019
@golang golang locked and limited conversation to collaborators Oct 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
error-handling Language & library change proposals that are about error handling. FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

7 participants