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: check - a minimal error handling improvement for readability #58520

Closed
mgustavsson opened this issue Feb 14, 2023 · 22 comments
Closed
Labels
error-handling Language & library change proposals that are about error handling. FrozenDueToAge LanguageChange Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Milestone

Comments

@mgustavsson
Copy link

mgustavsson commented Feb 14, 2023

Author:
I have been programming in Go since before the 1.0 release, have also used C/C++, java and python and other languages professionally.

Background:
There have been many proposals regarding error handling, such as #32437 which this proposal have some similarities with. Most Go programmers agree that something could be done to improve error handling, but so far no proposal has been accepted.

What makes this proposal different:
In my opinion, current error handling in Go have these 3 problems:

  1. verbose
  2. err variable pollutes the local namespace
  3. happy path and error handling are interleaved, making code (IMO) less readable

Most error handling proposals have focused on solving problem 1 - this proposal aims to solve all 3 problems. If this proposal is not accepted (statistically likely given the history of error handling proposals :) I at least hope that it will influence other proposals to pay more attention to problem 2 and 3.

Example:
Let's take this example code which IMO illustrates the 3 problems mentioned above:

func readFile(name string) ([]byte, error) {
	f, err := Open(name) // err unnecessarily pollutes local namespace
	if err != nil { // can't indent this line
		return nil, err
	}
	defer f.Close()

	contents, err := readContents(f) // err variable is reassigned a new value
	if err != nil { // can't indent this line
		return nil, fmt.Errorf("error reading %s: %w", name, err)
	}

	// the validate call is in the middle of a lot of boilerplate, impacting readability
	// here a new err variable shadows the existing one, a potential for confusion/bugs
	if err := validate(contents); err != nil { 
		return nil, err
	}

	return contents, nil
}

Here is how the code would look like with this proposal accepted:

func readFile(name string) ([]byte, error) {
	f := check Open(name)
	defer f.Close()

	contents := check readContents(f); err {
		return nil, fmt.Errorf("error reading %s: %w", name, err)
	}

	check validate(contents)

	return contents, nil
}

Further discussion:
This proposal aims to:

  1. mimimize boilerplate
  2. avoid polluting local namespace with error variables
  3. keep the happy path to the left of each line, move error handling to indented blocks to improve readability
  4. the "check" keyword still makes it clear where the function can return
  5. keep it easy to handle errors when necessary, such as logging or wrapping them

Details:
This is obviously not up to language spec standard, but I have tried to address all the specific details I could think of:

  • The check keyword can only be used as a top-level statement in a code block or on the right hand side of a assignment statement.

  • The check keyword is followed by a function call which has a last return parameter of type error OR an expression which has type error.

  • The resulting values of a check statement on a function call is the remaining return parameters without the last error return parameter.

  • If the check statement is applied to an expression with type error there is no resulting values and it is a compile error to try to use it on the right side of an assignment statement.

  • The function call/expression can be followed by a semicolon, a variable name and a code block. If the error value that was extracted by the check statement is not nil, the variable name will be assigned the error value and the code block will be executed. The variable is local to the check statements code block. If the error value is nil, no assignment will happen and the code block is not executed.

If the check statement does not have a code block then it behaves like this:

check f(); err {
	return ..., err
}

Where ... is replaced with the zero values of the remaining return types of the enclosing function. If a return parameter in ... is a named returned parameter, the current value of that parameter is returned.

Omitting the error handling block is only allowed inside a function which has a last return parameter of type error.

Alternative 1a/b:
Check could be replaced with "try". The semicolon could be replaced with else.

func readFile(name string) ([]byte, error) {
	f := try Open(name)
	defer f.Close()

	contents := try readContents(f) else err {
		return nil, fmt.Errorf("error reading %s: %w", name, err)
	}

	try validate(contents)

	return contents, nil
}

Alternative 2:
Alternatively a postfix operator could be used instead of the "check" keyword:

func readFile(name string) ([]byte, error) {
	f := Open(name)?
	defer f.Close()

	contents := readContents(f)? err {
		return nil, fmt.Errorf("error reading %s: %w", name, err)
	}

	validate(contents)?

	return contents, nil
}

Alternative 3:
Put the check keyword first on the line to simplify the grammar and maximize clarity of where the function can return.

func readFile(name string) ([]byte, error) {
	check f := Open(name)
	defer f.Close()

	check contents := readContents(f); err {
		return nil, fmt.Errorf("error reading %s: %w", name, err)
	}

	check validate(contents)

	return contents, nil
}
@gopherbot gopherbot added this to the Proposal milestone Feb 14, 2023
@seankhliao seankhliao added LanguageChange v2 A language change or incompatible library change error-handling Language & library change proposals that are about error handling. labels Feb 14, 2023
@seankhliao
Copy link
Member

Duplicate of #49091 #52175

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Feb 14, 2023
@mgustavsson
Copy link
Author

mgustavsson commented Feb 14, 2023

Fully understand that you are tired of error handling proposals @seankhliao :) - but I think it is unfortunate that this was closed so quickly as there are some differences from the other proposals you mentioned.

The proposal in #49091 was:
check doSomething with func(err error) {return fmt.Errorf("error doing something %w", err)}
which is significantly more verbose to read and write than this proposal thereby defeating the whole purpose of "reducing boilerplate". Also it is not detailed how the return from the anonymous function translates into the return value from the outer function. This proposal does not have these issues.

As for #52175 - @ianlancetaylor asked a couple of technical questions regarding how it should work - I think this proposal answers all such technical issues in the "Details" section above. The issue was then closed by the creator with this comment: #52175 (comment) without the issue being reviewed by the proposal team. I think this might have been premature as there seemed to be continuing discussion and interest after the closing of the issue and the "serious problem" referenced in that comment does arguably not apply to this proposal since the "check" keyword directly after the assignment operator makes it quite clear what is happening.

I would also like to mention that the purpose of this proposal is not only to reduce boiler plate, but to make go code in general more readable and reduce potential bugs with shadowing/reassigned error variables.

@seankhliao
Copy link
Member

#49091

allows plain

  • check f()
  • x := check f()

this is equivalent to the short forms here

#32848 #41908

is pretty much your extended form

@mgustavsson
Copy link
Author

Thanks for your reply, I appreciate you taking your time.

#49091 uses anonymous functions and function handlers everywhere, this proposal uses statement blocks. While the general idea is the same, there is a large difference in usability. It seems 49091 was rejected because of the issues in this comment: #49091 (comment). Of the 4 points in that comment only the first one is directly applicable to this proposal, the rest are fairly irrelevant because they are more about issues in the specific example code used in that proposal rather than the proposal itself.

#32848 and #41908 both have the err variable on the left side of ":=" - which pollutes the namespace with the variable which is one of the main issues this proposal tries to solve. The main reason 32848 was closed was due to "The scoping issue is not resolved." (#32848 (comment)) - scoping is clearly defined in this proposal so that reason does not apply.

In 41908 davecheney's critique was that the proposed change does not improve readability, in this proposal I provide an example of how readability is (IMO) improved by moving error handling inside blocks and spacially separating it from the core logic. @ianlancetaylor asked if the error blocks can be used on intermediary results and seemed to suggest that was not a good thing - in this proposal they can not.

Yes, all of these proposals you mention are very similar, i totally agree - but when I read the discussions on them they are all closed because of arguments which do not necessarily apply to this proposal. Sometimes small differences make a difference - or a fresh perspective / convincing argument. My main argument is that this proposal would make go code in general more readable to an acceptable cost in increased complexity of the language specification.

@ianlancetaylor
Copy link
Contributor

@mgustavsson Thanks for the proposal. This introduces a new kind of variable declaration and a new kind of code block. I'll reopen it since it's slightly different and we can see if people have comments. But it's quite unlikely that we would accept this as written.

@mgustavsson
Copy link
Author

mgustavsson commented Feb 14, 2023

Thanks, appreciate it!

Again the main argument is that this:

func readFile(name string) ([]byte, error) {
	f := check Open(name)
	defer f.Close()

	contents := check readContents(f); err {
		return nil, fmt.Errorf("error reading %s: %w", name, err)
	}

	check validate(contents)

	return contents, nil
}

is more readable than this:

func readFile(name string) ([]byte, error) {
	f, err := Open(name)
	if err != nil {
		return nil, err
	}
	defer f.Close()

	contents, err := readContents(f)
	if err != nil {
		return nil, fmt.Errorf("error reading %s: %w", name, err)
	}

	if err := validate(contents); err != nil { 
		return nil, err
	}

	return contents, nil
}

while also always keeping err variables local to the error handling scope, thanks.

@ianlancetaylor
Copy link
Contributor

Note that due to lexical semicolon insertion this:

	contents := check readContents(f); err {
		return nil, fmt.Errorf("error reading %s: %w", name, err)
	}

is equivalent to this:

	contents := check readContents(f)
	err {
		return nil, fmt.Errorf("error reading %s: %w", name, err)
	}

So I'm not sure this syntax quite works.

@mgustavsson
Copy link
Author

mgustavsson commented Feb 14, 2023

If semicolon is not an option i propose to use else instead (alternative 1b in the proposal):

	contents := check readContents(f) else err {
		return nil, fmt.Errorf("error reading %s: %w", name, err)
	}

That being said, for loops and if statements use semicolons to mark optional statements before a block so maybe it is possible here too.

@ianlancetaylor
Copy link
Contributor

This idea is very similar to #21161 (comment) (although as far as I know that was never filled out into a full proposal).

@apparentlymart
Copy link

apparentlymart commented Feb 15, 2023

It seems like the closest current form to what's being proposed here is the if statement with a "simple statement" appearing before the predicate, like this part of the motivating example:

	if err := validate(contents); err != nil { 
		return nil, err
	}

The form above already confines the err symbol to the body of the if statement, solving your problem statement 2, and the happy/error path interleaving seems the same as your proposal and so I think both this and your proposal are equal under problem statement 3.

I believe the following is the closest equivalent to the above example under your proposal:

	check validate(contents); err {
		return nil, err
	}

This has:

  • Replaced if with check.
  • Made the err != nil predicate implied rather than explicit.
  • Moved the err variable declaration to after the semicolon, replacing the now-implied predicate.

I would agree that there's less boilerplate in this form than in the if statement form I included above, and therefore it might represent an improvement to problem statement 1: "verbose".

A typical reason I've heard for not using this if err := ...; err != nil { pattern is that to some readers it "hides" the most important information -- the function call -- behind some other content. It therefore makes it harder to skim the code. I don't personally have a strong opinion on that, but it does seem like your proposal reduces the amount of content appearing before the function call, reducing it to just the single keyword check. Perhaps with some time to grow accustomed to it Go readers would find that comfortable to skim.

Based just on the above I don't feel super convinced this is enough of an improvement to justify the exception in the language. I do agree that it is an improvement, but the improvement feels marginal by this criteria.


I think you've buried the lede a bit in your proposal because what you suggested does introduce something new that's materially different from the typical if statement but that I didn't quite lock onto until a second read: it allows declaring new symbols both in the parent scope (the non-error results) and the child scope (the error result) without requiring a separate declaration statement.

I missed it on my first read because the example you chose for the if block was a function that only returns an error, but let's consider how my if statement would look for your readContents example.

I see two main ways to lay it out. The first would be to indent both the happy path and the error path so that neither appears in the parent scope:

	if contents, err := readContents(f); err == nil {
		// happy path using "contents"
	} else {
		// error path using "err"
	}

I'm sure most would agree that the above is not idiomatic because we typically expect the happy path to be unindented and only the error branch to be indented.

The other way is to hoist one of the variable declarations out of the if:

	var contents []byte
	if contents, err := readContents(f); err != nil {
		// error path using "err"
	}
	// happy path using "contents"

Of course in practice I'd probably write this just like you wrote it in your original example, with the whole call on the line above the if statement and only if err != nil, but I've written it this way because it's the closest I can get to meeting your criteria 2 (err only being declared in the nested scope) with today's Go. And of course this is only true if you exclusively write it this way, so that no other earlier statements have already declared an err in the parent scope.

One thing I quite like about your proposal then is that it quite neatly handles this "scope splitting" goal, allowing contents and err to both be declared by the same statement but for err to belong only to the child scope:

	contents := check readContents(f); err {
		// error path using "err"
	}
	// happy path using "contents"

I will say that the exact details of the syntax feel a little awkward to me -- it isn't super clear that the err after the semicolon is effectively a variable declaration -- but I think the broad idea is interesting and does add something new to the language, albeit perhaps a relatively small thing. It is at least cosmetically similar to the switch statement form that has a simple statement embedded in it, like switch foo := bar(); foo {, but even in this case that terminal foo is an expression rather than a declaration.

But I think it's probably more important to discuss what the desirable properties of a solution are than to debate the exact details of syntax, so I won't quibble too much about the syntax details. If it seems (to others in this discussion) like this "scope splitting" behavior is desirable then I'm sure there are various other possible syntax shapes to consider that would make different tradeoffs of terseness vs. similarity to existing language constructs.

I'm also not super convinced that this new capability is sufficiently useful to justify the extra language complexity, but I could imagine myself getting used to this pattern and enjoying it if it were in the language.

@earthboundkid
Copy link
Contributor

One issue with the old try as an expression proposal is that you can do try try try a()()() and other weird things. If it’s an if-like statement, you don’t have that problem. Swift has guard. You could say guard x := f() has an implicit if err != nil return clause, and

guard x, err := f() {
  // err is in scope here
}
// but not here

@mgustavsson
Copy link
Author

This idea is very similar to #21161 (comment) (although as far as I know that was never filled out into a full proposal).

@ianlancetaylor Yes that comment seems the same as this proposal's "Alternative 2" with "::" instead of "?" and with the code block required and not optional.

guard x, err := f() {
  // err is in scope here
}
// but not here

@carlmjohnson very similar to this, but with that syntax it seems confusing to me that x and err have different scopes.

The form above already confines the err symbol to the body of the if statement, solving your problem statement 2, and the happy/error path interleaving seems the same as your proposal and so I think both this and your proposal are equal under problem statement 3.

@apparentlymart I think there is significantly less noise around the core logic here:

	check validate(contents); err {
		return nil, err
	}

	// or just:
	check validate(contents)

than here:

	if err := validate(contents); err != nil { 
		return nil, err
	}

so IMO there is a fairly big readability difference, especially if you take into account that you could apply this in a lot of places. It could probably shrink many code bases by 10-20%.

	var contents []byte
	if contents, err := readContents(f); err != nil {
		// error path using "err"
	}
	// happy path using "contents"

That will actually not work, it creates a new contents variable instead of assigning to the outer one: https://go.dev/play/p/oHMR4b0hS_j

@mgustavsson
Copy link
Author

Just as an example, I noticed that @rsc posted the following code in this comment #58000 (comment):

func tarAddFS(w *tar.Writer, fsys fs.FS) error {
	return fs.WalkDir(fsys, ".", func(name string, d fs.DirEntry, err error) error {
		if err != nil {
			return err
		}
		if d.IsDir() {
			return nil
		}
		info, err := d.Info()
		if err != nil {
			return err
		}
		h, err := tar.FileInfoHeader(info, "")
		if err != nil {
			return err
		}
		h.Name = name
		if err := w.WriteHeader(h); err != nil {
			return err
		}
		f, err := fsys.Open(name)
		if err != nil {
			return err
		}
		defer f.Close()
		_, err = io.Copy(w, f)
		return err
	})
}

To me, the sheer amount of error handling here obscures the core logic.

With this proposal it could be written like this:

func tarAddFS(w *tar.Writer, fsys fs.FS) error {
	return fs.WalkDir(fsys, ".", func(name string, d fs.DirEntry, err error) error {
		check err

		if d.IsDir() {
			return nil
		}

		info := check d.Info()
		h := check tar.FileInfoHeader(info, "")
		h.Name = name
		check w.WriteHeader(h)

		f := check fsys.Open(name)
		defer f.Close()

		_, err = io.Copy(w, f)
		return err
	})
}

That is a reduction of non-empty lines in the function body of 52% (from 25 to 12).

Not sure if it was intended to add any error wrapping to this code or not, with error handling applied the reduction in lines would be less dramatic (but still significant) - but readability would still be improved significantly IMO.

@mgustavsson
Copy link
Author

mgustavsson commented Feb 15, 2023

@mgustavsson Thanks for the proposal. This introduces a new kind of variable declaration and a new kind of code block. I'll reopen it since it's slightly different and we can see if people have comments. But it's quite unlikely that we would accept this as written.

@ianlancetaylor yes - the check statement in this proposal is a fairly large addition to the language. But improving error handling has been identified as one of the top priorities along with generics and the community has been looking for a solution for half a decade. Any viable proposal to error handling is going to be a major change.

@generikvault
Copy link

I like the implicit non nil checks with the explicit return of the error from your proposal. I understand why you write the declaration of err but I think this could still be part of the normal variable declaration. The ´check´ could be tied to the variable instead of the function call, indicating that the variable will be non nil checked

The following would be a 'smaller' language change, could work with other types than error and would still fullfill most of the goals of this proposal:

func readFile(name string) ([]byte, error) {
    f, check err := Open(name) {
        return nil, err
    }
    defer f.Close()

    contents, check err := readContents(f) {
        return nil, fmt.Errorf("error reading %s: %w", name, err)
    }

    // the validate call is in the middle of a lot of boilerplate, impacting readability
    // here a new err variable shadows the existing one, a potential for confusion/bugs
    check err := validate(contents) { 
        return nil, err
    }

    return contents, nil
}

Since it is impossible to confuse assignments and comparissons in declarations because you cannot compare a not declared variable. This could actually use the existing if keyword instead of check:

func readFile(name string) ([]byte, error) {
    f, if err := Open(name) {
        return nil, err
    }
    defer f.Close()

    contents, if err := readContents(f) {
        return nil, fmt.Errorf("error reading %s: %w", name, err)
    }

    // the validate call is in the middle of a lot of boilerplate, impacting readability
    // here a new err variable shadows the existing one, a potential for confusion/bugs
    if err := validate(contents) { 
        return nil, err
    }

    return contents, nil
}

@apparentlymart
Copy link

Thanks for the extra context! I think on first read I didn't notice the possibility of using check without a block after it, and so my examples were focused on the case where there is a block. I think previous proposals have been criticized for having an implied return statement that isn't explicitly written in the program, and so I expect there will be some similar concerns here too. In my first read I liked the examples that still include return statements, because they address that concern from earlier discussions.

And indeed you're right about my example of declaring a variable in the parent scope and then shadowing it in the if statement. I think that observation only strengthens the benefit I was intending to draw attention to: there is currently no way to assign results in the parent scope while having the error declared only in the child scope (if we assume that's important, which I did here because it was one of your explicit design goals.)

@mgustavsson
Copy link
Author

@apparentlymart the ability to omit the error handling block could be dropped from this proposal if the consensus is that that is preferable.

However, I think having the option to omit them is worthwhile - it is not realistic or even desirable to handle/annotate all errors multiple times at every level of the call stack. As an example, the code by Russ Cox I posted above exclusively used bare returns.

Another interesting statistic I found was this: #31442 (comment) - according to this comment the number of annotated error returns in the kubernetes code base was 2,800 while the number of bare error returns was 30,000 ...

@mgustavsson
Copy link
Author

Regarding how this would be integrated into the language grammar - there are of course several possible ways to do it depending on how many places you would want to allow checks to be used. Given that one of the criticisms of proposal #32437 was that try could be used in any expression and thus lead to very complicated code my suggestion would be the following:

Statement =
	Declaration | LabeledStmt | SimpleStmt |
	GoStmt | ReturnStmt | BreakStmt | ContinueStmt | GotoStmt |
	FallthroughStmt | Block | IfStmt | SwitchStmt | SelectStmt | ForStmt |
	DeferStmt | CheckStmt .

CheckStmt = [IdentifierList ":="] "check" Expression [";" identifier Block] .

";" would be replaced with "else" if that is considered preferable.

From a grammar point of view it would be cleaner to put the check keyword first in the statement to be more similar to "if", "for" etc.. in that case the grammar would be:

CheckStmt = "check" [IdentifierList ":="] Expression [";" identifier Block] .

This is not as clean from a readability standpoint IMO, but it would on the other hand make check always be first on the line so that it becomes as visible as return.

@ianlancetaylor
Copy link
Contributor

As discussed, this is similar to other ideas that have been proposed before. While error handling remains something to consider, this change doesn't bring us enough benefit for the cost of the new syntax. It introduces a new keyword, a new declaration syntax, new control flow if there is no block, and an unusual use of semicolon or else to introduce an associated block.

Therefore, this is a likely decline. Leaving open for four weeks for final comments.

@bdandy
Copy link

bdandy commented Mar 9, 2023

I like check syntax, but to make real difference I would like to add note.

Pretty ofter I would like to use

if a, err := test(); err != nil {
   /// err here
}
// use a here

but it's not possible without defining a outside the if statement, which is odd.

So by using check syntax it would be lovely to have

a, b, c := check test(); err {
   // error handling
}
// using a, b, c return values

Also, instead of new keyword we could use check function, but it's not possible to make return from lambda like this:

func readFile(name string) ([]byte, error) {
    check := func(e err) {
          break nil, fmt.Errorf("readfile: %w", e) // returns from the readFile
    }

    f, err := Open(name)
    check(err) // returns from here if error
    defer f.Close()

    contents, err := readContents(f) 
    check(err) // returns from here if error

    err := validate(contents) 
    check(err) // returns from here if error

    return contents, nil
}

In C you can use goto keyword, but that's bad practice as it's hard to follow path.

@Pat3ickI
Copy link

Why don’t make it as a directive ? like //go:check {NilType Return Name}, this can accept a wide range and not just errors but any types that can have nil value. in code

func readFile(name string) (content []byte, err error) {
	//go:check err 
        f, err := Open(name)
	defer f.Close()

	content, err = func(fs *File) (f, []byte, err error) {
	        
	        //go:check f, err
                f, err = fs.Body()          

        }
        //go:check err
	err = validate(contents)
	return 
}

Since func() (err error) “err” can either return the default or the value assigned to before //go:check

@ianlancetaylor
Copy link
Contributor

No change in consensus.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Apr 12, 2023
@golang golang locked and limited conversation to collaborators Apr 11, 2024
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 Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

9 participants