Skip to content
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

Closed
jmillikin mannequin opened this issue May 5, 2009 · 27 comments
Closed

PyMapping_Check returns 1 for lists #50195

jmillikin mannequin opened this issue May 5, 2009 · 27 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes docs Documentation in the Doc dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@jmillikin
Copy link
Mannequin

jmillikin mannequin commented May 5, 2009

BPO 5945
Nosy @birkenfeld, @rhettinger, @pitrou, @benjaminp, @jmillikin, @bukzor, @serhiy-storchaka, @ilevkivskyi, @miss-islington
PRs
  • bpo-5945: mapping.rst: PyMapping_Check(list) returns 1 #144
  • bpo-5945: Improve mappings and sequences C API docs. #7029
  • [3.7] bpo-5945: Improve mappings and sequences C API docs. (GH-7029) #7045
  • [3.6] bpo-5945: Improve mappings and sequences C API docs. (GH-7029). #7049
  • 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:

    assignee = None
    closed_at = <Date 2018-06-04.10:38:51.810>
    created_at = <Date 2009-05-05.21:47:15.152>
    labels = ['interpreter-core', '3.8', 'type-bug', '3.7', 'docs']
    title = 'PyMapping_Check returns 1 for lists'
    updated_at = <Date 2018-06-04.10:38:51.809>
    user = 'https://github.com/jmillikin'

    bugs.python.org fields:

    activity = <Date 2018-06-04.10:38:51.809>
    actor = 'levkivskyi'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-06-04.10:38:51.810>
    closer = 'levkivskyi'
    components = ['Documentation', 'Interpreter Core']
    creation = <Date 2009-05-05.21:47:15.152>
    creator = 'jmillikin'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 5945
    keywords = ['patch']
    message_count = 27.0
    messages = ['87291', '87751', '87752', '87757', '87758', '125279', '125288', '125289', '125299', '125335', '125336', '125348', '125351', '125355', '125517', '125518', '125520', '125522', '125529', '235282', '235283', '289070', '317229', '317263', '317264', '317270', '318527']
    nosy_count = 9.0
    nosy_names = ['georg.brandl', 'rhettinger', 'pitrou', 'benjamin.peterson', 'jmillikin', 'bukzor', 'serhiy.storchaka', 'levkivskyi', 'miss-islington']
    pr_nums = ['144', '7029', '7045', '7049']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue5945'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @jmillikin
    Copy link
    Mannequin Author

    jmillikin mannequin commented May 5, 2009

    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

    @jmillikin jmillikin mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels May 5, 2009
    @rhettinger rhettinger assigned rhettinger and unassigned rhettinger May 11, 2009
    @pitrou
    Copy link
    Member

    pitrou commented May 14, 2009

    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.

    @pitrou pitrou added docs Documentation in the Doc dir interpreter-core (Objects, Python, Grammar, and Parser dirs) and removed stdlib Python modules in the Lib dir labels May 14, 2009
    @benjaminp
    Copy link
    Contributor

    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.

    @rhettinger
    Copy link
    Contributor

    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?

    @pitrou
    Copy link
    Member

    pitrou commented May 14, 2009

    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.

    @birkenfeld birkenfeld removed their assignment Jul 29, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Jan 4, 2011

    Guido, can we have your take on this?

    @rhettinger
    Copy link
    Contributor

    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)) {

    @gvanrossum
    Copy link
    Member

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 4, 2011

    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()?

    @gvanrossum
    Copy link
    Member

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 4, 2011

    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.

    @gvanrossum
    Copy link
    Member

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 4, 2011

    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

    @gvanrossum
    Copy link
    Member

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 6, 2011

    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?

    @rhettinger
    Copy link
    Contributor

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 6, 2011

    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.

    @rhettinger
    Copy link
    Contributor

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 6, 2011

    > 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).

    @bukzor
    Copy link
    Mannequin

    bukzor mannequin commented Feb 2, 2015

    We've hit this problem today.

    What are we supposed to do in the meantime?

    @benjaminp
    Copy link
    Contributor

    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\>


    @serhiy-storchaka
    Copy link
    Member

    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?

    @serhiy-storchaka
    Copy link
    Member

    I propose an alternate PR 7029 which also adds other improvements to the documentation of mappings and sequences C API.

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life 3.8 only security fixes labels May 21, 2018
    @serhiy-storchaka
    Copy link
    Member

    New changeset f5b1183 by Serhiy Storchaka in branch 'master':
    bpo-5945: Improve mappings and sequences C API docs. (GH-7029)
    f5b1183

    @miss-islington
    Copy link
    Contributor

    New changeset e1a78ca by Miss Islington (bot) in branch '3.7':
    bpo-5945: Improve mappings and sequences C API docs. (GH-7029)
    e1a78ca

    @serhiy-storchaka
    Copy link
    Member

    New changeset 93e9fb5 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-5945: Improve mappings and sequences C API docs. (GH-7029). (GH-7049)
    93e9fb5

    @serhiy-storchaka
    Copy link
    Member

    Are these changes enough? Can this issue be closed now?

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes docs Documentation in the Doc dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants