nogil check changes; fixes #338 and #329

Cython/Compiler/ExprNodes.py

@@ -330,12 +330,12 @@ class ExprNode(Node):
     def analyse_target_types(self, env):
         self.analyse_types(env)
 
-    def gil_check(self, env):
+    def nogil_check(self, env):
         # By default, any expression based on Python objects is
         # prevented in nogil environments.  Subtypes must override
         # this if they can work without the GIL.
         if self.type.is_pyobject:
-            self._gil_check(env)
+            self.gil_error()
 
     def gil_assignment_check(self, env):
         if env.nogil and self.type.is_pyobject:
@@ -599,7 +599,8 @@ class NoneNode(PyConstNode):
     value = "Py_None"
 
     constant_result = None
-    gil_check = None
+    
+    nogil_check = None
 
     def compile_time_value(self, denv):
         return None
@@ -621,7 +622,7 @@ class ConstNode(AtomicExprNode):
     # value     string      C code fragment
     
     is_literal = 1
-    gil_check = None
+    nogil_check = None
 
     def is_simple(self):
         return 1
@@ -1065,14 +1066,14 @@ class NameNode(AtomicExprNode):
             self.is_used_as_rvalue = 1
             env.use_utility_code(get_name_interned_utility_code)
 
-    def gil_check(self, env):
+    def nogil_check(self, env):
         if self.is_used_as_rvalue:
             entry = self.entry
             if entry.is_builtin:
                 # if not Options.cache_builtins: # cached builtins are ok
-                self._gil_check(env)
+                self.gil_error()
             elif entry.is_pyglobal:
-                self._gil_check(env)
+                self.gil_error()
 
     gil_message = "Accessing Python global or builtin"
 
@@ -1671,24 +1672,17 @@ class IndexNode(ExprNode):
                     error(self.pos,
                         "Invalid index type '%s'" %
                             self.index.type)
-        self.gil_check(env)
-
-    def gil_check(self, env):
-        if not self.is_buffer_access:
-            if self.base.type.is_pyobject:
-                self._gil_check(env)
-
     gil_message = "Indexing Python object"
 
-    def gil_check(self, env):
-        if self.is_buffer_access and env.nogil:
+    def nogil_check(self, env):
+        if self.is_buffer_access:
             if env.directives['boundscheck']:
                 error(self.pos, "Cannot check buffer index bounds without gil; use boundscheck(False) directive")
                 return
             elif self.type.is_pyobject:
                 error(self.pos, "Cannot access buffer with object dtype without gil")
                 return
-        super(IndexNode, self).gil_check(env)
+        super(IndexNode, self).nogil_check(env)
 
 
     def check_const_addr(self):
@@ -1946,7 +1940,7 @@ class SliceIndexNode(ExprNode):
             self.stop = self.stop.coerce_to(c_int, env)
         self.is_temp = 1
 
-    gil_check = ExprNode._gil_check
+    nogil_check = Node.gil_error
     gil_message = "Slicing Python object"
 
     def generate_result_code(self, code):
@@ -2164,12 +2158,12 @@ class CallNode(ExprNode):
             self.coerce_to(type, env)
             return True
 
-    def gil_check(self, env):
+    def nogil_check(self, env):
         func_type = self.function_type()
         if func_type.is_pyobject:
-            self._gil_check(env)
+            self.gil_error()
         elif not getattr(func_type, 'nogil', False):
-            self._gil_check(env)
+            self.gil_error()
 
     gil_message = "Calling gil-requiring function"
 
@@ -2466,7 +2460,7 @@ class GeneralCallNode(CallNode):
     
     subexprs = ['function', 'positional_args', 'keyword_args', 'starstar_arg']
 
-    gil_check = CallNode._gil_check
+    nogil_check = Node.gil_error
 
     def compile_time_value(self, denv):
         function = self.function.compile_time_value(denv)
@@ -2568,7 +2562,7 @@ class AsTupleNode(ExprNode):
         self.type = tuple_type
         self.is_temp = 1
 
-    gil_check = ExprNode._gil_check
+    nogil_check = Node.gil_error
     gil_message = "Constructing Python tuple"
 
     def generate_result_code(self, code):
@@ -2799,9 +2793,9 @@ class AttributeNode(ExprNode):
                     "Object of type '%s' has no attribute '%s'" %
                     (obj_type, self.attribute))
 
-    def gil_check(self, env):
+    def nogil_check(self, env):
         if self.is_py_attr:
-            self._gil_check(env)
+            self.gil_error()
 
     gil_message = "Accessing Python attribute"
 
@@ -3595,7 +3589,7 @@ class DictItemNode(ExprNode):
     # value        ExprNode
     subexprs = ['key', 'value']
 
-    gil_check = None # handled by DictNode
+    nogil_check = None # Parent DictNode takes care of it
 
     def calculate_constant_result(self):
         self.constant_result = (
@@ -3772,9 +3766,9 @@ class UnopNode(ExprNode):
     def is_py_operation(self):
         return self.operand.type.is_pyobject
 
-    def gil_check(self, env):
+    def nogil_check(self, env):
         if self.is_py_operation():
-            self._gil_check(env)
+            self.gil_error()
 
     def coerce_operand_to_pyobject(self, env):
         self.operand = self.operand.coerce_to_pyobject(env)
@@ -3979,9 +3973,9 @@ class TypecastNode(ExprNode):
             if self.typecheck and self.type.is_extension_type:
                 self.operand = PyTypeTestNode(self.operand, self.type, env)
 
-    def gil_check(self, env):
-        if self.type.is_pyobject and self.is_temp:
-            self._gil_check(env)
+    def nogil_check(self, env):
+        if self.type and self.type.is_pyobject and self.is_temp:
+            self.gil_error()
 
     def check_const(self):
         self.operand.check_const()
@@ -4186,9 +4180,9 @@ class BinopNode(ExprNode):
         return (self.operand1.type.is_pyobject 
             or self.operand2.type.is_pyobject)
 
-    def gil_check(self, env):
+    def nogil_check(self, env):
         if self.is_py_operation():
-            self._gil_check(env)
+            self.gil_error()
     
     def coerce_operands_to_pyobjects(self, env):
         self.operand1 = self.operand1.coerce_to_pyobject(env)
@@ -5137,7 +5131,7 @@ class PyTypeTestNode(CoercionNode):
         self.type = dst_type
         self.result_ctype = arg.ctype()
 
-    gil_check = CoercionNode._gil_check
+    nogil_check = Node.gil_error
     gil_message = "Python type test"
     
     def analyse_types(self, env):
@@ -5293,9 +5287,9 @@ class CoerceToBooleanNode(CoercionNode):
         if arg.type.is_pyobject:
             self.is_temp = 1
 
-    def gil_check(self, env):
+    def nogil_check(self, env):
         if self.arg.type.is_pyobject:
-            self._gil_check(env)
+            self.gil_error()
 
     gil_message = "Truth-testing Python object"
     
@@ -5381,7 +5375,7 @@ class CloneNode(CoercionNode):
     #  node is responsible for doing those things.
     
     subexprs = [] # Arg is not considered a subexpr
-    gil_check = None
+    nogil_check = None
     
     def __init__(self, arg):
         CoercionNode.__init__(self, arg)

Cython/Compiler/Nodes.py

@@ -133,12 +133,9 @@ class Node(object):
     
     gil_message = "Operation"
 
-    gil_check = None
-    def _gil_check(self, env):
-        if env.nogil:
-            self.gil_error()
+    nogil_check = None
 
-    def gil_error(self):
+    def gil_error(self, env=None):
         error(self.pos, "%s not allowed without gil" % self.gil_message)
 
     def clone_node(self):
@@ -1365,14 +1362,14 @@ class CFuncDefNode(FuncDefNode):
     def need_gil_acquisition(self, lenv):
         return self.type.with_gil
 
-    def gil_check(self, env):
+    def nogil_check(self, env):
         type = self.type
         with_gil = type.with_gil
         if type.nogil and not with_gil:
             if type.return_type.is_pyobject:
                 error(self.pos,
                       "Function with Python return type cannot be declared nogil")
-            for entry in env.var_entries:
+            for entry in self.local_scope.var_entries:
                 if entry.type.is_pyobject:
                     error(self.pos, "Function declared nogil has Python locals or temporaries")
 
@@ -3190,7 +3187,7 @@ class PrintStatNode(StatNode):
         if len(self.arg_tuple.args) == 1 and self.append_newline:
             env.use_utility_code(printing_one_utility_code)
 
-    gil_check = StatNode._gil_check
+    nogil_check = Node.gil_error
     gil_message = "Python print statement"
 
     def generate_execution_code(self, code):
@@ -3232,7 +3229,7 @@ class ExecStatNode(StatNode):
             self.args[i] = arg
         env.use_utility_code(Builtin.pyexec_utility_code)
 
-    gil_check = StatNode._gil_check
+    nogil_check = Node.gil_error
     gil_message = "Python exec statement"
 
     def generate_execution_code(self, code):
@@ -3276,10 +3273,10 @@ class DelStatNode(StatNode):
                 error(arg.pos, "Deletion of non-Python object")
             #arg.release_target_temp(env)
 
-    def gil_check(self, env):
+    def nogil_check(self, env):
         for arg in self.args:
             if arg.type.is_pyobject:
-                self._gil_check(env)
+                self.gil_error()
 
     gil_message = "Deleting Python object"
 
@@ -3363,9 +3360,9 @@ class ReturnStatNode(StatNode):
                 and not return_type.is_returncode):
                     error(self.pos, "Return value required")
 
-    def gil_check(self, env):
+    def nogil_check(self, env):
         if self.return_type.is_pyobject:
-            self._gil_check(env)
+            self.gil_error()
 
     gil_message = "Returning Python object"
 
@@ -3425,7 +3422,7 @@ class RaiseStatNode(StatNode):
         env.use_utility_code(raise_utility_code)
         env.use_utility_code(restore_exception_utility_code)
 
-    gil_check = StatNode._gil_check
+    nogil_check = Node.gil_error
     gil_message = "Raising exception"
 
     def generate_execution_code(self, code):
@@ -3477,7 +3474,7 @@ class ReraiseStatNode(StatNode):
         env.use_utility_code(raise_utility_code)
         env.use_utility_code(restore_exception_utility_code)
 
-    gil_check = StatNode._gil_check
+    nogil_check = Node.gil_error
     gil_message = "Raising exception"
 
     def generate_execution_code(self, code):
@@ -3503,7 +3500,7 @@ class AssertStatNode(StatNode):
             self.value.analyse_types(env)
             self.value = self.value.coerce_to_pyobject(env)
 
-    gil_check = StatNode._gil_check
+    nogil_check = Node.gil_error
     gil_message = "Raising exception"
     
     def generate_execution_code(self, code):
@@ -3820,6 +3817,13 @@ class ForFromStatNode(LoopNode, StatNode):
     py_loopvar_node = None
     from_range = False
 
+    gil_message = "For-loop using object bounds or target"
+
+    def nogil_check(self, env):
+        for x in (self.target, self.bound1, self.bound2):
+            if x.type.is_pyobject:
+                self.gil_error()
+
     def analyse_declarations(self, env):
         self.target.analyse_target_declaration(env)
         self.body.analyse_declarations(env)
@@ -4038,7 +4042,7 @@ class TryExceptStatNode(StatNode):
         if self.else_clause:
             self.else_clause.analyse_expressions(env)
 
-    gil_check = StatNode._gil_check
+    nogil_check = Node.gil_error
     gil_message = "Try-except statement"
 
     def generate_execution_code(self, code):
@@ -4325,7 +4329,7 @@ class TryFinallyStatNode(StatNode):
         self.body.analyse_expressions(env)
         self.finally_clause.analyse_expressions(env)
 
-    gil_check = StatNode._gil_check
+    nogil_check = Node.gil_error
     gil_message = "Try-finally statement"
 
     def generate_execution_code(self, code):
@@ -4489,8 +4493,7 @@ class GILStatNode(TryFinallyStatNode):
         TryFinallyStatNode.analyse_expressions(self, env)
         env.nogil = was_nogil
 
-    def gil_check(self, env):
-        pass
+    nogil_check = None
 
     def generate_execution_code(self, code):
         code.mark_pos(self.pos)

Cython/Compiler/ParseTreeTransforms.py

@@ -866,30 +866,32 @@ class GilCheck(VisitorTransform):
     """
     def __call__(self, root):
         self.env_stack = [root.scope]
+        self.nogil = False
         return super(GilCheck, self).__call__(root)
 
     def visit_FuncDefNode(self, node):
         self.env_stack.append(node.local_scope)
-        if node.gil_check is not None:
-            node.gil_check(self.env_stack[-1])
+        was_nogil = self.nogil
+        self.nogil = node.local_scope.nogil
+        if self.nogil and node.nogil_check:
+            node.nogil_check(node.local_scope)
         self.visitchildren(node)
         self.env_stack.pop()
+        self.nogil = was_nogil
         return node
 
     def visit_GILStatNode(self, node):
-        # FIXME: should we do some kind of GIL checking here, too?
-        # if node.gil_check is not None:
-        #     node.gil_check(self.env_stack[-1])
         env = self.env_stack[-1]
-        was_nogil = env.nogil
-        env.nogil = node.state == 'nogil'
+        if self.nogil and node.nogil_check: node.nogil_check()
+        was_nogil = self.nogil
+        self.nogil = (node.state == 'nogil')
         self.visitchildren(node)
-        env.nogil = was_nogil
+        self.nogil = was_nogil
         return node
 
     def visit_Node(self, node):
-        if self.env_stack and node.gil_check is not None:
-            node.gil_check(self.env_stack[-1])
+        if self.env_stack and self.nogil and node.nogil_check:
+            node.nogil_check(self.env_stack[-1])
         self.visitchildren(node)
         return node
 

tests/errors/nogil.pyx

@@ -76,18 +76,26 @@ cdef class C:
 cdef void t(C c) nogil:
     pass
 
+def ticket_338():
+    cdef object obj
+    with nogil:
+        for obj from 0 <= obj < 4:
+            pass
+
+# For m(), the important thing is that there are errors on all lines in the range 23-69
+# except these: 34, 44, 56, 58, 60, 62-64
 
 _ERRORS = u"""
-1: 5: Function with Python return type cannot be declared nogil
-6: 6: Assignment of Python object not allowed without gil
-4: 5: Function declared nogil has Python locals or temporaries
-11: 5: Function with Python return type cannot be declared nogil
-15: 5: Calling gil-requiring function not allowed without gil
-24: 9: Calling gil-requiring function not allowed without gil
+1:5: Function with Python return type cannot be declared nogil
+4:5: Function declared nogil has Python locals or temporaries
+6:6: Assignment of Python object not allowed without gil
+11:5: Function with Python return type cannot be declared nogil
+15:5: Calling gil-requiring function not allowed without gil
+24:9: Calling gil-requiring function not allowed without gil
 26:12: Assignment of Python object not allowed without gil
 28:16: Constructing complex number not allowed without gil
 29:12: Accessing Python global or builtin not allowed without gil
-30: 8: Backquote expression not allowed without gil
+30:8: Backquote expression not allowed without gil
 31:15: Assignment of Python object not allowed without gil
 31:15: Python import not allowed without gil
 32:13: Python import not allowed without gil
@@ -101,31 +109,32 @@ _ERRORS = u"""
 37:15: Converting to Python object not allowed without gil
 37:17: Converting to Python object not allowed without gil
 38:11: Accessing Python attribute not allowed without gil
-39: 9: Constructing Python tuple not allowed without gil
-40: 8: Constructing Python list not allowed without gil
-41: 8: Constructing Python dict not allowed without gil
+39:9: Constructing Python tuple not allowed without gil
+40:8: Constructing Python list not allowed without gil
+41:8: Constructing Python dict not allowed without gil
 42:12: Truth-testing Python object not allowed without gil
 43:13: Python type test not allowed without gil
-#44: 4: Converting to Python object not allowed without gil
 45:10: Operation not allowed without gil
-46: 8: Operation not allowed without gil
+46:8: Operation not allowed without gil
 47:10: Assignment of Python object not allowed without gil
 47:14: Assignment of Python object not allowed without gil
-48: 9: Assignment of Python object not allowed without gil
+48:9: Assignment of Python object not allowed without gil
 48:13: Assignment of Python object not allowed without gil
 48:16: Creating temporary Python reference not allowed without gil
 48:19: Creating temporary Python reference not allowed without gil
-49:11: Indexing Python object not allowed without gil
 49:11: Assignment of Python object not allowed without gil
+49:11: Indexing Python object not allowed without gil
 50:11: Accessing Python attribute not allowed without gil
 50:11: Assignment of Python object not allowed without gil
-51: 8: Constructing Python tuple not allowed without gil
-51: 8: Python print statement not allowed without gil
-52: 8: Deleting Python object not allowed without gil
-53: 8: Returning Python object not allowed without gil
-54: 8: Raising exception not allowed without gil
+51:8: Constructing Python tuple not allowed without gil
+51:8: Python print statement not allowed without gil
+52:8: Deleting Python object not allowed without gil
+53:8: Returning Python object not allowed without gil
+54:8: Raising exception not allowed without gil
 55:14: Truth-testing Python object not allowed without gil
 57:17: Truth-testing Python object not allowed without gil
-61: 8: Try-except statement not allowed without gil
-65: 8: Try-finally statement not allowed without gil
+59:8: For-loop using object bounds or target not allowed without gil
+61:8: Try-except statement not allowed without gil
+65:8: Try-finally statement not allowed without gil
+82:8: For-loop using object bounds or target not allowed without gil
 """

tests/run/nogil.pyx

@@ -20,3 +20,5 @@ cdef int g(int x) nogil:
         cdef int y
         y = x + 42
         return y
+
+