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: selector-specific control flow & gofmt rules to improve error-checking readability #39372

Closed
Splizard opened this issue Jun 3, 2020 · 9 comments

Comments

@Splizard
Copy link

@Splizard Splizard commented Jun 3, 2020

In order to reduce basic error-checking bloat in Go software and improve readability, I propose shifting eligible error-checking to the right by the addition of control-flow that operates on the error type and a related gofmt rule.

Errors passed to the caller should not interrupt logical-flow

Go has an error-checking issue. Whenever a function is called that returns a value and an error. At least 3 additional lines of code are required to check it.

This isn't an issue when the code is actually handling the error (as the error handling becomes part of the logical-flow of the code) but when the error is simply passed to the caller, the error-checking obstructs the logical-flow of the code and makes it more difficult to understand what the code is trying to do.

In a perfect world, there wouldn't be any errors and IO functions could be written as such.
The behaviour of this function is easy to reason about.

func OpenImage(name string) image.Image {
	return image.Decode(os.Open(name))
}

Now consider the OpenImage functions below, OpenImage2 checks errors and OpenImage1 does not. The logical-flow of OpenImage1 is easier to read than OpenImage2.

func OpenImage1(name string) image.Image {
	f, _ := os.Open(name)
	img, _, _ := image.Decode(f)
	return img
}

func OpenImage2(name string) (image.Image, error) {
	f, err := os.Open(name)
	if err != nil {
		return nil, fmt.Errorf("OpenImage: %w", err)
	}
	img, _, err := image.Decode(f)
	if err != nil {
		return nil, fmt.Errorf("OpenImage: %w", err)
	}
	return img, nil
}

Most of the time, errors do need to be checked though and this causes the logical-flow of the code to be obstructed. Here is another example from here.

func (ds *GitDataSource) Fetch(from, to string) ([]string, error) {
	fmt.Printf("Fetching data from %s into %s...\n", from, to)
	if err := createFolderIfNotExist(to); err != nil {
		return nil, err
	}
	if err := clearFolder(to); err != nil {
		return nil, err
	}
	if err := cloneRepo(to, from); err != nil {
		return nil, err
	}
	dirs, err := getContentFolders(to)
	if err != nil {
		return nil, err
	}
	fmt.Print("Fetching complete.\n")
	return dirs, nil
}

Trying to read functions like this is jarring as the reader is interrupted by error checking at almost every step.

Proposal: selector-specific control flow

x.return [ ExpressionList ] .

For a value x of type error, x.return inside of function F terminates the execution of F
if x != nil, and optionally provides one or more resulting error values. Any functions deferred by F are executed before F returns to its caller.
There are three ways to return values from a function with a result type:

  1. The return value or values may be explicitly listed in the "x.return" statement. Each expression must be single-valued and be assignable to a subsequent error-typed element of the function's result types, all non-error results are set to their zero values.
func OpenImage(name string) (image.Image, error) {
	f, err := os.Open(name);	err.return fmt.Errorf("failed to open image: %w", err) 
	img, _, err := image.Decode(f);	err.return fmt.Errorf("failed to decode image: %w", err)

	return img, nil
}

func OpenImageSpecificErrors(name string) (img image.Image, openErr error, decodeErr error) {
	f, err := os.Open(name);	err.return fmt.Errorf("failed to open image: %w", err), nil
	img, _, err := image.Decode(f);	err.return nil, fmt.Errorf("failed to decode image: %w", err)

	return img, nil, nil
}
  1. The expression list in the "return" statement may be a single call to a multi-valued function. The effect is as if each value returned from that function were assigned to a temporary variable with the type of the respective value, followed by a "return" statement listing these variables, at which point the rules of the previous case apply.
  2. The expression list may be empty if the function's result type contains only one error type. The "return" statement returns the selected error and all other results set to their zero values.
func OpenImage(name string) (image.Image, error) {
	f, err := os.Open(name);	err.return
	img, _, err := image.Decode(f);	err.return

	return img, nil
}

Proposal: gofmt rules for error.return

  1. An error.return following an error value assignment should always be placed on the same line as the error assignment (or the same line as the last line of a multiline assignment).
  2. IfStmts that are on the subsequent or same line of an error assignment and are equivalent to an error.return statement should be replaced with an error.return statement that abides by the previous rule.
  3. If an error.return is both on the current line and on the previous line, the indentation of both should be set such that both error.return's are aligned. As shown in the selector-specific control flow examples.

Result

func OpenImage(name string) (image.Image, error) {
	wrap := func(in error) error {
		return fmt.Errorf("OpenImage: %w", in)
	}

	f, err := os.Open(name);	err.return wrap(err)
	img, _, err := image.Decode(f);	err.return wrap(err)

	return img, nil
}

func (ds *GitDataSource) Fetch(from, to string) ([]string, error) {
	fmt.Printf("Fetching data from %s into %s...\n", from, to)

	err := createFolderIfNotExist(to);	err.return
	err = clearFolder(to);			err.return
	err = cloneRepo(to, from);		err.return
	dirs, err := getContentFolders(to);	err.return

	fmt.Print("Fetching complete.\n")
	return dirs, nil
}

What impacts will this have on Go

Pros

  • Backwards-compatible with all existing Go code.
  • error.return acts like an annotation, complementing readability rather than interrupting it.
  • The control-flow is mostly explicit, branching behaviour is easily deduced.
  • Fixes the readability of logical-flow in functions that do more error-passing than error-handling.
  • Gofmt enforces single way of passing errors to the caller.
  • No manual changes needed to stdlib or existing Go software's error handling (gofmt will auto-migrate).
  • Not specific to any one pattern of function signature, supports multiple errors, errors that are not the last return type. This makes it easier to copy and paste code with error-passing between functions.
  • Allows error-filtering and error-wrapping patterns by applying a function to the values in an error.return ie filter(err).return and err.return wrap(err)
  • Errors are not hidden.
  • May encourage error wrapping over lazy errors.

Cons

  • Longer lines.
  • Behaviour of error.return and return differ slightly and this could be confusing.
  • Branching behaviour of err.return not explicit.
  • Not found in other programming languages, unfamiliar pattern.
  • May encourage error passing over handling.
@gopherbot gopherbot added this to the Proposal milestone Jun 3, 2020
@gopherbot gopherbot added the Proposal label Jun 3, 2020
@erifan
Copy link
Contributor

@erifan erifan commented Jun 3, 2020

@erifan erifan closed this Jun 3, 2020
@erifan erifan reopened this Jun 3, 2020
@erifan
Copy link
Contributor

@erifan erifan commented Jun 3, 2020

Sorry for the misoperation

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 4, 2020

Has similarities to #33177, #32946, #38151, #39148, and likely others.

@andig
Copy link
Contributor

@andig andig commented Jun 11, 2020

I agree that the error checking and returning in the simple case if verbose in go. On the other hand it forces the developer to think about the actual handling, i.e. to not only return the error but also implement additional cleanup tasks (closing files etc).

In the proposal here I'm unclear how return values for the non-error results would be passed back?

@Splizard
Copy link
Author

@Splizard Splizard commented Jun 11, 2020

@andig
In regards to cleanup tasks, that is what defer and wrapper functions are for.

I want to make the distinction between handling an error (where appropriate action is taken and the error is not returned) and not handling an error (the error is transformed in some way, clean-up tasks are run and the error is returned so that somewhere up the stack it can get handled).

In Go, errors are explicit and a decision to handle needs to be made every time an error value is recieved. This is one of Go's strengths.

Handling errors in Go is great. Returning errors are... not so great. It looks like the error is being handled and this obstructs the readabillity of the code.

In my experience, Go doesn't force you to handle errors, it can actually discourage you from doing so. "My function is incredibly long and unreadable from all this error checking, why would I want to make it worse, I'll just return the error".

An argument can be made that having a shorter way to return errors will encourage less error handling. However I would speculate that the inverse can be true, having shorter functions psychologically gives the developer more room for error-handling. Less is more!

Keep in mind that this proposal is not strictly about saving characters, it's about moving error-passing to the right to improve readability. Personally I'm not a fan of previous check/try/? proposals (not explicit enough, everything is hidden, etc).

To clarify the behavior of non-error results:
error.return returns non-error results set to their zero value. If you need to return different values then you must use a standard return statement.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 16, 2020

It seems like having very long lines with error handling on the right will tend to obscure the error handling. I understand that that is kind of the point of this issue. But errors do need to be handled. Hiding the handling at the end of a long line doesn't clarify the function, it obscures it.

If we don't try to put the error handling at the end of a long line, we could put it on the next line. But then we aren't saving all that much with this approach. We're saving an if, a != nil, and a couple of braces. And admittedly a couple of line breaks.

I feel like the try proposal (#32437) tried to address error handling at a deeper level. That proposal was not accepted, but it had the advantage of trying to recast error handling. This proposal seems to be tweaking error handling a bit. Is the benefit worth the cost of a language change?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 23, 2020

Based on the comments above, and the lack of support in emoji voting, this proposal is a likely decline. Leaving open for four weeks for final comments.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 21, 2020

No further comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.