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

typeobject provides incorrect __mul__ #37666

Closed
nascheme opened this issue Dec 30, 2002 · 12 comments
Closed

typeobject provides incorrect __mul__ #37666

nascheme opened this issue Dec 30, 2002 · 12 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@nascheme
Copy link
Member

BPO 660144
Nosy @gvanrossum, @jackjansen, @nascheme
Files
  • getargs_int.diff: Disallow float for 'i' and 'l' types
  • getargs_int_warn.diff
  • 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/nascheme'
    closed_at = <Date 2003-02-04.21:02:41.000>
    created_at = <Date 2002-12-30.20:29:19.000>
    labels = ['interpreter-core']
    title = 'typeobject provides incorrect __mul__'
    updated_at = <Date 2003-02-04.21:02:41.000>
    user = 'https://github.com/nascheme'

    bugs.python.org fields:

    activity = <Date 2003-02-04.21:02:41.000>
    actor = 'nascheme'
    assignee = 'nascheme'
    closed = True
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2002-12-30.20:29:19.000>
    creator = 'nascheme'
    dependencies = []
    files = ['720', '721']
    hgrepos = []
    issue_num = 660144
    keywords = []
    message_count = 12.0
    messages = ['13728', '13729', '13730', '13731', '13732', '13733', '13734', '13735', '13736', '13737', '13738', '13739']
    nosy_count = 3.0
    nosy_names = ['gvanrossum', 'jackjansen', 'nascheme']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue660144'
    versions = []

    @nascheme
    Copy link
    Member Author

    type __mul__ is wierd:

    >> 'a'.__mul__(3.4)
    'aaa'
    >>> [1].__mul__(3.4)
    [1, 1, 1]

    Floating point numbers with fractions should not be
    accepted.
    I think the problem is that __mul__ should not be trying to
    implement sq_repeat behavior (although I haven't dug
    deeply into this problem yet). Also, I think the code is
    vulnerable to integer overflow.

    Should also check __imul__ __add__ __iadd__.

    @nascheme nascheme self-assigned this Dec 30, 2002
    @nascheme nascheme added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Dec 30, 2002
    @nascheme nascheme self-assigned this Dec 30, 2002
    @nascheme nascheme added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Dec 30, 2002
    @nascheme
    Copy link
    Member Author

    Logged In: YES
    user_id=35752

    I think the problem is that wrap_intargfunc and
    wrap_intintargfunc use PyArg_ParseTuple(args, "i", &i).
    This bug also is present in methods like __getitem__:

    >>> "Python"[1.2]
    Traceback (most recent call last):
      File "<stdin>", line 1, in ?
    TypeError: sequence index must be integer
    >>> "Python".__getitem__(1.2)
    'y'

    I think the right fix is to use explictly only allow only
    ints and longs to wrap_intargfunc and friends. If Guido
    agrees I will cook up a patch. It seems like we should have
    a code for PyParse_Tuple that only allows ints and longs.

    @gvanrossum
    Copy link
    Member

    Logged In: YES
    user_id=6380

    This is an age-old problem that crops up whenever "i" and
    similar format codes are used.

    The problem is that on the one hand you want "i" to accept
    other int-ish types that have an __int__ method. But on the
    other hand you don't want it to accept float, which also has
    an __int__ method. Or other float-ish types.

    I think the "i" format code has to be fixe, but I'm not sure
    how -- maybe as a start it would be sufficient to test
    explicitly for float and its subclasses and reject those.
    That would still allow 3rd party float-ish classes that
    implement __int__, but that's not so important.

    @nascheme
    Copy link
    Member Author

    Logged In: YES
    user_id=35752

    Would this be a change for the 2.3 release? I tried adding
    a PyFloat_Check
    test to 'i' and 'l' in getargs.c. It looks like all the
    unit tests pass. I agree that
    checking for float catches the important cases.

    @gvanrossum
    Copy link
    Member

    Logged In: YES
    user_id=6380

    Yes, this should go into 2.3a2.

    Your attachment upload failed somehow.

    @nascheme
    Copy link
    Member Author

    Logged In: YES
    user_id=35752

    The small patch is attached. I only added the checking for
    'i' and 'l'. I wonder if it should be done for the other
    integer codes as well (e.g. 'b', 'h').

    @gvanrossum
    Copy link
    Member

    Logged In: YES
    user_id=6380

    Looks like a good patch. And yes, the other types should be
    doing this as well.

    @nascheme
    Copy link
    Member Author

    Logged In: YES
    user_id=35752

    I added the check for the other integer codes. All the
    tests pass
    so I checked it in. Hope that's okay.

    @jackjansen
    Copy link
    Member

    Logged In: YES
    user_id=45365

    Neil,
    your mod to PyArg_Parse is far too aggressive. Apparently the integer formats have accepted floating point arguments since day one, and all weekend I'm running across places where floats are passed to integers. For example, the MacPython IDE does this all the time, and I wouldn't be surprised if there's lots of windowing code that does this (compute pixel positions in floating point and then passing them to something expecting an integer).

    If this is fixed in this way (disallowing floats where integers are expected) it should at least be done with a DeprecationWarning for one release cycle. I think the fix probably is a good idea in the long run, but it will break large bodies of code...

    @nascheme
    Copy link
    Member Author

    Logged In: YES
    user_id=35752

    Attached is a patch that signals a DepreciationWarning
    instead of rasing a TypeError. I used a static function to
    avoid duplication of code. My version of GCC inlines it.

    @gvanrossum
    Copy link
    Member

    Logged In: YES
    user_id=6380

    Loosk good -- please check it in. Probably requires
    something in NEWS too.

    @nascheme
    Copy link
    Member Author

    nascheme commented Feb 4, 2003

    Logged In: YES
    user_id=35752

    Checked in as getargs.c 2.96 with NEWS.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 9, 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)
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants