starlarkjson: a standard json module for Starlark #179

alandonovan · 2019-03-29T18:47:23Z

This is a sketch of a standard module for JSON encoding/decoding for Starlark.

It is intended to subsume, generalize, and eventually
replace the ill-conceived struct.to_json method.

See related issues:
bazelbuild/bazel#7896
https://b/23962735 (Googlers only)
https://b/70210417 (Googlers only)
bazelbuild/bazel#7879 (comment)
bazelbuild/bazel#5542

alandonovan · 2019-03-29T18:47:54Z

@laurentlb

jayconrod · 2019-04-01T17:51:20Z

starlark/testdata/json.star

+# Exercise JSON string coding by round-tripping a string with every 16-bit code point.
+def codec(x):
+  return json.decode(json.encode(x))
+codepoints = ''.join(['%c' %c for c in range(65536)])


Nit: add space between % operator and c identifier.

Funny! Fixed.

jayconrod · 2019-04-01T18:23:24Z

starlarkjson/json.go

+func quote(buf *bytes.Buffer, s string) {
+	if goQuoteIsSafe(s) {
+		var quoteSpace [128]byte
+		buf.Write(strconv.AppendQuote(quoteSpace[:0], s))


Are you trying to avoid memory allocation with this case? I'm not sure how inlining and escape analysis will interact, but it seems like quoteSpace may escape, and you'll end up making a copy on the heap anyway.

Perhaps instead, narrow the optimized case to strings that don't contain anything that needs to be escaped (adding " and \ to the list of bad runes). Then you could use buf.WriteString(s).

Quite right; this was reduced from code in herb's JSON encoder which reused the quoteSpace variable as an object field across many calls, but it makes no sense here.

I've moved quote and quoteSpace inside encode, so the closure acts like an object and quoteSpace like a field.

jmillikin-stripe · 2019-08-26T03:52:34Z

I have an outstanding request with Bazel to add JSON support in the Java implementation (bazelbuild/bazel#3732). To see if we can settle on a reasonable shared JSON API, I sent bazelbuild/starlark#83 as a proposed standard JSON module in the Starlark spec.

alandonovan

Hi Jay, I have finally gotten back to this CL (motivated by yet more bugs in struct.to_json). I've rewritten the decode function and added more tests. Care to take another look?

thanks
A

jayconrod · 2020-03-06T00:16:25Z

I'll take a look on Monday if that's all right. I'm just heading out now, and taking a vacation day tomorrow.

alandonovan · 2020-03-06T03:45:07Z

Is something about this CL not giving you a sense of utmost urgency? ; )

Enjoy your vacation.

jayconrod · 2020-03-10T15:28:02Z

starlarkjson/json.go

+	"go.starlark.net/starlarkstruct"
+)
+
+// Module is a Starlark module of JSON-related functions.


Module json

jayconrod · 2020-03-10T15:29:17Z

starlarkjson/json.go

+// which it converts to JSON by cases:
+// - A Starlark value that implements Go's standard json.Marshal
+//   interface defines its own JSON encoding.
+// - None, True, and False are converted their corresponding JSON atoms.


are converted to...

Though maybe just: "null, true, and false, respectively."

jayconrod · 2020-03-10T15:44:52Z

starlarkjson/json.go

+}
+
+// isASCII reports whether s contains only ASCII.
+func isASCII(s string) bool {


Nit: rename to isPrintableASCII?

jayconrod · 2020-03-10T15:49:43Z

starlarkjson/json.go

+	// In principle, we could parameterize it to allow the caller to
+	// control the returned types, but there's no compelling need yet.
+
+	type failure string


A brief comment here would be useful. It's not obvious why this is needed without scrolling much further down.

jayconrod · 2020-03-10T15:57:56Z

starlarkjson/json.go

+			return starlark.String(r)
+
+		case 'n':
+			if strings.HasPrefix(s[i:], "null") {


Given a string like nulll, I think this will read null, then report an unexpected character l after that.

Error messages would be a little clearer if you scan a run of identifier characters, then check if they are one of null, true, false.

I did it this way so that the parser never consumes more than it needs, as we may want a stream decoder in the future (like json.NewDecoder in Go), from which you can read adjacent values such as "truefalse".

jayconrod · 2020-03-10T16:34:22Z

starlarkjson/json.go

+				i = j
+
+				// Unlike most C-like languages,
+				// JSON disallows a leading before a digit.


a leading 0?

jayconrod · 2020-03-10T16:40:38Z

starlarkjson/json.go

+
+				// Unlike most C-like languages,
+				// JSON disallows a leading before a digit.
+				if z := boolint(num[0] == '-'); num[z] == '0' && z+1 < len(num) && isdigit(num[z+1]) {


I think num == "-" is possible here, so this might panic.

Also, the boolint logic is a little confusing. Maybe drop boolint and spell it out here?

Good catch. Fixed, simplified, and tested.

alandonovan

Thanks!

alandonovan · 2020-03-05T23:18:05Z

starlarkjson/json.go

+		if isASCII(s) {
+			buf.Write(strconv.AppendQuote(quoteSpace[:0], s))
+		} else {
+			data, _ := json.Marshal(s)


I just learned about RFC 8259, which mandates UTF-8 encodings for JSON. I haven't looked at the details yet, but perhaps this would simplify the encoder here.

alandonovan · 2020-06-11T21:31:36Z

starlarkjson/json.go

+	"go.starlark.net/starlarkstruct"
+)
+
+// Module is a Starlark module of JSON-related functions.


alandonovan · 2020-06-11T21:32:05Z

starlarkjson/json.go

+// which it converts to JSON by cases:
+// - A Starlark value that implements Go's standard json.Marshal
+//   interface defines its own JSON encoding.
+// - None, True, and False are converted their corresponding JSON atoms.


alandonovan · 2020-06-11T21:33:45Z

starlarkjson/json.go

+}
+
+// isASCII reports whether s contains only ASCII.
+func isASCII(s string) bool {


alandonovan · 2020-06-11T21:34:45Z

starlarkjson/json.go

+	// In principle, we could parameterize it to allow the caller to
+	// control the returned types, but there's no compelling need yet.
+
+	type failure string


alandonovan · 2020-06-11T21:45:47Z

starlarkjson/json.go

+			return starlark.String(r)
+
+		case 'n':
+			if strings.HasPrefix(s[i:], "null") {


I did it this way so that the parser never consumes more than it needs, as we may want a stream decoder in the future (like json.NewDecoder in Go), from which you can read adjacent values such as "truefalse".

alandonovan · 2020-06-11T21:46:04Z

starlarkjson/json.go

+				i = j
+
+				// Unlike most C-like languages,
+				// JSON disallows a leading before a digit.


alandonovan · 2020-06-11T21:53:54Z

starlarkjson/json.go

+
+				// Unlike most C-like languages,
+				// JSON disallows a leading before a digit.
+				if z := boolint(num[0] == '-'); num[z] == '0' && z+1 < len(num) && isdigit(num[z+1]) {


Good catch. Fixed, simplified, and tested.

This change defines a standard Starlark module for JSON encoding and decoding. See json.go for documentation. It is intended to subsume, generalize, and eventually replace Bazel's ill-conceived struct.to_json method. The json module is predeclared in the Starlark REPL environment. See related issues: bazelbuild/bazel#7896 https://buganizer.corp.google.com/issues/23962735 https://buganizer.corp.google.com/issues/70210417 bazelbuild/bazel#7879 (comment) bazelbuild/bazel#5542 bazelbuild/bazel#10176 bazelbuild/starlark#83 bazelbuild/bazel#3732 Change-Id: I297ffaee9349eedeeb52f5a88f40636a4095f997

alandonovan requested review from jayconrod and josharian March 29, 2019 18:47

alandonovan force-pushed the json branch from 6998490 to d0840be Compare March 29, 2019 18:53

alandonovan mentioned this pull request Mar 29, 2019

Add indentation to struct.to_json() bazelbuild/bazel#7879

Closed

jayconrod approved these changes Apr 1, 2019

View reviewed changes

alandonovan force-pushed the json branch from d0840be to 9eb4e37 Compare April 2, 2019 01:00

alandonovan mentioned this pull request Jul 2, 2019

How to read/write json data using starlark? #215

Closed

bpowers mentioned this pull request Feb 24, 2020

proposal: JSON Module bazelbuild/starlark#83

Open

alandonovan force-pushed the json branch from 9eb4e37 to 269f33c Compare March 5, 2020 22:33

alandonovan commented Mar 5, 2020

View reviewed changes

alandonovan force-pushed the json branch from 269f33c to bb34377 Compare March 5, 2020 23:36

alandonovan force-pushed the json branch from bb34377 to 9b56494 Compare March 6, 2020 14:15

jayconrod reviewed Mar 10, 2020

View reviewed changes

alandonovan commented Jun 11, 2020

View reviewed changes

alandonovan force-pushed the json branch from 9b56494 to d2cb3ad Compare June 11, 2020 21:55

alandonovan merged commit ac23acb into master Jun 11, 2020

alandonovan deleted the json branch June 11, 2020 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

starlarkjson: a standard json module for Starlark #179

starlarkjson: a standard json module for Starlark #179

alandonovan commented Mar 29, 2019 •

edited

Loading

alandonovan commented Mar 29, 2019

jayconrod Apr 1, 2019

alandonovan Apr 2, 2019

jayconrod Apr 1, 2019

alandonovan Apr 2, 2019 •

edited

Loading

jmillikin-stripe commented Aug 26, 2019

alandonovan left a comment

jayconrod commented Mar 6, 2020

alandonovan commented Mar 6, 2020

jayconrod Mar 10, 2020

alandonovan Jun 11, 2020

jayconrod Mar 10, 2020

alandonovan Jun 11, 2020

jayconrod Mar 10, 2020

alandonovan Jun 11, 2020

jayconrod Mar 10, 2020

alandonovan Jun 11, 2020

jayconrod Mar 10, 2020

alandonovan Jun 11, 2020

jayconrod Mar 10, 2020

alandonovan Jun 11, 2020

jayconrod Mar 10, 2020

alandonovan Jun 11, 2020

alandonovan left a comment

alandonovan Mar 5, 2020

alandonovan Jun 11, 2020

alandonovan Jun 11, 2020

alandonovan Jun 11, 2020

alandonovan Jun 11, 2020

alandonovan Jun 11, 2020

alandonovan Jun 11, 2020

alandonovan Jun 11, 2020

starlarkjson: a standard json module for Starlark #179

starlarkjson: a standard json module for Starlark #179

Conversation

alandonovan commented Mar 29, 2019 • edited Loading

alandonovan commented Mar 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alandonovan Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

jmillikin-stripe commented Aug 26, 2019

alandonovan left a comment

Choose a reason for hiding this comment

jayconrod commented Mar 6, 2020

alandonovan commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alandonovan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alandonovan commented Mar 29, 2019 •

edited

Loading

alandonovan Apr 2, 2019 •

edited

Loading