Repository: cython/cython
Pull request: Fix behaviour of __class__ special variable
Finishes up https://github.com/cython/cython/pull/2916 and closes https://github.com/cython/cython/issues/2912

Uses `ClassClassNode` for regular classes to get the attribute reliably at runtime (for `cdef class` it can be resolved at compile-time).

It essentially adds the line:

```
__class__ = <ClassCellNode>
```

at the start of the top-level function. This seems slightly clunky, but the advantage is that `__class__` is automatically picked up by `locals()` and is also automatically captured into the scope of any inner functions. No other approach looked like it would handle inner functions particularly easily.

Problem statement:
Cython mishandles PEP-3135 __class__ cell in methods
Hello, I am having trouble with some inconsistent behaviour round the value of `__class__` in Cythonized code. This is on Python **3.6.7** with Cython **0.29.1**. 

Briefly, in uncythonized code `__class__` inside a method on a `class` refers to the `class` object, whereas once cythonized, it seems to refer to `<class 'module'>`. This behaviour is a bit unintuitive, and has caused a few issues in a codebase I'm working on, but it's possible I have misunderstood the language semantics! 

I have tried to search for reasons / documentation why this is the case, but I couldn't turn up anything. I apologise if there is something really obvious I'm missing here; let me know if so and I'll close the issue at once. 

Example:

`minimal.py`:
```
class Something:
    def method(self):
        print(__class__)
```

(in an interpreter, uncythonized):
```
from minimal import Something
Something().method()
# prints <class 'minimal.Something'>
```
(after running `cythonize minimal.py -3`):
```
from minimal import Something
Something().method()
# prints <class 'module'>   
```

I believe there is an obvious workaround -- in instance methods, `self.__class__` should still have the correct value. But I was curious if this is intended behaviour, and if so, is there something in the docs about it?

Hints from issue discussion:
I think we handle `super()` separately but not `__class__` (as defined by [PEP-3135](https://www.python.org/dev/peps/pep-3135/)).

PR welcome, it's probably quite easy. See `TransformBuiltinMethods._inject_super()` in `ParseTreeTransforms.py`. The `NameNode` with name `__class__` should be replaced in methods y a `ClassCellNode`. Tests can go into `py3k_super.pyx`, although a new test file like `pep3135_class_cell.py` that also runs uncompiled in Python would also be nice (add a `#tag: pure3.0` comment at the top of the file to prevent it from running in Python 2).

Commit message: Add missing attribute to pxd file

for "all" tests

Patch to review:
diff --git a/Cython/Compiler/ParseTreeTransforms.pxd b/Cython/Compiler/ParseTreeTransforms.pxd
index 4026429ac65..9098b180886 100644
--- a/Cython/Compiler/ParseTreeTransforms.pxd
+++ b/Cython/Compiler/ParseTreeTransforms.pxd
@@ -78,4 +78,5 @@ cdef class GilCheck(VisitorTransform):
     cdef bint nogil_declarator_only
 
 cdef class TransformBuiltinMethods(EnvTransform):
+    cdef dict def_node_body_insertions
     cdef visit_cython_attribute(self, node)
diff --git a/Cython/Compiler/ParseTreeTransforms.py b/Cython/Compiler/ParseTreeTransforms.py
index 9ed29830c6b..a7151369c22 100644
--- a/Cython/Compiler/ParseTreeTransforms.py
+++ b/Cython/Compiler/ParseTreeTransforms.py
@@ -3063,6 +3063,9 @@ class TransformBuiltinMethods(EnvTransform):
     """
     Replace Cython's own cython.* builtins by the corresponding tree nodes.
     """
+    def __init__(self, *args, **kwds):
+        super(TransformBuiltinMethods, self).__init__(*args, **kwds)
+        self.def_node_body_insertions = dict()
 
     def visit_SingleAssignmentNode(self, node):
         if node.declaration_only:
@@ -3076,6 +3079,11 @@ def visit_AttributeNode(self, node):
         return self.visit_cython_attribute(node)
 
     def visit_NameNode(self, node):
+        if node.name == u'__class__':
+            lenv = self.current_env()
+            entry = lenv.lookup_here(u'__class__')
+            if not entry:
+                node = self._inject_class(node)
         return self.visit_cython_attribute(node)
 
     def visit_cython_attribute(self, node):
@@ -3174,6 +3182,67 @@ def _inject_eval(self, node, func_name):
                     node.pos, self.current_scope_node(), lenv))
         return node
 
+    def _inject_class(self, node):
+        # bare __class__ reference inside function
+        current_def_node = self.current_scope_node()
+
+        if not isinstance(current_def_node, Nodes.FuncDefNode):
+            return node
+        for n in range(len(self.env_stack[-1::-1]), 0, -1):
+            class_node, class_scope = self.env_stack[n-1]
+            if isinstance(class_node, Nodes.ClassDefNode):
+                break
+        else:
+            # failed to find a suitable class node
+            return node
+
+        is_generator = False
+        # Now go from the class_node and find the first FuncDefNode in the stack
+        for n in range(n, len(self.env_stack)):
+            fdef_node, fdef_scope = self.env_stack[n]
+            if isinstance(fdef_node, Nodes.GeneratorDefNode):
+                original_fdef_node = fdef_node
+                fdef_node = fdef_node.gbody
+                is_generator = True
+                break
+            elif isinstance(fdef_node, Nodes.FuncDefNode):
+                break
+
+        else:
+            return node  # no suitable def_node in the stack
+
+        # now we arrange to inject:
+        #  __class__ = ... at the start of the def_node body
+        # The advantage of doing it like this is that it automatically appears in locals()
+        # and it can be captured by inner functions
+        if fdef_node not in self.def_node_body_insertions:
+            pos = fdef_node.body.pos
+            if class_scope.is_c_class_scope:
+                # c-classes can be resolved at compile-time, so they have a simpler
+                # implementation
+                rhs = ExprNodes.NameNode(
+                            pos, name=class_node.scope.name,
+                            entry=class_node.entry)
+            elif class_scope.is_py_class_scope:
+                rhs = ExprNodes.ClassCellNode(pos, is_generator=is_generator)
+                if is_generator:
+                    original_fdef_node.requires_classobj = True
+                else:
+                    fdef_node.requires_classobj = True
+                class_node.class_cell.is_active = True
+            else:
+                return node  # should never happen
+
+            assign_node = Nodes.SingleAssignmentNode(pos,
+                lhs=ExprNodes.NameNode(pos, name=EncodedString("__class__")),
+                rhs=rhs)
+
+            assign_node.analyse_declarations(fdef_scope)
+
+            self.def_node_body_insertions[fdef_node] = assign_node
+
+        return node
+
     def _inject_super(self, node, func_name):
         lenv = self.current_env()
         entry = lenv.lookup_here(func_name)
@@ -3201,6 +3270,25 @@ def _inject_super(self, node, func_name):
                 ]
         return node
 
+    def _do_body_insertion(self, node):
+        body_insertion = self.def_node_body_insertions.pop(node, None)
+        if body_insertion:
+            if isinstance(node.body, Nodes.StatListNode):
+                node.body.stats.insert(0, body_insertion)
+            else:
+                node.body = Nodes.StatListNode(node.body.pos,
+                                               stats=[body_insertion, node.body])
+
+    def visit_FuncDefNode(self, node):
+        node = super(TransformBuiltinMethods, self).visit_FuncDefNode(node)
+        self._do_body_insertion(node)
+        return node
+
+    def visit_GeneratorBodyDefNode(self, node):
+        node = super(TransformBuiltinMethods, self).visit_GeneratorBodyDefNode(node)
+        self._do_body_insertion(node)
+        return node
+
     def visit_SimpleCallNode(self, node):
         # cython.foo
         function = node.function.as_cython_attribute()
diff --git a/tests/run/pep3135_class_cell.py b/tests/run/pep3135_class_cell.py
new file mode 100644
index 00000000000..51b0917f709
--- /dev/null
+++ b/tests/run/pep3135_class_cell.py
@@ -0,0 +1,295 @@
+# cython: language_level=3str
+# mode: run
+# tag: pep3135, pure3.0
+
+import cython
+
+class C(object):
+    """
+    >>> obj = C()
+    >>> obj.method_1()
+    2
+    >>> obj.method_2()
+    3
+    >>> obj.method_3()
+    ['__class__', 'self']
+    >>> obj.method_4()
+    ['self']
+    >>> list(obj.generator())
+    ['C']
+
+    # Python 2 has some difference with binding methods that aren't worth working round for this test
+    >>> import sys
+    >>> C.l() == C if sys.version_info[0] > 2 else True
+    True
+    """
+
+    @classmethod
+    def class_method(cls):
+        return 2
+
+    @staticmethod
+    def static_method():
+        return 3
+
+    def method_1(self):
+        return __class__.class_method()
+
+    def method_2(self):
+        return __class__.static_method()
+
+    def method_3(self):
+        __class__
+        return sorted(list(locals().keys()))
+
+    def method_4(self):
+        return sorted(list(locals().keys()))
+
+    def generator(self):
+        yield __class__.__name__
+
+    l = lambda: __class__
+
+
+@cython.cclass
+class CC(object):
+    """
+    >>> obj = CC()
+    >>> obj.method_1()
+    2
+    >>> obj.method_2()
+    3
+    >>> obj.method_3()
+    ['__class__', 'self']
+    >>> obj.method_4()
+    ['self']
+    >>> list(obj.generator())
+    ['CC']
+
+    # Python 2 has some difference with binding methods that aren't worth working round for this test
+    >>> import sys
+    >>> CC.l() == CC if sys.version_info[0] > 2 else True
+    True
+    """
+
+    @classmethod
+    def class_method(cls):
+        return 2
+
+    @staticmethod
+    def static_method():
+        return 3
+
+    def method_1(self):
+        return __class__.class_method()
+
+    def method_2(self):
+        return __class__.static_method()
+
+    def method_3(self):
+        __class__
+        return sorted(list(locals().keys()))
+
+    def method_4(self):
+        return sorted(list(locals().keys()))
+
+    def generator(self):
+        yield __class__.__name__
+
+    l = lambda: __class__
+
+
+class D:
+    """
+    >>> obj = D()
+    >>> obj.method(1)
+    1
+    >>> obj.method(0)
+    Traceback (most recent call last):
+    ...
+    UnboundLocalError: local variable '__class__' referenced before assignment
+    """
+    def method(self, x):
+        if x: __class__ = x
+        print(__class__)
+
+
+@cython.cclass
+class CD:
+    """
+    >>> obj = CD()
+    >>> obj.method(1)
+    1
+    >>> obj.method(0)
+    Traceback (most recent call last):
+    ...
+    UnboundLocalError: local variable '__class__' referenced before assignment
+    """
+    def method(self, x):
+        if x: __class__ = x
+        print(__class__)
+
+
+class E:
+    """
+    >>> obj = E()
+    >>> obj.method()().__name__
+    'E'
+    >>> obj.method2()()().__name__
+    'E'
+    """
+    def method(self):
+        def inner(): return __class__
+        return inner
+
+    def method2(self):
+        def inner():
+            def inner_inner():
+                return __class__
+            return inner_inner
+        return inner
+
+
+@cython.cclass
+class CE:
+    """
+    >>> obj = CE()
+    >>> obj.method()().__name__
+    'CE'
+    >>> obj.method2()()().__name__
+    'CE'
+    """
+    def method(self):
+        def inner(): return __class__
+        return inner
+
+    def method2(self):
+        def inner():
+            def inner_inner():
+                return __class__
+            return inner_inner
+        return inner
+
+
+class G:
+    """
+    >>> obj = G()
+    >>> obj.method().__name__
+    'H'
+    """
+    def method(self):
+        class H:
+            def inner(self):
+                return __class__
+        return H().inner()
+
+
+class I:
+    """
+    >>> obj = I()
+    >>> obj.method()()().__name__
+    'J'
+    """
+    def method(self):
+        def inner():
+            class J:
+                def inner(self):
+                    return __class__
+            return J().inner
+        return inner
+
+
+class K:
+    def method(self): return __class__
+
+
+@cython.cclass
+class CK:
+    def method(self): return __class__
+
+
+__test__ = {
+    "k": """
+    >>> OldK = K
+    >>> K = None
+    >>> OldK().method().__name__
+    'K'
+    """,
+    "ck": """
+    >>> OldCK = CK
+    >>> CK = None
+    >>> OldCK().method().__name__
+    'CK'
+    """
+}
+
+class L:
+    """
+    >>> OldL().method().__name__
+    'L'
+    """
+    def method(self): return self.__class__
+
+OldL = L
+L = None
+
+class M:
+    """
+    >>> M().method()
+    'overwritten'
+    """
+    def method(self):
+        __class__ = 'overwritten'
+        return __class__
+
+@cython.cclass
+class CM:
+    """
+    >>> CM().method()
+    'overwritten'
+    """
+    def method(self):
+        __class__ = 'overwritten'
+        return __class__
+
+
+class N:
+    """
+    >>> N().method().__name__
+    'N'
+    """
+    __class__ = 'N'
+    def method(self):
+        return __class__
+
+if cython.compiled:
+    @cython.cclass
+    class CDefFuncTest:
+        """
+        >>> obj = CDefFuncTest()
+        >>> obj.call_cfunc1().__name__
+        'CDefFuncTest'
+
+        #>>> obj.call_cfunc2()().__name__ - GH 4092
+        #'CDefFuncTest'
+        >>> obj.call_cfunc3()
+        ['__class__', 'self']
+        """
+        @cython.cfunc
+        def cfunc1(self):
+            return __class__
+        def call_cfunc1(self):
+            return self.cfunc1()
+        #@cython.cfunc - disabled, GH 4092. This works outside pure Python mode
+        #def cfunc2(self):
+        #    def inner():
+        #        return __class__
+        #    return inner
+        def call_cfunc2(self):
+            return self.cfunc2()
+        @cython.cfunc
+        def cfunc3(self):
+            __class__
+            return sorted(list(locals().keys()))
+        def call_cfunc3(self):
+            return self.cfunc3()
diff --git a/tests/run/py3k_super.pyx b/tests/run/py3k_super.pyx
index 2968b99b2ae..a9e7a8cb3b5 100644
--- a/tests/run/py3k_super.pyx
+++ b/tests/run/py3k_super.pyx
@@ -16,6 +16,8 @@ class A(object):
     def generator_test(self):
         return [1, 2, 3]
 
+    def super_class(self):
+        return __class__
 
 class B(A):
     """
@@ -28,6 +30,24 @@ class B(A):
     3
     >>> list(obj.generator_test())
     [1, 2, 3]
+    >>> obj.star_method()
+    1
+    >>> obj.starstar_method()
+    1
+    >>> obj.starstarstar_method()
+    1
+    >>> obj.star_class_method()
+    2
+    >>> obj.starstar_class_method()
+    2
+    >>> obj.starstarstar_class_method()
+    2
+    >>> obj.star_static_method(obj)
+    3
+    >>> obj.starstar_static_method(obj)
+    3
+    >>> obj.starstarstar_static_method(obj)
+    3
     """
     def method(self):
         return super().method()
@@ -44,6 +64,153 @@ class B(A):
         for i in super().generator_test():
             yield i
 
+    def star_method(self, *args):
+        return super().method()
+
+    def starstar_method(self, **kwargs):
+        return super().method()
+
+    def starstarstar_method(cls, *args, **kwargs):
+        return super().method()
+
+    @classmethod
+    def star_class_method(cls, *args):
+        return super().class_method()
+
+    @classmethod
+    def starstar_class_method(cls, **kwargs):
+        return super().class_method()
+
+    @classmethod
+    def starstarstar_class_method(cls, *args, **kwargs):
+        return super().class_method()
+
+    @staticmethod
+    def star_static_method(instance, *args):
+        return super().static_method()
+
+    @staticmethod
+    def starstar_static_method(instance, **kwargs):
+        return super().static_method()
+
+    @staticmethod
+    def starstarstar_static_method(instance, *args, **kwargs):
+        return super().static_method()
+
+
+class C(A):
+    """
+    >>> obj = C()
+    >>> obj.method_1()
+    2
+    >>> obj.method_2()
+    3
+    >>> obj.method_3()
+    ['__class__', 'self']
+    >>> obj.method_4()
+    ['self']
+    >>> obj.method_5()
+    <class 'py3k_super.C'>
+    >>> obj.super_class()
+    <class 'py3k_super.A'>
+    """
+    
+    def method_1(self):
+        return __class__.class_method()
+
+    def method_2(self):
+        return __class__.static_method()
+
+    def method_3(self):
+        __class__
+        return sorted(list(locals().keys()))
+
+    def method_4(self):
+        return sorted(list(locals().keys()))
+
+    def method_5(self):
+        return __class__
+
+
+class D:
+    """
+    >>> obj = D()
+    >>> obj.method(1)
+    1
+    >>> obj.method(0)
+    Traceback (most recent call last):
+    ...
+    UnboundLocalError: local variable '__class__' referenced before assignment
+    """
+    def method(self, x):
+        if x: __class__ = x
+        print(__class__)
+
+
+class E:
+    """
+    >>> obj = E()
+    >>> obj.method()().__name__
+    'E'
+    """
+    def method(self):
+        def inner(): return __class__
+        return inner
+
+
+class F:
+    """
+    >>> obj = F()
+    >>> obj.method()()().__name__
+    'F'
+    """
+    def method(self):
+        def inner():
+            def inner_inner():
+                return __class__
+            return inner_inner
+        return inner
+
+
+class G:
+    """
+    >>> obj = G()
+    >>> obj.method().__name__
+    'H'
+    """
+    def method(self):
+        class H:
+            def inner(self):
+                return __class__
+        return H().inner()
+
+
+class I:
+    """
+    >>> obj = I()
+    >>> obj.method()()().__name__
+    'J'
+    """
+    def method(self):
+        def inner():
+            class J:
+                def inner(self):
+                    return __class__
+            return J().inner
+        return inner
+
+
+class K:
+    def method(self): return __class__
+
+__test__ = {
+    "k": """
+    >>> OldK = K
+    >>> K = None
+    >>> OldK().method().__name__
+    'K'
+    """
+}
 
 def test_class_cell_empty():
     """


Code review comments:
diff_hunk: @@ -3063,6 +3063,9 @@ class TransformBuiltinMethods(EnvTransform):
     """
     Replace Cython's own cython.* builtins by the corresponding tree nodes.
     """
+    def __init__(self, *args, **kwds):
+        super(TransformBuiltinMethods, self).__init__(*args, **kwds)
+        self.def_node_body_insertions = dict()
path: Cython/Compiler/ParseTreeTransforms.py
text: @user1:
```suggestion
        self.def_node_body_insertions = {}
```

diff_hunk: @@ -3076,6 +3079,11 @@ def visit_AttributeNode(self, node):
         return self.visit_cython_attribute(node)
 
     def visit_NameNode(self, node):
+        if node.name == u'__class__':
+            lenv = self.current_env()
+            entry = lenv.lookup_here(u'__class__')
+            if not entry:
+                node = self._inject_class(node)
         return self.visit_cython_attribute(node)
path: Cython/Compiler/ParseTreeTransforms.py
text: @user1:
Putting this into `TransformBuiltinMethods` (where "builtin" refers to `cython.*`) seems stretching it a bit. Not sure if too much…

@author:
`TransformBuiltinMethods` also deals with `locals()/vars()/super()/eval()`. I realise that `__class__` isn't a *method* here but it certainly seems similar to some of those (especially `super()`) which is why it ended up here.

For now I'll update the docstring for `TransformBuiltinMethods` since it seems to have acquired this secondary, undocumented purpose.

diff_hunk: @@ -3174,6 +3182,67 @@ def _inject_eval(self, node, func_name):
                     node.pos, self.current_scope_node(), lenv))
         return node
 
+    def _inject_class(self, node):
+        # bare __class__ reference inside function
+        current_def_node = self.current_scope_node()
+
+        if not isinstance(current_def_node, Nodes.FuncDefNode):
+            return node
+        for n in range(len(self.env_stack[-1::-1]), 0, -1):
+            class_node, class_scope = self.env_stack[n-1]
+            if isinstance(class_node, Nodes.ClassDefNode):
+                break
+        else:
+            # failed to find a suitable class node
+            return node
+
+        is_generator = False
+        # Now go from the class_node and find the first FuncDefNode in the stack
+        for n in range(n, len(self.env_stack)):
+            fdef_node, fdef_scope = self.env_stack[n]
+            if isinstance(fdef_node, Nodes.GeneratorDefNode):
+                original_fdef_node = fdef_node
path: Cython/Compiler/ParseTreeTransforms.py
text: @user1:
It's usually not good style to introduce variables conditionally that are used later on in non-trivial contexts (that one needs to reason about in order to understand that their usage is correct).

Just initialise `original_fdef_node` to None before the loop. Also, `generator_node` might be a better name for it. And finally, we prefer duck-typey attribute tests over inheritance based type tests, so use `fdef_node.is_generator` instead of `isinstance()`.

@author:
At the moment `fdef_node.is_generator` isn't defined for `ClassDefNode` - it's only to distinguish function types, so I've reverted to the type test.

diff_hunk: @@ -3174,6 +3182,67 @@ def _inject_eval(self, node, func_name):
                     node.pos, self.current_scope_node(), lenv))
         return node
 
+    def _inject_class(self, node):
+        # bare __class__ reference inside function
+        current_def_node = self.current_scope_node()
+
+        if not isinstance(current_def_node, Nodes.FuncDefNode):
+            return node
+        for n in range(len(self.env_stack[-1::-1]), 0, -1):
path: Cython/Compiler/ParseTreeTransforms.py
text: @user1:
This seems a bit convoluted. I think `len(self.env_stack[-1::-1])` just means `len(self.env_stack) - 1`, right?

And I don't think we need two loops for this. If one goes right-to-left until it stops, and the second goes left-to-right from there, then they look at the same scopes. I think we can just remember the last generator/funcdefnode that we see before the class node, going from right-to-left. And that should be doable with standard `reversed()` list iteration, not with index loops.

@author:
Yes it can be changed into a single loop. That does seem clearer