Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Odd behavior when slicing with None #2508

Closed
cjgibson opened this issue Jul 20, 2018 · 6 comments
Closed

Odd behavior when slicing with None #2508

cjgibson opened this issue Jul 20, 2018 · 6 comments

Comments

@cjgibson
Copy link
Contributor

Source code for slicing.pyx

def run_test(list example):
    midpoint = len(example) // 2
    print "example[midpoint:] ->", example[midpoint:]
    print "example[slice(midpoint, None)] ->", example[slice(midpoint, None)]
    print "example[midpoint:None] ->", example[midpoint:None]
    print
    endpoint = None
    print "example[slice(midpoint, endpoint)] ->", example[slice(midpoint, endpoint)]
    print "example[midpoint:endpoint] ->", example[midpoint:endpoint]

Steps to reproduce:

$ cythonize -i slicing.pyx &> /dev/null; python -c "from slicing import run_test; run_test(range(5))"
example[midpoint:] -> [2, 3, 4]
example[slice(midpoint, None)] -> [2, 3, 4]
example[midpoint:None] -> [2, 3, 4]

example[slice(midpoint, endpoint)] -> [2, 3, 4]
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "slicing.pyx", line 9, in slicing.run_test
    print "example[midpoint:endpoint] ->", example[midpoint:endpoint]
TypeError: 'NoneType' object cannot be interpreted as an index
@scoder
Copy link
Contributor

scoder commented Jul 21, 2018

Agreed that this should be supported. Probably easy to fix since it really only needs a Py_None check that uses PY_SSIZE_T_MAX if true (or 0 for None in the first slice index), and use the current integer conversion code otherwise. PR welcome.

@maxkrivich
Copy link

@scoder I want to take this issue. Could you possibly point me in the right direction to get started?

@scoder
Copy link
Contributor

scoder commented Aug 5, 2018

Sure!

First, add tests to tests/run/typed_slice.pyx that do not use C integer variables as slice arguments. Run them with python runtests.py -vv --debug typed_slice (or, more slicing tests with just slice instead of typed_slice). Look at the generated C code under TEST_TMP/run/ for the test that you added.

Then, the 2-element slicing case is handled in SliceIndexNode.analyse_types() in ExprNodes.py. Near the end, you find two coercions of start and stop to C integer types for the case that the result type of the slicing node is not just an arbitrary object (py_object_type) but a known object type. Each of them needs to be replaced with a CondExprNode that checks for None (-> PrimaryCmpNode) and then either uses the coerced value as before, or a constant integer value (0 or PY_SSIZE_T_MAX). Look at Parsing.py to see how nodes are created. You can dump the current syntax subtree at any time with print(self.dump()).

Ask back if things are unclear.

@cjgibson
Copy link
Contributor Author

cjgibson commented Aug 5, 2018

Hey @maxkrivich - I had started work towards remedying this issue, but haven't worked on it in several days, and am more than willing to pass it off to someone with more free time. If you'd like, please feel free to leverage the handful of failing tests I have on my forked branch at master...cjgibson:2508-slicing-fixes.

@scoder
Copy link
Contributor

scoder commented Aug 6, 2018

Ah, what a nice suite of tests. That should help a lot in getting this right.

I would still split up the doctests with the double/triple nested loops (at least by slice type) to keep things readable. And use print() in it to also show the start and stop indices. Otherwise, it's entirely unclear where to look if one of the examples starts failing.

@cjgibson
Copy link
Contributor Author

cjgibson commented Aug 7, 2018

Hahah, @scoder - that's a realization I came to a little late. Had I started with the fully nested case first, I hope I would have resisted the urge to make the tests so large. I'll make time this weekend to trim down the number of tested cases to the minimal set required to cover the usual troublemakers (empty slice, oversized bounds, etc.) and include the requested verbosity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants