classification
Title: PyMapping_Check returns 1 for lists
Type: behavior Stage: resolved
Components: Documentation, Interpreter Core Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, bukzor, georg.brandl, jmillikin, levkivskyi, miss-islington, pitrou, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2009-05-05 21:47 by jmillikin, last changed 2018-06-04 10:38 by levkivskyi. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 144 closed jbarlow, 2017-02-18 01:26
PR 7029 merged serhiy.storchaka, 2018-05-21 10:52
PR 7045 merged miss-islington, 2018-05-22 08:02
PR 7049 merged serhiy.storchaka, 2018-05-22 10:27
Messages (27)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2011-01-04 01:58
Guido, can we have your take on this?
msg125288 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 "<stdin>", line 1, in <module>
  File "<string>", line 1, in <module>
TypeError: list indices must be integers, not str

$ ./python -c "import os; os.execle('/bin/ls', 'ls', [])"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2018-06-03 08:03
Are these changes enough? Can this issue be closed now?
History
Date User Action Args
2018-06-04 10:38:51levkivskyisetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-06-03 08:03:35serhiy.storchakasetmessages: + msg318527
2018-05-22 11:54:16serhiy.storchakasetmessages: + msg317270
2018-05-22 10:27:22serhiy.storchakasetpull_requests: + pull_request6686
2018-05-22 08:23:23miss-islingtonsetnosy: + miss-islington
messages: + msg317264
2018-05-22 08:02:55miss-islingtonsetpull_requests: + pull_request6684
2018-05-22 08:02:53serhiy.storchakasetmessages: + msg317263
2018-05-21 15:50:42gvanrossumsetnosy: - gvanrossum
2018-05-21 10:55:14serhiy.storchakasetpriority: high -> normal
versions: + Python 3.7, Python 3.8, - Python 3.5
2018-05-21 10:54:48serhiy.storchakasetmessages: + msg317229
2018-05-21 10:52:38serhiy.storchakasetkeywords: + patch
pull_requests: + pull_request6677
2017-03-06 07:50:42serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg289070
2017-03-06 06:11:04Mariattasetstage: needs patch -> patch review
versions: - Python 3.4
2017-02-18 01:26:59jbarlowsetpull_requests: + pull_request105
2015-07-18 05:31:09christian.barcenassetversions: + Python 3.6
2015-06-29 12:12:46levkivskyisetnosy: + levkivskyi
2015-02-02 22:24:42pitrousetversions: + Python 3.5, - Python 3.2, Python 3.3
2015-02-02 19:07:16benjamin.petersonsetmessages: + msg235283
2015-02-02 19:02:45bukzorsetnosy: + bukzor
messages: + msg235282
2012-11-18 20:07:47ezio.melottisetversions: + Python 3.4
2012-05-08 21:55:03ezio.melottisetstage: needs patch
2011-06-12 21:35:47terry.reedysetversions: + Python 3.3, - Python 3.1
2011-01-06 09:16:22pitrousetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125529
2011-01-06 08:46:55rhettingersetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125522
2011-01-06 08:27:51pitrousetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125520
2011-01-06 08:17:23rhettingersetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125518
2011-01-06 07:52:07pitrousetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125517
2011-01-04 19:01:43gvanrossumsetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125355
2011-01-04 18:47:32pitrousetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125351
2011-01-04 18:36:54gvanrossumsetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125348
2011-01-04 16:14:41pitrousetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125336
2011-01-04 15:55:24gvanrossumsetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125335
2011-01-04 10:31:21pitrousetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125299
2011-01-04 03:46:35gvanrossumsetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125289
2011-01-04 03:22:17rhettingersetnosy: gvanrossum, georg.brandl, rhettinger, pitrou, benjamin.peterson, jmillikin
messages: + msg125288
2011-01-04 01:58:22pitrousetpriority: normal -> high
nosy: + gvanrossum
messages: + msg125279

2010-07-29 13:53:42georg.brandlsetpriority: high -> normal
assignee: georg.brandl ->
2010-05-11 20:53:05terry.reedysetversions: + Python 3.2, - Python 3.0
2009-05-14 19:33:38pitrousetmessages: + msg87758
2009-05-14 19:26:08rhettingersetmessages: + msg87757
2009-05-14 18:42:01benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg87752
2009-05-14 18:24:30pitrousetpriority: critical -> high

nosy: + georg.brandl, pitrou
messages: + msg87751

assignee: georg.brandl
components: + Documentation, Interpreter Core, - Library (Lib)
2009-05-14 13:48:09rhettingersetassignee: rhettinger -> (no value)
2009-05-12 13:27:23pitrousetpriority: critical
versions: + Python 3.1
2009-05-11 20:36:28rhettingersetassignee: rhettinger

nosy: + rhettinger
2009-05-05 21:47:15jmillikincreate