-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
PyMapping_Check returns 1 for lists #50195
Comments
In Python 2, PyMapping_Check will return 0 for list objects. In Python Since most of the PyMapping_* functions don't seem to work properly on The behavior can be seen from a C extension, or if you're lazy, using 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 |
I understand this is indeed unintuitive. The reason list objects support Unfortunately, right now there is no easy way in C to check that an |
Personally, I think PyMapping_Check and PySequence_Check should be |
Why did the list implementation get changed in Py3.x? Is it now |
I think it's a case of foolish consistency. In py3k there are no opcodes |
Guido, can we have your take on this? |
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 Objects/abstract.c Objects/frameobject.c PC/_subprocess.c Python/bltinmodule.c |
Because we decided to get rid of the sq_slice and sq_ass_slice slots
Yes, if the type wants to support slicing. The reason is that while
The changes are briefly mentioned in PEP-3100 without any motivation. 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
Right, calling PyMapping_Check() was never particularly reliable, and Perhaps it would be nice if we provided a C API to at least some of |
In the meantime, would it be reasonable to add the moral equivalent of |
That all depends on what it is used for. Which is hard to say without someone following more of the links that Raymond posted. |
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). 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. |
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. |
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 |
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. |
Actually, making PyMapping_Check() more robust produces a test failure in test_builtin, because of the following code:
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? |
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. |
Do you also advocate deprecating PySequence_Check()? Both are useful As for the "clean C version of the ABCs", I'm afraid we could wait quite
Well, I'm not proposing to backport it, but to fix things in 3.2. |
Perhaps just document PyMapping_Check() as being less informative than before (now it has false positives for sequences).
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 |
We've hit this problem today. What are we supposed to do in the meantime? |
Not use PyMapping_Check? On Mon, Feb 2, 2015, at 14:02, Buck Golemon wrote:
|
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 |
I propose an alternate PR 7029 which also adds other improvements to the documentation of mappings and sequences C API. |
Are these changes enough? Can this issue be closed now? |
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: