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

Pure-Python approach to closure cell rewriting #522

Merged
merged 5 commits into from May 8, 2019

Conversation

@oremanj
Copy link
Contributor

@oremanj oremanj commented Mar 27, 2019

Instead of using ctypes, make a function that sets a cellvar and then fiddle with the code object to make it set a freevar instead. PyPy still uses __setstate__ because it is easy and fast. I believe this is similar to the approach used by the cloudpickle package, but I didn't directly reference their code when writing this.

The performance of the new approach is comparable to the old one; timeit on py2.7 gives 645ns for the new approach (down from 691ns for the old), and on py3.6 gives 822ns for the new approach (up from 659ns for the old). And dropping the use of ctypes seems like a win.

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

  • Added tests for changed code.
  • New features have been added to our Hypothesis testing strategy. [N/A]
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi). [N/A]
    • ...and used in the stub test file tests/typing_example.py. [N/A]
  • Updated documentation for changed code. [N/A]
    • New functions/classes have to be added to docs/api.rst by hand. [N/A]
    • Changes to the signature of @attr.s() have to be added by hand too. [N/A]
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. [N/A]
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

@Tinche
Copy link
Member

@Tinche Tinche commented Mar 27, 2019

Interesting. I originally wrote the ctypes approach, but I used ctypes because I was not aware of a different way of doing it. If the end result is the same and we stop using ctypes, I think that'd be a win.

@hynek hynek added this to the 19.2 milestone Mar 27, 2019
@oremanj
Copy link
Contributor Author

@oremanj oremanj commented May 1, 2019

One-month checkin on this? The end result is the same as far as I can tell.

Copy link

@auvipy auvipy left a comment

looks great

hynek
hynek approved these changes May 8, 2019
Copy link
Member

@hynek hynek left a comment

Thanks I've procrastinated on it because I wasn't comfortable to review this amazing hack myself. I've just hijacked @markshannon at the core sprints to have a look.

Thank you very much, let's hope this is the last we hear about cells. ;)

@hynek hynek merged commit 0acfba6 into python-attrs:master May 8, 2019
3 checks passed
@hynek
Copy link
Member

@hynek hynek commented May 31, 2019

Looks like this is breaking on 3.8 😞:

=================================== FAILURES ===================================
_______________________ test_init_subclass_vanilla[True] _______________________

slots = True

    @pytest.mark.parametrize("slots", [True, False])
    def test_init_subclass_vanilla(slots):
        """
        `super().__init_subclass__` can be used if the subclass is not an attrs
        class both with dict and slotted classes.
        """
    
        @attr.s(slots=slots)
        class Base:
            def __init_subclass__(cls, param, **kw):
                super().__init_subclass__(**kw)
                cls.param = param
    
>       class Vanilla(Base, param="foo"):

tests/test_init_subclass.py:25: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'tests.test_init_subclass.test_init_subclass_vanilla.<locals>.Vanilla'>
param = 'foo', kw = {}

    def __init_subclass__(cls, param, **kw):
>       super().__init_subclass__(**kw)
E       TypeError: super(type, obj): obj must be an instance or subtype of type

tests/test_init_subclass.py:22: TypeError
_____________ TestClosureCellRewriting.test_closure_cell_rewriting _____________

self = <tests.test_slots.TestClosureCellRewriting object at 0x7f49887f6940>

    def test_closure_cell_rewriting(self):
        """
        Slotted classes support proper closure cell rewriting.
    
        This affects features like `__class__` and the no-arg super().
        """
        non_slot_instance = C1(x=1, y="test")
        slot_instance = C1Slots(x=1, y="test")
    
        assert non_slot_instance.my_class() is C1
>       assert slot_instance.my_class() is C1Slots
E       AssertionError: assert <class 'tests.test_slots.C1Slots'> is C1Slots
E        +  where <class 'tests.test_slots.C1Slots'> = <bound method C1Slots.my_class of C1Slots(x=1, y='test')>()
E        +    where <bound method C1Slots.my_class of C1Slots(x=1, y='test')> = C1Slots(x=1, y='test').my_class

tests/test_slots.py:354: AssertionError
__________________ TestClosureCellRewriting.test_inheritance ___________________

self = <tests.test_slots.TestClosureCellRewriting object at 0x7f4988a85970>

    def test_inheritance(self):
        """
        Slotted classes support proper closure cell rewriting when inheriting.
    
        This affects features like `__class__` and the no-arg super().
        """
    
        @attr.s
        class C2(C1):
            def my_subclass(self):
                return __class__
    
        @attr.s
        class C2Slots(C1Slots):
            def my_subclass(self):
                return __class__
    
        non_slot_instance = C2(x=1, y="test")
        slot_instance = C2Slots(x=1, y="test")
    
        assert non_slot_instance.my_class() is C1
>       assert slot_instance.my_class() is C1Slots
E       AssertionError: assert <class 'tests.test_slots.C1Slots'> is C1Slots
E        +  where <class 'tests.test_slots.C1Slots'> = <bound method C1Slots.my_class of C2Slots(x=1, y='test')>()
E        +    where <bound method C1Slots.my_class of C2Slots(x=1, y='test')> = C2Slots(x=1, y='test').my_class

tests/test_slots.py:381: AssertionError
________________ TestClosureCellRewriting.test_cls_static[True] ________________

self = <tests.test_slots.TestClosureCellRewriting object at 0x7f4988ab7160>
slots = True

    @pytest.mark.parametrize("slots", [True, False])
    def test_cls_static(self, slots):
        """
        Slotted classes support proper closure cell rewriting for class- and
        static methods.
        """
        # Python can reuse closure cells, so we create new classes just for
        # this test.
    
        @attr.s(slots=slots)
        class C:
            @classmethod
            def clsmethod(cls):
                return __class__
    
>       assert C.clsmethod() is C
    
        @attr.s(slots=slots)
E       AssertionError: assert <class 'tests.test_slots.TestClosureCellRewriting.test_cls_static.<locals>.C'> is <class 'tests.test_slots.TestClosureCellRewriting.test_cls_static.<locals>.C'>
E        +  where <class 'tests.test_slots.TestClosureCellRewriting.test_cls_static.<locals>.C'> = <bound method TestClosureCellRewriting.test_cls_static.<locals>.C.clsmethod of <class 'tests.test_slots.TestClosureCellRewriting.test_cls_static.<locals>.C'>>()
E        +    where <bound method TestClosureCellRewriting.test_cls_static.<locals>.C.clsmethod of <class 'tests.test_slots.TestClosureCellRewriting.test_cls_static.<locals>.C'>> = <class 'tests.test_slots.TestClosureCellRewriting.test_cls_static.<locals>.C'>.clsmethod

tests/test_slots.py:405: AssertionError

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants