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: fmt: add an additional %j verb to wrap errors #58107

Closed
alexispires opened this issue Jan 27, 2023 · 12 comments
Closed

proposal: fmt: add an additional %j verb to wrap errors #58107

alexispires opened this issue Jan 27, 2023 · 12 comments
Labels
Milestone

Comments

@alexispires
Copy link

The problem

Currently the error handling in go is a bit weird to me. Let's see this with an example.

Imagine a simple program that updates a profile and then tries to store it on disk :

func writeOnDisk() error {
	return errors.New("no space left on device")
}

func storeProfile() error {
	if err := writeOnDisk(); err != nil {
		return err
	}

	return nil
}

func updateProfile() error {
	if err := storeProfile(); err != nil {
		return err
	}

	return nil
}

func main() {
	err := updateProfile()

In this example, the write operation on disk fails and the error is surfaced an printed from the main function. If we try to log it we will have the following message:

2023/01/27 09:42:51 no space left on device

It lacks a bit of context and it's not a good way to handle an error.

So a better way to handle the error would be to wrap it to add a bit of context :

func writeOnDisk() error {
	return errors.New("no space left on device")
}

func storeProfile() error {
	if err := writeOnDisk(); err != nil {
		return fmt.Errorf("can't store profile %w", err)
	}

	return nil
}

func updateProfile() error {
	if err := storeProfile(); err != nil {
		return fmt.Errorf("can't update profile %w", err)
	}

	return nil
}

With this program, we will have the following message :

2023/01/27 09:42:51 can't update profile can't store profile no space left on device

Not very clear until we try to format it using the fmt package. But in my opinion it doesn't make sense to spend time to find out the right printing format for an error message and in the future it will be more difficult since soon we could wrap multiple errors.

So let's imagine a basic (dirty) function that follow the error's chain and print them, like a stacktrace we will see the errors stacked from context to details:

func printWrappedErrors(err error) {
	var message string
	i := 1
	for currentErr := err; currentErr != nil; currentErr = errors.Unwrap(currentErr) {
		message = fmt.Sprintf("%s\nError #%02d: %s", message, i, currentErr)
		i++
	}
	log.Println(message)
}

Now if we try to print the previous error with this function we will have the following trace:

2023/01/27 10:06:15
Error #01: can't update profile can't store profile no space left on device
Error #02: can't store profile no space left on device
Error #03: no space left on device

The trace is not very clear, the same error is surfaced several times because the fmt package add the child error message to its own.

Actually the best trace one's could be expect is the following one:

2023/01/27 10:06:15
Error #01: can't update profile
Error #02: can't store profile
Error #03: no space left on device

The proposal

Add a %j verb in the fmt package that wrap an error but does not include it in the parent error message. With it my example will trace the following message :

2023/01/27 10:06:15
Error #01: can't update profile
Error #02: can't store profile
Error #03: no space left on device
@gopherbot gopherbot added this to the Proposal milestone Jan 27, 2023
@robpike
Copy link
Contributor

robpike commented Jan 27, 2023

Ignoring for the moment any discussion of the need for the proposed functionality, it might suffice to implement some flags for the %w verb, which I don't believe accepts any at the moment.

@ianlancetaylor
Copy link
Contributor

See also #25675 and #27020.

@neild
Copy link
Contributor

neild commented Jan 27, 2023

%w accepts the same flags as %v.

Is the proposed %j the same as %.0w?

e1 := errors.New("1")
e2 := fmt.Errorf("2%.0w", e1)
fmt.Printf("%q", e2)
// "2"

@alexispires
Copy link
Author

alexispires commented Jan 27, 2023

Yes it's the same behaviour, to be honest I didn't know that flag on %w and it looks more like a kind of trick imo.

@rittneje
Copy link

rittneje commented Jan 28, 2023

It's not clear to me where this proposal would be useful.

If the error is coming from a third-party library (or even the standard library), what's their motivation for using %j instead of %w? Most clients are just going to log the error as-is instead of recursively unwrapping it, meaning that any use of %j will exclude the original source of the error. So in all likelihood, they will always use %w. That means you cannot get your stack-trace-like behavior from third-party errors. (Plus there's also the matter of them having their own error types, which would be in a similar boat.)

If the error is coming from your own code, then you can just use your own error type instead of fmt.Errorf to get what you want. For example:

type wrappedError struct {
    message string
    err error
}

func (we wrappedError) Error() string {
    return we.message
}

func (we wrappedError) Unwrap() error {
    return we.err
}

func WrapError(message string, err error) error {
    return wrappedError{message: message, err: err}
}

func updateProfile() error {
	if err := storeProfile(); err != nil {
		return WrapError("can't update profile", err)
	}
	return nil
}

There's also the option to just put a newline in your format string, which might be sufficient for what you are trying to do anyway.

func updateProfile() error {
	if err := storeProfile(); err != nil {
		return fmt.Errorf("can't update profile\n%w", err)
	}
	return nil
}

@alexispires
Copy link
Author

Using %j instead of %w allow to :

  • Use errors.Is() without exposing the error message due to security concerns.
  • Print the error stack as I did on my first message. Useful for a web server if you log all the errors from a middleware. In a web server there is a lot of information to log and the result is unreadable.

@rsc
Copy link
Contributor

rsc commented Mar 15, 2023

This seems too special. If you want custom semantics, it is easy to write a function or error implementation that creates those semantics. %w answers a fundamental question: does it unwrap or not. I don't think %j has a place here: it doesn't even format.

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@08d2
Copy link

08d2 commented Apr 7, 2023

func storeProfile() error {
	if err := writeOnDisk(); err != nil {
		return fmt.Errorf("can't store profile %w", err)
	}

When wrapping an error (which you should definitely always do!) you should provide context which is not already available to the caller. In this case "store profile" is what the caller has called, so it's not useful as an error prefix. Instead, that prefix should be "can't write on disk" which is information that the caller has no access to. And every prefix should in general terminate with a colon, so in this case return fmt.Errorf("write on disk: %w", err). This produces output which is clear.

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@alexispires
Copy link
Author

func storeProfile() error {
	if err := writeOnDisk(); err != nil {
		return fmt.Errorf("can't store profile %w", err)
	}

When wrapping an error (which you should definitely always do!) you should provide context which is not already available to the caller. In this case "store profile" is what the caller has called, so it's not useful as an error prefix. Instead, that prefix should be "can't write on disk" which is information that the caller has no access to. And every prefix should in general terminate with a colon, so in this case return fmt.Errorf("write on disk: %w", err). This produces output which is clear.

What about errors that wrap multiple errors ? Should we use multiple prefix ? It'll look very messy.

@rsc
Copy link
Contributor

rsc commented Apr 19, 2023

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

8 participants