Repository: python-attrs/attrs
Pull request #522 (2019-03-27 06:51:09)

Pure-Python approach to closure cell rewriting
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](https://www.attrs.org/en/latest/contributing.html) 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.

- [x] Added **tests** for changed code.
- [x] New features have been added to our [Hypothesis testing strategy](https://github.com/python-attrs/attrs/blob/master/tests/strategies.py). [N/A]
- [x] Changes or additions to public APIs are reflected in our type stubs (files ending in ``.pyi``). [N/A]
    - [x] ...and used in the stub test file `tests/typing_example.py`. [N/A]
- [x] Updated **documentation** for changed code. [N/A]
    - [x] New functions/classes have to be added to `docs/api.rst` by hand. [N/A]
    - [x] Changes to the signature of `@attr.s()` have to be added by hand too. [N/A]
    - [x] Changed/added classes/methods/functions have appropriate `versionadded`, `versionchanged`, or `deprecated` [directives](http://www.sphinx-doc.org/en/stable/markup/para.html#directive-versionadded). [N/A]
- [x] Documentation in `.rst` files is written using [semantic newlines](https://rhodesmill.org/brandon/2012/one-sentence-per-line/).
- [x] Changes (and possible deprecations) have news fragments in [`changelog.d`](https://github.com/python-attrs/attrs/blob/master/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: 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.
oremanj: One-month checkin on this? The end result is the same as far as I can tell.

Patch:
diff --git a/changelog.d/522.change.rst b/changelog.d/522.change.rst
new file mode 100644
index 0000000..4b827e8
--- /dev/null
+++ b/changelog.d/522.change.rst
@@ -0,0 +1,1 @@
+Slotted classes now use a pure Python mechanism to rewrite the ``__class__`` cell when rebuilding the class, so ``super()`` works even on environments where ``ctypes`` is not installed.
diff --git a/src/attr/_compat.py b/src/attr/_compat.py
index 9a99dcd..f84c6c8 100644
--- a/src/attr/_compat.py
+++ b/src/attr/_compat.py
@@ -106,7 +106,8 @@ else:  # Python 3 and later.
         consequences of not setting the cell on Python 2.
         """
         warnings.warn(
-            "Missing ctypes.  Some features like bare super() or accessing "
+            "Running interpreter doesn't sufficiently support code object "
+            "introspection.  Some features like bare super() or accessing "
             "__class__ will not work with slotted classes.",
             RuntimeWarning,
             stacklevel=2,
@@ -124,36 +125,98 @@ else:  # Python 3 and later.
         return types.MappingProxyType(dict(d))
 
 
-def import_ctypes():
-    """
-    Moved into a function for testability.
-    """
-    import ctypes
-
-    return ctypes
-
-
 def make_set_closure_cell():
+    """Return a function of two arguments (cell, value) which sets
+    the value stored in the closure cell `cell` to `value`.
     """
-    Moved into a function for testability.
-    """
+    # pypy makes this easy. (It also supports the logic below, but
+    # why not do the easy/fast thing?)
     if PYPY:  # pragma: no cover
 
         def set_closure_cell(cell, value):
             cell.__setstate__((value,))
 
+        return set_closure_cell
+
+    # Otherwise gotta do it the hard way.
+
+    # Create a function that will set its first cellvar to `value`.
+    def set_first_cellvar_to(value):
+        x = value
+        return
+
+        # This function will be eliminated as dead code, but
+        # not before its reference to `x` forces `x` to be
+        # represented as a closure cell rather than a local.
+        def force_x_to_be_a_cell():  # pragma: no cover
+            return x
+
+    try:
+        # Extract the code object and make sure our assumptions about
+        # the closure behavior are correct.
+        if PY2:
+            co = set_first_cellvar_to.func_code
+        else:
+            co = set_first_cellvar_to.__code__
+        if co.co_cellvars != ("x",) or co.co_freevars != ():
+            raise AssertionError  # pragma: no cover
+
+        # Convert this code object to a code object that sets the
+        # function's first _freevar_ (not cellvar) to the argument.
+        args = [co.co_argcount]
+        if not PY2:
+            args.append(co.co_kwonlyargcount)
+        args.extend(
+            [
+                co.co_nlocals,
+                co.co_stacksize,
+                co.co_flags,
+                co.co_code,
+                co.co_consts,
+                co.co_names,
+                co.co_varnames,
+                co.co_filename,
+                co.co_name,
+                co.co_firstlineno,
+                co.co_lnotab,
+                # These two arguments are reversed:
+                co.co_cellvars,
+                co.co_freevars,
+            ]
+        )
+        set_first_freevar_code = types.CodeType(*args)
+
+        def set_closure_cell(cell, value):
+            # Create a function using the set_first_freevar_code,
+            # whose first closure cell is `cell`. Calling it will
+            # change the value of that cell.
+            setter = types.FunctionType(
+                set_first_freevar_code, {}, "setter", (), (cell,)
+            )
+            # And call it to set the cell.
+            setter(value)
+
+        # Make sure it works on this interpreter:
+        def make_func_with_cell():
+            x = None
+
+            def func():
+                return x  # pragma: no cover
+
+            return func
+
+        if PY2:
+            cell = make_func_with_cell().func_closure[0]
+        else:
+            cell = make_func_with_cell().__closure__[0]
+        set_closure_cell(cell, 100)
+        if cell.cell_contents != 100:
+            raise AssertionError  # pragma: no cover
+
+    except Exception:
+        return just_warn
     else:
-        try:
-            ctypes = import_ctypes()
-
-            set_closure_cell = ctypes.pythonapi.PyCell_Set
-            set_closure_cell.argtypes = (ctypes.py_object, ctypes.py_object)
-            set_closure_cell.restype = ctypes.c_int
-        except Exception:
-            # We try best effort to set the cell, but sometimes it's not
-            # possible.  For example on Jython or on GAE.
-            set_closure_cell = just_warn
-    return set_closure_cell
+        return set_closure_cell
 
 
 set_closure_cell = make_set_closure_cell()


Test patch:
diff --git a/tests/test_slots.py b/tests/test_slots.py
index f8007e3..fab28fa 100644
--- a/tests/test_slots.py
+++ b/tests/test_slots.py
@@ -2,6 +2,7 @@
 Unit tests for slots-related functionality.
 """
 
+import types
 import weakref
 
 import pytest
@@ -411,14 +412,17 @@ class TestClosureCellRewriting(object):
 
         assert D.statmethod() is D
 
-    @pytest.mark.skipif(PYPY, reason="ctypes are used only on CPython")
-    def test_missing_ctypes(self, monkeypatch):
+    @pytest.mark.skipif(PYPY, reason="set_closure_cell always works on PyPy")
+    def test_code_hack_failure(self, monkeypatch):
         """
-        Keeps working if ctypes is missing.
+        Keeps working if function/code object introspection doesn't work
+        on this (nonstandard) interpeter.
 
         A warning is emitted that points to the actual code.
         """
-        monkeypatch.setattr(attr._compat, "import_ctypes", lambda: None)
+        # This is a pretty good approximation of the behavior of
+        # the actual types.CodeType on Brython.
+        monkeypatch.setattr(types, "CodeType", lambda: None)
         func = make_set_closure_cell()
 
         with pytest.warns(RuntimeWarning) as wr:
@@ -427,7 +431,8 @@ class TestClosureCellRewriting(object):
         w = wr.pop()
         assert __file__ == w.filename
         assert (
-            "Missing ctypes.  Some features like bare super() or accessing "
+            "Running interpreter doesn't sufficiently support code object "
+            "introspection.  Some features like bare super() or accessing "
             "__class__ will not work with slotted classes.",
         ) == w.message.args