Skip to content

Commit

Permalink
Fix minor regressions in legacy error strings
Browse files Browse the repository at this point in the history 
The Go 1 compatability document does not guarantee that
error strings remain unchanged across Go releases.
However, there is a practical benefit to reducing the churn
even if this is not a guaranteed property.

Adjust the formatting of v1 error strings to better match
how they were historically rendered. This does not ensure
100% equivalency with legacy error strings, but
does a better job at mostly preserving the exact string.

In the jsontext and jsonwire packages, we make changes
to assist in the construction legacy error values.
We also make syntactic errors returned by the Decoder more
consistent regardless of whether they were produced by a
ReadValue or ReadToken call. All changes made to jsontext
and jsonwire are an improvement, so we are not taking on
technical debt in those packages to support legacy error strings.
All legacy transformations are kept in the v1 code.
  • Loading branch information
@dsnet
dsnet committed Jan 14, 2025
1 parent d8c9bc4 commit d3b4f71
Show file tree
Hide file tree
Showing 19 changed files with 221 additions and 154 deletions.
12 changes: 6 additions & 6 deletions arshal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2531,7 +2531,7 @@ func TestMarshal(t *testing.T) {
name: jsontest.Name("Structs/InlinedFallback/TextValue/InvalidObjectEnd"),
in: structInlineTextValue{X: jsontext.Value(` { "name" : false , } `)},
want: `{"name":false`,
wantErr: EM(newInvalidCharacterError(",", "before next token", len64(` { "name" : false `), "")).withPos(`{"name":false,`, "").withType(0, T[jsontext.Value]()),
wantErr: EM(newInvalidCharacterError(",", "at start of value", len64(` { "name" : false `), "")).withPos(`{"name":false,`, "").withType(0, T[jsontext.Value]()),
}, {
name: jsontest.Name("Structs/InlinedFallback/TextValue/InvalidDualObject"),
in: structInlineTextValue{X: jsontext.Value(`{}{}`)},
Expand Down Expand Up @@ -6626,7 +6626,7 @@ func TestUnmarshal(t *testing.T) {
inBuf: `{"A":1,"fizz":nil,"B":2}`,
inVal: new(structInlineTextValue),
want: addr(structInlineTextValue{A: 1, X: jsontext.Value(`{"fizz":`)}),
wantErr: newInvalidCharacterError("i", "within literal null (expecting 'u')", len64(`{"A":1,"fizz":n`), "/fizz"),
wantErr: newInvalidCharacterError("i", "in literal null (expecting 'u')", len64(`{"A":1,"fizz":n`), "/fizz"),
}, {
name: jsontest.Name("Structs/InlinedFallback/TextValue/CaseSensitive"),
inBuf: `{"A":1,"fizz":"buzz","B":2,"a":3}`,
Expand Down Expand Up @@ -6736,13 +6736,13 @@ func TestUnmarshal(t *testing.T) {
inBuf: `{"A":1,"fizz":nil,"B":2}`,
inVal: new(structInlineMapStringAny),
want: addr(structInlineMapStringAny{A: 1, X: jsonObject{"fizz": nil}}),
wantErr: newInvalidCharacterError("i", "within literal null (expecting 'u')", len64(`{"A":1,"fizz":n`), "/fizz"),
wantErr: newInvalidCharacterError("i", "in literal null (expecting 'u')", len64(`{"A":1,"fizz":n`), "/fizz"),
}, {
name: jsontest.Name("Structs/InlinedFallback/MapStringAny/MergeInvalidValue/Existing"),
inBuf: `{"A":1,"fizz":nil,"B":2}`,
inVal: addr(structInlineMapStringAny{A: 1, X: jsonObject{"fizz": true}}),
want: addr(structInlineMapStringAny{A: 1, X: jsonObject{"fizz": true}}),
wantErr: newInvalidCharacterError("i", "within literal null (expecting 'u')", len64(`{"A":1,"fizz":n`), "/fizz"),
wantErr: newInvalidCharacterError("i", "in literal null (expecting 'u')", len64(`{"A":1,"fizz":n`), "/fizz"),
}, {
name: jsontest.Name("Structs/InlinedFallback/MapStringAny/CaseSensitive"),
inBuf: `{"A":1,"fizz":"buzz","B":2,"a":3}`,
Expand Down Expand Up @@ -6959,13 +6959,13 @@ func TestUnmarshal(t *testing.T) {
inBuf: `{"A":1,"fizz":nil,"B":2}`,
inVal: new(structInlineMapNamedStringAny),
want: addr(structInlineMapNamedStringAny{A: 1, X: map[namedString]any{"fizz": nil}}),
wantErr: newInvalidCharacterError("i", "within literal null (expecting 'u')", len64(`{"A":1,"fizz":n`), "/fizz"),
wantErr: newInvalidCharacterError("i", "in literal null (expecting 'u')", len64(`{"A":1,"fizz":n`), "/fizz"),
}, {
name: jsontest.Name("Structs/InlinedFallback/MapNamedStringAny/MergeInvalidValue/Existing"),
inBuf: `{"A":1,"fizz":nil,"B":2}`,
inVal: addr(structInlineMapNamedStringAny{A: 1, X: map[namedString]any{"fizz": true}}),
want: addr(structInlineMapNamedStringAny{A: 1, X: map[namedString]any{"fizz": true}}),
wantErr: newInvalidCharacterError("i", "within literal null (expecting 'u')", len64(`{"A":1,"fizz":n`), "/fizz"),
wantErr: newInvalidCharacterError("i", "in literal null (expecting 'u')", len64(`{"A":1,"fizz":n`), "/fizz"),
}, {
name: jsontest.Name("Structs/InlinedFallback/MapNamedStringAny/CaseSensitive"),
inBuf: `{"A":1,"fizz":"buzz","B":2,"a":3}`,
Expand Down
12 changes: 6 additions & 6 deletions internal/jsonwire/decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func ConsumeTrue(b []byte) int {
func ConsumeLiteral(b []byte, lit string) (n int, err error) {
for i := 0; i < len(b) && i < len(lit); i++ {
if b[i] != lit[i] {
return i, NewInvalidCharacterError(b[i:], "within literal "+lit+" (expecting "+strconv.QuoteRune(rune(lit[i]))+")")
return i, NewInvalidCharacterError(b[i:], "in literal "+lit+" (expecting "+strconv.QuoteRune(rune(lit[i]))+")")
}
}
if len(b) < len(lit) {
Expand Down Expand Up @@ -240,7 +240,7 @@ func ConsumeStringResumable(flags *ValueFlags, b []byte, resumeOffset int, valid
// Handle invalid control characters.
case r < ' ':
flags.Join(stringNonVerbatim | stringNonCanonical)
return n, NewInvalidCharacterError(b[n:], "within string (expecting non-control character)")
return n, NewInvalidCharacterError(b[n:], "in string (expecting non-control character)")
default:
panic("BUG: unhandled character " + QuoteRune(b[n:]))
}
Expand Down Expand Up @@ -374,7 +374,7 @@ func AppendUnquote[Bytes ~[]byte | ~string](dst []byte, src Bytes) (v []byte, er
// Handle invalid control characters.
case r < ' ':
dst = append(dst, src[i:n]...)
return dst, NewInvalidCharacterError(src[n:], "within string (expecting non-control character)")
return dst, NewInvalidCharacterError(src[n:], "in string (expecting non-control character)")
default:
panic("BUG: unhandled character " + QuoteRune(src[n:]))
}
Expand Down Expand Up @@ -513,7 +513,7 @@ beforeInteger:
}
state = withinIntegerDigits
default:
return n, state, NewInvalidCharacterError(b[n:], "within number (expecting digit)")
return n, state, NewInvalidCharacterError(b[n:], "in number (expecting digit)")
}

// Consume optional fractional component.
Expand All @@ -527,7 +527,7 @@ beforeFractional:
case '0' <= b[n] && b[n] <= '9':
n++
default:
return n, state, NewInvalidCharacterError(b[n:], "within number (expecting digit)")
return n, state, NewInvalidCharacterError(b[n:], "in number (expecting digit)")
}
for uint(len(b)) > uint(n) && ('0' <= b[n] && b[n] <= '9') {
n++
Expand All @@ -549,7 +549,7 @@ beforeExponent:
case '0' <= b[n] && b[n] <= '9':
n++
default:
return n, state, NewInvalidCharacterError(b[n:], "within number (expecting digit)")
return n, state, NewInvalidCharacterError(b[n:], "in number (expecting digit)")
}
for uint(len(b)) > uint(n) && ('0' <= b[n] && b[n] <= '9') {
n++
Expand Down
42 changes: 21 additions & 21 deletions internal/jsonwire/decode_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ func TestConsumeLiteral(t *testing.T) {
{"null", "nul", 3, io.ErrUnexpectedEOF},
{"null", "null", 4, nil},
{"null", "nullx", 4, nil},
{"null", "x", 0, NewInvalidCharacterError("x", "within literal null (expecting 'n')")},
{"null", "nuxx", 2, NewInvalidCharacterError("x", "within literal null (expecting 'l')")},
{"null", "x", 0, NewInvalidCharacterError("x", "in literal null (expecting 'n')")},
{"null", "nuxx", 2, NewInvalidCharacterError("x", "in literal null (expecting 'l')")},

{"false", "", 0, io.ErrUnexpectedEOF},
{"false", "f", 1, io.ErrUnexpectedEOF},
Expand All @@ -59,17 +59,17 @@ func TestConsumeLiteral(t *testing.T) {
{"false", "fals", 4, io.ErrUnexpectedEOF},
{"false", "false", 5, nil},
{"false", "falsex", 5, nil},
{"false", "x", 0, NewInvalidCharacterError("x", "within literal false (expecting 'f')")},
{"false", "falsx", 4, NewInvalidCharacterError("x", "within literal false (expecting 'e')")},
{"false", "x", 0, NewInvalidCharacterError("x", "in literal false (expecting 'f')")},
{"false", "falsx", 4, NewInvalidCharacterError("x", "in literal false (expecting 'e')")},

{"true", "", 0, io.ErrUnexpectedEOF},
{"true", "t", 1, io.ErrUnexpectedEOF},
{"true", "tr", 2, io.ErrUnexpectedEOF},
{"true", "tru", 3, io.ErrUnexpectedEOF},
{"true", "true", 4, nil},
{"true", "truex", 4, nil},
{"true", "x", 0, NewInvalidCharacterError("x", "within literal true (expecting 't')")},
{"true", "trux", 3, NewInvalidCharacterError("x", "within literal true (expecting 'e')")},
{"true", "x", 0, NewInvalidCharacterError("x", "in literal true (expecting 't')")},
{"true", "trux", 3, NewInvalidCharacterError("x", "in literal true (expecting 'e')")},
}

for _, tt := range tests {
Expand Down Expand Up @@ -120,9 +120,9 @@ func TestConsumeString(t *testing.T) {
{` ""x`, false, 0, 0, 0, "", NewInvalidCharacterError(" ", "at start of string (expecting '\"')"), errPrev, errPrev},
{`"hello`, false, 6, 6, 0, "hello", io.ErrUnexpectedEOF, errPrev, errPrev},
{`"hello"`, true, 7, 7, 0, "hello", nil, nil, nil},
{"\"\x00\"", false, 1, 1, stringNonVerbatim | stringNonCanonical, "", NewInvalidCharacterError("\x00", "within string (expecting non-control character)"), errPrev, errPrev},
{"\"\x00\"", false, 1, 1, stringNonVerbatim | stringNonCanonical, "", NewInvalidCharacterError("\x00", "in string (expecting non-control character)"), errPrev, errPrev},
{`"\u0000"`, false, 8, 8, stringNonVerbatim, "\x00", nil, nil, nil},
{"\"\x1f\"", false, 1, 1, stringNonVerbatim | stringNonCanonical, "", NewInvalidCharacterError("\x1f", "within string (expecting non-control character)"), errPrev, errPrev},
{"\"\x1f\"", false, 1, 1, stringNonVerbatim | stringNonCanonical, "", NewInvalidCharacterError("\x1f", "in string (expecting non-control character)"), errPrev, errPrev},
{`"\u001f"`, false, 8, 8, stringNonVerbatim, "\x1f", nil, nil, nil},
{`"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"`, true, 54, 54, 0, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz", nil, nil, nil},
{"\" !#$%'()*+,-./0123456789:;=?@[]^_`{|}~\x7f\"", true, 41, 41, 0, " !#$%'()*+,-./0123456789:;=?@[]^_`{|}~\x7f", nil, nil, nil},
Expand Down Expand Up @@ -227,13 +227,13 @@ func TestConsumeNumber(t *testing.T) {
wantErr error
}{
{"", false, 0, io.ErrUnexpectedEOF},
{`"NaN"`, false, 0, NewInvalidCharacterError("\"", "within number (expecting digit)")},
{`"Infinity"`, false, 0, NewInvalidCharacterError("\"", "within number (expecting digit)")},
{`"-Infinity"`, false, 0, NewInvalidCharacterError("\"", "within number (expecting digit)")},
{".0", false, 0, NewInvalidCharacterError(".", "within number (expecting digit)")},
{`"NaN"`, false, 0, NewInvalidCharacterError("\"", "in number (expecting digit)")},
{`"Infinity"`, false, 0, NewInvalidCharacterError("\"", "in number (expecting digit)")},
{`"-Infinity"`, false, 0, NewInvalidCharacterError("\"", "in number (expecting digit)")},
{".0", false, 0, NewInvalidCharacterError(".", "in number (expecting digit)")},
{"0", true, 1, nil},
{"-0", false, 2, nil},
{"+0", false, 0, NewInvalidCharacterError("+", "within number (expecting digit)")},
{"+0", false, 0, NewInvalidCharacterError("+", "in number (expecting digit)")},
{"1", true, 1, nil},
{"-1", false, 2, nil},
{"00", true, 1, nil},
Expand All @@ -248,8 +248,8 @@ func TestConsumeNumber(t *testing.T) {
{"-9876543210", false, 11, nil},
{"9876543210x", true, 10, nil},
{"-9876543210x", false, 11, nil},
{" 9876543210", true, 0, NewInvalidCharacterError(" ", "within number (expecting digit)")},
{"- 9876543210", false, 1, NewInvalidCharacterError(" ", "within number (expecting digit)")},
{" 9876543210", true, 0, NewInvalidCharacterError(" ", "in number (expecting digit)")},
{"- 9876543210", false, 1, NewInvalidCharacterError(" ", "in number (expecting digit)")},
{strings.Repeat("9876543210", 1000), true, 10000, nil},
{"-" + strings.Repeat("9876543210", 1000), false, 1 + 10000, nil},
{"0.", false, 1, io.ErrUnexpectedEOF},
Expand All @@ -266,16 +266,16 @@ func TestConsumeNumber(t *testing.T) {
{"-0E0", false, 4, nil},
{"0.0123456789", false, 12, nil},
{"-0.0123456789", false, 13, nil},
{"1.f", false, 2, NewInvalidCharacterError("f", "within number (expecting digit)")},
{"-1.f", false, 3, NewInvalidCharacterError("f", "within number (expecting digit)")},
{"1.e", false, 2, NewInvalidCharacterError("e", "within number (expecting digit)")},
{"-1.e", false, 3, NewInvalidCharacterError("e", "within number (expecting digit)")},
{"1.f", false, 2, NewInvalidCharacterError("f", "in number (expecting digit)")},
{"-1.f", false, 3, NewInvalidCharacterError("f", "in number (expecting digit)")},
{"1.e", false, 2, NewInvalidCharacterError("e", "in number (expecting digit)")},
{"-1.e", false, 3, NewInvalidCharacterError("e", "in number (expecting digit)")},
{"1e0", false, 3, nil},
{"-1e0", false, 4, nil},
{"1E0", false, 3, nil},
{"-1E0", false, 4, nil},
{"1Ex", false, 2, NewInvalidCharacterError("x", "within number (expecting digit)")},
{"-1Ex", false, 3, NewInvalidCharacterError("x", "within number (expecting digit)")},
{"1Ex", false, 2, NewInvalidCharacterError("x", "in number (expecting digit)")},
{"-1Ex", false, 3, NewInvalidCharacterError("x", "in number (expecting digit)")},
{"1e-0", false, 4, nil},
{"-1e-0", false, 5, nil},
{"1e+0", false, 4, nil},
Expand Down
4 changes: 2 additions & 2 deletions internal/jsonwire/wire.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,9 @@ func NewInvalidEscapeSequenceError[Bytes ~[]byte | ~string](what Bytes) error {
return r == '`' || r == utf8.RuneError || unicode.IsSpace(r) || !unicode.IsPrint(r)
}) >= 0
if needEscape {
return errors.New("invalid " + label + " " + strconv.Quote(string(what)) + " within string")
return errors.New("invalid " + label + " " + strconv.Quote(string(what)) + " in string")
} else {
return errors.New("invalid " + label + " `" + string(what) + "` within string")
return errors.New("invalid " + label + " `" + string(what) + "` in string")
}
}

Expand Down
22 changes: 19 additions & 3 deletions jsontext/decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,9 +364,22 @@ func (d *decoderState) CountNextDelimWhitespace() int {

// checkDelim checks whether delim is valid for the given next kind.
func (d *decoderState) checkDelim(delim byte, next Kind) error {
where := "at start of value"
switch d.Tokens.needDelim(next) {
case delim:
return nil
case ':':
where = "after object name (expecting ':')"
case ',':
if d.Tokens.Last.isObject() {
where = "after object value (expecting ',' or '}')"
} else {
where = "after array element (expecting ',' or ']')"
}
}
pos := d.prevEnd // restore position to right after leading whitespace
pos += jsonwire.ConsumeWhitespace(d.buf[pos:])
err := d.Tokens.checkDelim(delim, next)
err := jsonwire.NewInvalidCharacterError(d.buf[pos:], where)
return wrapSyntacticError(d, err, pos, 0)
}

Expand Down Expand Up @@ -618,7 +631,7 @@ func (d *decoderState) ReadToken() (Token, error) {
return ArrayEnd, nil

default:
err = jsonwire.NewInvalidCharacterError(d.buf[pos:], "at start of token")
err = jsonwire.NewInvalidCharacterError(d.buf[pos:], "at start of value")
return Token{}, wrapSyntacticError(d, err, pos, +1)
}
}
Expand Down Expand Up @@ -833,6 +846,9 @@ func (d *decoderState) consumeValue(flags *jsonwire.ValueFlags, pos, depth int)
case '[':
return d.consumeArray(flags, pos, depth)
default:
if (d.Tokens.Last.isObject() && next == ']') || (d.Tokens.Last.isArray() && next == '}') {
return pos, errMismatchDelim
}
return pos, jsonwire.NewInvalidCharacterError(d.buf[pos:], "at start of value")
}
if err == io.ErrUnexpectedEOF {
Expand Down Expand Up @@ -1068,7 +1084,7 @@ func (d *decoderState) consumeArray(flags *jsonwire.ValueFlags, pos, depth int)
pos++
return pos, nil
default:
return pos, jsonwire.NewInvalidCharacterError(d.buf[pos:], "after array value (expecting ',' or ']')")
return pos, jsonwire.NewInvalidCharacterError(d.buf[pos:], "after array element (expecting ',' or ']')")
}
}
}
Expand Down
Loading

0 comments on commit d3b4f71

Please sign in to comment.