Handles errors in the xpath queries gracefully

[ncordon]

Recovers from the panic, in case it is thrown when executing the xpath query and throws an error instead. When executing, in the python-client after this changes, the query:

import bblfsh

client = bblfsh.BblfshClient("localhost:9432")
ctx = client.parse("./file.php")
ctx.filter("//*[@role='Variable']*//Name")

the python interpreter no longer explodes and we are shown a friendlier message:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.7/site-packages/bblfsh/result_context.py", line 34, in filter
    return NodeIterator(self.ctx.filter(query), self.ctx)
RuntimeError: Error executing the xPath query, maybe wrong syntax?

Since the modified method was already returning an error (but was not handling the panic and therefore Go killed the process of the client), this change should not carry cascade changes to the clients if they were already checking for an error when performing filter (python-client was catching the exception, for example).

Closes #424

Signed-off-by: ncordon nacho.cordon.castillo@gmail.com

---

This change is 

[ncordon]

---

uast/query/xpath/xpath.go, line 44 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

As you may imagine, it's not an ideal fix, but we agreed that it makes sense for now. Can you please add a `TODO` comment to fix XPath library in the future (if we decide to support it)?

Yes, of course

[ncordon]

---

uast/query/xpath/xpath.go, line 46 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

It's also worth adding the panic value to the message - it usually adds some context: ```golang fmt.Errorf("panic when executing the XPath query; wrong syntax? recovered from: %v", r) ```

I explicitly omitted that part because it only adds noise: RuntimeError: Error executing the xPath query, maybe wrong syntax? recovered from: runtime error: invalid memory address or nil pointer dereference, but we can add it

[ncordon]

---

uast/query/xpath/xpath.go, line 77 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Yeah, sorry, also for the context, returning `smth, err2` will act as a `_, err = smth, err2; return` (assuming suggestion above). So you don't need to return `err` specifically if you define a name for it.

Thanks about the comments about the shadowing :smile: , I did not know that could such a big problem

[ncordon]

---

uast/query/xpath/xpath.go, line 44 at r1 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

Yes, of course

I am going to open an issue also, because often TODO comments are just forgotten in time :trollface:

[ncordon]

---

uast/query/xpath/xpath.go, line 43 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Yeah, I totally agree. In this case, however, we need to name the error variable at least (for obvious reasons). Everything else can be `_`.

Done.

[ncordon]

a discussion (no related file):

Previously, dennwc (Denys Smirnov) wrote…

Also, one more thing, can we add a test case?

Added!

---

[ncordon]

---

uast/query/xpath/xpath.go, line 43 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Thanks, but could you please also rename the error as I mentioned in the first comment? The code is not wrong, it's just easier to break it accidentally by naming an error `err`.

Changed now!

[ncordon]

---

uast/query/xpath/xpath.go, line 46 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Maybe adding a newline will help with it? I think it may be useful to have those messages in logs, because the cause of the panic may be different next time.

Done!

[ncordon]

---

uast/query/xpath/xpath.go, line 44 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Also, please follow this format: `// TODO(ncordon): ...`

Done!

[ncordon]

---

uast/query/xpath/xpath.go, line 77 at r1 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

Thanks about the comments about the shadowing :smile: , I did not know that could such a big problem

Done.