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

int() accepts float number base #60976

Closed
serhiy-storchaka opened this issue Dec 24, 2012 · 24 comments
Closed

int() accepts float number base #60976

serhiy-storchaka opened this issue Dec 24, 2012 · 24 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 16772
Nosy @birkenfeld, @terryjreedy, @gpshead, @mdickinson, @ezio-melotti, @cjerdonek, @serhiy-storchaka
Files
  • issue16772.patch
  • issue16772_v2.patch
  • 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/mdickinson'
    closed_at = <Date 2013-01-27.10:20:30.412>
    created_at = <Date 2012-12-24.20:10:16.529>
    labels = ['interpreter-core', 'type-bug']
    title = 'int() accepts float number base'
    updated_at = <Date 2013-01-27.16:50:53.992>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2013-01-27.16:50:53.992>
    actor = 'ezio.melotti'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2013-01-27.10:20:30.412>
    closer = 'mark.dickinson'
    components = ['Interpreter Core']
    creation = <Date 2012-12-24.20:10:16.529>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['28465', '28466']
    hgrepos = []
    issue_num = 16772
    keywords = ['patch']
    message_count = 24.0
    messages = ['178095', '178135', '178136', '178137', '178147', '178162', '178163', '178166', '178172', '178185', '178188', '178223', '178259', '178279', '178280', '178377', '178378', '178379', '178380', '178381', '178382', '178406', '178460', '180758']
    nosy_count = 8.0
    nosy_names = ['georg.brandl', 'terry.reedy', 'gregory.p.smith', 'mark.dickinson', 'ezio.melotti', 'chris.jerdonek', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue16772'
    versions = ['Python 3.2', 'Python 3.3', 'Python 3.4']

    @serhiy-storchaka
    Copy link
    Member Author

    I'm not sure this is a bug. In 2.7 int() and long() with float number base raises TypeError. But in 3.x int() accepts float number base and truncate it to int.

    @serhiy-storchaka serhiy-storchaka added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Dec 24, 2012
    @ezio-melotti ezio-melotti added the type-bug An unexpected behavior, bug, or error label Dec 24, 2012
    @birkenfeld
    Copy link
    Member

    Can you give examples? I'm unable to guess what exactly you are reporting from quick experiments.

    @ezio-melotti
    Copy link
    Member

    2.7$ ./python -c 'int("5", 12.5)'
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    TypeError: integer argument expected, got float
    3.2$ ./python -c 'int("5", 12.5)'
    3.2$

    @birkenfeld
    Copy link
    Member

    Ah. I was thinking of things like int('1.2', 10), not the base itself being a float.

    In this case, looks like a bug to me.

    @mdickinson
    Copy link
    Member

    I agree that this should be fixed.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 25, 2012

    New changeset e510e028c486 by Gregory P. Smith in branch 'default':
    Fixes issue bpo-16772: int() constructor second argument (base) must be an int.
    http://hg.python.org/cpython/rev/e510e028c486

    @gpshead
    Copy link
    Member

    gpshead commented Dec 25, 2012

    If someone thinks this should go into 3.2 and 3.3 they're welcome to do it; no objections from me. (The behavior was unintentional and thus a bug, but it is still a minor behavior change)

    @cjerdonek
    Copy link
    Member

    A test also needs to be added, though I'm sure one will be added as part of bpo-16761.

    @mdickinson
    Copy link
    Member

    Greg: please could you add a test?

    I think that the new check may be too strict: should we also be prepared to accept any object that implements __index__ as a base? (Similar to the way that round accepts an __index__-aware object for its second argument.)

    <grump> It would have been nice to have a chance to review this change before it was committed. </grump>

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 26, 2012

    New changeset 60f7197f991f by Gregory P. Smith in branch 'default':
    Test for bpo-16772 and redoes the previous fix to accept __index__-aware
    http://hg.python.org/cpython/rev/60f7197f991f

    @gpshead
    Copy link
    Member

    gpshead commented Dec 26, 2012

    Thanks for the pointer to round(). PyNumber_AsSsize_t was just what the doctor ordered.

    PS Grump acknowledged and accepted. Its trunk and we're nowhere near a freeze so I figured it'd be fine to iterate on it in trunk if needed.

    @serhiy-storchaka
    Copy link
    Member Author

    Why does PyNumber_AsSsize_t() used instead PyLong_AsLongAndOverflow()? The range of "long" enough for all valid values of "base" and code with PyLong_AsLongAndOverflow() produces better error messages. It looks as a regression for me.

    @gpshead
    Copy link
    Member

    gpshead commented Dec 26, 2012

    I used it because Mark suggested it could have the same behavior as round() and that is the more abstract API that round uses. Regardless, at this point I've added tests and tested for the TypeError when a float is passed as well as testing that the only valid values (2..36) all work.

    I did not test to see that something supporting __index__ works as Mark suggests because I don't know off the top of my head of anything that does and it seems like a pointless thing to care about for this API.

    If you want something different, feel free to change the code. I'm done with this issue.

    @serhiy-storchaka
    Copy link
    Member Author

    But you have "if (!PyLong_Check(obase))" check before. Only ints acceptable. The only difference with previous code is that now OverflowError raised for large bases instead of ValueError. int.__round__ doesn't produce OverflowError.

    In any case the final version of those changes should be applied to 3.2 and 3.3 too (if no one objects).

    @cjerdonek
    Copy link
    Member

    In any case the final version of those changes should be applied to 3.2 and 3.3 too (if no one objects).

    +1

    @mdickinson
    Copy link
    Member

    I'd suggest leaving 3.2 and 3.3 as they are: the bug is fairly benign, but fixing it could break existing code unnecessarily. That's something that we should try hard not to do in a bugfix release.

    As to PyNumber_AsSsize_t() used instead PyLong_AsLongAndOverflow(), I *do* think that in general interfaces for built-in functions and methods that accept an integer should be prepared to accept anything that has an __index__. If we can find a way to do that with sane exception types / messages, so much the better. (One common application of __index__-able types occurs when using NumPy, where it's easy to end up with instances of numpy.int32 or numpy.int64 instead of regular Python ints.) I agree with Serhiy that ValueError is the appropriate exception for out-of-range values.

    [A side-issue here is that the various PyLong_As* utility functions are a mess: some use __int__, some use __index__, etc. I have some thoughts about how to fix this, but that's another issue.]

    @mdickinson
    Copy link
    Member

    The only difference with previous code is that now OverflowError raised > for large bases instead of ValueError.

    Serhiy: can you clarify this remark? Where do you see the OverflowError? The current exception and message look fine to me, so maybe I'm misunderstanding what you're talking about:

    Python 3.4.0a0 (default:1b2134a78c17, Dec 28 2012, 10:06:47) 
    [GCC 4.2.1 (Apple Inc. build 5664)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> int('34', base=2**100)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ValueError: int() base must be >= 2 and <= 36
    [66206 refs, 23451 blocks]

    I actually think this issue can be closed as fixed: the current code looks fine to me, and I don't think the fix should be backported.

    @mdickinson
    Copy link
    Member

    I actually think this issue can be closed as fixed:

    Ah, whoops; I failed to understand Serhiy's comment about the still existing "if (!PyLong_Check(obase))", which does indeed mean that the code *still* doesn't work for __index__-able types.

    Here's a fix for that, with tests.

    @cjerdonek
    Copy link
    Member

    I actually think this issue can be closed as fixed: the current code looks fine to me, and I don't think the fix should be backported.

    How about backporting the tests? In addition to adding tests for the fix, Greg added more comprehensive tests for the existing behavior (i.e. test_int_base_limits()). Backporting the latter would help prevent regressions from future fixes in earlier versions.

    Also, if we don't backport shouldn't there be a version changed in the docs?

    @mdickinson
    Copy link
    Member

    Sure, I don't see any issue with backporting test_int_base_limits; that has little to do with this issue, though, so shouldn't prevent closing this one.

    I'll add a Misc/NEWS entry when I commit; not sure it's meaningful to add doc changes, since I the 3.2 and 3.3 acceptance of floats is undocumented anyway. I can and will add a versionchanged entry for the acceptance of __index__-able types, though.

    @mdickinson
    Copy link
    Member

    Patch including doc update.

    @serhiy-storchaka
    Copy link
    Member Author

    Serhiy: can you clarify this remark? Where do you see the OverflowError?
    The current exception and message look fine to me, so maybe I'm
    misunderstanding what you're talking about:

    Sorry, I have been confused (and confuse you) by the variety of PyLong_As* and
    PyNumber_* functions. Now I see PyNumber_AsSsize_t(x, NULL) doesn't raise
    OverflowError but truncates its argument.

    The patch LGTM.

    @terryjreedy
    Copy link
    Member

    To me the doc strongly implies, but does not boldly and baldly say, that base should be an int.

    "A base-n literal consists of the digits 0 to n-1, with a to z (or A to Z) having values 10 to 35. The default base is 10. The allowed values are 0 and 2-36. Base-2, -8, and -16 literals can be optionally prefixed with 0b/0B, 0o/0O, or 0x/0X, as with integer literals in code. Base 0 means to interpret exactly as a code literal, so that the actual base is 2, 8, 10, or 16, and so that int('010', 0) is not legal, while int('010') is, as well as int('010', 8)."

    I think it should be revised to say "The allowed values are ints 0 and 2-36." (or 'integers' or 'index values'). If it had been that already, the current behavior would clearly be a bug and eligible to be fixed in 3.2/3. As it is, I will not argue with whatever others do.

    (I strongly suspect there are other places in the docs where int args are similarly implied but not explicit, but int-ness *is* checked. If someone were to complain about 0.0 being rejected, I expect we would correct the doc, not the code.;-)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 27, 2013

    New changeset 79db70bd3188 by Mark Dickinson in branch 'default':
    Issue bpo-16772: in int(x, base), non-integer bases must have an __index__ method.
    http://hg.python.org/cpython/rev/79db70bd3188

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants