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

PEP487: Simpler customization of class creation #71553

Closed
tecki mannequin opened this issue Jun 22, 2016 · 20 comments
Closed

PEP487: Simpler customization of class creation #71553

tecki mannequin opened this issue Jun 22, 2016 · 20 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@tecki
Copy link
Mannequin

tecki mannequin commented Jun 22, 2016

BPO 27366
Nosy @ncoghlan, @berkerpeksag, @tecki, @Vgr255
Files
  • pep487.patch: The patch for PEP 487
  • pep487.patch: Changes proposed by Nick
  • pep487.patch: The patch for PEP 487
  • issue27366_tweaks.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 = 'https://github.com/ncoghlan'
    closed_at = <Date 2016-07-31.02:46:45.773>
    created_at = <Date 2016-06-22.07:19:27.687>
    labels = ['interpreter-core', 'type-feature']
    title = 'PEP487: Simpler customization of class creation'
    updated_at = <Date 2016-08-20.00:38:51.495>
    user = 'https://github.com/tecki'

    bugs.python.org fields:

    activity = <Date 2016-08-20.00:38:51.495>
    actor = 'python-dev'
    assignee = 'ncoghlan'
    closed = True
    closed_date = <Date 2016-07-31.02:46:45.773>
    closer = 'ncoghlan'
    components = ['Interpreter Core']
    creation = <Date 2016-06-22.07:19:27.687>
    creator = 'Martin.Teichmann'
    dependencies = []
    files = ['43835', '43854', '43882', '43946']
    hgrepos = []
    issue_num = 27366
    keywords = ['patch']
    message_count = 20.0
    messages = ['269050', '269723', '271129', '271132', '271137', '271141', '271163', '271287', '271665', '271667', '271668', '271672', '271673', '271682', '271683', '271717', '271718', '271720', '271721', '273174']
    nosy_count = 5.0
    nosy_names = ['ncoghlan', 'python-dev', 'berker.peksag', 'Martin.Teichmann', 'abarry']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27366'
    versions = ['Python 3.6']

    @tecki
    Copy link
    Mannequin Author

    tecki mannequin commented Jun 22, 2016

    This is the implementation of PEP-487.

    It adds a metaclass to types that calls a method on a class
    once it is subclassed. This way one can customize the creation
    of classes without the need to write an own metaclass.

    As a second functionality, it calls a method on each descriptor
    in a class, such that descriptors know their name.

    @tecki tecki mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jun 22, 2016
    @tecki
    Copy link
    Mannequin Author

    tecki mannequin commented Jul 2, 2016

    This is a C implementation of PEP-487, directly in the Python core

    @berkerpeksag berkerpeksag added interpreter-core (Objects, Python, Grammar, and Parser dirs) and removed stdlib Python modules in the Lib dir labels Jul 22, 2016
    @ncoghlan
    Copy link
    Contributor

    I started reviewing Martin's patch, and I initially thought I had found a problem with the way __init_subclass__ is currently defined. It turned out I was wrong about it actually being broken, but I *do* now think it's inherently confusing, and we may be able to do something different that's more obviously correct (or at least easier to document - it was proposing revisions to the documentation that got me thinking along this path).

    Specifically, I was thinking using super() in either the zero argument form or the explicit form could create an infinite loop due to the way we're currently proposing to interact with the MRO. Consider:

        class BaseClass:
            @classmethod
            def __init_subclass__(cls):
                super(cls, BaseClass).__init_subclass__()
    
        class SubClass(BaseClass):
            pass

    If the initial call made by type.__new__() is effectively "SubClass.mro()[1].__init_subclass__()", then the super() call is going to call BaseClass.__init_subclass__ again.

    However, it turned out I was wrong, as that's not what happens: the call made by the type machinery is instead "super(SubClass, SubClass).__init_subclass__", which gets it to the right place in the MRO and causes further super() calls to do the right thing.

    However, the "more obviously correct" signature that occurred to me was to do this instead:

        class BaseClass:
            @classmethod
            def __init_subclass__(cls, subcls):
                super(cls, BaseClass).__init_subclass__(subcls)
    
        class SubClass(BaseClass):
            pass

    Then the invocation from type.__new__ could be defined more simply as:

    SubClass.mro()[1].__init_subclass__(SubClass)
    

    In all cases then (regardless of where you were in the MRO), "cls" would refer to "the class first in the MRO after the class being defined" and "subcls" would refer to "the class currently being defined".

    If you consider the plugin example in the PEP, with the revised signature, it would look like:

        class PluginBase:
            subclasses = []
    
            def __init_subclass__(cls, subcls, **kwargs):
                super().__init_subclass__(**kwargs)
                cls.subclasses.append(subcls)

    And *even if the subclass being defined shadowed the "subclasses" attribute*, this initialisation would still work. (You can still get yourself in trouble if a subclass somewhere else in the MRO shadows the attribute, but that's life in complex type hierarchies)

    In the version in the PEP, the fact that "cls" is actually a subclass, and we're relying on the MRO to find "subclasses" is a really subtle implementation detail, while having two parameters makes it clear that "the class defining __init_subclass__" is distinct from the "new subclass being defined".

    @ncoghlan
    Copy link
    Contributor

    Scratch that, my revised idea is fundamentally broken, as this example illustrates:

    >>> class BaseClass: pass
    ... 
    >>> class OtherClass: pass
    ... 
    >>> class SubClass(OtherClass, BaseClass): pass
    ... 
    >>> SubClass.mro()
    [<class '__main__.SubClass'>, <class '__main__.OtherClass'>, <class '__main__.BaseClass'>, <class 'object'>]

    The PEP as written handles this correctly, while the alternative signature would fail miserably ("OtherClass" wouldn't chain up to "BaseClass" properly). So I'll review the rest of the patch, and we can figure out the documentation problem later.

    @ncoghlan ncoghlan self-assigned this Jul 24, 2016
    @ncoghlan
    Copy link
    Contributor

    Martin's current implementation basically looks good to me - I've mainly suggested some changes to the documentation and additional test cases that help stress test some of the more complex inheritance hierarchies described above, although there are a few other other suggestions as well.

    @tecki
    Copy link
    Mannequin Author

    tecki mannequin commented Jul 24, 2016

    Thanks for the nice review!

    I made the proposed changes, see attached patch.

    I am still waiting for a decision whether type.__setattr__ should call __set_name__ from python-dev, once that's sorted out I'll implement and test the one or the other behavior.

    @gvanrossum
    Copy link
    Member

    That's a no from me.

    On Sunday, July 24, 2016, Martin Teichmann <report@bugs.python.org> wrote:

    Martin Teichmann added the comment:

    Thanks for the nice review!

    I made the proposed changes, see attached patch.

    I am still waiting for a decision whether type.__setattr__ should call
    __set_name__ from python-dev, once that's sorted out I'll implement and
    test the one or the other behavior.

    ----------
    Added file: http://bugs.python.org/file43854/pep487.patch


    Python tracker <report@bugs.python.org <javascript:;>>
    <http://bugs.python.org/issue27366\>


    @tecki
    Copy link
    Mannequin Author

    tecki mannequin commented Jul 25, 2016

    I looked over the patch once more and found some places where
    I didn't follow normal coding standards. I changed that, namely
    now the code returns -1 meaning an exception happened and 0 on
    success, which is what many functions in typeobject.c do.

    So I think this patch should be ready.

    @ncoghlan
    Copy link
    Contributor

    Martin's latest patch looks good to me, so I'm applying it now and will push it once a local run of the test suite gives it the thumbs up :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 30, 2016

    New changeset ecc7bff738e0 by Nick Coghlan in branch 'default':
    Issue bpo-27366: Implement PEP-487
    https://hg.python.org/cpython/rev/ecc7bff738e0

    @ncoghlan
    Copy link
    Contributor

    Thanks once again Martin, especially for your patience with the long process in getting this proposal all the way through to resolution :)

    I mostly applied your patch as-is, but tweaked a few aspects of the documentation before committing it (mainly expanding the What's New entry, and showing a few more of the moving parts in the __init_subclass__ example).

    @berkerpeksag
    Copy link
    Member

    Sorry, I'm a bit late to the party. Here are some tweaks to ecc7bff738e0. I've added versionadded directives, updated the tests to use new style classes and removed a duplicate sentence from the __init_subclass__ docstring.

    @ncoghlan
    Copy link
    Contributor

    Good catches Berker - go ahead and apply the improvements whenever's convenient :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 30, 2016

    New changeset 8747e3455ecb by Berker Peksag in branch 'default':
    Issue bpo-27366: Tweak PEP-487 documentation
    https://hg.python.org/cpython/rev/8747e3455ecb

    @berkerpeksag
    Copy link
    Member

    Thanks for the review, Nick! (and also thanks to Martin for the great PEP!)

    @Vgr255
    Copy link
    Mannequin

    Vgr255 mannequin commented Jul 31, 2016

    Hello Martin, and thank you for your patch! However, this patch removed the ability to pass keyword arguments to type, and it's not documented anywhere. I believe it should be documented at least in e.g. the "Changes in the Python API" of the What's New in Python 3.6 document. Anyone cares to submit a patch?

    @Vgr255 Vgr255 mannequin reopened this Jul 31, 2016
    @ncoghlan
    Copy link
    Contributor

    D'oh, another good catch Emanuel - and I'm even the one that raised the need to mention that in the Porting notes during the python-dev discussion :P

    I'll post an update to the What's New shortly.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 31, 2016

    New changeset 313e8fdb0d0c by Nick Coghlan in branch 'default':
    bpo-27366: PEP-487 docs updates
    https://hg.python.org/cpython/rev/313e8fdb0d0c

    @ncoghlan
    Copy link
    Contributor

    The new porting note doesn't quite capture all the subtleties of the situation, but should be sufficient for folks to get any affected custom metaclasses working again (since the key is to avoid passing those parameters up to type.__new__).

    I also added a note about the fact that __init_subclass__ implementations don't have access to the metaclass hint, but can retrieve the actual metaclass based on the passed in class object.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 20, 2016

    New changeset 545bfa4c20eb by Victor Stinner in branch 'default':
    Issue bpo-27366: Fix init_subclass()
    https://hg.python.org/cpython/rev/545bfa4c20eb

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants