Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: Go 2: Error-Handling Paradigm #60720

Closed
nixpare opened this issue Jun 10, 2023 · 28 comments
Closed

proposal: Go 2: Error-Handling Paradigm #60720

nixpare opened this issue Jun 10, 2023 · 28 comments
Labels
error-handling Language & library change proposals that are about error handling. LanguageChange Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Milestone

Comments

@nixpare
Copy link

nixpare commented Jun 10, 2023

Author background

  • Would you consider yourself a novice, intermediate, or experienced Go programmer?
    I'm a student, but i'm mainly using go from the first day I started programming and now I'm using
    it for 3 years. I coded a bunch of libraries, some targeting specific needs for different
    OSes, the largest one is a TCP/HTTPs server library (Server v2).
  • What other languages do you have experience with?
    I use regularly Javascript, Java, SQL databases (HTML and CSS if you consider them programming languages).

Related proposals

  • Has this idea, or one like it, been proposed before?
    Yes, there have been a lot of proposals related to this topic.
    • If so, how does this proposal differ?
      First of all, this proposal matches the official guidelines related to this topic.
      Then it will be completely backwards compatible and will not introduce any weird
      constructs that will preserve the readability and simplicity of the Go code.
  • Does this affect error handling?
    Yes, the topic is the error handling.
    • If so, how does this differ from previous error handling proposals?
      This does not to implement anything like a try-catch, considering that this is already
      too similar to the panic-recover already present.

      This proposal has more of a macro-like behaviour, reducing the possibility of overworking
      on a new mechanic, with the risk of diverging from the Go principle of "error checking".

Error Handling Proposal

This proposal addresses the problem that affects every project written in Go
that has to deal with a lot of error handling, resulting in a very verbose
code, without removing the principle that you should always handle every
error returned by a function in the best way.

This would benefit every go programmer, considering it's about error handling, and
is a widely discussed topic among every programmer, even non-go ones.

Changes

The proposal, by adding only a new keyword to the language, the catch one, will add
to the language 2 new strictly related construct (more like 1 construct and 1 prefix):

  • the definition of an error-handler function that is relevant only for
    the function it's declared in; this construct will be presented below
  • the explicit statement that you want to trigger the previously-declared function above;
    this prefix will be presented below

The backwards compatibility will be discussed below along with code snippets showing the changes.

We will discuss also interaction between other language features, like goroutine functions ran with
the go keyword (this proposal has a similar concept, that is adding a prefix to a function call to
explicitly say that you want to tread that function differently from normal).

This proposal is not about performance improvements, but (with my knowledge) should not even affect it
in a bad way.

Introduction

Here is a good example of a code that would benefit from the introduction
of this concept:

package myPackage

import (
	"encoding/json"
	"fmt"
	"io"
	"os"
	"time"
)

type MyStruct struct {
	Message string    `json:"message"`
	Time    time.Time `json:"time"`
}

func LoadJSONFile(filePath string) (*MyStruct, error) {
	f, err := os.Open(filePath)
	if err != nil {
		return nil, fmt.Errorf("could not load json file <%s>: %w", filePath, err)
	}
        defer f.Close()

	data, err := io.ReadAll(f)
	if err != nil {
		return nil, fmt.Errorf("could not load json file <%s>: %w", filePath, err)
	}

	var result *MyStruct
	err = json.Unmarshal(data, result)
	if err != nil {
		return nil, fmt.Errorf("could not load json file <%s>: %w", filePath, err)
	}

	return result, nil
}

It's obvious that there is a lot of code that is redundant and verbose: this could be
fixed if we could tell the compiler what to do every time a function returns as the
last return-value an error and it is not nil, like a macro. Here is an example:

func LoadJSONFile2(filePath string) (*MyStruct, error) {
	// declaration of the `catch function`
	catch (err error) {
		// the return statement (if present) must match the calling function signature
		return nil, fmt.Errorf("could not load json file <%s>: %w", filePath, err)
	}

	f := catch os.Open(filePath)
        defer f.Close()

	data := catch io.ReadAll(f)

	var result *MyStruct
	catch json.Unmarshal(data, result)

	return result, nil
}

Note: in this proposal there are two different usage of the catch
keyword:

  • one is the declaration of the macro function, I will refer to
    it as catch function
  • the other one is the request to handle an error with the catch function,
    I will refer to it as catch prefix in the context of a function call

First Usage

The first usage of the catch keyword is to tell the compiler that from
this point until the end of the function, without inheritance
from other function calls inside, if you call other functions
with the catch prefix, it should implicitly call the catch function
only if err != nil.

To set the catch function you must use this syntax only:

catch (<variable_name> error) { ... }

without using func() { ... } or any pre-declared function (like with defer or go)
to underline that this catch function should be explicitly made
for this function errors and should not be handled by a function
that is used for every error in the program/package.
However once inside the catch function body, it's always possible
to call other functions as a normal function.

The catch function must not have a return signature (like a 'void'
function) because in reality it's implicitly matching the one
of the calling function (in this case (*MyStruct, error)).
This is also why the catch function must be declared inline.

Second Usage

The second use case of the catch keyword is by placing it
in front of a function call that has as the last return type an error.
By doing this you can avoid taking this last error and
tell the compiler to handle it automatically with the catch
function if the error is not nil (removing the need constantly use of the
construct if err != nil { ... }).

Function with named parameters in the return statement

The catch argument can have whatever name you desire, but it
must be an error. In this case if you named it err, the
one declared in the function signature would be shadowed.

func LoadJSONFile3(filePath string) (result *MyStruct, err error) {
	catch (catchErr error) {
		// This is what the catch function can be if the return
		// signature has variable names
		err = fmt.Errorf("could not load json file: %w", catchErr)
		return

		// Or directly
		return nil, fmt.Errorf("could not load json file: %w", err)
		// It has all the possible return statements that you
		// would have in a normal function body
	}

	// Same stuff ...

	return
}

Illegal Usage and Skip error check

func LoadJSONFile4(filePath string) (*MyStruct, error) {
	// This is illegal, because the `catch function` is not
	// declared at this point (like accessing a non-already
	// declared variable)
	fileStats := catch os.Stat()   // ILLEGAL

	catch (err error) {
		return nil, fmt.Errorf("could not load json file: %w", err)
	}

	// this is obviously illegal because i have not used the catch
	// prefix on the function call, preserving backwards compatibility
	f := os.Open(filePath)   // ILLEGAL
        defer f.Close()

	// This is how we already explicitly ignore returned errors and
	// this will be the only way to really ignore them
	data, _ := io.ReadAll(f)

	// also this is how we already ignore returned errors if the
	// function only returns one error. Not the absence of the
	// `catch` keyword before the function
	json.Unmarshal(data, result)
	return
}

Manually trigger the catch function

From now we are going to change the context a little bit
for the next sections

We will be using the same MyStruct with a message and a timestamp
and two functions, one private and one public with different
arguments but the same return signature, both used to create a new
instance of the struct.

This is widely used in packages that has a private method that exposes
more options and a public one that has less arguments and maybe also
does some sanity check first

The private method just checks if the message is not empty and asks
for an explicit timestamp that will be directly injected in the returned
struct.
The public method instead only asks for a message, the time will be
time.Now(), and also checks that the message is not on multiple lines.

var (
	MultilineError = errors.New("message is multiline")
)

func NewMyStruct(message string) (*MyStruct, error) {
	catch (err error) {
		return nil, fmt.Errorf("could not create MyStruct: %w", err)
	}

	if strings.Contains(message, "\n") {
		catch errors.New("message is multiline")
		// or
		catch MultilineError
		// or (similar)
		catch fmt.Errorf("message of length %d is multiline", len(message))
	}

	//... will be continued in the next code snippet
}

The catch function can be manually triggered by using the
catch keyword followed by an error value, this being the only
returned value of a function or a pre-existing variable.

In reality this is no so much different from what we have already
discussed above, but has a slightly different purpose, so here it is.
In fact the only thing that is doing is calling a function that returns only
an error (like the json.Unmarshal(...) before), but we surely know that the
returned error will not be nil.

Discussion

Functions returning other functions

Now let's see when a function returns in the end another function
with the same return signature. Here is the original code snippet:

func newMyStruct(message string, t time.Time) (*MyStruct, error) {
	if message == "" {
		return nil, errors.New("message can't be empty")
	}

	return &MyStruct{ Message: message, Time: t }, nil
}

func NewMyStruct(message string) (*MyStruct, error) {
	// previous stuff ...

	result, err := newMyStruct(message, time.Now())
	if err != nil {
		return nil, fmt.Errorf("could not create MyStruct: %w", err)
	}
	return result, err
}

In this case this is what the last return statement
will be equal to with the catch:

func newMyStruct(message string, t time.Time) (*MyStruct, error) {
	// Same as above ...
}

func NewMyStruct(message string) (*MyStruct, error) {
	catch (err error) {
		return nil, fmt.Errorf("could not create MyStruct: %w", err)
	}

	if strings.Contains(message, "\n") {
		catch errors.New("message is multiline")
	}

	return catch newMyStruct(message, time.Now())
}

And if you think that you should not modify the returned
values, you can always directly use

return newMyStruct(message, time.Now())

without the catch prefix: the compiler will know that you do not want
to execute the catch function on the error.

Inheritance

As previously mentioned, this is not like a defer-recover construct:
any error, even in the function called, is not handled by the most
recent version of the catch version, but only by the one inside the same
function body
. In this way it be easier for an LSP (go vet, staticcheck
or others similar) to detect an improper use of the catch function.

One thing to be pointed out is that the catch function should not
be treated as a function variable, because there could be problems with
inline-declared function, especially used with goroutines:

A function like that does not have to match the "parent" function return
statement
in which is declared, and normally doesn't (goroutines
function usually does not return anything): this breaks the principle behind the catch macro style and also goes against the reason why the
catch function must be declared for each distinct function,
that is to discourage the usage of a universal function handler for every
function in the program/package.
So:

func Foo() error {
	catch (err error) {
		return nil, fmt.Errorf("could not load json file: %w", err)
	}

	// Some stuff with error handling with the catch ...

	wg := new(sync.WaitGroup)
	wg.Add(2)

	go func() {
		defer wg.Done()
		value1, value2 := catch SomeFunctionThatReturnsAnError() // This is invalid

		// Use of the values ...
	}()

	f2 := func() {
		defer wg.Done()

		catch (err error) {
			log.Printf("error occurred in the second goroutine: %v\n", err)
			return
		}

		value1, value2 := catch SomeFunctionThatReturnsAnError() // This is completely valid

		// Use of the values ...
	}
	go f2()

	wg.Wait()
	return nil
}

The return statement inside the catch function

It was not previously mentioned, but the return statement
inside the catch function is not mandatory, in fact if you
would not use the return statement, the catch function will
be executed normally, doing everything it contains, and then
continuing the function execution.

Again, one thing is what the language lets you do, one thing
is what is actually correct to do in a program.
An example that demonstrates that this kind of logic is already
possible is the following:

func LoadJSONFileWithLogging(filePath string) *MyStruct {
	f, err := os.Open(filePath)
	if err != nil {
		log.Printf("could not load json file: %v", err)
	}
        defer f.Close()

	data, err := io.ReadAll(f)
	if err != nil {
		log.Printf("could not load json file: %v", err)
	}

	var result *MyStruct
	err = json.Unmarshal(data, result)
	if err != nil {
		log.Printf("could not load json file: %v", err)
	}

	return result
}
func LoadJSONFileWithCatchLogging(filePath string) *MyStruct {
	catch (err error) {
		log.Printf("could not load json file: %v", err)
	}

	f := catch os.Open(filePath)
        defer f.Close()
	
	data := catch io.ReadAll(f)

	var result *MyStruct
	catch json.Unmarshal(data, result)

	return result
}

Note: both functions do not return any error

Change of the catch function

One last thing that can be discussed is the possibility to change the
catch function behaviour inside the same function: honestly, I thing this
would be kind of overkill, if you have the need to constantly changing the
catch function, you would probably be better controlling each error
like we are used to. But an example would be like this:

func LoadJSONFile5(filePath string) (*MyStruct, error) {
	catch (err error) {
		return nil, fmt.Errorf("could not load json file: %w", err)
	}

	f := catch os.Open(filePath)
        defer f.Close()

	// Other stuff with more error handling with the first catch function ...

	catch (err error) {
		return nil, fmt.Errorf("another error while loading json file: %w", err)
	}
	// From now, every catch prefix will use this second catch function

	data := catch io.ReadAll(f)

	var result *MyStruct
	catch json.Unmarshal(data, result)

	return result, nil
}

How the compiler should behave (conceptually)

At compile time, considering the catch function is valid, the compiler should
literally insert that function's code in the right places as can be evinced from
the differences between the old and the new code.

Conclusions

I really think that this would make people save a lot of lines of code,
while not compromising readability of the code and backwards
compatibility. I think this could be even implemented, without considering
development time, in a go v1.21 or go v1.22.

This change will not make Go easier to learn, but not even harder. This should
make writing Go code, both for production and for personal use, more pleasing
and less tedious.

As stated before, this should not have any performance impact on programs, thanks
to the nature of this feature, that is being a like a macro. This obviously will
put more strain on the compiler and any LSP, but I don't think this will be huge.

This is my first proposal I do in my life so sorry if I missed some
aspects and I remain available for discussions.

Hope you like this idea. Thanks for the time dedicated to this proposal

@gopherbot gopherbot added this to the Proposal milestone Jun 10, 2023
@seankhliao seankhliao added error-handling Language & library change proposals that are about error handling. v2 A language change or incompatible library change labels Jun 10, 2023
@seankhliao seankhliao changed the title proposal: Error-Handling Paradigm (backwards compatible) proposal: Go 2: Error-Handling Paradigm Jun 10, 2023
@seankhliao
Copy link
Member

The introduction of a new keyword makes this not backwards compatible.

This looks (almost?) identical to the error handling draft, feedback.
What's new from that?

@nixpare
Copy link
Author

nixpare commented Jun 10, 2023

Okay, genuinely I did not know anything about this draft and I feel bad about this. However, after reading it, the only things that differ are:

  • the handlers chaining possibility, obviously a nice to have
  • the existence of a default handler which is a return statement with zero values and the error, which makes impossible to handle errors and continue the execution of the function normally

For the first difference, I am sure that the chaining possibility has more value than being able to change the handler, considering that it is still possible by adding to the chain an handler with a return statement, essentially shadowing every previous handler.

But for the second difference, I think that would be sort of a limitation to have to return at the end of the chain:
Maybe a good way of dealing with this would be to have a default handler that is used when a catch statement is called without having explicitly declared any handler in scope, but as soon as you declare the first handler in the chain, the default handler is ignored.

One last thing, and it is for my better understanding: i thought that "being backwards compatible" meant that in the new version, every type of code written in previous versions would just work without any difference, but obviously any newer code written with the new statements and constructs would not work on older versions. So adding a keyword would break backwards compatibility only because someone in his code maybe have used the same work for a variable (for example) right? Or there is another aspect that I am not aware of?

I'm again sorry for not being aware of this draft.
Thanks for the time

@ianlancetaylor
Copy link
Contributor

Yes, as you say, adding a keyword is pretty much always incompatible, because it breaks code that has variables/types/functions/constants whose name is that keyword.

That doesn't mean that we can never add a new keyword. After all, it's not hard to automatically rename all such objects. But it does impose a significant cost. All language changes have costs and benefits, and whether to adopt the language change requires weighing those costs and benefits.

@nixpare
Copy link
Author

nixpare commented Jun 11, 2023

Thanks for the insight.

But going back to the error handling, I am curious about the reasons why the catch statement would result in the enclosing function to exit (if the error is not nil), without having the possibility to handle the error in some way (maybe, like in my proposal, just by logging it) and let the function continue.

I also think that having a built-in implicit default handler is a really "nice to have", but maybe would encourage people to not handle errors in the most reasonable why. Normally is just enough to wrap that error inside a new error with additional description of the context: in fact, I don't think that this code

handle err {
    err = fmt.Errorf("this is the context: %w", err)
}

is so different and more verbose than

handle err {
    return zero_value_1, ..., zero_value_n-1, fmt.Errorf("this is the context: %w", err)
}

also because you have circumstances where the zero value for an operation is different from the zero value of the type (I am thinking about searching the index of something in a string, array, slice, ecc.)

@nixpare
Copy link
Author

nixpare commented Jun 11, 2023

No okay, as @seankhliao pointed out, now we are talking about changes that, at least for me, should be made to the original draft about error handling. Maybe this proposal should have a different title now and I should add at the top of my proposal a note that guides to what we are talking about now, leaving the rest of the message intact to keep the entire context.

This looks (almost?) identical to the error handling draft, feedback. What's new from that?

So, given that we take everything as it is in the draft mentioned above, that has all the same principle as mine (just more polished and with some syntax differences), the only problem that I have with the original draft is that if you call a function with a check, the enclosing function at the end will always return, there is no way you can keep the function alive (quote taken from the draft:)

For non-nil values, check calls the handler chain with this value, sets the return values, if any, with the results, and returns from the enclosing function.

For dealing with this problem, in my previous post, I proposed the fact that the default handler (which is the cause for inevitable return statement) should be there until the first handle statement was declared in the function explicitly.

Maybe another way to handle this (sorry for the word) is to add the possibility to control the chain flow with continue and break statements, where the first one would jump the execution to the next handler directly, and the second one would instead exit the handler chain.

@rjeczalik
Copy link

All of these examples in this proposal are leaking a file descriptor, by not closing a file on error. I might have missed it, but I don't see a way to register cleanup functions, which could have helped here.

@nixpare
Copy link
Author

nixpare commented Jun 11, 2023

I you are talking about the examples in my code, you kind of right, I missed the f.close(), but that's not the point of this discussion (defer function would still work without problems).

However, for the sake of correctness, I will update the code snippets.

@rjeczalik
Copy link

defer function would still work without problems

No, it won't. For a simple cases, yes. But for some resources you want to close them as soon as you are done with them. Defering every close by default would make you to double-close such resources. Defer is not a solution for the problem I mentioned.

However, for the sake of correctness, I will update the code snippets.

It would be greatly appreciated, a proposal should have if not real-world use-cases, then at least correct examples.

@nixpare
Copy link
Author

nixpare commented Jun 11, 2023

I think to have understood what you mean and you are completely right. I'll try to explain with some
prototype code:

Old and wrong way to do it:

func heavyWorkload() error {
	resource, err := loadHeavyResource()
	if err != nil {
		return err
	}
	defer resource.release()

	data, err := resource.useResource()
	if err != nil {
		return err
	}

	// some other long procedures that use data

	return nil
}

In this case the heavy resource will be released only at the end of the enclosing function.
Old and right way to do it:

func heavyWorkload() error {
	resource, err := loadHeavyResource()
	if err != nil {
		return err
	}

	data, err := useResource(resource)
	resource.release()
	if err != nil {
		return err
	}

	// some other long procedures that use data from the resource

	return nil
}

One way to handle this would be to wrap a portion of the code in another function
(maybe an anonymous one) like so:

func heavyWorkload() error {
	handle err {
		return fmt.Errorf("heavy workload error: %w", err)
	}

	data := catch func() ([]byte, error) {
		resource := catch loadHeavyResource()
		defer resource.release()
                
                data := catch useResource(resource)
		return data, nil
	}()

	// some other long procedures that use data from the resource

	return nil
}

In this case, I admit, it's a little bit clunky, but would also add the benefit
of releasing the resource if a panic occurs in the function useResource(resource).

Or another way of addressing this would be to use the catch <error_value> statement:

func heavyWorkload() error {
	handle err {
		return fmt.Errorf("heavy workload error: %w", err)
	}

	resource := catch loadHeavyResource()

	data, err := useResource(resource)
	resource.release()
	catch err

	// some other long procedures that use data from the resource

	return nil
}

Probably a lot more readable, but would not work if you have to use the resource more
than one time and the are more point of "failure". For dealing with that you just could use a flag:

func heavyWorkload() error {
	handle err {
		return fmt.Errorf("heavy workload error: %w", err)
	}

	resource := catch loadHeavyResource()

	var resourceReleased bool // set to false
	releaseResourceFunc := func() {
		if !resourceReleased {
			resource.release()
			resourceReleased = true
		}
	}

	handle err {
		releaseResourceFunc()
	}

	data1 := catch useResourceFirstTime(resource)

	// use data1 ...
	
	data2 := catch useResourceSecondTime(resource)
	releaseResourceFunc()

	// some other long procedures that use data2 from the resource

	return nil
}

Again, not a huge fan of this implementation ...

Maybe the best way to handle this is:

  • have the resource aware of its state and allow for multiple
    calls of the method resource.release()
  • create a function in some ways that manipulates the resource in the best
    way and returns to the enclosing function as soon as the resource is not
    needed anymore
  • in this sensitive case, maybe handle the resource without any handle - catch
    construct

I think sometimes one feature can't be used for every use case, and will only
affect some people in not every scenario.

One last thing I did not have understoop from your comment (and don't get me wrong ...
you are completely right): do you think this is a problem that only my proposal erases or
you find it also in the Official Draft for Go 2 mentioned above?

@nixpare
Copy link
Author

nixpare commented Jun 11, 2023

@rjeczalik, I just thought about a less clunky way to handle the cleanup with a flag that would extend to other use cases (not only flags).
This is a soluzion that should replace the last code snippet in the previous post:

func heavyWorkload() error {
	handle err {
		return fmt.Errorf("heavy workload error: %w", err)
	}

	resource := catch loadHeavyResource()

	var resourceReleased bool
	handle err if !resourceReleased {
		resource.release()
	}

	data1 := catch useResourceFirstTime(resource)

	// use data1 ...
	
	data2 := catch useResourceSecondTime(resource)

        resourceReleased = true
	resource.release()

	// some other long procedures that use data2 from the resource

	return nil
}

So after the second handle declaration, when a function is called with the check statement, the resulting control statement underneath would be if err != nil && (<boolean_expression>) { ... }

@webbongithub
Copy link

This hurts readability too much, and holds too much overlap with just using a function in my opinion.

func LoadJSONFile(filePath string) (*MyStruct, error) {
	var err error
	errf := func() (*MyStruct, error) {
		return nil, fmt.Errorf("could not load json file <%s>: %w", filePath, err)
	}

	f, err := os.Open(filePath)
	if err != nil {
		return errf()
	}
        defer f.Close()

	data, err := io.ReadAll(f)
	if err != nil {
		return errf()
	}

	var result *MyStruct
	err = json.Unmarshal(data, result)
	if err != nil {
		return errf()
	}

	return result, nil
}

@apparentlymart
Copy link

Thanks for the effort you've put into this proposal!

Considering your first example using catch, I can imagine a quite similar structure that works in today's Go:

func LoadJSONFile2(filePath string) (result *MyStruct, err error) {
	defer func() {
		if err != nil {
			err = fmt.Errorf("could not load json file <%s>: %w", filePath, err)
		}
	}()

	f, err := os.Open(filePath)
	if err != nil {
		return err
	}
        defer f.Close()

	data := io.ReadAll(f)
	if err != nil {
		return err
	}

	err = json.Unmarshal(data, result)
	if err != nil {
		return err
	}

	return result, nil
}

This variant preserves your goal of writing the error-wrapping call with its common prefix only once, but it does still require an if statement for each fallible call. In previous discussions it's been quite contentious whether removing the explicit error handling control flow is an advantage of a disadvantage, so of course it's debatable whether that aspect of my alternative above is an improvement or a drawback relative to what you showed.

The other notable drawback of my variant is that it requires naming the return values so that it's possible for the defer to mutate err while returning. Some argue that named return values should be avoided unless they are helpful for documentation, so this would appear to be a drawback for those people.

I don't really have a strong opinion on this proposal yet, but when thinking about proposals I like to try to imagine ways to achieve something as similar as possible using the current language, because that helps to consider what value the new language features are bringing. To me the above seems like a reasonable solution to the problem as stated, but I must admit I would be unlikely to actually write code like this because I tend not to make functions generate errors with information that the caller of the function already knows, and instead focus on describing the aspects of the problem that the caller cannot determine from its own context about what it was calling and what it passed in. The problem statement therefore doesn't really match a problem I have, but I acknowledge that others have different preferences for error message content.

@shynome
Copy link

shynome commented Jun 14, 2023

hi, I also have a proposal for Go 2: Error-Handling, can you also comment it? thanks for your see
#60779

@y1rn
Copy link

y1rn commented Jun 17, 2023

Can go support no if err != nil by default

func LoadJSONFile(filePath string) (*MyStruct, error) {//error must define
	f := os.Open(filePath)
        defer f.Close()
	data:= io.ReadAll(f)
	var result *MyStruct
	json.Unmarshal(data, result)
	return result, nil
}

@nixpare
Copy link
Author

nixpare commented Jun 17, 2023

This hurts readability too much, and holds too much overlap with just using a function in my opinion.

@webbongithub, thanks for your opinion. Personally, I know this is a subjective topic don' get me wrong, I don't think if hurts readability that much. I agree that there is a significant overlap with the alternative solution you proposed, but adding a new way to do the same things with a slightly different syntax it's not always a bad thing.
I see a relation like this behing the one between the standard for loop and the for range, let me explain:

  • they technically do the same things (the for range just let you avoid using the v[i] syntax;
  • the for range (over an array, e.g.) does not even let you modify the value stored persistently (expect using the standard syntax with the index);
  • the for range does not even reduce the number of lines to write and the readability (personally) is quite the same;

If someone told me that they would remove for ranges in Go 2, I would be really mad.

func LoadJSONFile(filePath string) (*MyStruct, error) {
	handle err {
		return nil, fmt.Errorf("could not load json file <%s>: %w", filePath, err)
	}

	f := catch os.Open(filePath)
        defer f.Close()

	data := catch io.ReadAll(f)

	var result *MyStruct
	catch json.Unmarshal(data, result)

	return result, nil
}

I honestly don't see why this would be so much unreadable. Just my honest opinion.
The thing is that nobody will prevent you from writing more "legacy" code.
Tell me what you think.

@nixpare
Copy link
Author

nixpare commented Jun 17, 2023

Can go support no if err != nil by default

@y1rn, The problem with this is that you are actually ignoring the error, and this is really a bad practice, in general.
It is not clear if you are ignoring a potential error from the function or if the function really never returns an error.
Instead if you wrote f, _ := os.Open(filePath), everyone would know that you are ignoring something, without having to
look at the documentation.
This is why the proposal has the catch statement, so that it is clear that you are handling an error returned in a "standard" way (has declared by the handle statement)

@y1rn
Copy link

y1rn commented Jun 17, 2023

Can go support no if err != nil by default

@y1rn, The problem with this is that you are actually ignoring the error, and this is really a bad practice, in general. It is not clear if you are ignoring a potential error from the function or if the function really never returns an error. Instead if you wrote f, _ := os.Open(filePath), everyone would know that you are ignoring something, without having to look at the documentation. This is why the proposal has the catch statement, so that it is clear that you are handling an error returned in a "standard" way (has declared by the handle statement)

I mean f := os.Open(filePath) still stop process and pass err to caller for somewhere doesn't want to handle error if err != nil {return nil,err}, or do unified handle with defer

func LoadJSONFile(filePath string) (result *MyStruct, err error) {//error must define
       defer func(){
         if errors.Is(err, BaseErr) {
           //...
         }
        }()
	f := os.Open(filePath)
        defer f.Close()
	data:= io.ReadAll(f)
	err = json.Unmarshal(data, result)
	if err != nil {
		return nil, fmt.Errorf("fail to unmarshal json data: %w", err)
	}
	return result, nil
}

This looks do not need to change go syntax and good to write code, Problem is reviewer doesn't know where may return err.

@nixpare
Copy link
Author

nixpare commented Jun 17, 2023

@y1rn sorry but I can't understand if you are proposing another feature or if you are providing an example that would do the job in the current version of Go. Because if this is the second case, this code will never compile.
Instead if this is a proposal, I would be completely against it because you can't be sure about what the fuction called in the code you are reviewing really returns, maybe who wrote the program decided to ignore some return values, but this is not explicit anymore.

@nixpare
Copy link
Author

nixpare commented Jun 17, 2023

@apparentlymart I completely understand your approach on things and, as I also replied to another person, I understand that there are already ways to do the same thing with nothing more, nothing less.
But sometimes a feature does not exist just to implement a new concept not doable before, but to polish some already established ways of doing things that could make the language more enjoyable for someone else, especially if it comes from another language.
I think it's a known fact that people that come from a try catch kind of language hate the verbosity of the error handling in Go, this would make the language more inviting (maybe).
Tell me what you think

@ianlancetaylor
Copy link
Contributor

Thanks for the proposal and discussion. As pointed out above, this is fairly similar to the earlier try/handle design draft, that was not adopted. This direction seems to run into a lot of resistance. Sorry.

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

@peter7891
Copy link

peter7891 commented Aug 1, 2023

I am not sure that i understand what you mean by macro.
It's like a macro in which language?

I personally think this is verbose and doesn't fix things much.
Go should add the question mark operator, like Rust to automatically return errors.

func heavyWorkload() error {
	res1 := somethingThatCanFail()?
        res2 := somethingThatCanFail2()?

	return nil
}

To anotate errors, the question mark operator should generate code that calls a method on an interface that does the conversion. Just as they do in Rust.

@gophun
Copy link

gophun commented Aug 1, 2023

@peter7891 Please see #32437. It was spelled try(), but basically the same idea. It was not well received.

@peter7891
Copy link

peter7891 commented Aug 1, 2023

@peter7891 Please see #32437. It was spelled try(), but basically the same idea. It was not well received.

i don't see any better solution than this.
Perhaps, the majority of the Go community doesn't see this as a problem.
In my opinion, the noise from error handling has a much more negative impact than adding this slightly more complicated Rust-like feature.
It is true that a lot of the people won't anotate errors, by implementing the interface.
While anotating errors is important for debugging, what is even more important is small code you can quickly understand.

From what i see, people complained it wasn't clear or it was hard to see where functions returned.
I think, it should be new syntax, not a function call.

Something like Zig

const number = try parseU64(str, 10);

Although, i do prefer the question mark, after the function call, like Rust.

let number: i64 = "1".parse()?;

I think its easier to spot stuff at the end of the line than in the middle.

@ofabricio
Copy link

How about something like this:

func LoadJSONFile(filePath string) (*MyStruct, error) {

    f := os.Open(filePath) catch ("could not open json file")
    defer f.Close()

    data := io.ReadAll(f) catch ("could not read json file") {
        // This is an optional code block to close something if needed.
        // After running this block it jumps to the catcher label below.
    }

    var result *MyStruct
    json.Unmarshal(data, result) catch ("could not unmarshal json file")

    return result, nil

catcher(err error, msg string):
    return nil, fmt.Errorf("%s <%s>: %w", msg, filePath, err)
}

catch catches the error then jumps to catcher label to handle the error. It would allow passing any number of arguments to catcher. Also catch can have an optional execution block that runs when an error occurrs, then jumps to catcher after that.

@ianlancetaylor
Copy link
Contributor

No change in consensus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error-handling Language & library change proposals that are about error handling. LanguageChange Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

13 participants