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

object.__new__ argument calling autodetection faulty #49572

Open
mitsuhiko opened this issue Feb 19, 2009 · 31 comments
Open

object.__new__ argument calling autodetection faulty #49572

mitsuhiko opened this issue Feb 19, 2009 · 31 comments
Assignees
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@mitsuhiko
Copy link
Member

BPO 5322
Nosy @doko42, @scoder, @vstinner, @larryhastings, @benjaminp, @ned-deily, @mitsuhiko, @Trundle, @serhiy-storchaka, @jdemeyer, @pppery, @iritkatriel
Files
  • update_one_slot.patch
  • update_one_slot2-3.x.patch
  • update_one_slot2-2.7.patch
  • UnsupportedOperation-bases-order.patch
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = None
    created_at = <Date 2009-02-19.20:34:23.266>
    labels = ['interpreter-core', 'type-bug', '3.9', '3.10', '3.11']
    title = 'object.__new__ argument calling autodetection faulty'
    updated_at = <Date 2022-01-25.17:58:17.453>
    user = 'https://github.com/mitsuhiko'

    bugs.python.org fields:

    activity = <Date 2022-01-25.17:58:17.453>
    actor = 'vstinner'
    assignee = 'serhiy.storchaka'
    closed = False
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2009-02-19.20:34:23.266>
    creator = 'aronacher'
    dependencies = []
    files = ['13408', '45525', '45526', '45784']
    hgrepos = []
    issue_num = 5322
    keywords = []
    message_count = 31.0
    messages = ['82497', '82505', '84112', '85828', '86111', '86702', '145289', '281064', '282224', '282225', '282605', '282612', '282615', '282617', '282852', '282942', '282969', '282974', '282976', '282979', '282999', '283170', '283178', '283209', '283243', '283245', '284637', '411478', '411479', '411480', '411647']
    nosy_count = 17.0
    nosy_names = ['doko', 'scoder', 'vstinner', 'larry', 'sebastinas', 'benjamin.peterson', 'ned.deily', 'aronacher', 'prologic', 'Trundle', 'Ringding', 'python-dev', 'serhiy.storchaka', 'jdemeyer', 'ppperry', 'thansen', 'iritkatriel']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue5322'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @mitsuhiko
    Copy link
    Member Author

    In 2.6 a deprecation warning was added if object.__new__ was called
    with arguments. Per se this is fine, but the detection seems to be faulty.

    The following code shows the problem:

    >>> class A(object):
    ...     def __new__(self):
    ...         raise TypeError('i do not exist')
    ... 
    >>> class B(A):
    ...     __new__ = object.__new__
    ...     def __init__(self, x):
    ...         self.x = x
    ... 
    >>> B(1)
    __main__:1: DeprecationWarning: object.__new__() takes no parameters
    <__main__.B object at 0x88dd0>

    In the B case __new__ is not overridden (in the sense that it
    differs from object.new) but __init__ is. Which is the default
    behaviour. Nonetheless a warning is raised.

    I used the pattern with the "__new__ switch" to achieve a
    cStringIO.StringIO behavior that supports typechecks: IterIO() returns
    either a IterI or IterO object, both instances of IterIO so that
    typechecks can be performed.

    Real-world use case here:
    http://dev.pocoo.org/projects/werkzeug/browser/werkzeug/contrib/iterio.py

    @mitsuhiko mitsuhiko added the type-bug An unexpected behavior, bug, or error label Feb 19, 2009
    @mitsuhiko
    Copy link
    Member Author

    The problem seems to be caused by tp_new being slot_tp_new which then
    invokes whatever __new__ in the class dict is.

    I'm not so sure what would be the solution to this. One could of course
    check if tp_new is either object_new or slot_tp_new and in the latter
    case check if the class dict's __new__ item is object_new...

    @Trundle
    Copy link
    Mannequin

    Trundle mannequin commented Mar 24, 2009

    I think the real problem here is update_one_slot and not object_new. It
    is impossible to set "new" to a PyCFunction inside Python code, which
    may be a feature, but is in fact very irritating.

    For example the following snippet:

    >>> class Dict(dict): __new__ = object.__new__
    ...
    >>> Dict.__new__ is object.__new__
    True
    >>> Dict()
    {}

    I would rather expect this behaviour (or at least that Dict.__new__ is not
    object.__new__):

    >>> Dict.__new__ is object.__new__
    True
    >>> Dict()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: object.__new__(Dict) is not safe, use dict.__new__()

    The attached patch leads to that behaviour, which also fixes the argument
    calling autodetection of object.__new__.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 9, 2009

    I'm sorry, I don't have any opinion on this.

    @pitrou pitrou removed their assignment Apr 9, 2009
    @Trundle
    Copy link
    Mannequin

    Trundle mannequin commented Apr 18, 2009

    The problem is that type_setattro() sets the new "new" attribute
    in the type's dict (through PyObject_GenericSetAttr()), but the
    corresponding slot will never be updated if the new "new" is a
    PyCFunction.

    The affected code in update_one_slot() was added by Guido van Rossum
    in r28090, so maybe he would like to comment on that.

    @Trundle
    Copy link
    Mannequin

    Trundle mannequin commented Apr 27, 2009

    See also issue bpo-1694663.

    @benjaminp
    Copy link
    Contributor

    I think it needs tests.

    @serhiy-storchaka
    Copy link
    Member

    Here are updated patches with tests for 3.x and 2.7.

    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.7 (EOL) end of life labels Nov 17, 2016
    @serhiy-storchaka serhiy-storchaka self-assigned this Nov 30, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 2, 2016

    New changeset a37cc3d926ec by Serhiy Storchaka in branch '2.7':
    Issue bpo-5322: Fixed setting __new__ to a PyCFunction inside Python code.
    https://hg.python.org/cpython/rev/a37cc3d926ec

    @serhiy-storchaka
    Copy link
    Member

    Will commit to 3.5-3.7 after releasing 3.6.0.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 7, 2016

    New changeset 1f31bf3f76f5 by Serhiy Storchaka in branch '3.5':
    Issue bpo-5322: Fixed setting __new__ to a PyCFunction inside Python code.
    https://hg.python.org/cpython/rev/1f31bf3f76f5

    New changeset 747de8acb7e4 by Serhiy Storchaka in branch '3.6':
    Issue bpo-5322: Fixed setting __new__ to a PyCFunction inside Python code.
    https://hg.python.org/cpython/rev/747de8acb7e4

    New changeset 9605c558ab58 by Serhiy Storchaka in branch 'default':
    Issue bpo-5322: Fixed setting __new__ to a PyCFunction inside Python code.
    https://hg.python.org/cpython/rev/9605c558ab58

    @vstinner
    Copy link
    Member

    vstinner commented Dec 7, 2016

    test_file started to crash after the change "Issue bpo-5322: Fixed setting __new__ to a PyCFunction inside Python code." :-/ (so all buildbots became red.)

    Can someone fix it or revert it? (Sorry, I don't have the bandwith right to investigate the crash.)

    @serhiy-storchaka
    Copy link
    Member

    The test is fixed if change order of base classes of UnsupportedOperation. This is rather a workaround, we should find more general fix.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 7, 2016

    New changeset 4a610bc8577b by Serhiy Storchaka in branch '3.5':
    Change order of io.UnsupportedOperation base classes.
    https://hg.python.org/cpython/rev/4a610bc8577b

    @thansen
    Copy link
    Mannequin

    thansen mannequin commented Dec 10, 2016

    This change breaks backward compatibility in Python 2.7. This is the example that also broke in bpo-25731. In that case the change was reverted. See https://bugs.python.org/issue25731#msg262922

    $ cat foo.pxd 
    cdef class B:
        cdef object b
    $ cat foo.pyx 
    cdef class A:
        pass

    cdef class B:
    def __init__(self, b):
    self.b = b
    $ cat bar.py
    from foo import A, B

    class C(A, B):
        def __init__(self):
            B.__init__(self, 1)
    
    C()
    $ cython foo.pyx && gcc -I/usr/include/python2.7 -Wall -shared -fPIC -o foo.so foo.c
    $ python -c 'import bar'
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "bar.py", line 7, in <module>
        C()
    TypeError: foo.A.__new__(C) is not safe, use foo.B.__new__()

    @jdemeyer
    Copy link
    Contributor

    Here is more minimal breaking example. This clearly shows that this patch breaks backwards compatibility.

    $ cat obj.pyx
    cdef class OBJ(object):
        pass
    
    $ ipython
    Python 2.7.13rc1 (default, Dec 11 2016, 14:21:24) 
    Type "copyright", "credits" or "license" for more information.
    
    IPython 5.1.0 -- An enhanced Interactive Python.
    ?         -> Introduction and overview of IPython's features.
    %quickref -> Quick reference.
    help      -> Python's own help system.
    object?   -> Details about 'object', use 'object??' for extra details.
    
    In [1]: import pyximport; pyximport.install()
    Out[1]: (None, <pyximport.pyximport.PyxImporter at 0x7f8e8c585910>)
    
    In [2]: import obj
    
    In [3]: class X(obj.OBJ, dict):
       ...:     pass
       ...: 
    
    In [4]: X()
    ---------------------------------------------------------------------------
    TypeError                                 Traceback (most recent call last)
    <ipython-input-4-a7d4f7b89654> in <module>()
    ----> 1 X()
    
    TypeError: obj.OBJ.__new__(X) is not safe, use dict.__new__()
    

    @serhiy-storchaka
    Copy link
    Member

    Does changing the order of base classes help or there is an unavoidable conflict?

    @doko42
    Copy link
    Member

    doko42 commented Dec 12, 2016

    https://trac.sagemath.org/ticket/22037 reports about another regression.

    @jdemeyer
    Copy link
    Contributor

    @serhiy.storchaka: yes, changing the order of the base classes fixes the issue with __new__. Also manually assigning __new__ works, like

    class C(A, B):
        __new__ = B.__new__

    What is broken by this patch is only the auto-detection of which __new__ (really, which tp_new) should be called.

    @Doko: not "another regression", it's exactly the one that we are talking about.

    @serhiy-storchaka
    Copy link
    Member

    Thank you Jeroen. It looks to me that all problems can be resolved by reordering base classes and making Cython not generating trivial __new__. But that is possible only in new Python version. In maintained versions we should keep the old behavior for backward compatibility even if it contradicts normal rules for method resolution and the behavior of Python classes. We should find other solution for making explicit __new__ assigning working.

    @jdemeyer
    Copy link
    Contributor

    Wouldn't it be possible to fix assignment of __new__ without breaking backwards compatibility (and then apply the same patch for all Python versions)? I have a feeling that breaking the auto-detection of tp_new is a new bug introduced by this patch and not a fundamental feature needed to fix assignment of __new__.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 14, 2016

    New changeset 5315db3171b0 by Benjamin Peterson in branch '2.7':
    revert a37cc3d926ec (bpo-5322)
    https://hg.python.org/cpython/rev/5315db3171b0

    @vstinner
    Copy link
    Member

    Since this change seems to break the backward compatibility, is it safe to apply it to Python 3.5.x and Python 3.6.x? The bug was reported in 2009, 7 years ago. Can the fix wait for Python 3.7?

    test_file contains code which worked well before the change and started to crash after the change. If it occurs for an application, I expect users to be unhappy of getting such "behaviour change" in a minor release, no?

    --

    Is it possible to prevent the crash of test_file without modifying its code (without the change 4a610bc8577b "Change order of io.UnsupportedOperation base classes")? Sorry, I didnd't follow this issue.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 14, 2016

    New changeset f89ef18f9824 by Serhiy Storchaka in branch '2.7':
    Issue bpo-5322: Restored tests for __new__.
    https://hg.python.org/cpython/rev/f89ef18f9824

    New changeset 06e4b9f2e4b0 by Serhiy Storchaka in branch '3.5':
    Revert changeset 1f31bf3f76f5 (bpo-5322) except tests.
    https://hg.python.org/cpython/rev/06e4b9f2e4b0

    @benjaminp
    Copy link
    Contributor

    BTW, at least for bpo-25731, I think the right approach in the MI case is to synthesize a __new__ on the subclass that calls the solid base __new__.

    @serhiy-storchaka
    Copy link
    Member

    Yes, it was what the patch did by setting tp_new to slot_tp_new. The problem is that the same code is used for inherited __new__ and assigned in class body. It is hard to distinguish between these cases.

    In any case I think that Cython shouldn't generate trivial __new__. This will help to change the order of __new__ resolution at least in 3.7.

    @jdemeyer
    Copy link
    Contributor

    jdemeyer commented Jan 4, 2017

    It worries me that nothing in the Python docs nor in any PEP describes how tp_new is inherited. In my opinion, ​this patch makes a significant change which should be subject to a PEP. However, neither the old nor new behaviour is described anywhere. This also makes it harder to argue which behaviour is correct.

    @pppery pppery mannequin changed the title Python 2.6 object.__new__ argument calling autodetection faulty object.__new__ argument calling autodetection faulty Aug 8, 2018
    @iritkatriel
    Copy link
    Member

    Can we close this now?

    >>> class A(object):
    ...     def __new__(self):
    ...          raise TypeError('i do not exist')
    ... 
    >>> class B(A):
    ...     __new__ = object.__new__
    ...     def __init__(self, x):
    ...         self.x = x
    ... 
    >>> B(1)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: object.__new__() takes exactly one argument (the type to instantiate)
    >>>

    @mitsuhiko
    Copy link
    Member Author

    The bug is still there, just that it's now not just a warning but an error. The auto detection is incorrect here. It should allow the instantiation of the object with arguments.

    @iritkatriel
    Copy link
    Member

    Right, I see now.

    @iritkatriel iritkatriel added 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes and removed 3.7 (EOL) end of life labels Jan 24, 2022
    @vstinner
    Copy link
    Member

    Python has the same behavior since Python 2.6. While it annoys a few persons, the majority doesn't care. I suggest to close the issue.

    It's easy to workaround limitation that object.__new__() which doesn't accept arguments.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes 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