classification
Title: itemgetter/attrgetter/methodcaller objects ignore keyword arguments
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: josh.r, martin.panter, python-dev, rhettinger, serhiy.storchaka, vstinner, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-04-21 19:23 by serhiy.storchaka, last changed 2017-02-06 08:11 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
operator_getters_kwargs.patch serhiy.storchaka, 2016-04-21 19:23 review
operator_getters_kwargs_speedup.patch serhiy.storchaka, 2016-04-24 21:35 review
Messages (15)
msg263933 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-21 19:23
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.
msg263936 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2016-04-21 22:15
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 #25454 disagrees, but it seems very strange how `methodcaller` is like `functools.partial`, but without call time argument binding, only definition time.
msg263947 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-04-22 02:27
This makes sense. It makes the C version and Python version consistent.
msg263962 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-22 06:09
> 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.
msg263974 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-22 07:40
This seems like a good change, and the patch looks correct.
msg264054 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-23 07:54
New changeset 16461a0016bf by Serhiy Storchaka in branch '3.5':
Issue #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 #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 #26822: itemgetter, attrgetter and methodcaller objects no longer
https://hg.python.org/cpython/rev/5faccb403ad8
msg264125 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-04-24 19:35
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.
msg264133 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-24 21:35
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.
msg264233 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-04-26 08:18
The patch looks good and is ready to apply.  The macro also is a fine idea.
msg264465 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-29 06:13
New changeset 54663cbd0de1 by Serhiy Storchaka in branch '3.5':
Issue #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 #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 #26822: Decreased an overhead of using _PyArg_NoKeywords() in calls of
https://hg.python.org/cpython/rev/d738f268a013
msg264466 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-29 06:22
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).
msg264491 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-29 12:01
-    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?
msg264531 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-29 22:27
> 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.
msg264574 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-30 18:49
> 2) 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.
msg287099 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-02-06 08:11
Seems I missed to attach a patch. Opened issue29460 for this tweak.
History
Date User Action Args
2017-02-06 08:11:14serhiy.storchakasetmessages: + msg287099
2016-04-30 18:49:16serhiy.storchakasetmessages: + msg264574
2016-04-29 22:27:31serhiy.storchakasetmessages: + msg264531
2016-04-29 12:01:29vstinnersetnosy: + vstinner
messages: + msg264491
2016-04-29 06:22:05serhiy.storchakasetstatus: open -> closed

messages: + msg264466
2016-04-29 06:13:03python-devsetmessages: + msg264465
2016-04-26 08:18:17rhettingersetmessages: + msg264233
2016-04-24 21:35:15serhiy.storchakasetfiles: + operator_getters_kwargs_speedup.patch

messages: + msg264133
2016-04-24 19:35:29rhettingersetstatus: closed -> open
nosy: + rhettinger
messages: + msg264125

2016-04-23 07:55:00serhiy.storchakasetstatus: open -> closed
assignee: serhiy.storchaka
resolution: fixed
stage: patch review -> resolved
2016-04-23 07:54:07python-devsetnosy: + python-dev
messages: + msg264054
2016-04-22 07:40:49martin.pantersetnosy: + martin.panter
messages: + msg263974
2016-04-22 06:09:13serhiy.storchakasetmessages: + msg263962
2016-04-22 02:27:43xiang.zhangsetnosy: + xiang.zhang
messages: + msg263947
2016-04-21 22:15:03josh.rsetnosy: + josh.r
messages: + msg263936
2016-04-21 19:23:41serhiy.storchakacreate