Skip to content

Itertools Recipes - iter_index() silently suppresses ValueError #107208

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

Closed
vbrozik opened this issue Jul 24, 2023 · 4 comments
Closed

Itertools Recipes - iter_index() silently suppresses ValueError #107208

vbrozik opened this issue Jul 24, 2023 · 4 comments
Assignees
Labels
docs Documentation in the Doc dir

Comments

@vbrozik
Copy link

vbrozik commented Jul 24, 2023

Documentation

In Itertools Recipes there is a bug in the iter_index() function. The function silently suppresses ValueError exception raised by a generator given to the argument iterable.

The bug was introduced by this pull request: gh-102088 Optimize iter_index itertools recipe #102360
Commit: 148bde6

Code to reproduce the bug:

def assert_no_value(iterable, forbidden_value):
    """Pass the iterable but raise ValueError if forbidden_value is found."""
    for item in iterable:
        if item == forbidden_value:
            raise ValueError(f'Value {forbidden_value!r} is not allowed.')
        yield item

# Here we should get ValueError exception
# but it is being silently suppressed by iter_index()
list(iter_index(assert_no_value('AABCADEAF', 'B'), 'A'))

Complete notebook reproducing the bug:
https://github.com/vbrozik/python-ntb/blob/main/problems_from_forums/2023-07-24_iter_index.ipynb

Possible solutions which come to my mind:

  • revert the commit 148bde6
  • check the value of ValueError inside the iter_index() function
  • open discussion to determine whether operator.indexOf() should use ValueError

Note: I already mentioned the bug in the package more-itertools which inherited it:
Update recipes.iter_index to match CPython PR 102360 #690

Linked PRs

@AlexWaygood
Copy link
Member

Cc. @pochmann

@pochmann
Copy link
Contributor

The "Fast path for sequences" btw has the same issue(?), as can be seen with this sequence version of your example:

from collections.abc import Sequence

class AssertNoValue(Sequence):
    def __init__(self, iterable, forbidden_value):
        self.iterable = iterable
        self.forbidden_value = forbidden_value

    def __getitem__(self, index):
        item = self.iterable[index]
        if item == self.forbidden_value:
            raise ValueError(f'Value {self.forbidden_value!r} is not allowed.')
        return item

    def __len__(self):
        return len(self.iterable)

# Here we should get ValueError exception
# but it is being silently suppressed by iter_index()
list(iter_index(AssertNoValue('AABCADEAF', 'B'), 'A'))

Attempt This Online!

Granted, that's perhaps more artificial/unrealistic than an iterator raising a ValueError.

@rhettinger
Copy link
Contributor

rhettinger commented Jul 25, 2023

Interestingly, indexOf('AABCADEAF', 'X') and indexOf(assert_no_value('AABCADEAF', 'B'), 'X') both raise a ValueError. It seems that the two cases can only be distinguished by examining the exception message.

A more robust way to distinguish between the two cases would be to provide a more refined exception class as was done for FileNotFoundError vs OSError and ModuleNotFoundError vs ImportError:

class ValueNotFoundError(ValueError):
    "Value was not found"

@rhettinger
Copy link
Contributor

The proposal to add ValueNotFoundError did not go well. This is unfortunate because putting in a new exception is a big step that requires a consensus.

Unless someone with more social capital steps forward to champion ValueNotFoundError, it looks like operator.indexOf will remain broken in perpetuity and we will need to roll back @pochmann 's optimization.

rhettinger added a commit to rhettinger/cpython that referenced this issue Sep 3, 2023
rhettinger added a commit that referenced this issue Sep 3, 2023
gh-107208: iter_index now supports "stop" and no longer swallows ValueError
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 3, 2023
…ythongh-108835)

(cherry picked from commit f373c6b)

Co-authored-by: Raymond Hettinger <[email protected]>
pythongh-107208: iter_index now supports "stop" and no longer swallows ValueError
Yhg1s pushed a commit that referenced this issue Sep 4, 2023
…h-108835) (#108837)

gh-107208: Fix iter_index() recipe to not swallow exceptions (gh-108835)
(cherry picked from commit f373c6b)


gh-107208: iter_index now supports "stop" and no longer swallows ValueError

Co-authored-by: Raymond Hettinger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir
Projects
None yet
Development

No branches or pull requests

4 participants