classification
Title: Argument Clinic: Support for __new__ not checking _PyArg_NoKeywords for sub-classes
Type: enhancement Stage:
Components: Build, Demos and Tools Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: larry, taleinat
Priority: normal Keywords:

Created on 2014-01-24 21:35 by taleinat, last changed 2014-01-26 14:21 by taleinat. This issue is now closed.

Files
File name Uploaded Description Edit
larry.clinic.rollup.jan.24.diff.1.txt larry, 2014-01-24 22:49 review
itertoolsmodule.c taleinat, 2014-01-25 17:42 Buggy partially converted Modules/itertoolsmodule.c
Messages (19)
msg209122 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-24 21:35
To quote issue20294 where Larry Hastings added AC support for __new__ and __init__:

"
* __init__ and __new__ always take kwargs.
  * if the function signature doesn't accept keyword arguments,
    it calls _PyArg_NoKeywords().
"

However, due to issue1486663, many classes only do a _PyArg_NoKeywords if the type is the original class, but not for sub-classses. Examples are _random.Random (in which case I found a workaround) and many itertools classes, for which there are also tests that check this.

One possibility is to simply allow these classes to also accept keyword arguments. It doesn't break backwards compatibility, and once there are proper argument names and doc-strings, why not?

Otherwise, currently I have to make generated __new__ functions accept keyword arguments, and then wrap them with a function that checks _PyArg_NoKeywords only if the the type is the original class.
msg209124 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-24 21:44
How about I add the "if type == <our own type>" checks as per #1486663?
msg209127 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-24 22:05
In general, that sounds like a reasonable suggestion to me. A quick search shows that most classes that call _PyArg_NoKeywords check the type first. I think we should strive for uniformity in this respect, so I'm +1 for your suggestion.

I do see some classes which accept only positional arguments but where that check isn't in place. Examples:

* itertools.tee just calls PyArg_ParseTuple without checking _PyArg_NoKeywords (is this a bug?) (this is fairly common, I think)

* range always calls _PyArg_NoKeywords, without checking the type (is this a bug?) (this is rare)
msg209128 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-24 22:14
I think it's a bug that they don't do the subclass check.  In practice, nobody subclasses range and itertools.tee.  But they should probably still be fixed.
msg209130 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-24 22:28
Great. Most of the classes in itertools could use this.

BTW, you'll need to supply a way to supply the C name of the type, to use in the type check.
msg209131 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-24 22:30
Way ahead of you.  The "class" directive will have to change:

    class name typedef type_object

On the other hand, this means that Argument Clinic can automatically give the right type to the default self converter.  So most people won't have to write custom self converter classes anymore.  Argument Clinic giveth, and it taketh away.
msg209132 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-24 22:35
+1 for that as well, I've written quite a few self converters for that reason.
msg209136 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-24 22:49
How about this?  I'm going to do a rollup patch today with three fixes: this, #20381, and changing the default filename for the "file" destination.
msg209168 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-25 08:55
I've found two bugs:

1) In the type check, a '&' needs to be added before the type name.
2) Setting template_dict['self_type_object'] fails for module functions
msg209170 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-25 09:03
Also, I believe the the type of the first argument passed to a method is a pointer to the typedef object, so a '*' needs to be added after the typedef name wherever it is used in such functions.
msg209176 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-25 09:36
My last comment was wrong. There is a bug regarding the first argument to new methods; It should just remain a PyTypeObject*.
msg209177 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-25 09:38
1) That's not a bug, that's the API.  If you used the dynamic API to create a type it wouldn't take the &.  So I can't guess in advance what type it is, nor can I assume I always add the &.

2) Will fix.
msg209207 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-25 15:30
The bug you cited is fixed in today's rollup patch, #20390.  (I don't know how to denote the dependency between the two issues, maybe someone else can do that for me?)
msg209219 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-25 17:26
I'm still seeing the first argument to a __new__ function as "groupbyobject *" instead of "PyTypeObject *". This causes the following error (for example):

./Modules/itertoolsmodule.c:112:34: error: no member named 'tp_alloc' in 'groupbyobject'
    gbo = (groupbyobject *)type->tp_alloc(type, 0);
msg209220 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-25 17:28
To clarify my previous comment, I was referring to the first argument passed to the generated 'impl' function.

Context: I'm attempting to convert 'itertools.groupby' in Modules/itertoolsmodule.c.
msg209221 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-25 17:33
Also, I'm seeing this in the generated code for __new__ methods:

if (({self_name} == {self_type_object}) &&
msg209222 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-25 17:42
To make reproducing these bugs easier, I'm attaching my partially converted version of Modules/itertoolsmodules.c, which has the buggy generated code inside.

"Partially converted" means that I've only converted some of the functions requiring conversion. This file should be in a working state.
msg209258 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-26 00:18
Can you try locally applying my patch for #20390 and seeing if you still have the problem?  I tried applying it to the itertoolmodule.c you attached to the issue and it seemed to fix the problem.
msg209322 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2014-01-26 14:21
Using the latest clinic.py from default branch, everything in itertools works like a charm :)
History
Date User Action Args
2014-01-26 14:21:24taleinatsetstatus: open -> closed
resolution: fixed
messages: + msg209322
2014-01-26 00:18:31larrysetmessages: + msg209258
2014-01-25 17:43:00taleinatsetfiles: + itertoolsmodule.c

messages: + msg209222
2014-01-25 17:33:08taleinatsetmessages: + msg209221
2014-01-25 17:28:44taleinatsetmessages: + msg209220
2014-01-25 17:26:50taleinatsetmessages: + msg209219
2014-01-25 15:30:47larrysetmessages: + msg209207
2014-01-25 09:38:44larrysetmessages: + msg209177
2014-01-25 09:36:03taleinatsetmessages: + msg209176
2014-01-25 09:03:23taleinatsetmessages: + msg209170
2014-01-25 08:55:57taleinatsetmessages: + msg209168
2014-01-24 22:49:30larrysetfiles: + larry.clinic.rollup.jan.24.diff.1.txt

messages: + msg209136
2014-01-24 22:35:18taleinatsetmessages: + msg209132
2014-01-24 22:30:13larrysetmessages: + msg209131
2014-01-24 22:28:00taleinatsetmessages: + msg209130
2014-01-24 22:14:43larrysetmessages: + msg209128
2014-01-24 22:05:15taleinatsetmessages: + msg209127
2014-01-24 21:44:42larrysetmessages: + msg209124
2014-01-24 21:35:41taleinatcreate