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

itemgetter/attrgetter/methodcaller objects ignore keyword arguments #71009

Closed
serhiy-storchaka opened this issue Apr 21, 2016 · 15 comments
Closed
Assignees
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 26822
Nosy @rhettinger, @vstinner, @vadmium, @serhiy-storchaka, @MojoVampire, @zhangyangyu
Files
  • operator_getters_kwargs.patch
  • operator_getters_kwargs_speedup.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/serhiy-storchaka'
    closed_at = <Date 2016-04-29.06:22:05.166>
    created_at = <Date 2016-04-21.19:23:41.636>
    labels = ['extension-modules', 'type-bug']
    title = 'itemgetter/attrgetter/methodcaller objects ignore keyword arguments'
    updated_at = <Date 2017-02-06.08:11:14.086>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2017-02-06.08:11:14.086>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2016-04-29.06:22:05.166>
    closer = 'serhiy.storchaka'
    components = ['Extension Modules']
    creation = <Date 2016-04-21.19:23:41.636>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['42560', '42583']
    hgrepos = []
    issue_num = 26822
    keywords = ['patch']
    message_count = 15.0
    messages = ['263933', '263936', '263947', '263962', '263974', '264054', '264125', '264133', '264233', '264465', '264466', '264491', '264531', '264574', '287099']
    nosy_count = 7.0
    nosy_names = ['rhettinger', 'vstinner', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'josh.r', 'xiang.zhang']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26822'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @serhiy-storchaka
    Copy link
    Member Author

    itemgetter(), attrgetter() and methodcaller() objects require one argument. They raise TypeError if less or more than one positional argument is provided. But they totally ignore any keyword arguments.

    >>> import operator
    >>> f = operator.itemgetter(1)
    >>> f('abc', spam=3)
    'b'
    >>> f = operator.attrgetter('index')
    >>> f('abc', spam=3)
    <built-in method index of str object at 0xb7172b20>
    >>> f = operator.methodcaller('upper')
    >>> f('abc', spam=3)
    'ABC'

    Proposed patch makes these objects raise TypeError if keyword arguments are provided.

    @serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Apr 21, 2016
    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Apr 21, 2016

    Seems sensible for itemgetter and attrgetter, where all but the first argument is nonsensical anyway.

    It really seems like methodcaller should allow additional arguments (positional and keyword though), a la functools.partial (the difference being the support for duck-typed methods, where functools.partial only supports unbound methods for a specific type). I suppose bpo-25454 disagrees, but it seems very strange how methodcaller is like functools.partial, but without call time argument binding, only definition time.

    @zhangyangyu
    Copy link
    Member

    This makes sense. It makes the C version and Python version consistent.

    @serhiy-storchaka
    Copy link
    Member Author

    It really seems like methodcaller should allow additional arguments (positional and keyword though)

    This is good argument for raising an exception. Wrong expectations can lead to incorrect code.

    @vadmium
    Copy link
    Member

    vadmium commented Apr 22, 2016

    This seems like a good change, and the patch looks correct.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 23, 2016

    New changeset 16461a0016bf by Serhiy Storchaka in branch '3.5':
    Issue bpo-26822: itemgetter, attrgetter and methodcaller objects no longer
    https://hg.python.org/cpython/rev/16461a0016bf

    New changeset 9b565815079a by Serhiy Storchaka in branch '2.7':
    Issue bpo-26822: itemgetter, attrgetter and methodcaller objects no longer
    https://hg.python.org/cpython/rev/9b565815079a

    New changeset 5faccb403ad8 by Serhiy Storchaka in branch 'default':
    Issue bpo-26822: itemgetter, attrgetter and methodcaller objects no longer
    https://hg.python.org/cpython/rev/5faccb403ad8

    @rhettinger
    Copy link
    Contributor

    In general there should be an early out test for keywords==NULL before calling the comparatively high overhead function _PyArg_NoKeywords.

    That function adds overhead everywhere it is used and provides zero benefit to correct programs in order to provide earlier detection of a very rare class of errors.

    Some care needs to be taken before slapping _PyArg_NoKeywords onto every function in the core that doesn't use keyword arguments.

    @rhettinger rhettinger reopened this Apr 24, 2016
    @serhiy-storchaka
    Copy link
    Member Author

    Here is a patch that adds fast checks before calling _PyArg_NoKeywords().

    Other option is redefine _PyArg_NoKeywords as a macro:

    #define _PyArg_NoKeywords(name, kw)  ((kw) != NULL && _PyArg_NoKeywords((name), (kw)))

    This will affect all usages of _PyArg_NoKeywords.

    @rhettinger
    Copy link
    Contributor

    The patch looks good and is ready to apply. The macro also is a fine idea.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 29, 2016

    New changeset 54663cbd0de1 by Serhiy Storchaka in branch '3.5':
    Issue bpo-26822: Decreased an overhead of using _PyArg_NoKeywords() in calls of
    https://hg.python.org/cpython/rev/54663cbd0de1

    New changeset 22caee20223e by Serhiy Storchaka in branch 'default':
    Issue bpo-26822: Decreased an overhead of using _PyArg_NoKeywords() in calls of
    https://hg.python.org/cpython/rev/22caee20223e

    New changeset d738f268a013 by Serhiy Storchaka in branch '2.7':
    Issue bpo-26822: Decreased an overhead of using _PyArg_NoKeywords() in calls of
    https://hg.python.org/cpython/rev/d738f268a013

    @serhiy-storchaka
    Copy link
    Member Author

    Upon closer inspection, redefine _PyArg_NoKeywords as a macro turned out to be not such a good idea. Other use of _PyArg_NoKeywords are in __new__ and __init__ methods. Create these objects in most cases (except may be slice) is performance critical operation than calling itemgetter, attrgetter or methodcaller object. It is better to add this microoptimization on cases (set and frozenset already have it).

    @vstinner
    Copy link
    Member

    • if (!_PyArg_NoKeywords("itemgetter", kw))
      + if (kw != NULL && !_PyArg_NoKeywords("itemgetter", kw))

    Can't we use a macro to implement this micro-optimization, instead of modifying each call to _PyArg_NoKeywords?

    @serhiy-storchaka
    Copy link
    Member Author

    Can't we use a macro to implement this micro-optimization, instead of modifying each call to _PyArg_NoKeywords?

    I proposed this idea above. But then I have found that 1) most usages of _PyArg_NoKeywords are not in performance critical code and 2) my attempt caused a crash. Thus I have committed simpler patch.

    @serhiy-storchaka
    Copy link
    Member Author

    1. my attempt caused a crash.

    I found an error in my attempt. Here is a patch that correctly define macros for _PyArg_NoKeywords and _PyArg_NoPositional micro-optimization.

    I don't know wherever it is worth to push it.

    @serhiy-storchaka
    Copy link
    Member Author

    Seems I missed to attach a patch. Opened bpo-29460 for this tweak.

    @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
    extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants