| msg87291 - (view) |
Author: John Millikin (jmillikin) |
Date: 2009-05-05 21:47 |
| In Python 2, PyMapping_Check will return 0 for list objects. In Python 3, it returns 1. Obviously, this makes it rather difficult to differentiate between mappings and other sized iterables. In addition, it differs from the behavior of the ``collections.Mapping`` ABC -- isinstance([], collections.Mapping) returns False. Since most of the PyMapping_* functions don't seem to work properly on lists, I believe this behavior is erroneous. The behavior can be seen from a C extension, or if you're lazy, using ctypes: Python 2.6.2 (release26-maint, Apr 19 2009, 01:56:41) [GCC 4.3.3] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import ctypes >>> ctypes.CDLL('libpython2.6.so').PyMapping_Check(ctypes.py_object([])) 0 Python 3.0.1+ (r301:69556, Apr 15 2009, 15:59:22) [GCC 4.3.3] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import ctypes >>> ctypes.CDLL('libpython3.0.so').PyMapping_Check(ctypes.py_object([])) 1 |
|
|
| msg87751 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2009-05-14 18:24 |
| I understand this is indeed unintuitive. The reason list objects support the C "mapping protocol" in 3.x is that it is how slicing of lists (and tuples, for that matter) is implemented. Perhaps the documentation should carry a warning about this. Unfortunately, right now there is no easy way in C to check that an object implements a given ABC. |
|
|
| msg87752 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2009-05-14 18:42 |
| Personally, I think PyMapping_Check and PySequence_Check should be deprecated and removed. Like their Python counterparts, operator.isMappingType() and operation.isSequenceType(), they are unreliable at best in the face of not LBYL and abcs. |
|
|
| msg87757 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2009-05-14 19:26 |
| Why did the list implementation get changed in Py3.x? Is it now necessary for any subscripting type to put the same method in both the sequence methods and mapping methods? Was this change necessary? |
|
|
| msg87758 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-05-14 19:33 |
| > Why did the list implementation get changed in Py3.x? Is it now > necessary for any subscripting type to put the same method in both the > sequence methods and mapping methods? Was this change necessary? I think it's a case of foolish consistency. In py3k there are no opcodes dedicated to slicing anymore, instead the slice object is passed to the mapping's getitem method. |
|
|
| msg125279 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-01-04 01:58 |
| Guido, can we have your take on this? |
|
|
| msg125288 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2011-01-04 03:22 |
| Here's a code search link: http://www.google.com/codesearch?as_q=PyMapping_Check And here's how it currently used internal to the Python codebase: $ ack --cc PyMapping_CheckInclude/abstract.h 1125: PyAPI_FUNC(int) PyMapping_Check(PyObject *o); Modules/posixmodule.c 3585: if (!PyMapping_Check(env)) { 3764: if (!PyMapping_Check(env)) { 3950: if (!PyMapping_Check(env)) { Objects/abstract.c 1987:PyMapping_Check(PyObject *o) Objects/frameobject.c 605: (locals != NULL && !PyMapping_Check(locals))) { PC/_subprocess.c 340: if (! PyMapping_Check(environment)) { Python/bltinmodule.c 700: if (locals != Py_None && !PyMapping_Check(locals)) { 705: PyErr_SetString(PyExc_TypeError, PyMapping_Check(globals) ? 794: if (!PyMapping_Check(locals)) { |
|
|
| msg125289 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2011-01-04 03:46 |
| > Why did the list implementation get changed in Py3.x? Because we decided to get rid of the sq_slice and sq_ass_slice slots in PySequenceMethods, and that in turn was because we got rid of the slice-related opcodes and the separate __getslice__ and __setslice__ magic methods. > Is it now necessary for any subscripting type to put the same method > in both the sequence methods and mapping methods? Yes, if the type wants to support slicing. The reason is that while we changed many things, we didn't want to change the signature of the methods that we kept, and the sq_item/sq_ass_item signatures have arguments that make it impossible to pass the info necessary for a slice. > Was this change necessary? The changes are briefly mentioned in PEP 3100 without any motivation. However I think the rationale was the observation that the old sq_slice / sq_ass_slice took only integers (really ssize_t), meaning that it was impossible to implement a sequence type taking a slice with arguments outside the range supported by ssize_t (e.g. a custom range type supporting huge longs). Or with non-integral arguments. This problem never existed for non-slice __get__ since one could always implement mp_subscript / mp_ass_subscript. Was it necessary? I'm not sure -- but that's water under the bridge. Was it a good change? From the POV of Python code, yes. The old approach caused some odd problems for classes implementing __getslice__ / __setslice__ (since those were invoked after the arguments had been pushed through the sq_slice / sq_ass_slice API). > Personally, I think PyMapping_Check and PySequence_Check should be > deprecated and removed. Like their Python counterparts, > operator.isMappingType() and operation.isSequenceType(), they are > unreliable at best in the face of not LBYL and abcs. Right, calling PyMapping_Check() was never particularly reliable, and extension modules depending on it probably always had subtle bugs. Perhaps it would be nice if we provided a C API to at least some of the ABC package. |
|
|
| msg125299 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-01-04 10:31 |
| > Right, calling PyMapping_Check() was never particularly reliable, and > extension modules depending on it probably always had subtle bugs. > Perhaps it would be nice if we provided a C API to at least some of > the ABC package. In the meantime, would it be reasonable to add the moral equivalent of `hasattr(type(op), 'items')` to PyMapping_Check()? |
|
|
| msg125335 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2011-01-04 15:55 |
| > In the meantime, would it be reasonable to add the moral equivalent > of `hasattr(type(op), 'items')` to PyMapping_Check()? That all depends on what it is used for. Which is hard to say without someone following more of the links that Raymond posted. |
|
|
| msg125336 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-01-04 16:14 |
| Modules/posixmodule.c: uses PyMapping_Size(), PyMapping_Keys() and PyMapping_Values() to parse an environment mapping (for execve() and friends). PyFrame_New(): validates the "locals" argument in pydebug mode (only used for module-level code). Note that functions in frameobject.c have "assert(PyDict_Check(dict))" where "dict" is that same locals argument (copied into f_locals)... PC/_subprocess.c: uses PyMapping_Size(), PyMapping_Keys() and PyMapping_Values() to parse an environment mapping (for CreateProcessW()). Python/btninmodule.c: validates the "locals" argument to eval(). There are also a couple of places where the PyMapping API (such PyMapping_Keys()) is used (e.g. _json), but without calling PyMapping_Check first. |
|
|
| msg125348 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2011-01-04 18:36 |
| The question is, if PyMapping_Check() returns True, and a list is passed, will the code segfault or raise an exception? A segfault would be unacceptable; an exception would be acceptable assuming that the code would have raised an exception anyway if PyMapping_Check() had returned False. |
|
|
| msg125351 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-01-04 18:47 |
| Exceptions seem to be raised (for code that can be exercised in Python), but not very obvious ones: >>> eval("x", {}, []) Traceback (most recent call last): File "", line 1, in File "", line 1, in TypeError: list indices must be integers, not str $ ./python -c "import os; os.execle('/bin/ls', 'ls', [])" Traceback (most recent call last): File "", line 1, in File "/home/antoine/py3k/__svn__/Lib/os.py", line 320, in execle execve(file, args[:-1], env) AttributeError: values |
|
|
| msg125355 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2011-01-04 19:01 |
| It looks like PyMapping_Check() already checks for the presence of a fairly arbitrary special operation (mp_subscript). It sounds fine to replace that with a check for the presence of a keys() or items() method (I'm not sure which one is more representative). I wish the check can be done fast -- but I fear that it'll involve a dict lookup. So be it. |
|
|
| msg125517 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-01-06 07:52 |
| Actually, making PyMapping_Check() more robust produces a test failure in test_builtin, because of the following code: # Verify locals stores (used by list comps) eval('[locals() for i in (2,3)]', g, d) eval('[locals() for i in (2,3)]', g, collections.UserDict()) class SpreadSheet: "Sample application showing nested, calculated lookups." _cells = {} def __setitem__(self, key, formula): self._cells[key] = formula def __getitem__(self, key): return eval(self._cells[key], globals(), self) ss = SpreadSheet() ss['a1'] = '5' ss['a2'] = 'a1*6' ss['a3'] = 'a2*7' self.assertEqual(ss['a3'], 210) Should I fix the example to include a keys() method? |
|
|
| msg125518 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2011-01-06 08:17 |
| Rather than introduce "fixes" that break code and hurt performance, I think it would be better to deprecate PyMapping_Check() and wait for a fast, clean C version of the ABCs (that is supposed to be our one obvious way to do it). FWIW, the spreadsheet example has been around for years and I know of more than one private company that has made heavy use of code modeled on that example (not for spreadsheets, but as a hook for eval). So, I don't think the new "keys" check should be backported. |
|
|
| msg125520 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-01-06 08:27 |
| > Rather than introduce "fixes" that break code and hurt performance, I > think it would be better to deprecate PyMapping_Check() and wait for a > fast, clean C version of the ABCs (that is supposed to be our one > obvious way to do it). Do you also advocate deprecating PySequence_Check()? Both are useful APIs to know what you're dealing with. As for the "clean C version of the ABCs", I'm afraid we could wait quite a bit, since that's a lot more work and nobody seems really interested in the matter. > FWIW, the spreadsheet example has been around for years and I know of > more than one private company that has made heavy use of code modeled > on that example (not for spreadsheets, but as a hook for eval). So, I > don't think the new "keys" check should be backported. Well, I'm not proposing to backport it, but to fix things in 3.2. |
|
|
| msg125522 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2011-01-06 08:46 |
| > Do you also advocate deprecating PySequence_Check()? Perhaps just document PyMapping_Check() as being less informative than before (now it has false positives for sequences). > As for the "clean C version of the ABCs", > I'm afraid we could wait quite a bit That may be true. I hope not. The ABCs were meant to solve exactly this problem. At this point, I would rather ignore the problem for 3.2 than to implement a less clean alternative. |
|
|
| msg125529 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-01-06 09:16 |
| > > As for the "clean C version of the ABCs", > > I'm afraid we could wait quite a bit > > That may be true. I hope not. The ABCs were meant to solve exactly > this problem. At this point, I would rather ignore the problem for > 3.2 than to implement a less clean alternative. Also, please note that checking for ABCs would not solve the test_builtin failure, since the class there doesn't implement the Mapping ABC (and doesn't claim to either). |
|
|
| msg235282 - (view) |
Author: Buck Evan (bukzor) * |
Date: 2015-02-02 19:02 |
| We've hit this problem today. What are we supposed to do in the meantime? |
|
|
| msg235283 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2015-02-02 19:07 |
| Not use PyMapping_Check? On Mon, Feb 2, 2015, at 14:02, Buck Golemon wrote: > > Buck Golemon added the comment: > > We've hit this problem today. > > What are we supposed to do in the meantime? > > ---------- > nosy: +bukzor > > _______________________________________ > Python tracker <report@bugs.python.org> > <http://bugs.python.org/issue5945> > _______________________________________ |
|
|
| msg289070 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-03-06 07:50 |
| Proposed wording doesn't looks much informative to me. Maybe just say that PyMapping_Check() also returns 1 for sequences that support slicing? And recommend `PyMapping_Check() && !PySequence_Check()` for true mapping test? |
|
|
| msg317229 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-05-21 10:54 |
| I propose an alternate PR 7029 which also adds other improvements to the documentation of mappings and sequences C API. |
|
|
| msg317263 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-05-22 08:02 |
| New changeset f5b1183610d5888db3bbd639b1a0c945dbd8f8dd by Serhiy Storchaka in branch 'master': bpo-5945: Improve mappings and sequences C API docs. (GH-7029) https://github.com/python/cpython/commit/f5b1183610d5888db3bbd639b1a0c945dbd8f8dd |
|
|
| msg317264 - (view) |
Author: miss-islington (miss-islington) |
Date: 2018-05-22 08:23 |
| New changeset e1a78cacf65f007b1000966ce3aac6ac2eaa5cfc by Miss Islington (bot) in branch '3.7': bpo-5945: Improve mappings and sequences C API docs. (GH-7029) https://github.com/python/cpython/commit/e1a78cacf65f007b1000966ce3aac6ac2eaa5cfc |
|
|
| msg317270 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-05-22 11:54 |
| New changeset 93e9fb5664e56c02c9aa89098b556929735b35db by Serhiy Storchaka in branch '3.6': [3.6] bpo-5945: Improve mappings and sequences C API docs. (GH-7029). (GH-7049) https://github.com/python/cpython/commit/93e9fb5664e56c02c9aa89098b556929735b35db |
|
|
| msg318527 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-06-03 08:03 |
| Are these changes enough? Can this issue be closed now? |
|
|