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

Remove unused and undocumented PyGen_NeedsFinalizing() function #59293

Closed
ncoghlan opened this issue Jun 17, 2012 · 13 comments
Closed

Remove unused and undocumented PyGen_NeedsFinalizing() function #59293

ncoghlan opened this issue Jun 17, 2012 · 13 comments
Labels
3.9 only security fixes docs Documentation in the Doc dir

Comments

@ncoghlan
Copy link
Contributor

BPO 15088
Nosy @ncoghlan, @pitrou, @vstinner, @csabella, @nanjekyejoannah
PRs
  • bpo-15088 : Remove PyGen_NeedsFinalizing() #15702
  • Files
  • Issue15088.diff
  • 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 2019-09-06.15:43:02.433>
    created_at = <Date 2012-06-17.04:21:01.434>
    labels = ['3.9', 'docs']
    title = 'Remove unused and undocumented PyGen_NeedsFinalizing() function'
    updated_at = <Date 2019-09-06.15:43:02.428>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2019-09-06.15:43:02.428>
    actor = 'vstinner'
    assignee = 'docs@python'
    closed = True
    closed_date = <Date 2019-09-06.15:43:02.433>
    closer = 'vstinner'
    components = ['Documentation']
    creation = <Date 2012-06-17.04:21:01.434>
    creator = 'ncoghlan'
    dependencies = []
    files = ['30079']
    hgrepos = []
    issue_num = 15088
    keywords = ['patch']
    message_count = 13.0
    messages = ['163012', '188170', '217456', '314420', '314791', '314808', '314809', '314902', '351204', '351207', '351208', '351258', '351259']
    nosy_count = 7.0
    nosy_names = ['ncoghlan', 'pitrou', 'vstinner', 'docs@python', 'plasticgap', 'cheryl.sabella', 'nanjekyejoannah']
    pr_nums = ['15702']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue15088'
    versions = ['Python 3.9']

    @ncoghlan
    Copy link
    Contributor Author

    Currently undocumented: http://docs.python.org/py3k/c-api/gen.html
    Public API in a released version of Python: http://hg.python.org/cpython/file/3.2/Include/genobject.h

    @ncoghlan ncoghlan added the docs Documentation in the Doc dir label Jun 17, 2012
    @plasticgap
    Copy link
    Mannequin

    plasticgap mannequin commented Apr 30, 2013

    Please correct me if I'm wrong, but I think PyGen_NeedsFinalizing should not be an API function because it is only used, nor _PyGen_FetchStopIterationValue. In the attached patch I've removed PyGen_NeedsFinalizing and _PyGen_FetchStopIterationValue fom the public API.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 29, 2014

    For the record, PyGen_NeedsFinalizing still exists but it isn't used anymore in the code base (following PEP-442).

    @csabella
    Copy link
    Contributor

    What should be done for the PyGen_NeedsFinalizing API? Does it need to be documented or should it be removed since it's no longer used in the code base?

    Thanks!

    @csabella csabella added 3.7 (EOL) end of life 3.8 only security fixes labels Mar 25, 2018
    @pitrou
    Copy link
    Member

    pitrou commented Apr 1, 2018

    Cheryl: it may be useful to do a code search to find out whether any third-party projects are relying on PyGen_NeedsFinalizing.

    @csabella
    Copy link
    Contributor

    csabella commented Apr 2, 2018

    Thanks Antoine.

    Do you have a good way for searching third party projects? I searched on Github and I'm getting a lot of references to the CPython filenames. I don't know how to check if anyone is making calls to the function. But, maybe that's the point and it can't be removed if there's a chance that anyone uses it? Thanks!

    This is what I tried on Github:
    PyGen_NeedsFinalizing -filename:ceval -filename:genobject -filename:gcmodule

    @pitrou
    Copy link
    Member

    pitrou commented Apr 2, 2018

    I think https://searchcode.com/ may have a larger indexing base than GitHub alone.

    And, yes, you'll see many duplicates of the CPython source code... Visual inspection may be needed :-/

    @csabella
    Copy link
    Contributor

    csabella commented Apr 3, 2018

    Thanks again, Antoine. I'll see what I can come up with. :-)

    @nanjekyejoannah
    Copy link
    Member

    My searches show references to this function in CPython cloned repositories. From @pitrous's views, I think it is better to deprecate it with a removal notice for a later release.

    I have deprecated the Function instead and will be removed in the next release in this PR #15702 .

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2019

    PyGen_NeedsFinalizing() was added by:

    commit 49fd7fa
    Author: Thomas Wouters <thomas@python.org>
    Date: Fri Apr 21 10:40:58 2006 +0000

    Merge p3yk branch with the trunk up to revision 45595. This breaks a fair
    number of tests, all because of the codecs/_multibytecodecs issue described
    here (it's not a Py3K issue, just something Py3K discovers):
    http://mail.python.org/pipermail/python-dev/2006-April/064051.html
    
    Hye-Shik Chang promised to look for a fix, so no need to fix it here. The
    tests that are expected to break are:
    
    test_codecencodings_cn
    test_codecencodings_hk
    test_codecencodings_jp
    test_codecencodings_kr
    test_codecencodings_tw
    test_codecs
    test_multibytecodec
    
    This merge fixes an actual test failure (test_weakref) in this branch,
    though, so I believe merging is the right thing to do anyway.
    

    It was used in this gcmodule.c function:

    /* Return true if object has a finalization method.
     * CAUTION:  An instance of an old-style class has to be checked for a
     *__del__ method, and earlier versions of this used to call PyObject_HasAttr,
     * which in turn could call the class's __getattr__ hook (if any).  That
     * could invoke arbitrary Python code, mutating the object graph in arbitrary
     * ways, and that was the source of some excruciatingly subtle bugs.
     */
    static int
    has_finalizer(PyObject *op)
    {
            if (PyInstance_Check(op)) {
                    assert(delstr != NULL);
                    return _PyInstance_Lookup(op, delstr) != NULL;
            }
            else if (PyType_HasFeature(op->ob_type, Py_TPFLAGS_HEAPTYPE))
                    return op->ob_type->tp_del != NULL;
            else if (PyGen_CheckExact(op))
                    return PyGen_NeedsFinalizing((PyGenObject *)op);
            else
                    return 0;
    }

    (2) The PEP-442 implementation made PyGen_NeedsFinalizing() useless:

    commit 796564c
    Author: Antoine Pitrou <solipsis@pitrou.net>
    Date: Tue Jul 30 19:59:21 2013 +0200

    Issue bpo-18112: PEP-442 implementation (safe object finalization).
    

    Replaced:

    /* Return true if object has a finalization method. */
    static int
    has_finalizer(PyObject *op)
    {
        if (PyGen_CheckExact(op))
            return PyGen_NeedsFinalizing((PyGenObject *)op);
        else
            return op->ob_type->tp_del != NULL;
    }

    with:

    /* Return true if object has a pre-PEP 442 finalization method. */
    static int
    has_legacy_finalizer(PyObject *op)
    {
        return op->ob_type->tp_del != NULL;
    }

    --

    The last reference to PyGen_NeedsFinalizing() can be found in ceval.c:

        case TARGET(SETUP_FINALLY): {
            /* NOTE: If you add any new block-setup opcodes that
               are not try/except/finally handlers, you may need
               to update the PyGen_NeedsFinalizing() function.
               */
    
            PyFrame_BlockSetup(f, SETUP_FINALLY, INSTR_OFFSET() + oparg,
                               STACK_LEVEL());
            DISPATCH();
        }
    

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2019

    The function is not documented nor tested.

    I searched for usage of PyGen_NeedsFinalizing() in C code in GitHub:
    https://github.com/search?l=C&p=1&q=PyGen_NeedsFinalizing&type=Code

    I only found copies of the CPython code source: PyGen_NeedsFinalizing() definition in genobject.h.

    IHMO we can safely remove the function right now. If someone complains, we can reintroduce it later. We just have to document clearly its removal at:
    https://docs.python.org/dev/whatsnew/3.9.html#build-and-c-api-changes

    @vstinner
    Copy link
    Member

    vstinner commented Sep 6, 2019

    New changeset 74b662c by Victor Stinner (Joannah Nanjekye) in branch 'master':
    bpo-15088 : Remove PyGen_NeedsFinalizing() (GH-15702)
    74b662c

    @vstinner
    Copy link
    Member

    vstinner commented Sep 6, 2019

    I merged Joannah's change: thanks.

    If anyone uses the removed function, please complain before Python 3.9.0 release :-)

    @vstinner vstinner added 3.9 only security fixes and removed 3.7 (EOL) end of life 3.8 only security fixes labels Sep 6, 2019
    @vstinner vstinner closed this as completed Sep 6, 2019
    @vstinner vstinner changed the title PyGen_NeedsFinalizing is public, but undocumented Remove unused and undocumented PyGen_NeedsFinalizing() function Sep 6, 2019
    @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.9 only security fixes docs Documentation in the Doc dir
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants