lib/json: don't fatal on Attr errors when encoding

[andreimatei]

Before this patch, the json library would log.Fatal() when encoding a HasAttrs struct whose .Attr(name) return nil or an error for a name that was previously listed by AttrNames(). Fatal'ing in a library is a bit presumptuous. In particular, Delve the Go debugger uses go-starlark to run scripts, and also have implementations of HasAttr that can fail at runtime (e.g. a struct modelling a variable from the debugged process implements Attr uses magic to present the field names of the target variable in AttrNames, and the Attr implementation returns errors if one particular field cannot be loaded). Delve even goes as far as recovering panics from script execution, but that doesn't work for log.Fatal.

This patch returns the error (thus also failing the script) instead of the log.Fatal.

[adonovan]

I agree that log.Fatal is perhaps a little too aggressive a way to deal with inconsistency between AttrNames and Attr, but emitting the error message as a string seems too timid; it brushes the problem under the rug. The more defensible thing to do would be to return an error, which is what the JSON encoder does when it encounters a Starlark value that cannot be JSON encoded, for example because it is cyclic, or a nonfinite number, or a dict whose keys are not strings.

Callers shouldn't expect any old Starlark value to be JSON encodable.

[andreimatei]

I obviously have no authority here, so I'm happy to do whatever you think is best. But I would challenge:

Callers shouldn't expect any old Starlark value to be JSON encodable.

Why not? I would treat json encoding like the print function - have it always succeed (within reason - perhaps not for cycles), and do its best. Or at least offer maximum tolerance as an option, if not the default.

As an anecdote - I got here at the bottom of a rabbit hole where I'm extending the existing support for Starlark in Delve to support sending scripts for execution through an RPC. These scripts have access to, and want to return, objects internal to Delve - and Delve has this utility that can adapt any regular go struct to be usable from starlark using reflection. Some particular internal structs are extra magical and have their own dedicated adapters, with their own dynamic algorithm for implementing Attr and AttrNames (example). Now, for my RPC purposes, I'd like to return one of these internal structs in a format that the RPC client can comsume - thus my interest in this json library. The first thing I tried was to simply json.encode(<my value>), except that sometimes it doesn't work because of these Attr errors. In particular, these errors are produced by structs representing variables in the debugged program, when part of the variable has not been loaded from the target's memory, and yet this struct is exactly the thing that I'd want to be able to encode as JSON. Another argument for maximum tolerance is that, unless I'm missing something, the script does not have any way to recover from json.encode returning an error (or does it?).

Assuming that adding tolerance for the nil, nil return from Attr (no value, no error) is acceptable, you may suggest that making the Attr function in question not return an error in this semi-expected case of "value not loaded", and just return nil, nil. I could try to propose such a patch to Delve but, in contexts other than json marshalling, it's probably a good idea for Attr to return an error in that case (i.e. when using starlark in Delve through the bult-in REPL).

If taking this patch as is is still not palatable, perhaps I could make json.encode take an optional argument that controls error behavior?

[adonovan]

Callers shouldn't expect any old Starlark value to be JSON encodable. Why not? I would treat json encoding like the print function - have it always succeed (within reason - perhaps not for cycles), and do its best. Or at least offer maximum tolerance as an option, if not the default.

Because that's not how JSON encoding has ever worked in Starlark, or Python, or Go. Or even Java or C++ for that matter. Even JavaScript's JSON.stringify fails when it encounters a cycle. And generally this kind of strictness is a good thing too as it prevents bugs in one program from spreading carnage throughout distributed systems (servers, files, databases, etc).

If you really want "just cram the round peg into the square hole already" semantics, you can implement this by defining wrapper types for IterableMapping, Iterable, and HasAttr and wrapping your root object before JSON encoding. The wrapper could then define whatever semantics it wants for Attr, Index, etc so that every subvalue it returns is itself recursively JSON encodable. It might not be much less work than writing the JSON encoder directly, but it would definitely work.

Another argument for maximum tolerance is that, unless I'm missing something, the script does not have any way to recover from json.encode returning an error (or does it?).

No, and that's a good point, but there's an effort right now to fix that; see #465.

I'd be happy to accept a change that turns log.Fatal into return fmt.Errorf.

[andreimatei]

So where do you stand on the behavior when Attr returns nil, nil? Are you OK with tolerating that one?

And just to make sure - you're not in favor of adding arguments controlling the error behavior (or perhaps another flavor of the json.encode builtin altogether)?

[adonovan]

So where do you stand on the behavior when Attr returns nil, nil? Are you OK with tolerating that one?

I think it's a bug for a value to respond to inconsistent sets of names in the AttrName and Attr methods.

And just to make sure - you're not in favor of adding arguments controlling the error behavior (or perhaps another flavor of the json.encode builtin altogether)?

The proposal I linked to does add an argument controlling the error behavior, in effect, though it's a very crude kind of control. But otherwise, no, not in favour. New API needs to go through the bazelbuild/starlark spec proposal process.

[andreimatei]

So where do you stand on the behavior when Attr returns nil, nil? Are you OK with tolerating that one?

I think it's a bug for a value to respond to inconsistent sets of names in the AttrName and Attr methods.

ack

The proposal I linked to does add an argument controlling the error behavior, in effect, though it's a very crude kind of control. But otherwise, no, not in favour. New API needs to go through the bazelbuild/starlark spec proposal process.

What I see in https://github.com/google/starlark-go/pull/466 seems to be exclusively about decode (whereas here we're talking about encode). Am I missing something?

[adonovan]

What I see in https://github.com/google/starlark-go/pull/466 seems to be exclusively about decode (whereas here we're talking about encode). Am I missing something?

No, you're quite right, sorry for the confusion. I suggest you comment on that proposal while it's still in process, and ask for (say) an optional keyword parameter on encode that causes it to report a success bool rather than fail in case of nonencodable values.

This does rather get to the problem of error handling in Starlark, about which there isn't yet a clear plan for a consistent solution. (In a build tool, the domain for which Starlark was originally designed, there is rarely any need to recover from fallible operations.) One possibility is an extra parameter that causes the function to return a pair of its normal result and an error (like in Go); alternatively this could be separate function. Some care is required to ensure that we don't inadvertently provide a general exception handler mechanism, as would be the case, for example, if json.encode called back into application code.

[andreimatei]

I've changed this patch to return the error. I've taken some liberties with the error message; let me know if you'd like something else.

---

I suggest you comment on that proposal while it's still in process, and ask for (say) an optional keyword parameter on encode that causes it to report a success bool rather than fail in case of nonencodable values.

Done - https://github.com/google/starlark-go/issues/465#issuecomment-1563185082

But, for the record, neither this present patch in its current form, nor some sort of error tolerance for encode that doesn't give me back a partial json solves my personal problem, so I'm personally not motivated to work on it. My goal is to get a partial JSON, so I'll have to either hack it, or patch Delve to implement custom json.Marshaler on the types of interest (which I guess is your answer to my predicament?). I hear you that "this is not how json encoding works in other settings", but I'd argue that these other settings don't have attribute accessors that are fallible - that's what makes our starlark-go setting unique for the particular case of HasAttrs. I would also note that Python's json.dump takes arguments like skipkeys and check_circular that afford control over its dynamic error behavior. Let me know if any of this makes you more sympathetic to more optional control.

[adonovan]

My goal is to get a partial JSON

Does the approach I suggested earlier not work for you? That is, define three wrapper types, one for each interface relevant to the JSON encoder, and have each one wrap (or otherwise convert) the subelements it returns to ensure that they are recursively encodable. Any time you aren't certain that a Starlark value isn't recursively encodable, apply the appropriate wrapper before calling encode.

Alternatively you could write a function (in Go or possibly even in Starlark) that recursively converts an arbitrary value into a tree of bool/int/float/string/list/dict/struct that is guaranteed to be recursively encodable, applying arbitrary transformations as desired.

I would also note that Python's json.dump takes arguments like skipkeys and check_circular that afford control over its dynamic error behavior. Let me know if any of this makes you more sympathetic to more optional control.

Yes, some encoders do have a lot of additional control options, and we've been trying to avoid the need for that here (just as Go's main JSON encoder avoids them, though it's true that there are many different JSON encoders in the Go ecosystem). II would want to fully explore the transformation function approach describe above first before complicating the encoder.

[andreimatei]

Does the approach I suggested earlier not work for you? That is, define three wrapper types, one for each interface relevant to the JSON encoder, and have each one wrap (or otherwise convert) the subelements it returns to ensure that they are recursively encodable. Any time you aren't certain that a Starlark value isn't recursively encodable, apply the appropriate wrapper before calling encode.

The problem with the non-fallible wrapper approach (as well as implementing json.Marshal) is that they would need to leave in Delve code - and kinda the point of Starlark is that I shouldn't have to modify the host in order to extend it. And not only that the wrapper structs needs to be added to Delve, but I also need to add a new function callable from Starlark that wraps a Starlark.Value in such a wrapper (right?). But, yeah, this might be what I'll do.

Alternatively you could write a function (in Go or possibly even in Starlark) that recursively converts an arbitrary value into a tree of bool/int/float/string/list/dict/struct that is guaranteed to be recursively encodable, applying arbitrary transformations as desired.

If I write such a function in Go, this would be basically equivalent to implementing my own json.encode, wouldn't it? Writing it in Starlark could be interesting - but how would I do it? I'd have to use dir to enumerate the AttrNames, and then I'd have the same problem that accessing one of the fields for which Attr(name) returns an error would stop my script - right?

[adonovan]

If I write such a function in Go, this would be basically equivalent to implementing my own json.encode, wouldn't it?

It might overall be a similar quantity of code, but it would probably be simpler code because it would be concerned only with Starlark values in your domain, and not with the textual business of JSON encoding, for which you could rely on the existing function.

I'd have to use dir to enumerate the AttrNames, and then I'd have the same problem that accessing one of the fields for which Attr(name) returns an error would stop my script - right?

Yes, you would use dir to enumerate names, and getattr to obtain each one. If you pass a sentinel default value to getattr you can detect when the attribute is missing (or any other error) and choose what to do.

[andreimatei]

Yes, you would use dir to enumerate names, and getattr to obtain each one. If you pass a sentinel default value to getattr you can detect when the attribute is missing (or any other error) and choose what to do.

Interesting, I didn't know about getattr. Thanks for the suggestions!