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: SyntaxError does not have "json:" prefix #36221

Open
kevinburkemeter opened this issue Dec 19, 2019 · 9 comments
Open

encoding/json: SyntaxError does not have "json:" prefix #36221

kevinburkemeter opened this issue Dec 19, 2019 · 9 comments
Milestone

Comments

@kevinburkemeter
Copy link

@kevinburkemeter kevinburkemeter commented Dec 19, 2019

It was my understanding that most errors created inside of a standard library package should be prefixed with the package name. For example, the error message for sql.ErrNoRows is errors.New("sql: no rows in result set"), not errors.New("no rows in result set").

Some errors in the json package are returned without a leading prefix. I noticed this because I hit one and it took some digging to figure out where in the codebase the error was actually coming from.

These tests in scanner_test.go check that the error matches a SyntaxError without a leading prefix.

var indentErrorTests = []indentErrorTest{
	{`{"X": "foo", "Y"}`, &SyntaxError{"invalid character '}' after object key", 17}},
	{`{"X": "foo" "Y": "bar"}`, &SyntaxError{"invalid character '\"' after object key:value pair", 13}},
}

func TestIndentErrors(t *testing.T) {
	for i, tt := range indentErrorTests {
		slice := make([]uint8, 0)
		buf := bytes.NewBuffer(slice)
		if err := Indent(buf, []uint8(tt.in), "", ""); err != nil {
			if !reflect.DeepEqual(err, tt.err) {
				t.Errorf("#%d: Indent: %#v", i, err)
				continue
			}
		}
	}
}

Is that right? Shouldn't those error messages have a leading "json:" prefix?

@kevinburkemeter
Copy link
Author

@kevinburkemeter kevinburkemeter commented Dec 19, 2019

I can submit a fix, just let me know if that's something it would be good to fix

@mvdan
Copy link
Member

@mvdan mvdan commented Dec 19, 2019

I assume you mean that their Error() string methods should begin with json: . I'd probably agree there, though I haven't taken a closer look at why the errors in the package aren't stringified in a consistent way.

I'm not sure that the prefix should be part of the error value itself, though. If the error type is json.SyntaxError, having its message also contain json: seems a bit redundant. The fmt.Errorf errors do contain the prefix, but that's just because they are generic errors.

@dmitshur dmitshur added this to the Backlog milestone Dec 20, 2019
@dpinela
Copy link
Contributor

@dpinela dpinela commented Dec 28, 2019

I've been thrown off by this before.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 19, 2020

Change https://golang.org/cl/263619 mentions this issue: encoding/json: add "json: " prefix to SyntaxError messages

@mvdan
Copy link
Member

@mvdan mvdan commented Oct 19, 2020

I'm still not sure about this. Is there a precedent with other exported error types in the standard library whose Error method uses a prefix? It feels redundant for a json error to prepend its own package name. I also can't recall any other standard library error behaving this way, besides non-typed fmt.Errorf("pkgname: message") errors.

For example, the os errors use the operation or syscall as the prefix, so you get open: ... not os: open: ....

@kevinburkemeter
Copy link
Author

@kevinburkemeter kevinburkemeter commented Oct 19, 2020

Every other named error in that package prepends the prefix:

  • UnmarshalTypeError
  • UnmarshalFieldError
  • InvalidUnmarshalError
  • InvalidUTF8Error
  • MarshalerError
  • UnsupportedTypeError
  • UnsupportedValueError

SyntaxError is the only one that doesn't.

I wasn't aware that this wasn't common practice across the standard library, if it isn't then maybe we should leave it as is.

@mvdan
Copy link
Member

@mvdan mvdan commented Oct 19, 2020

Ah, you're right, there's plenty of precedent in the same package. I wonder if we would break any existing valid programs, but I would imagine not. I'll go review.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 28, 2020

Change https://golang.org/cl/273747 mentions this issue: encoding/json: revert "add "json: " prefix to SyntaxError messages"

@dsnet dsnet reopened this Dec 1, 2020
gopherbot pushed a commit that referenced this issue Dec 1, 2020
This reverts commit 6af088b.

Reason for revert: Broke many tests inside Google which implies many
tests were broken outside of Google as well. The tests may be brittle
but still would require work to change and it's not clear it's worth
the benefit.

Updates #36221
Fixes #42675

Change-Id: Id3a14eb37e7119f5abe50e80dfbf120fdc44db72
Reviewed-on: https://go-review.googlesource.com/c/go/+/273747
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Trust: Joe Tsai <thebrokentoaster@gmail.com>
@dsnet
Copy link
Member

@dsnet dsnet commented Dec 1, 2020

Re-opening. I still think this is worth doing in the future, but we may want to batch it with other small changes that may technically break users who depend on unspecified behavior. See #42675

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants