Error reporting on ParseUAST to avoid panic: runtime error

[bzz]

Right now resp, err := client.ParseUAST(context.TODO(), req) may fail with error beeing NOT nil but actual parsing has failed and there are resp.Errors and resp.status is fatal.

resp may be nil (i.e if server is not running) which is absolutely fine.

It seems like quite un-obvious behavior (at least for Golang) that may either be possible to fix or at least to documented everywhere, including https://doc.bblf.sh/user/server-grpc-example.html#full-source-of-the-example

Otherwise API users will on random NPE panics on further response.UAST manipulations (which do not propagate any error message).

Here is the example of client that works around current behavior, which boils down to

 resp, err := client.ParseUAST(context.TODO(), req)
 if err != nil {
 	fmt.Printf("Error - ParseUAST failed, reposne is nil, error:%v for %s\n", err, f.Name)
 } else if resp == nil {
 	fmt.Printf("No error, but - ParseUAST failed, response is nil\n")
 } else if (len(resp.Errors) != 0) || (resp.Status != protocol.Ok) {
 	fmt.Printf("No error, but - ParseUAST status:%s, error:%v for %s\n", resp.Status, resp.Errors, f.Name)
 }

[juanjux]

I think there are two questions here.

1. Should the response from a not running server be nil? I would like to avoid nil as much as possible, but if it's nil and err is not nil I don't see a big problem here (that behaviour is probably from the Go version of gRPC itself, but I'm not sure).

2. ParseUAST should load err if resp.status != Ok? I think it should so the user shouldn't have to check it again on the err != nil path and should only have to check resp.Errors on the error path. This is probably the real bug.

[bzz]

@juanjux agree on both points, sorry for confusion. Updated the description above

[smola]

Documentation fixed here: bblfsh/documentation#53

Note that the Babelfish protocol defines its own error handling (status and errors) for parsing. This is then wrapped in gRPC which generates its own error handling (as second return value: err). There's not much we can do here unless we want to offer a more high level wrapper on the client side.

[bzz]

Sounds good.