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

Modernize integer test/conversion in randrange() #86388

Closed
rhettinger opened this issue Oct 31, 2020 · 16 comments
Closed

Modernize integer test/conversion in randrange() #86388

rhettinger opened this issue Oct 31, 2020 · 16 comments
Assignees
Labels
3.11 only security fixes stdlib Python modules in the Lib dir

Comments

@rhettinger
Copy link
Contributor

BPO 42222
Nosy @tim-one, @rhettinger, @terryjreedy, @encukou, @ambv, @serhiy-storchaka, @vedgar
PRs
  • bpo-42222: Modernize integer test/conversion in randrange() #23064
  • bpo-42222: Remove deprecated support for non-integer values #28983
  • bpo-42222: Improve tests for invalid argument types in randrange() #29021
  • 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 2020-12-28.19:11:34.467>
    created_at = <Date 2020-10-31.17:27:12.134>
    labels = ['library', '3.11']
    title = 'Modernize integer test/conversion in randrange()'
    updated_at = <Date 2022-02-03.10:41:29.426>
    user = 'https://github.com/rhettinger'

    bugs.python.org fields:

    activity = <Date 2022-02-03.10:41:29.426>
    actor = 'petr.viktorin'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2020-12-28.19:11:34.467>
    closer = 'rhettinger'
    components = ['Library (Lib)']
    creation = <Date 2020-10-31.17:27:12.134>
    creator = 'rhettinger'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42222
    keywords = ['patch']
    message_count = 16.0
    messages = ['380082', '380083', '380084', '380085', '380086', '380089', '380096', '380099', '380111', '380498', '380499', '383776', '383916', '404088', '404327', '412435']
    nosy_count = 7.0
    nosy_names = ['tim.peters', 'rhettinger', 'terry.reedy', 'petr.viktorin', 'lukasz.langa', 'serhiy.storchaka', 'veky']
    pr_nums = ['23064', '28983', '29021']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue42222'
    versions = ['Python 3.11']

    @rhettinger
    Copy link
    Contributor Author

    Move the int(x)==x test and conversion into the C code for operator.index().

    @rhettinger rhettinger added 3.10 only security fixes stdlib Python modules in the Lib dir labels Oct 31, 2020
    @rhettinger
    Copy link
    Contributor Author

    And, if we were willing to correct the exception type from ValueError to TypeError, the code could be made simpler, faster, and more in line with user expectations.

    @rhettinger
    Copy link
    Contributor Author

    I had forgotten. It looks like float arguments were allowed:

        >>> randrange(10.0, 20.0, 2.0)
        16

    Is this worth going through a deprecation cycle to get the code cleaned-up or should we live with it as is?

    @serhiy-storchaka
    Copy link
    Member

    It changes the behavior. Currently randrange(10.0) works, but with PR 23064 it would fail.

    See bpo-40046 with a ready PR for increasing coverage for the random module. If it would accepted, some tests would fail with PR 23064.

    If you want to deprecate accepting float arguments, there was bpo-40046 with a ready PR.

    These propositions were rejected by you. Have you reconsidered your decision?

    @rhettinger
    Copy link
    Contributor Author

    These propositions were rejected by you.
    Have you reconsidered your decision?

    I was reluctant to break any existing code.
    Now, I'm unsure and am inclined to harmonize it with range().

    What do you think?
    Should we have ever supported float arguments
    for an integer domain function?

    @serhiy-storchaka
    Copy link
    Member

    I think and always thought that integer domain functions should not accept non-integer arguments even with integer value. This is why I submitted numerous patches for deprecating and finally removing support of non-integer arguments in most of integer domain functions. C implemented functions which use PyArg_Parse("i") or PyLong_AsLong() for parsing arguments use now index() instead of int(). They emit a deprecation warning for non-integers in 3.8 and 3.9 and raise type error since 3.10. math.factorial() emits a warning only in 3.9.

    bpo-37319 (sorry, I wrote incorrect issue number in msg380085) was initially opened for 3.9, so we could convert warnings into errors in 3.10 or 3.11.

    Currently randrange(1e25) can return value larger than 10**25, because int(1e25) == 10000000000000000905969664 > 10**25.

    @rhettinger
    Copy link
    Contributor Author

    User feedback concur with making the change: https://twitter.com/raymondh/status/1322607969754775552

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Oct 31, 2020

    Yes, the ability to write randrange(1e9) is sometimes nice. And the fact that it might give the number outside the intended range with probability 1e-17 is not really an important argument (people have bad intuitions about very small probabilities). But if we intend to be consistent with range, then of course this must go.

    @rhettinger
    Copy link
    Contributor Author

    10**9 isn't much harder than 10E9 ;-)

    @terryjreedy
    Copy link
    Member

    To me, ValueError("non-integer arg 1 for randrange()") (ValueError('bad type') is a bit painful to read. We do sometime fix such bugs, when not documented, in future releases.

    Current the doc, "Return a randomly selected element from range(start, stop, step). This is equivalent to choice(range(start, stop, step))", implies that both accept the same values, which most would expect anyway from the names. Being selectively 'generous' in what is accepted is confusing.

    For the future: both range and math.factorial raise
    TypeError: 'float' object cannot be interpreted as an integer
    The consistency is nice. randrange should say the same after deprecation.

    @terryjreedy
    Copy link
    Member

    To put what I said another way: both items are mental paper cuts and I see benefit to both coredevs and users in getting rid of them. That is not to say 'no cost', but that there is a real benefit to be balanced against the real cost.

    @rhettinger
    Copy link
    Contributor Author

    There is another randrange() oddity. If stop is None, the step argument is ignored:

        >>> randrange(100, stop=None, step=10)
        4

    If we want to fully harmonize with range(), then randrange() should only accept positional arguments and should not allow None for the stop argument. That would leave the unoptimized implementation equivalent to:

        def randrange(self, /, *args):
            return self.choice(range(*args))

    The actual implementation can retain its fast paths and have a nicer looking signature perhaps using __text_signature__.

    @rhettinger
    Copy link
    Contributor Author

    New changeset a9621bb by Raymond Hettinger in branch 'master':
    bpo-42222: Modernize integer test/conversion in randrange() (bpo-23064)
    a9621bb

    @rhettinger
    Copy link
    Contributor Author

    New changeset 5afa0a4 by Raymond Hettinger in branch 'main':
    bpo-42222: Remove deprecated support for non-integer values (GH-28983)
    5afa0a4

    @ambv
    Copy link
    Contributor

    ambv commented Oct 19, 2021

    New changeset 5742416 by Serhiy Storchaka in branch 'main':
    bpo-42222: Improve tests for invalid argument types in randrange() (GH-29021)
    5742416

    @ambv ambv added 3.11 only security fixes and removed 3.10 only security fixes labels Oct 19, 2021
    @encukou
    Copy link
    Member

    encukou commented Feb 3, 2022

    Since this is a user-visible change in 3.11, could you add a What's New entry?

    @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.11 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants