classification
Title: Over-zealous keyword-arguments check for built-in set class
Type: Stage:
Components: Interpreter Core Versions: Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: georg.brandl Nosy List: dib_at_work, georg.brandl, nnorwitz, rhettinger, zseil
Priority: release blocker Keywords:

Created on 2006-05-11 16:17 by dib_at_work, last changed 2007-02-19 08:21 by rhettinger. This issue is now closed.

Files
File name Uploaded Description Edit
BuiltinSetKeywordArgumentsCheckBroken.py dib_at_work, 2006-05-11 16:17 BuiltinSetKeywordArgumentsCheckBroken.py
nokeywordchecks.diff georg.brandl, 2007-01-11 19:56
Messages (16)
msg28478 - (view) Author: dib (dib_at_work) Date: 2006-05-11 16:17
The fix for bug #1119418 (xrange() builtin accepts
keyword arg silently) included in Python 2.4.2c1+
breaks code that passes keyword argument(s) into
classes derived from the built-in set class, even if
those derived classes explictly accept those keyword
arguments and avoid passing them down to the built-in
base class.

Simplified version of code in attached
BuiltinSetKeywordArgumentsCheckBroken.py fails at (G)
due to bug #1119418 if version < 2.4.2c1; if version >=
2.4.2c1 (G) passes thanks to that bug fix, but instead
(H) incorrectly-in-my-view fails.

[Presume similar cases would fail for xrange and the
other classes mentioned in #1119418.]

  -- David Bruce

(Tested on 2.4, 2.4.2, 2.5a2 on linux2, win32.)
msg28479 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2006-05-11 17:23
Logged In: YES 
user_id=849994

Raymond, what to do in this case?

Note that other built-in types, such as list(), do accept
keyword arguments.
msg28480 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2006-05-19 20:02
Logged In: YES 
user_id=1326842

This bug was introduced as part of the fix for bug #1119418.

It also affects collections.deque.

Can't the _PyArg_NoKeywords check simply be moved
to set_init and deque_init as it was done for
zipimport.zipimporter?

array.array doesn't need to be changed, since it
already does all of its initialization in its
__new__ method.

The rest of the types changed in that fix should not
be affected, since they are immutable.
 
msg28481 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2006-05-20 01:19
Logged In: YES 
user_id=1326842

See patch #1491939
msg28482 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2007-01-06 02:26
I prefer the approach used by list().
msg28483 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-01-10 21:30
I'll do that, only in set_init, you have 
if (!PyArg_UnpackTuple(args, self->ob_type->tp_name, 0, 1, &iterable))

Changing this to use PyArg_ParseTupleAndKeywords would require a format string of
"|O:" + self->ob_type->tp_name

Is it worth constructing that string each time set_init() is called or should it just be "|O:set" for
sets and frozensets?
msg28484 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2007-01-11 00:49
My proposed solution:

- if(!PyArg_NoKeywords("set()", kwds)
+ if(type == &PySet_Type && !PyArg_NoKeywords("set()", kwds)
msg28485 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2007-01-11 18:30
I fixed setobject.c in revisions 53380 and 53381.

Please apply similar fixes to all the other places being bitten my the pervasive NoKeywords tests.
msg28486 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-01-11 19:56
Attaching patch.
File Added: nokeywordchecks.diff
msg28487 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2007-01-11 20:43
That looks about right.  Please add test cases that fail without the patch and succeed with the patch.  Also, put a comment in Misc/NEWS.  If the whole test suite passes, go ahead and check-in to Py2.5.1 and the head.  

Thanks, 

Raymond
msg28488 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-01-17 07:22
Were these changes applied by Raymond?  I don't think there were NEWS entries though.
msg28489 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-01-17 09:13
I'll create the testcases and commit the patch (as well as NEWS entries :) when I find the time.
msg28490 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-01-21 10:29
Committed as rev. 53509, 53510 (2.5).
msg28491 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2007-02-19 02:38
There are more of these that need to be fixed:

bufferobject.c:236:     if (!_PyArg_NoKeywords("buffer()", kw))
classobject.c:2261:     if (!_PyArg_NoKeywords("instancemethod", kw))
exceptions.c:57:    if (!_PyArg_NoKeywords(self->ob_type->tp_name, kwds))
funcobject.c:653:       if (!_PyArg_NoKeywords("classmethod", kwds))
funcobject.c:810:       if (!_PyArg_NoKeywords("staticmethod", kwds))
rangeobject.c:48:       if (!_PyArg_NoKeywords("xrange()", kw))
sliceobject.c:197:      if (!_PyArg_NoKeywords("slice()", kw))
typeobject.c:5773:      if (!_PyArg_NoKeywords("super", kwds))

msg28492 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-02-19 08:02
The exceptions.c one is in __init__, so it is not a problem when the derived class doesn't pass along kwargs to super.__init__.

I deliberately left the others as they were since their type objects lack the Py_TPFLAGS_BASETYPE flag.
msg28493 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2007-02-19 08:21
Okay, it looks like this bug was already fixed.
History
Date User Action Args
2006-05-11 16:17:04dib_at_workcreate