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.

Sign up for GitHub

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

Jump to bottom

Fix uncaught ValueError on inference of __getitem__ with slice(..., ..., 0) #1844

Merged
merged 1 commit into from
Nov 13, 2022
Merged

Fix uncaught ValueError on inference of __getitem__ with slice(..., ..., 0) #1844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ Release date: TBA

Refs PyCQA/pylint#7706

* Catch ``ValueError`` when indexing some builtin containers and sequences during inference.

Closes #1843

What's New in astroid 2.12.12?
==============================
Expand Down
2 changes: 2 additions & 0 deletions astroid/inference.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
AstroidError,
AstroidIndexError,
AstroidTypeError,
AstroidValueError,
AttributeInferenceError,
InferenceError,
NameInferenceError,
Expand Down Expand Up @@ -441,6 +442,7 @@ def infer_subscript(
except (
AstroidTypeError,
AstroidIndexError,
AstroidValueError,
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
AttributeInferenceError,
AttributeError,
) as exc:
Expand Down
12 changes: 12 additions & 0 deletions astroid/nodes/node_classes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from astroid.exceptions import (
AstroidIndexError,
AstroidTypeError,
AstroidValueError,
InferenceError,
NoDefault,
ParentMissingError,
Expand Down Expand Up @@ -234,6 +235,13 @@ def _container_getitem(instance, elts, index, context=None):
return new_cls
if isinstance(index, Const):
return elts[index.value]
except ValueError as exc:
raise AstroidValueError(
message="Slice {index!r} cannot index container",
node=instance,
index=index,
context=context,
) from exc
Comment on lines +238 to +244
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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:

  1. Does the LHS have a custom __getitem__ method? If so, we're probably in the uninferable realm
  2. Try to defer the computed indexes to slice.indices since this is the canonical source of the raised Type and ValueErrors:
>>> [][: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.

Copy link
Member

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 error

I think the appropriate fix is probably:

  1. Does the LHS have a custom __getitem__ method? If so, we're probably in the uninferable realm
  2. Try to defer the computed indexes to slice.indices since this is the canonical source of the raised Type and ValueErrors:

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 in Const.getitem similarly what was added here in _container_getitem.

Sounds good.

except IndexError as exc:
raise AstroidIndexError(
message="Index {index!s} out of range",
Expand Down Expand Up @@ -2030,6 +2038,10 @@ def getitem(self, index, context=None):
try:
if isinstance(self.value, (str, bytes)):
return Const(self.value[index_value])
except ValueError as exc:
raise AstroidValueError(
f"Could not index {self.value!r} with {index_value!r}"
) from exc
except IndexError as exc:
raise AstroidIndexError(
message="Index {index!r} out of range",
Expand Down
10 changes: 10 additions & 0 deletions tests/unittest_inference.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5316,6 +5316,16 @@ def test_slice_inference_in_for_loops_not_working() -> None:
assert inferred == util.Uninferable


def test_slice_zero_step_does_not_raise_ValueError() -> None:
node = extract_node("x = [][::0]; x")
assert next(node.infer()) == util.Uninferable
cdce8p marked this conversation as resolved.
Show resolved Hide resolved


def test_slice_zero_step_on_str_does_not_raise_ValueError() -> None:
node = extract_node('x = ""[::0]; x')
assert next(node.infer()) == util.Uninferable


def test_unpacking_starred_and_dicts_in_assignment() -> None:
node = extract_node(
"""
Expand Down