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

randrange() mishandles step when stop is None #86938

Closed
rhettinger opened this issue Dec 28, 2020 · 8 comments
Closed

randrange() mishandles step when stop is None #86938

rhettinger opened this issue Dec 28, 2020 · 8 comments
Assignees
Labels
3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@rhettinger
Copy link
Contributor

BPO 42772
Nosy @tim-one, @rhettinger, @serhiy-storchaka
PRs
  • bpo-42772: Step argument ignored when stop is None. #24018
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/rhettinger'
    closed_at = <Date 2021-01-02.18:25:13.759>
    created_at = <Date 2020-12-28.19:23:37.912>
    labels = ['type-bug', 'library', '3.9', '3.10']
    title = 'randrange() mishandles step when stop is None'
    updated_at = <Date 2021-01-02.18:25:13.759>
    user = 'https://github.com/rhettinger'

    bugs.python.org fields:

    activity = <Date 2021-01-02.18:25:13.759>
    actor = 'rhettinger'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2021-01-02.18:25:13.759>
    closer = 'rhettinger'
    components = ['Library (Lib)']
    creation = <Date 2020-12-28.19:23:37.912>
    creator = 'rhettinger'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42772
    keywords = ['patch']
    message_count = 8.0
    messages = ['383919', '383922', '383928', '383930', '383931', '383938', '384097', '384235']
    nosy_count = 3.0
    nosy_names = ['tim.peters', 'rhettinger', 'serhiy.storchaka']
    pr_nums = ['24018']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue42772'
    versions = ['Python 3.9', 'Python 3.10']

    @rhettinger
    Copy link
    Contributor Author

    When stop is None, the step argument is ignored:

    >>> randrange(1000, None, 100)
    651
    >>> randrange(1000, step=100)
    673

    @rhettinger rhettinger added 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Dec 28, 2020
    @rhettinger rhettinger self-assigned this Dec 28, 2020
    @rhettinger rhettinger added the 3.9 only security fixes label Dec 28, 2020
    @rhettinger rhettinger removed their assignment Dec 28, 2020
    @rhettinger
    Copy link
    Contributor Author

    One fix is to move up the code for converting step to istep and then modify the early out test to:

            if stop is None and istep == 1:
                if istart > 0:
                    return self._randbelow(istart)

    That would have the downside of slowing down the common case.

    Another possibility is changing the default step to a sentinel object and just testing for that:

            if stop is None and step is sentinel:
                if istart > 0:
                    return self._randbelow(istart)

    That would be user visible in the function signature but it would run fast.

    We could bite the bullet and fully harmonize the randrange() signature with range(). That would entail switching to *args and no longer accepting keyword arguments:

         def randrange(self, /, *args):
             "Choose random item from range(stop) or range(start, stop[, step])."
             return self.choice(range(*args))

    This would most closely match user expectations but is potentially a breaking change for existing code that uses keyword arguments. Such code probably exists but is probably not common.

    For speed, the actual implementation could still have fast paths for common cases.

    For help() and tooltips, we could add a __text_signature__ to cover-up the *args. However, signature objects currently aren't capable of describing range(). The best we could do is:

    randrange.__text_signature__ = '($self, start, stop, step, /)'
    

    That would give help() that looks like this:

        >>> help(randrange)
        Help on method randrange in module Random:
    randrange(start, stop, step, /) method of __main__.R instance
        Choose random item from range(stop) or range(start, stop[, step]).
    

    @rhettinger
    Copy link
    Contributor Author

    Note, we can't actually use "self.choice(range(*args))" because the underlying len() call can Overflow. If it does, the components need to be computed manually.

    @rhettinger
    Copy link
    Contributor Author

    Another solution is to use an identity test for the step argument

        _one = 1
    
        class Random:
            def randrange(start, stop=None, step=_one):
                ...
                if stop is None and step is _one:
                    if istart > 0:
                        return self._randbelow(istart)

    This has the advantage of keeping the API unchanged while still keeping the fast path fast.

    @serhiy-storchaka
    Copy link
    Member

    Another solution is to use an identity test for the step argument

    Should I restore that optimization in bpo-37319?

    @rhettinger
    Copy link
    Contributor Author

    Should I restore that optimization in bpo-37319?

    Yes, but to fix the bug it needs to occur earlier in the code (currently line 314):

        if stop is None and step is _ONE: <-- Line 314
                       ^^^^^^^^^^^^^^^^^  <-- Add this   

    And again later:
    if istep == 1 and width > 0: <-- Line 348
    ^-- change to _ONE

    As a standalone optimization, it wasn't worth it, but as a way to fix the bug without a regression, it is reasonable.

    @rhettinger
    Copy link
    Contributor Author

    Upon further reflection, I think these cases should raise a TypeError because the meaning is ambiguous:

        randrange(1000, step=10) # Could only mean start=0 stop=1000 step=10

    but that conflicts with:

        randrange(1000, None, 100)  # Has no reasonable interpretation

    For comparison, this currently raises a TypeError because None isn't a sensible argument for stop:

        range(1000, None, 100)

    @rhettinger rhettinger self-assigned this Dec 31, 2020
    @rhettinger
    Copy link
    Contributor Author

    New changeset 768fa14 by Raymond Hettinger in branch 'master':
    bpo-42772: Step argument ignored when stop is None. (GH-24018)
    768fa14

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants