-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
Fix uncaught ValueError on inference of __getitem__ with slice(..., ..., 0) #1844
Conversation
Pull Request Test Coverage Report for Build 3453801068
💛 - Coveralls |
Sorry for the cache miss, we have an issue right now, maybe Marc fixed it on main but I'm not certain of it. |
85b34d6
to
92ba74e
Compare
except ValueError as exc: | ||
raise AstroidValueError( | ||
message="Slice {index!r} cannot index container", | ||
node=instance, | ||
index=index, | ||
context=context, | ||
) from exc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make more sense to catch it even earlier in _infer_slice
. 0
is always an invalid value for step
. So we shouldn't infer a slice
object if we encounter it.
https://github.com/PyCQA/astroid/blob/4acf5785c54f4ccb8f41402991f6ddc5d8b28b89/astroid/nodes/node_classes.py#L211-L223
This would also have the benefit of fixing the other call to _infer_slice
as well.
https://github.com/PyCQA/astroid/blob/4acf5785c54f4ccb8f41402991f6ddc5d8b28b89/astroid/nodes/node_classes.py#L2022-L2023
To reproduce, run pylint for
for _ in ""[::0]:
pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is a cleaner solution. I'll get to it today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I've got a pretty generous definition of "today" these days. I think I originally considered this but stopped because it's not actually an error to create a slice
object with a step of 0, just an error to use it to index the built-in sequences:
>>> slice(None, 10, 0)
slice(None, 10, 0)
>>> [][slice(None, 10, 0)]
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: slice step cannot be zero
Authors can choose to handle extended indexes however they'd like, though the only example that comes to mind is numpy.nd_grid
: https://numpy.org/doc/stable/reference/generated/numpy.mgrid.html
>>> np.mgrid[0:3, 0:3]
array([[[0, 0, 0],
[1, 1, 1],
[2, 2, 2]],
[[0, 1, 2],
[0, 1, 2],
[0, 1, 2]]])
or range
supports sub-ranges through slices:
>>> range(20)
range(0, 20)
>>> range(20)[::3]
range(0, 20, 3)
>>> range(20)[:5]
range(0, 5)
It turns out that slice indices don't even have to be integers as I've just learned, https://docs.python.org/3/reference/datamodel.html#index-67:
... start is the lower bound; stop is the upper bound; step is the step value; each is None if omitted. These attributes can have any type.
Pylint has tests for these cases (such as the string indices), but at least one of them is incorrect and will need to be changed: https://github.com/PyCQA/pylint/blob/main/tests/functional/i/invalid/invalid_slice_index.py#L25-L28
# Valid indices
def function4():
"""integers used as indices"""
return TESTLIST[0:0:0] # no error
I think the appropriate fix is probably:
- Does the LHS have a custom
__getitem__
method? If so, we're probably in the uninferable realm - Try to defer the computed indexes to
slice.indices
since this is the canonical source of the raisedType
andValueError
s:
>>> [][:10:0]
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: slice step cannot be zero
>>> slice(None, 10, 0).indices(0)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: slice step cannot be zero
>>> (1, 2, 3)["a":"z"]
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: slice indices must be integers or None or have an __index__ method
>>> slice('a', 'z').indices(26)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: slice indices must be integers or None or have an __index__ method
I'm leaning towards adding the except ValueError
case in Const.getitem
similarly what was added here in _container_getitem
. We can refactor the duplication out (now or later) since the distinction isn't really between "const" and "container", it's between "indexables" and "sequences" https://docs.python.org/3/reference/datamodel.html#index-15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I originally considered this but stopped because it's not actually an error to create a
slice
object with a step of 0, just an error to use it to index the built-in sequences:
Good point!
Pylint has tests for these cases (such as the string indices), but at least one of them is incorrect and will need to be changed: https://github.com/PyCQA/pylint/blob/main/tests/functional/i/invalid/invalid_slice_index.py#L25-L28
# Valid indices def function4(): """integers used as indices""" return TESTLIST[0:0:0] # no errorI think the appropriate fix is probably:
- Does the LHS have a custom
__getitem__
method? If so, we're probably in the uninferable realm- Try to defer the computed indexes to
slice.indices
since this is the canonical source of the raisedType
andValueError
s:
Pylint does most of that already. It only emits invalid-slice-index
for a set of known objects which expect an int-like type. We would just need to special-case the 0
step error. https://github.com/PyCQA/pylint/blob/v2.15.5/pylint/checkers/typecheck.py#L1769-L1778
I'm leaning towards adding the
except ValueError
case inConst.getitem
similarly what was added here in_container_getitem
.
Sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a small changelog entry?
It occurs to me that these should probably return something useful for the Pylint # fixtures/slices-pylint.py
for _ in [][::0]:
pass
for _ in []["a"]:
pass
for _ in []["a":"z"]:
pass
for _ in ""[::0]:
pass
for _ in ""["a"]:
pass
for _ in ""["a":"z"]:
pass
class Custom:
def __getitem__(self, indices):
return "wow"
for _ in Custom()[::0]:
pass $ pylint fixtures/slices-pylint.py
************* Module slices-pylint
fixtures/slices-pylint.py:4:9: E1126: Sequence index is not an int, slice, or instance with __index__ (invalid-sequence-index)
fixtures/slices-pylint.py:7:12: E1127: Slice index is not an int, None, or instance with __index__ (invalid-slice-index)
fixtures/slices-pylint.py:7:16: E1127: Slice index is not an int, None, or instance with __index__ (invalid-slice-index)
fixtures/slices-pylint.py:13:9: E1126: Sequence index is not an int, slice, or instance with __index__ (invalid-sequence-index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It occurs to me that these should probably return something useful for the Pylint
invalid-sequence-index
checks. ReturningUninferable
makes these lines ignored instead of a useful pylint warning.
The Uninferable
isn't the issue here. The invalid-sequence-index
check doesn't call node.getitem
. Fixing the false-negatives should be possible in pylint alone from what I can tell.
--
Left some last comments. After that I think we can merge this one.
except ValueError as exc: | ||
raise AstroidValueError( | ||
message="Slice {index!r} cannot index container", | ||
node=instance, | ||
index=index, | ||
context=context, | ||
) from exc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I originally considered this but stopped because it's not actually an error to create a
slice
object with a step of 0, just an error to use it to index the built-in sequences:
Good point!
Pylint has tests for these cases (such as the string indices), but at least one of them is incorrect and will need to be changed: https://github.com/PyCQA/pylint/blob/main/tests/functional/i/invalid/invalid_slice_index.py#L25-L28
# Valid indices def function4(): """integers used as indices""" return TESTLIST[0:0:0] # no errorI think the appropriate fix is probably:
- Does the LHS have a custom
__getitem__
method? If so, we're probably in the uninferable realm- Try to defer the computed indexes to
slice.indices
since this is the canonical source of the raisedType
andValueError
s:
Pylint does most of that already. It only emits invalid-slice-index
for a set of known objects which expect an int-like type. We would just need to special-case the 0
step error. https://github.com/PyCQA/pylint/blob/v2.15.5/pylint/checkers/typecheck.py#L1769-L1778
I'm leaning towards adding the
except ValueError
case inConst.getitem
similarly what was added here in_container_getitem
.
Sounds good.
1c54d57
to
5883c65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @nelfin 👍🏻
Steps
Description
See #1843 for a reproducer.
Type of Changes
Related Issue
Closes #1843