Clarify error statuses returned by native drivers

[dennwc]

Babelfish drivers may return 3 main statuses: OK, Error, Fatal.

Currently, there is no guarantee that Error responses will have a partial AST, since it's not tested in any of the drivers.

At the same time, Error status is not considered an error/exception in clients as reported in bblfsh/java-driver#77, thus clients might not be aware that they receive a partial AST.

We should either:
a) Make a special type of error/exception that user code should assert to accept a partial AST. In this case clients will receive an error in case of any syntax erros, which might be a desired behavior.
b) Mention current behavior in Babelfish docs, clarify that user should check the status.

Personally, I would prefer the first option.

[dennwc]

Ping @smola @juanjux

[juanjux]

It's documented that the user should check the status code in the Reading and Interpreting the Response section.

[dennwc]

...only a value of protocol.Status.OK will indicate success.

This is what is written in docs. I think this means that clients should raise an error/exception for all statuses other than OK and users will not be forced to check it.

[juanjux]

The full quote, emphasis mine:

You should check the status (Status in the case of Go, since public members start with uppercase); only a value of protocol.Status.OK will indicate sucess.

I think it's clear in saying that the status should be checked. It doesn't say that the clients should raise an exception.

Once that's said, we can talk about if we want the clients to raise an exception for the Error status. Personally, I think a broken source file is not an exceptional failure (especially when you are parsing thousands and exceptions in Scala can have a performance penalty unlike in Python) and clients should check the status (it's there for a reason), but if other people and most importantly our existing users agree we could change that.

[dennwc]

Sure, it's written clearly, but bblfsh/java-driver#77 shows that no one reads it ;) So I think it might make sense to raise an error for these cases.

The point about performance penalty is significant enough. Will think about it a bit more.

[juanjux]

Could be an Optional in Scala which AFAIK doesn't have the performance penalty of exceptions and force the user to check them before usage.

[smola]

Scala's Option doesn't work here, since you could not check the error message in case of error. Option can be Some(result) or None.

You have other options to avoid exceptions:

Define a DriverException with a method to retrieve the failed UAST, and then one of the following:

1. Define a DriverException or with a method to retrieve the failed UAST, if any. Then your parse methods can return scala.util.Try[UAST]. Docs. This has the advantage of being
2. Use scala.Either[UAST, DriverException]. Docs.

Semantics of Try are more explicit, and slightly more convenient in some cases (e.g. you can map just as if it was an Option

[zurk]

I read the docs that you mentioned guys, and I personally think that we just should add something more to it. Because in case of not OK status it is unclear what does it mean.

I found another piece of documentation here: https://doc.bblf.sh/driver/internal-protocol.html

If the parsing is successful, status is ok. If the file could be parsed (AST was generated) but had parsing errors, status is error. If the file could not be parsed at all (no AST), status is fatal.

and maybe we should just add this clarification to the part @juanjux mentioned: https://doc.bblf.sh/user/server-grpc-example.html#reading-and-interpreting-the-response

I do not think this it is worth errors to throw an exception.

[dennwc]

@zurk Sorry, I wasn't clear about what I meant by errors/exceptions. It more in lines with "any native way for a language to return an error state with a failed AST". The way how it would be done in Go is very similar to a way how @smola proposed to handle it in Scala.

[juanjux]

@smola but this would require throwing an exception internally anyway, doesn't it? My concern is that Exceptions have an effect in performance in C++ and Java and by extension I guess in Scala (not much in Python unless things have changed since I tested it). Maybe we could write something similar to Option that returns the error and the partial UAST if the driver provides it instead of None.

[smola]

@juanjux Maybe we could write something similar to Option that returns the error and the partial UAST if the driver provides it instead of None -> That would be something like Either[UAST, (string, UAST)]. In the second case, there is an error that cannot be easily ignored. People would still be able to write a quick one liner to get the UAST without checking the error, but it would be quite hard, it needs to be a very conscious decision.

[smola]

Or maybe we should just stress this in our docs.

It doesn't help if our own official examples do not check status ;)

• https://doc.bblf.sh/using-babelfish/language-clients.html#example

And there could be further reminders here:

• https://doc.bblf.sh/using-babelfish/babelfish-protocol.html#parserequest

And expand the description about how to use Status here:

• https://godoc.org/gopkg.in/bblfsh/sdk.v1/protocol#Response

Maybe in some other places too.

[juanjux]

That example on the language clients page is pretty weak. Not only it doesn't check the Status but uses reflection to check the type of the root node (???). I must have been drunk or something when I reviewed that. Sorry. I'll fix that and the other page thanks for noticing.

[juanjux]

@smola PTAL bblfsh/documentation/pull/160

Personally, I preferred the previous version in Python before it was nuked by the previous lead since: 1) It's still a lot more used than Go and thus more readers would be able to get it and 2) Shows that bblfsh is not a "go-only" project for users. What do you think?

[smola]

@juanjux Examples in multiple languages would be nice in the docs. Go, Python and Scala, ideally.

[juanjux]

I agree, I'll do it.

[dennwc]

Since the v2 protocol is going live, it's the right time to revisit this for client libraries.

[dennwc]

Done, see #321