go-python / go-python/gopy

"returned a result with an error set" instead of Exception for functions that return a value type

Open
#254 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
2.3k
Forks
136
Avg merge
14m
Merged PRs (30d)
2

Description

I've encountered a situation where a certain wrapped function fails in the Go runtime, but the exception is swallowed and instead a generic Python exception is raised:

```
SystemError: returned a result with an error set
```

We can see an exiting example in the `gopy/_examples/pyerrors` test:

```go
func Div(i, j int) (int, error) {
if j == 0 {
return 0, errors.New("Divide by zero.")
}
return i / j, nil
}
```

```python
def div(a, b):
try:
r = pyerrors.Div(a, b)
print("pyerrors.Div(%d, %d) = %d"% (a, b, r))
except Exception as e:
print(e)

div(5,0)
# returned a result with an error set
```

This makes it pretty difficult to debug since one would have to track down the wrapped code and figure out which return statement is actually being hit, then add some logging to see the real error.

I've been digging through pygo and PyBindGen for a few hours to better understand this problem. It seems like @rcoreilly was already aware of this problem when the test was written, and coded around it to have this output actually be a passing test, with a TODO to fix it later? But it seems like there is a bit of a disconnect between the location where gopy generates the C code with python `C.PyErr_SetString` error handling (`bind/gen_func.go`) vs where PyBindGen accepts C functions being added. I don't understand where there is any existing PyBindGen API to indicate what return value should be considered an error and when to check for one.

Problem: If a Python error has been set, then a `PyObject*` should not be returned. gopy sets the error, but PyBindGen has no current logic in the generated C code that checks `PyErr_Occurred()` other than some special int pointer case.

I have no experience with PyBindGen, but a decent amount with Cython. In cython there are a few different ways where you can indicate that a wrapped function can raise an exception. You can tell it an exact value that indicate an exception (ie -1 always being an exception) or you can tell it to always check `PyErr_Occurred` at the extra expense of that check for each call. I don't know how to do that for PyBindGen but ultimately I think we need to figure out how to set that.

Here is an example of what PyBindGen generates:

```python
# build.py
mod.add_function('mypackage_DoThing', retval('int64_t'), [param('int64_t', '_handle')])
```

```C
PyObject *
_wrap__mypackage_DoThing(PyObject * PYBINDGEN_UNUSED(dummy), PyObject *args, PyObject *kwargs)
{
PyObject *py_retval;
int64_t retval;
int64_t _handle;
const char *keywords[] = {"_handle", NULL};

if (!PyArg_ParseTupleAndKeywords(args, kwargs, (char *) "L", (char **) keywords, &_handle)) {
return NULL;
}
retval = mypackage_DoThing(_handle);
py_retval = Py_BuildValue((char *) "L", retval);
return py_retval;
}
PyObject * _wrap__mypackage_DoThing(PyObject * PYBINDGEN_UNUSED(dummy), PyObject *args, PyObject *kwargs);
```

The key part is this:

```C
retval = mypackage_DoThing(_handle);
py_retval = Py_BuildValue((char *) "L", retval);
return py_retval;
```

If `mypackage_DoThing()` has an error, it sets `PyErr_SetString`, and in which case it should return `NULL`. Is there a way to tell PyBindGen how a wrapped C function indicates an error? Ultimately we want something like this:

```C
retval = mypackage_DoThing(_handle);
if (PyErr_Occurred()) {
return NULL;
}
py_retval = Py_BuildValue((char *) "L", retval);
return py_retval;
```

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.