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

Jump to bottom

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

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

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

kevinburke1 opened this issue Dec 19, 2019 · 9 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@kevinburke1
Copy link

kevinburke1 commented Dec 19, 2019 •
edited

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?
@kevinburke1
Copy link
Author

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

@mvdan
Copy link
Member

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.

@mvdan mvdan added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 19, 2019
@dmitshur dmitshur added this to the Backlog milestone Dec 20, 2019
@dpinela
Copy link
Contributor

dpinela commented Dec 28, 2019

I've been thrown off by this before.

@gopherbot
Copy link

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

@mvdan
Copy link
Member

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: ....

@kevinburke1
Copy link
Author

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 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.

@dsnet dsnet mentioned this issue Nov 17, 2020
Closed
@gopherbot
Copy link

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
@kevinburke @dsnet
encoding/json: revert "add "json: " prefix to SyntaxError messages"
283d654 
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 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

@johejo johejo mentioned this issue Jan 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@dmitshur @dpinela @mvdan @dsnet @gopherbot @kevinburke1