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

Implement PEP 422: Simple class initialisation hook #61246

Closed
durban mannequin opened this issue Jan 26, 2013 · 18 comments
Closed

Implement PEP 422: Simple class initialisation hook #61246

durban mannequin opened this issue Jan 26, 2013 · 18 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@durban
Copy link
Mannequin

durban mannequin commented Jan 26, 2013

BPO 17044
Nosy @jcea, @ncoghlan, @ezio-melotti, @florentx, @durban, @ericsnowcurrently, @csabella
Files
  • pep422_1.patch: PEP 422 implementation (1)
  • pep422_2.patch: PEP 422 implementation (2)
  • pep422_3.patch: PEP 422 implementation (3)
  • pep422_4.patch: PEP 422 implementation (4) (object.init_class)
  • pep422_5.patch: PEP 422 implementation (5) (document object.init_class)
  • pep422.patch: Patch to the PEP 422 itself
  • pep422_6.patch: PEP 422 implementation (6) (namespace argument of prepare and init_class)
  • 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-01-28.05:09:30.999>
    created_at = <Date 2013-01-26.21:29:18.323>
    labels = ['interpreter-core', 'type-feature', 'library']
    title = 'Implement PEP 422: Simple class initialisation hook'
    updated_at = <Date 2018-01-28.05:09:30.997>
    user = 'https://github.com/durban'

    bugs.python.org fields:

    activity = <Date 2018-01-28.05:09:30.997>
    actor = 'ncoghlan'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-01-28.05:09:30.999>
    closer = 'ncoghlan'
    components = ['Interpreter Core', 'Library (Lib)']
    creation = <Date 2013-01-26.21:29:18.323>
    creator = 'daniel.urban'
    dependencies = []
    files = ['28859', '28867', '28912', '29098', '29101', '29102', '29851']
    hgrepos = []
    issue_num = 17044
    keywords = ['patch', 'needs review']
    message_count = 18.0
    messages = ['180714', '180746', '180990', '181795', '182265', '182267', '182275', '184092', '184138', '184189', '184190', '184192', '184195', '184310', '184322', '186920', '310884', '310919']
    nosy_count = 8.0
    nosy_names = ['jcea', 'ncoghlan', 'ezio.melotti', 'Arfrever', 'flox', 'daniel.urban', 'eric.snow', 'cheryl.sabella']
    pr_nums = []
    priority = 'normal'
    resolution = 'out of date'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue17044'
    versions = ['Python 3.5']

    @durban
    Copy link
    Mannequin Author

    durban mannequin commented Jan 26, 2013

    The attached patch implements PEP-422 -- Simple class initialisation hook (init_class). It includes tests, but it doesn't include documentation yet.

    @durban durban mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jan 26, 2013
    @durban
    Copy link
    Mannequin Author

    durban mannequin commented Jan 27, 2013

    Thanks for the review Ezio!

    I've fixed the order of the arguments of assertEqual. Regarding your other comment: I agree that your code is shorter and clearer, but it doesn't do exactly the same thing. cls.__init_class__ can be None, and in that case, we should call it (the user should get a TypeError). So the getattr call would need another sentinel object. And with that sentinel, I don't think the code would be shorter and clearer any more.

    @durban
    Copy link
    Mannequin Author

    durban mannequin commented Jan 30, 2013

    I'm attaching a new patch with some documentation and one additional test.

    @ncoghlan
    Copy link
    Contributor

    I've belatedly applied the PEP update Daniel sent me, and added a reference to this issue from the PEP.

    The latest version of the patch looks very good to me, just one very minor nit with the phrasing in the docs. Specifically, it is better to replace "like" with "as" in the phrase "like in the following example" to get "as in the following example".

    @durban
    Copy link
    Mannequin Author

    durban mannequin commented Feb 17, 2013

    Thanks for the grammar correction, I've fixed it in the new patch.

    The new patch also adds object.__init_class__ (which is a no-op), to support cooperative multiple inheritance of __init_class__. (Adding type.__init_class__ was mentioned in the python-dev discussion, but that is not sufficient, so I've added this method to object instead of type.)

    Tests are also updated. I'll update the docs, if this change is OK.

    @ncoghlan
    Copy link
    Contributor

    Ah, I think I see your point - because __init_class__ is supposed to be a class method on instances of the metaclass, the anchor needs to be on object (the highest level instance of the default metaclass), not on type if we don't want super to behave strangely?

    I think that's a valid conclusion (albeit a very subtle difference relative to an ordinary method on type), and compared to the tapdancing we do in __new__ and __init__ to help them be valid anchors for cooperative multiple inheritance, adding object.__init_class__ is a relatively minor thing.

    If you wanted to propose a patch to the PEP as well, that would be great.

    @durban
    Copy link
    Mannequin Author

    durban mannequin commented Feb 17, 2013

    Yes, if we would add a regular (instance) method __init_class__ to type, it could (probably) work for regular (non-meta) classes, but not for metaclasses. If a metaclass Meta wouldn't define __init_class__ itself, calling Meta.__init_class__() in __build_class__ wouldn't work, since Meta would inherit the *instance* method from type (its superclass). We might be able to make this work somehow (I'm not sure), but I think adding it to object as a classmethod works fine, and doesn't require special casing.

    The attached patch documents, that object has an __init_class__ (and also adds some extra tests). I'll attach a patch to the PEP as well.

    @durban
    Copy link
    Mannequin Author

    durban mannequin commented Mar 13, 2013

    I've looked into implementing the changes in the new version of the PEP. It seems, that currently type.__new__ copies the dict returned by __prepare__ (http://hg.python.org/cpython/file/55806d234653/Objects/typeobject.c#l2058). I think this means that some of the new examples wouldn't work ("Order preserving classes" and "Extending a class"). It also means, that currently it is impossible for two classes to share a namespace object (possibly this was the intention of copying the dict). So the "Note" in the PEP wouldn't be necessary (unless type.__new__ were modified).

    I'm not sure if the PEP proposes to change this behavior of type.__new__ or not...

    @ncoghlan
    Copy link
    Contributor

    I hadn't noticed that type.__new__ copied the contents (it surprises me that it does both that *and* restricts the input type to a true dict instance).

    The "Extending a class" example should still work as shown, since the magic of that happens while the body of ExtendedExample is running.

    For the order preserving case, it turns out CPython already keeps a copy of the original namespace around as cls.__locals__, but this is currently undocumented (as far as I can tell anyway).

    If we elevate that to documented behaviour, then __init_class__ implementations can reference both the original object, as well as the snapshot underlying the class object.

    Given that, it is probably also better to revert the namespace keyword to accepting an instance rather than a factory function, since the copy operation after execution of the class body is automatic.

    @ericsnowcurrently
    Copy link
    Member

    We should definitely have a way to expose the original dictionary from __prepare__(). Along with Nick's point, another reason is to allow class decorators to have access to that original, which is important to any use case that involves post-creation introspection of the definition order within the class namespace.

    Nick, where did you see cls.__locals__? I'm not finding any mention of __locals__ outside compiler.c and symtable.c. I agree that such an attribute on classes would be helpful, even if by another name, and that it should be documented.

    @ericsnowcurrently
    Copy link
    Member

    I've also opened bpo-17421 for dropping the restriction on the namespace passed to the metaclass and bpo-17422 for documenting that the passed namespace is copied into a new dict.

    @ericsnowcurrently
    Copy link
    Member

    Given that, it is probably also better to revert the namespace keyword
    to accepting an instance rather than a factory function, since the copy
    operation after execution of the class body is automatic.

    Agreed. Of course, the related note is rendered superfluous. There is still the possibility of reusing the same namespace across successive class definitions, with the associated consequences. However, since type.__new__() copies the namespace into a new dict, it mitigates the main concern: implicit modification to an existing class in the definition of another.

    @ncoghlan
    Copy link
    Contributor

    Oh, that's bizarre - the presence of __locals__ is a side effect of
    calling locals() in the class body. So perhaps passing the namespace
    as a separate __init_class__ parameter is a better option.

    @ncoghlan
    Copy link
    Contributor

    Eric and I discussed this further this morning. We were interested in two main points:

    1. When no custom namespace is used, we definitely shouldn't double the storage requirements for the class object
    2. If a custom namespace is used, it would be nice to make it available to both the __init_class__ hook *and* to class decorators

    The design we came up with for doing so is for type.__new__ to include the equivalent of:

        if type(namespace) != dict:
            cls.__namespace__ = types.MappingProxyType(namespace)

    The practical downside of that approach is that it still doubles the storage requirements for every class that uses a custom namespace, even if the custom namespace isn't actually needed after creation. The upside is that you can write a class decorator that needs ordering information and say "to use this decorator, you must also specify 'namespace=collections.OrderedDict()'"

    There's also a major complexity downside to that approach - the distinction between __dict__ and __namespace__ is somewhat subtle, especially since __namespace__ won't see later changes made through setattr() and delattr(), and __dict__ won't see changes made through any external references to the custom namespace.

    That means my current inclination is to instead change the signature of __init_class__ to accept a read-only view of the execution namespace as a second parameter. Decorators won't have access to the details lost by the copying of attributes into an ordinary dict unless used with a metaclass or base class that takes steps to attach it to the created class object. I can live with that limitation, and it means we only have to properly document the "the contents of the class execution namespace are copied into an ordinary dict instance when the class is created" behaviour of type to explain why __init_class__ has the second parameter.

    @ericsnowcurrently
    Copy link
    Member

    I'm fine with that. Would it make sense to have the signature of __init_class__ parallel meta.__init__():

      __init_class__(cls, name, bases, namespace)

    or even

      __init_class__(cls, name, bases, ns, *, namespace=None, **kwds)

    @durban
    Copy link
    Mannequin Author

    durban mannequin commented Apr 14, 2013

    I've attached a new patch. With this patch, type.__prepare__ has an optional keyword-only argument 'namespace', and returns it if it's specified. Also, __init_class__ is passed an argument: a mapping proxy of the mapping originally returned by __prepare__.

    Would it make sense to have the signature of __init_class__ parallel
    meta.__init__()
    I don't think so, because some of the arguments (name, bases) would be mostly useless, others would have a different meaning (namespace). Although, passing the keyword arguments from the class header might make some sense ... I'm not sure.

    If everybody agrees with these changes, I'll create a patch for the PEP too.

    @csabella
    Copy link
    Contributor

    Since PEP-422 was withdrawn in favor of PEP-487, should this issue be closed?

    @ncoghlan
    Copy link
    Contributor

    Indeed it should! Thanks for pointing that out :)

    @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) stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants