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

encoding/json: returned io.Writer errors can be confusing without wrapping #48899

Open
HymnsForDisco opened this issue Oct 10, 2021 · 2 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@HymnsForDisco
Copy link

HymnsForDisco commented Oct 10, 2021

What version of Go are you using (go version)?

$ go version 1.16

Does this issue reproduce with the latest release?

I must assume it does (not a bug, just poor design)

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
windows/amd64

What did you do?

package main

import (
	"encoding/json"
	"errors"
	"log"
	"os"
)

func main() {
	// read some data from myfile
	file, err := os.Open("myfile")
	if errors.Is(err, os.ErrNotExist) {
		// the file doesn't exist, let's create it

		// OOPS! I forgot to actually create the file!

		// Let's just try to populate it with some data
		var myData = 123
		err = json.NewEncoder(file).Encode(myData)
		if err != nil {
			log.Fatalln("error encoding:", err)
		}
	}
}

Of course, in a stripped-down example the error is obvious. And it was even obvious once I had a clue of what to look for. The reason why I'm posting this issue is because the error message was not only unhelpful, but very misleading. It is a result of the json (and gob and perhaps other) packages simply forwarding error messages from other packages without wrapping them for context.

What did you expect to see?

A useful error message, telling me why Encode had failed.

What did you see instead?

The plain error message "invalid argument", which is very misleading and caused some wasted time trying to source.
When you call a method and receive an error message "invalid argument", it is only natural to assume that the error was due to the arguments passed to said method. However, that is not the case here. The error is actually due to the file which was used to create the encoder. In addition, the error message is so short and plain, I had trouble trying to just determine from where it originated (due to poor searchability).

@mvdan mvdan changed the title Misleading error message in json package. encoding/json: returned io.Writer errors can be confusing without wrapping Oct 10, 2021
@mvdan
Copy link
Member

mvdan commented Oct 10, 2021

I think wrapping io.Reader and io.Writer errors with some context makes sense. I imagine the only reason that hasn't happened before is because error wrapping is a relatively new concept. cc @dsnet

This could potentially be a breaking change for dowstreams, if they haven't moved from checks like err == ErrSentinel to errors.Is(err, ErrSentinel). That recommendation to users has been in place since Go 1.13, and some linters warn about it, so I imagine it's fair game to expect that most users have already made the switch. And those that haven't could presumably do the switch fairly easily.

@dsnet
Copy link
Member

dsnet commented Oct 11, 2021

There's two related, but orthogonal issues going on here:

  1. Calling os.File.Write on a nil *os.File simply reports "invalid argument" (example). I guess it's complaining that the receiver "argument" is invalid, but it's not a normal interpretation of the word "argument". Perhaps a better message would be something like "Write called on nil *os.File" or something.
  2. Currently, all I/O errors encountered by json.Encoder are returned verbatim without wrapping. At the very least we could wrap it with something like fmt.Sprintf("json: write error: %w", err). That way the error would read as "json: write error: invalid argument", which helps the user identify where in the system something went wrong.

It's not clear whether we should address the first issue, the second issue, or both.

In both situations, we have to deal with the reality of Hyrum's law and the fact that changing errors at all are functionally "breaking changes" and have resulted in a revert close to release. There has however been a precedence set in recent years to allow changing error messages even if it breaks code.

@toothrot toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 13, 2021
@toothrot toothrot added this to the Backlog milestone Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants