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: spec: simple error wrapper handling using comments #47934

Closed
conradludgate opened this issue Aug 24, 2021 · 14 comments
Closed

proposal: Go 2: spec: simple error wrapper handling using comments #47934

conradludgate opened this issue Aug 24, 2021 · 14 comments
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

@conradludgate
Copy link

conradludgate commented Aug 24, 2021

Preface

There have been quite a few proposals for more concise error handling in go, and to be honest, a lot of them don't really fit with the 'go style' - admittedly that's a very subjective claim so I'll leave that up to others to decide relative to this proposal.

I've noticed in a couple places that Go already has set a precedence for using comments in code with semantic meaning.

  1. https://go.dev/blog/examples
  2. https://go.dev/blog/generate

Similarly being used by some linting/static analysis tools (eg https://golangci-lint.run/usage/false-positives/#nolint)

This proposal is for simple error handling and wrapping to follow a similar pattern

Example

Here's a very simple example that you might currently see in go code

func foo(x string) (string, error) {
	return "", fmt.Errorf("some error occured :(")
}

func bar1(x string) (string, error) {
	y, err := foo(x)
	if err != nil {
		return "", fmt.Errorf("could not call foo: %w", err)
	}
	
	return y, nil
}

and here is my proposed pattern:

func bar2(x string) (string, error) {
	// error: fmt.Errorf("could not call foo: %w", err)
	y := foo(x)

	return y, nil
}

equivalent form using a specified identifier name:

func bar3(x string) (string, error) {
	y := foo(x) // error: errValue => fmt.Errorf("could not call foo: %w", errValue)

	return y, nil
}

Backwards Compatibility

Since the code suggested will currently not compile (assignment mismatch: 1 variable but foo returns 2 values), this should be backwards compatible with go1

Non features

Any error handling beyond wrapping and returning are not supported. I believe those situations are unique enough where the current solutions are not overly verbose and will not benefit from any unique new syntax

Breakdown

The error comment will work either on the statement directly following, or apply to the current statement if added on the same line

func someFunctionThatErrors(arguments) (type1, type2, error) {
	// error: identifier => expression(identifier)
	a, b := anotherFunctionThatErrors(arguments)

	// ...
}

or

func someFunctionThatErrors(arguments) (type1, type2, error) {
	a, b := anotherFunctionThatErrors(arguments) // error: identifier => expression(identifier)

	// ...
}

will be pre-processed into

func someFunctionThatErrors(arguments) (type1, type2, error) {
	a, b, identifier := anotherFunctionThatErrors(arguments)
	if identifier != nil {
		return ZeroValue, ZeroValue, expression(identifier)
	}

	// ...
}

The identifier => part of the error comment can be skipped, defaulting to the err identifier instead

The expression(identifier) doesn't need to be a function, or even depend on the identifier in any way. It's just an expression that can be returned as an error. For instance,

// Wrap the error in a new error type
// error: ErrSomeThing { data: "foobar", err: err }
// For returning no error but exiting early
// error: nil

Final thoughts

I'm not suggesting this is the best solution, but one that I haven't seen discussed anywhere and thought it would be worth proposing. It's not the first proposal based on just error wrapping though. Most of those I have seen add new operators to the language. Go is not very operator heavy so they often feel out of place in the language, which is where I think this proposal fits better

@gopherbot gopherbot added this to the Proposal milestone Aug 24, 2021
@jfesler
Copy link

jfesler commented Aug 24, 2021

Putting executable code inside a comment serves no great purpose, and will complicate both machine and human parsers.

@seankhliao seankhliao changed the title proposal: simple error wrapper handling using comments proposal: Go 2: spec: simple error wrapper handling using comments Aug 24, 2021
@seankhliao seankhliao added error-handling Language & library change proposals that are about error handling. v2 A language change or incompatible library change LanguageChange labels Aug 24, 2021
@seankhliao
Copy link
Member

Please fill out https://github.com/golang/proposal/blob/master/go2-language-changes.md when proposing language changes

@conradludgate
Copy link
Author

conradludgate commented Aug 24, 2021

Would you consider yourself a novice, intermediate, or experienced Go programmer?

Experienced

What other languages do you have experience with?

Rust, TypeScript, C++

Would this change make Go easier or harder to learn, and why?

A touch harder but nothing meaningful. Existing error handling patterns will still be supported and any
new tutorials can feature this technique

Has this idea, or one like it, been proposed before?

If so, how does this proposal differ?

Similar (more consise error wrapping and returning) but not using this technique. They mostly add new operators or keywords

Who does this proposal help, and why?

Developers who find themselves consistently wrapping and returning errors in their functions

What is the proposed change?

Described above

Is this change backward compatible?

As far as I can tell, it is

Show example code before and after the change.

func bar1(x string) (string, error) {
	y, err := foo(x)
	if err != nil {
		return "", fmt.Errorf("could not call foo: %w", err)
	}
	
	return y, nil
}
func bar2(x string) (string, error) {
	// error: fmt.Errorf("could not call foo: %w", err)
	y := foo(x)

	return y, nil
}

What is the cost of this proposal? (Every language change has a cost).

The language will need to parse these error comments

How many tools (such as vet, gopls, gofmt, goimports, etc.) would be affected?

vet, imports, pls etc would all need to verify the code written in the error comments.
fmt might want to enforce spacing in these comments

What is the compile time cost?

I think minimal. This isn't a huge change

What is the run time cost?

None

Can you describe a possible implementation?

I haven't looked into it yet

How would the language spec change?

Add support for error comments into the spec

Orthogonality: how does this change interact or overlap with existing features?

No overlap or interaction.

Is the goal of this change a performance improvement?

No runtime change

Does this affect error handling?

If so, how does this differ from previous error handling proposals?

Yes. This differs by not adding any new operators or magic functions to the language

Is this about generics?

No

@earthboundkid
Copy link
Contributor

Like others, I don't see what using a comment buys you. Why not this?

func bar2(x string) (string, error) {
	guard fmt.Errorf("could not call foo: %w", err)
	y := foo(x)

	return y, nil
}

@conradludgate
Copy link
Author

conradludgate commented Aug 24, 2021

I don't see what using a comment buys you

I'm not necessarily arguing whether the comments idea is good or bad, but I think it's an idea worthy of discussion. When I discussed at work and with peers, it was controversial but no one dismissed it straight out.

Regarding your guard keyword, I don't think the semantics of guard fit with the rules I set out with the error comment. For instance

// If foo errors, exit early with no error.
// error: nil
y := foo(x)

Instead using guard, it doesn't seem to quite make sense.

guard nil
y := foo(x)

You could argue for ages about the semantics of which word to use as the keyword, so I'll move on to tackling keywords in general.

Currently we have go and defer keywords. Both of these expect to precede a function call. If we add a keyword for error handling, I think it might be confusing/conflicting to drop this requirement. This would make the following example not work

// error: ErrSomeThing { data: "foobar", err: err }
y := foo(x)

I suppose you could do

guard func() {
    return ErrSomeThing { data: "foobar", err: err }
}()
y := foo(x)

But there's no point in that.

Perhaps with some identity function

func identity[T any](t T) T {
    return t
}

// ...

guard identity(ErrSomeThing { data: "foobar", err: err })
y := foo(x)

Also, one thing I quite like about this comment proposal is that you can quite easily compact it down to 1 line in trivial cases

y := foo(x) // error: err

While I agree using comments could be confusing, I still think it's worth at least considering and discussing

@deanveloper
Copy link

If I remove all comments from a program, it should still operate the same way. There are some exceptions with compiler directives and build tags, but those should be avoided unless they are needed (ie when operating at the low level, or doing operations which are OS-specific).

@conradludgate
Copy link
Author

As you clearly stated there's already exceptions to your rule. And, if I may add, your rule is quite subjective. One could argue that comments, while usually not adding much to the runtime of a program, are still pretty integral. And I don't think someone would actually suggest to remove all the comments from a codebase

@deanveloper
Copy link

deanveloper commented Aug 25, 2021

As you clearly stated there's already exceptions to your rule. And, if I may add, your rule is quite subjective.

They are all also implemented specifically by the compiler, not the spec. This change would most definitely need spec-level support, as it is intended to be widely used and isn't for low-level control.

While it's subjective, I do believe that there is a very large difference between "run my code to handle an error" and "make sure that a uintptr passed to a function doesn't get cleaned up by the garbage collector"

One could argue that comments, while usually not adding much to the runtime of a program, are still pretty integral.

What you are suggesting is allowing comments affect the runtime of a program in a very significant way. The comments that you suggest are able to define variables, call functions, and really they can do anything that a Go program can do.

And I don't think someone would actually suggest to remove all the comments from a codebase

A code generation utility should be able to understand how your program works if it removes all comments. I am not suggesting that someone deletes all comments, I am suggesting that comments are usually safe to delete.

This might get a bit into bikeshedding, but the format you describe looks like a comment that would be safe to delete. Something like //go:noinline or // +build linux is pretty clearly a compiler directive, even if one doesn't know how compiler directives are formatted. However, // error: fmt.Errorf(...) is quite literally a valid line of Go code that is commented out.

Not to mention that this isn't backward-compatible, as comments such as // error: err may already exist on lines of functions which only return an error, but are assigned to nothing. (ie thisFunctionReturnsErr() // error: err) Speaking of which, I have some questions regarding invalid comments:

  1. What happens if I do // error: fmt.Errorf(...) but do not have import "fmt"?
  2. Would // error: page where page is an already-defined variable return page?
  3. Would an existing comment in a codebase such as // error: page not found produce a compiler error?

@conradludgate
Copy link
Author

Not to mention that this isn't backward-compatible, as comments such as // error: err may already exist.

Not an issue:

Exiting code that currently compiles, and should continue to.

// error: err
y, err := foo(x)

Modified code that currently would fail to compile since there's a mismatch in argument type

// error: err
y := foo(x)

If both an error comment exist and the returned error value is omitted will this compile path trigger.

What happens if I do // error: fmt.Errorf(...) but do not have import "fmt"?

That would be an error.

// error: fmt.Error(...)
y := foo(x)

would be pre-processed into the literal code

// error: fmt.Error(...)
y, err := foo(x)
if err != nil {
    return ..., fmt.Error(...)
}

Which requires fmt. This is why I suggested that goimports would need to be updated.

Would // error: page where page is an already-defined variable return page?

Yes

Would an existing comment in a codebase such as // error: page not found produce a compiler error?

See above. No. Error comments only get triggered if followed by a function call that returns an error where the error result value is omitted

@deanveloper
Copy link

Sorry I had put this in later in an edit, but a function which only returns an error would produce no compiler error when omitting the error. For instance,

func myFunction() error {
    ...
}

func main() {
    // error: fmt.Errorf("myFunction error")
    myFunction()
}

@conradludgate
Copy link
Author

conradludgate commented Aug 25, 2021

Touché. We could ignore all cases where there's no assignment at all. Admittedly that's a shame since I'd estimate roughly 25% of functions that return errors in go probably only return errors

Thanks for some actual valid, objective feedback 👍

@apparentlymart
Copy link

apparentlymart commented Aug 26, 2021

In the motivating example it seems like an implicit var err error is being declared somewhere, but it's not clear where exactly that is and what its scope would be.

func bar2(x string) (string, error) {
	// error: fmt.Errorf("could not call foo: %w", err)
	y := foo(x)

	return y, nil
}

Would it be valid to refer to err elsewhere in the function?

func bar2(x string) (string, error) {
	// error: fmt.Errorf("could not call foo: %w", err)
	y := foo(x)

	// Is this valid? (Note that it's just =, not :=)
	err = somethingElse()

	// is this valid?
	return y, err
}

I guess a different way to ask the question is: what exactly is this example syntactic sugar for? Two possible answers that are relevant to my question above would be:

func bar2(x string) (string, error) {
	y, err := foo(x)
	if err != nil {
		return "", fmt.Errorf("could not call foo: %w", err)
		// (I'm assuming you intended that all other return values would implicitly take their zero value)
	}

	// err is still in scope here

	return y, nil
}
func bar2(x string) (string, error) {
	var y string
	{
		var err error
		y, err = foo(x)
		if err != nil {
			return "", fmt.Errorf("could not call foo: %w", err)
		}
	}

	// err is _not_ in scope here, but y is

	return y, nil
}

@ianlancetaylor
Copy link
Contributor

We aren't going to parse comments such that they must be valid Go code. That's not how the language works.

This proposal does not have strong supported based on the emoji voting.

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

@ianlancetaylor
Copy link
Contributor

No further comments.

@golang golang locked and limited conversation to collaborators Oct 6, 2022
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

8 participants