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
Comments
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. |
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 |
This makes sense. It makes the C version and Python version consistent. |
This is good argument for raising an exception. Wrong expectations can lead to incorrect code. |
This seems like a good change, and the patch looks correct. |
New changeset 16461a0016bf by Serhiy Storchaka in branch '3.5': New changeset 9b565815079a by Serhiy Storchaka in branch '2.7': New changeset 5faccb403ad8 by Serhiy Storchaka in branch 'default': |
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. |
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. |
The patch looks good and is ready to apply. The macro also is a fine idea. |
New changeset 54663cbd0de1 by Serhiy Storchaka in branch '3.5': New changeset 22caee20223e by Serhiy Storchaka in branch 'default': New changeset d738f268a013 by Serhiy Storchaka in branch '2.7': |
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). |
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. |
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. |
Seems I missed to attach a patch. Opened bpo-29460 for this tweak. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: