replace the currently broken BoolBinopNode by GenericBoolBinopNode to fix it...

replace the currently broken BoolBinopNode by GenericBoolBinopNode to fix it and generally avoid redundancy

Cython/Compiler/ExprNodes.py

@@ -9638,13 +9638,18 @@ class PowNode(NumBinopNode):
 
 
 class BoolBinopNode(ExprNode):
-    #  Short-circuiting boolean operation.
-    #
-    #  operator     string
-    #  operand1     ExprNode
-    #  operand2     ExprNode
+    """
+    Short-circuiting boolean operation.
 
+    Note that this node provides the same code generation method as
+    BoolBinopResultNode to simplify expression nesting.
+
+    operator  string                              "and"/"or"
+    operand1  BoolBinopNode/BoolBinopResultNode   left operand
+    operand2  BoolBinopNode/BoolBinopResultNode   right operand
+    """
     subexprs = ['operand1', 'operand2']
+    is_temp = True
     operator = None
     operand1 = None
     operand2 = None
@@ -9676,22 +9681,6 @@ class BoolBinopNode(ExprNode):
         else:
             return operand1 or operand2
 
-    def coerce_to_boolean(self, env):
-        return BoolBinopNode.from_node(
-            self,
-            operator=self.operator,
-            operand1=self.operand1.coerce_to_boolean(env).coerce_to_simple(env),
-            operand2=self.operand2.coerce_to_boolean(env).coerce_to_simple(env),
-            type=PyrexTypes.c_bint_type,
-            is_temp=self.is_temp)
-
-    def coerce_to(self, dst_type, env):
-        if dst_type is PyrexTypes.c_bint_type:
-            return self.coerce_to_boolean(env)
-        return GenericBoolBinopNode.from_node(
-            self, env=env, type=dst_type,
-            operator=self.operator, operand1=self.operand1, operand2=self.operand2)
-
     def is_ephemeral(self):
         return self.operand1.is_ephemeral() or self.operand2.is_ephemeral()
 
@@ -9699,48 +9688,52 @@ class BoolBinopNode(ExprNode):
         # Note: we do not do any coercion here as we most likely do not know the final type anyway.
         # We even accept to set self.type to ErrorType if both operands do not have a spanning type.
         # The coercion to the final type and to a "simple" value is left to coerce_to().
-        self.operand1 = self.operand1.analyse_types(env)
-        self.operand2 = self.operand2.analyse_types(env)
+        operand1 = self.operand1.analyse_types(env)
+        operand2 = self.operand2.analyse_types(env)
         self.type = PyrexTypes.independent_spanning_type(
-            self.operand1.type, self.operand2.type)
-        self.is_temp = 1
+            operand1.type, operand2.type)
+        self.operand1 = BoolBinopResultNode(operand1, self.type, env)
+        self.operand2 = BoolBinopResultNode(operand2, self.type, env)
         return self
 
-    gil_message = "Truth-testing Python object"
+    def coerce_to_boolean(self, env):
+        return self.coerce_to(PyrexTypes.c_bint_type, env)
 
-    def check_const(self):
-        return self.operand1.check_const() and self.operand2.check_const()
+    def coerce_to(self, dst_type, env):
+        operand1 = self.operand1.coerce_to(dst_type, env)
+        operand2 = self.operand2.coerce_to(dst_type, env)
+        return BoolBinopNode.from_node(
+            self, type=dst_type,
+            operator=self.operator,
+            operand1=operand1, operand2=operand2)
 
-    def generate_evaluation_code(self, code):
-        if self.type is error_type:
-            # quite clearly, we did *not* coerce to boolean, but both operand types mismatch
-            error(self.pos, "incompatible types in short-circuiting boolean expression not resolved")
+    def generate_bool_evaluation_code(self, code, final_result_temp, and_label, or_label, end_label):
         code.mark_pos(self.pos)
-        self.operand1.generate_evaluation_code(code)
-        test_result, uses_temp = self.generate_operand1_test(code)
+
+        outer_labels = (and_label, or_label)
         if self.operator == 'and':
-            sense = ""
+            my_label = and_label = code.new_label('next_and')
         else:
-            sense = "!"
-        code.putln(
-            "if (%s%s) {" % (
-                sense,
-                test_result))
-        if uses_temp:
-            code.funcstate.release_temp(test_result)
-        self.operand1.generate_disposal_code(code)
-        self.operand2.generate_evaluation_code(code)
+            my_label = or_label = code.new_label('next_or')
+        self.operand1.generate_bool_evaluation_code(code, final_result_temp, and_label, or_label, end_label)
+
+        and_label, or_label = outer_labels
+
+        code.put_label(my_label)
+        self.operand2.generate_bool_evaluation_code(code, final_result_temp, and_label, or_label, end_label)
+
+    def generate_evaluation_code(self, code):
         self.allocate_temp_result(code)
-        self.operand2.make_owned_reference(code)
-        code.putln("%s = %s;" % (self.result(), self.operand2.result()))
-        self.operand2.generate_post_assignment_code(code)
-        self.operand2.free_temps(code)
-        code.putln("} else {")
-        self.operand1.make_owned_reference(code)
-        code.putln("%s = %s;" % (self.result(), self.operand1.result()))
-        self.operand1.generate_post_assignment_code(code)
-        self.operand1.free_temps(code)
-        code.putln("}")
+        or_label = and_label = None
+        end_label = code.new_label('bool_binop_done')
+        self.generate_bool_evaluation_code(code, self.result(), and_label, or_label, end_label)
+        if code.label_used(end_label):
+            code.put_label(end_label)
+
+    gil_message = "Truth-testing Python object"
+
+    def check_const(self):
+        return self.operand1.check_const() and self.operand2.check_const()
 
     def generate_subexpr_disposal_code(self, code):
         pass  # nothing to do here, all done in generate_evaluation_code()
@@ -9770,7 +9763,7 @@ class BoolBinopResultNode(ExprNode):
     of the overall expression to the target type.
 
     Note that this node provides the same code generation method as
-    GenericBoolBinopNode to simplify expression nesting.
+    BoolBinopNode to simplify expression nesting.
 
     arg     ExprNode    the argument to test
     value   ExprNode    the coerced result value node
@@ -9791,8 +9784,15 @@ class BoolBinopResultNode(ExprNode):
             value=CloneNode(arg).coerce_to(result_type, env))
 
     def coerce_to_boolean(self, env):
-        # coercing to simple boolean case after being instantiated => replace by simple coerced result
-        return self.arg.arg.coerce_to_boolean(env)
+        return self.coerce_to(PyrexTypes.c_bint_type, env)
+
+    def coerce_to(self, dst_type, env):
+        # unwrap, coerce, rewrap
+        arg = self.arg.arg
+        if dst_type is PyrexTypes.c_bint_type:
+            arg = arg.coerce_to_boolean(env)
+        # TODO: unwrap more coercion nodes?
+        return BoolBinopResultNode(arg, dst_type, env)
 
     def generate_operand_test(self, code):
         #  Generate code to test the truth of the first operand.
@@ -9852,59 +9852,6 @@ class BoolBinopResultNode(ExprNode):
         self.arg.free_temps(code)
 
 
-class GenericBoolBinopNode(BoolBinopNode):
-    """
-    BoolBinopNode with arbitrary non-bool result type.
-
-    Note that this node provides the same code generation method as
-    BoolBinopResultNode to simplify expression nesting.
-
-    operator  string                                     "and"/"or"
-    operand1  GenericBoolBinopNode/BoolBinopResultNode   left operand
-    operand2  GenericBoolBinopNode/BoolBinopResultNode   right operand
-    """
-    subexprs = ['operand1', 'operand2']
-    is_temp = True
-
-    def __init__(self, pos, env, type, operator, operand1, operand2, **kwargs):
-        super(GenericBoolBinopNode, self).__init__(
-            pos, operator=operator, type=type,
-            operand1=self._wrap_operand(operand1, type, env),
-            operand2=self._wrap_operand(operand2, type, env),
-            **kwargs)
-
-    def _wrap_operand(self, operand, result_type, env):
-        if isinstance(operand, (GenericBoolBinopNode, BoolBinopResultNode)):
-            return operand
-        if isinstance(operand, BoolBinopNode):
-            return operand.coerce_to(result_type, env)
-        else:
-            return BoolBinopResultNode(operand, result_type, env)
-
-    def generate_bool_evaluation_code(self, code, final_result_temp, and_label, or_label, end_label):
-        code.mark_pos(self.pos)
-
-        outer_labels = (and_label, or_label)
-        if self.operator == 'and':
-            my_label = and_label = code.new_label('next_and')
-        else:
-            my_label = or_label = code.new_label('next_or')
-        self.operand1.generate_bool_evaluation_code(code, final_result_temp, and_label, or_label, end_label)
-
-        and_label, or_label = outer_labels
-
-        code.put_label(my_label)
-        self.operand2.generate_bool_evaluation_code(code, final_result_temp, and_label, or_label, end_label)
-
-    def generate_evaluation_code(self, code):
-        self.allocate_temp_result(code)
-        or_label = and_label = None
-        end_label = code.new_label('bool_binop_done')
-        self.generate_bool_evaluation_code(code, self.result(), and_label, or_label, end_label)
-        if code.label_used(end_label):
-            code.put_label(end_label)
-
-
 class CondExprNode(ExprNode):
     #  Short-circuiting conditional expression.
     #

Cython/Compiler/Optimize.py

@@ -862,7 +862,7 @@ class SwitchTransform(Visitor.CythonTransform):
                     elif getattr(cond.operand1, 'entry', None) \
                              and cond.operand1.entry.is_const:
                         return not_in, cond.operand2, [cond.operand1]
-        elif isinstance(cond, (ExprNodes.BoolBinopNode, ExprNodes.GenericBoolBinopNode)):
+        elif isinstance(cond, (ExprNodes.BoolBinopNode, ExprNodes.BoolBinopNode)):
             if cond.operator == 'or' or (allow_not_in and cond.operator == 'and'):
                 allow_not_in = (cond.operator == 'and')
                 not_in_1, t1, c1 = self.extract_conditions(cond.operand1, allow_not_in)

tests/run/constant_folding.py

@@ -390,8 +390,8 @@ def combined():
     '//IntNode[@value = "4"]',
     '//IntNode[@value = "5"]',
     '//IntNode[@value = "7"]',
-    '//GenericBoolBinopNode//PrimaryCmpNode',
-    '//GenericBoolBinopNode[.//PrimaryCmpNode//IntNode[@value = "4"] and .//PrimaryCmpNode//IntNode[@value = "5"]]',
+    '//BoolBinopNode//PrimaryCmpNode',
+    '//BoolBinopNode[.//PrimaryCmpNode//IntNode[@value = "4"] and .//PrimaryCmpNode//IntNode[@value = "5"]]',
     '//PrimaryCmpNode[.//IntNode[@value = "2"] and .//IntNode[@value = "4"]]',
     '//PrimaryCmpNode[.//IntNode[@value = "5"] and .//IntNode[@value = "7"]]',
 )
@@ -423,11 +423,11 @@ def cascaded_cmp_with_partial_constants(a, b):
     '//IntNode[@value = "4"]',
     '//IntNode[@value = "5"]',
     '//IntNode[@value = "7"]',
-    '//GenericBoolBinopNode',
-    '//SingleAssignmentNode//GenericBoolBinopNode',
-    '//SingleAssignmentNode//GenericBoolBinopNode//NameNode[@name = "a"]',
-    '//SingleAssignmentNode//GenericBoolBinopNode//NameNode[@name = "b"]',
-    '//GenericBoolBinopNode[.//PrimaryCmpNode//IntNode[@value = "4"] and .//PrimaryCmpNode//IntNode[@value = "5"]]',
+    '//BoolBinopNode',
+    '//SingleAssignmentNode//BoolBinopNode',
+    '//SingleAssignmentNode//BoolBinopNode//NameNode[@name = "a"]',
+    '//SingleAssignmentNode//BoolBinopNode//NameNode[@name = "b"]',
+    '//BoolBinopNode[.//PrimaryCmpNode//IntNode[@value = "4"] and .//PrimaryCmpNode//IntNode[@value = "5"]]',
     '//BoolNode[@value = False]',
 )
 @cython.test_fail_if_path_exists(

tests/run/generator_expressions.pyx

@@ -47,3 +47,13 @@ def genexpr_of_lambdas(int N):
     [(0, 0), (1, 2), (2, 4), (3, 6), (4, 8)]
     """
     return ( ((lambda : x), (lambda : x*2)) for x in range(N) )
+
+
+def genexpr_with_bool_binop(values):
+    """
+    >>> values = [(1, 2, 3), (None, 4, None), (5, None, 6)]
+    >>> genexpr_with_bool_binop(values)
+    [(1, 2, 3), ('X', 4, 'X'), (5, 'X', 6)]
+    """
+    # copied from CPython's test_itertools.py
+    return [tuple((e is None and 'X' or e) for e in t) for t in values]

tests/run/notinop.pyx

@@ -83,7 +83,7 @@ def m_tuple(int a):
     return result
 
 @cython.test_assert_path_exists("//SwitchStatNode")
-@cython.test_fail_if_path_exists("//BoolBinopNode", "//GenericBoolBinopNode", "//PrimaryCmpNode")
+@cython.test_fail_if_path_exists("//BoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
 def m_set(int a):
     """
     >>> m_set(2)
@@ -97,7 +97,7 @@ def m_set(int a):
 cdef bytes bytes_string = b'abcdefg'
 
 @cython.test_assert_path_exists("//PrimaryCmpNode")
-@cython.test_fail_if_path_exists("//SwitchStatNode", "//BoolBinopNode", "//GenericBoolBinopNode")
+@cython.test_fail_if_path_exists("//SwitchStatNode", "//BoolBinopNode", "//BoolBinopNode")
 def m_bytes(char a):
     """
     >>> m_bytes(ord('f'))
@@ -109,7 +109,7 @@ def m_bytes(char a):
     return result
 
 @cython.test_assert_path_exists("//SwitchStatNode")
-@cython.test_fail_if_path_exists("//BoolBinopNode", "//GenericBoolBinopNode", "//PrimaryCmpNode")
+@cython.test_fail_if_path_exists("//BoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
 def m_bytes_literal(char a):
     """
     >>> m_bytes_literal(ord('f'))
@@ -127,7 +127,7 @@ cdef unicode klingon_character = u'\uF8D2'
 py_klingon_character = klingon_character
 
 @cython.test_assert_path_exists("//PrimaryCmpNode")
-@cython.test_fail_if_path_exists("//SwitchStatNode", "//GenericBoolBinopNode", "//BoolBinopNode")
+@cython.test_fail_if_path_exists("//SwitchStatNode", "//BoolBinopNode", "//BoolBinopNode")
 def m_unicode(Py_UNICODE a, unicode unicode_string):
     """
     >>> m_unicode(ord('f'), py_unicode_string)
@@ -147,7 +147,7 @@ def m_unicode(Py_UNICODE a, unicode unicode_string):
     return result
 
 @cython.test_assert_path_exists("//SwitchStatNode")
-@cython.test_fail_if_path_exists("//GenericBoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
+@cython.test_fail_if_path_exists("//BoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
 def m_unicode_literal(Py_UNICODE a):
     """
     >>> m_unicode_literal(ord('f'))
@@ -160,7 +160,7 @@ def m_unicode_literal(Py_UNICODE a):
     cdef int result = a not in u'abcdefg\u1234\uF8D2'
     return result
 
-@cython.test_assert_path_exists("//SwitchStatNode", "//GenericBoolBinopNode")
+@cython.test_assert_path_exists("//SwitchStatNode", "//BoolBinopNode")
 @cython.test_fail_if_path_exists("//PrimaryCmpNode")
 def m_tuple_in_or_notin(int a):
     """
@@ -174,7 +174,7 @@ def m_tuple_in_or_notin(int a):
     cdef int result = a not in (1,2,3,4) or a in (3,4)
     return result
 
-@cython.test_assert_path_exists("//SwitchStatNode", "//GenericBoolBinopNode")
+@cython.test_assert_path_exists("//SwitchStatNode", "//BoolBinopNode")
 @cython.test_fail_if_path_exists("//PrimaryCmpNode")
 def m_tuple_notin_or_notin(int a):
     """
@@ -189,7 +189,7 @@ def m_tuple_notin_or_notin(int a):
     return result
 
 @cython.test_assert_path_exists("//SwitchStatNode")
-@cython.test_fail_if_path_exists("//GenericBoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
+@cython.test_fail_if_path_exists("//BoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
 def m_tuple_notin_and_notin(int a):
     """
     >>> m_tuple_notin_and_notin(2)
@@ -202,7 +202,7 @@ def m_tuple_notin_and_notin(int a):
     cdef int result = a not in (1,2,3,4) and a not in (6,7)
     return result
 
-@cython.test_assert_path_exists("//SwitchStatNode", "//GenericBoolBinopNode")
+@cython.test_assert_path_exists("//SwitchStatNode", "//BoolBinopNode")
 @cython.test_fail_if_path_exists("//PrimaryCmpNode")
 def m_tuple_notin_and_notin_overlap(int a):
     """
@@ -217,7 +217,7 @@ def m_tuple_notin_and_notin_overlap(int a):
     return result
 
 @cython.test_assert_path_exists("//SwitchStatNode")
-@cython.test_fail_if_path_exists("//GenericBoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
+@cython.test_fail_if_path_exists("//BoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
 def conditional_int(int a):
     """
     >>> conditional_int(1)
@@ -230,7 +230,7 @@ def conditional_int(int a):
     return 1 if a not in (1,2,3,4) else 2
 
 @cython.test_assert_path_exists("//SwitchStatNode")
-@cython.test_fail_if_path_exists("//GenericBoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
+@cython.test_fail_if_path_exists("//BoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
 def conditional_object(int a):
     """
     >>> conditional_object(1)
@@ -243,7 +243,7 @@ def conditional_object(int a):
     return 1 if a not in (1,2,3,4) else '2'
 
 @cython.test_assert_path_exists("//SwitchStatNode")
-@cython.test_fail_if_path_exists("//GenericBoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
+@cython.test_fail_if_path_exists("//BoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
 def conditional_bytes(char a):
     """
     >>> conditional_bytes(ord('a'))
@@ -256,7 +256,7 @@ def conditional_bytes(char a):
     return 1 if a not in b'abc' else '2'
 
 @cython.test_assert_path_exists("//SwitchStatNode")
-@cython.test_fail_if_path_exists("//GenericBoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
+@cython.test_fail_if_path_exists("//BoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
 def conditional_unicode(Py_UNICODE a):
     """
     >>> conditional_unicode(ord('a'))
@@ -269,7 +269,7 @@ def conditional_unicode(Py_UNICODE a):
     return 1 if a not in u'abc' else '2'
 
 @cython.test_assert_path_exists("//SwitchStatNode")
-@cython.test_fail_if_path_exists("//GenericBoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
+@cython.test_fail_if_path_exists("//BoolBinopNode", "//BoolBinopNode", "//PrimaryCmpNode")
 def conditional_none(int a):
     """
     >>> conditional_none(1)