BUG: Propagate Python exceptions from c_is_list_like (#33721)
State: closed

- [x] closes #33721
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Bug
name: Segfault
description: Non-Recoverable Error

Comments:
May also be an issue that `is_array` is defined in `pandas/_libs/tslibs/util.pxd` but this seems to try and access from `pandas/_libs/util.pxd`
> May also be an issue that is_array is defined in pandas/_libs/tslibs/util.pxd but this seems to try and access from pandas/_libs/util.pxd

Out of skepticism I tried changing the cimport and it didnt change the segfault.
> I still don't think this is the right solution. As is this function would never return -1 so it's a little confusing to declare that a return value of -1 should propogate errors.

The fact that the function will never return -1 is exactly why it is used to signal exceptions. Here are the relevant Cython docs: 

https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#error-return-values

This pattern is also used in other places in `lib.pyx`:

https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/lib.pyx#L1096
> Do you know within this function where the segfault actually happens? I think the issue is probably with util.is_array if anything

In this particular case, a segfault happens because a `RecursionError` is thrown from within the function (because the stack happens to be exhausted there), but is ignored.
The whatsnew can be removed, then LGTM
Misunderstanding on my end - thanks for the link. 

Lgtm

Sent from my iPhone

> On Apr 23, 2020, at 10:10 AM, Jeff Reback <notifications@github.com> wrote:
> 
> ﻿
> @jreback requested changes on this pull request.
> 
> In pandas/_libs/lib.pxd:
> 
> > @@ -1 +1 @@
> -cdef bint c_is_list_like(object, bint)
> +cdef bint c_is_list_like(object, bint) except -1
> -1 is appropriate here as it signals cython that there maybe is an exception to check.
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or unsubscribe.

Thanks -- will do. Any suggestions for where to put that test?
> Thanks -- will do. Any suggestions for where to put that test?

pandas/tests/dtypes/test_inference.py
I'm now having second thoughts about the robustness of this test: it assumes that the `RecursionError` will be raised from within `c_is_list_like`, but it could as well be raised from anywhere else within `is_list_like()`. Granted, for now, `is_list_like()` just calls `c_is_list_like`, but that could conceivably change in the future.

Any thoughts here?
> Any thoughts here?

what if got rid of c_is_list_like and just made is_list_like `cpdef`?
That would solve this problem. But it looks like `c_is_list_like` is used elsewhere within the Cython (e.g., https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/reshape.pyx#L124).

Not sure whether the overhead that will be introduced by calling a `cpdef` method instead is something to worry about.
I think that having a fast `cdef` method is valuable here.
@shwina can you post the test that you have for now? Will be easier for review and filing comments
lgtm can u merge master and resolve conflicts and ping in green
Thanks @jreback - done.
thanks @shwina