merge

CHANGES.rst

@@ -13,6 +13,10 @@ Features added
 Bugs fixed
 ----------
 
+* C-to-Python type coercions during cascaded comparisons could generate invalid C code, specifically when using the 'in' operator.
+
+* "obj[1,]" passed a single integer into the item getter instead of a tuple.
+
 * Cyclic imports at module init time did not work in Py3.
 
 * The names of C++ destructors for template classes were built incorrectly.

Cython/Compiler/ExprNodes.py

@@ -8850,9 +8850,10 @@ class PrimaryCmpNode(ExprNode, CmpNode):
     #  Instead, we override all the framework methods
     #  which use it.
 
-    child_attrs = ['operand1', 'operand2', 'cascade']
+    child_attrs = ['operand1', 'operand2', 'coerced_operand2', 'cascade']
 
     cascade = None
+    coerced_operand2 = None
     is_memslice_nonecheck = False
 
     def infer_type(self, env):
@@ -8930,9 +8931,11 @@ class PrimaryCmpNode(ExprNode, CmpNode):
             self.coerce_operands_to(common_type, env)
 
         if self.cascade:
-            self.operand2 = self.cascade.optimise_comparison(
-                self.operand2.coerce_to_simple(env), env)
+            self.operand2 = self.operand2.coerce_to_simple(env)
             self.cascade.coerce_cascaded_operands_to_temp(env)
+            operand2 = self.cascade.optimise_comparison(self.operand2, env)
+            if operand2 is not self.operand2:
+                self.coerced_operand2 = operand2
         if self.is_python_result():
             self.type = PyrexTypes.py_object_type
         else:
@@ -9036,8 +9039,9 @@ class PrimaryCmpNode(ExprNode, CmpNode):
             self.generate_operation_code(code, self.result(),
                 self.operand1, self.operator, self.operand2)
             if self.cascade:
-                self.cascade.generate_evaluation_code(code,
-                    self.result(), self.operand2)
+                self.cascade.generate_evaluation_code(
+                    code, self.result(), self.coerced_operand2 or self.operand2,
+                    needs_evaluation=self.coerced_operand2 is not None)
             self.operand1.generate_disposal_code(code)
             self.operand1.free_temps(code)
             self.operand2.generate_disposal_code(code)
@@ -9072,9 +9076,10 @@ class CascadedCmpNode(Node, CmpNode):
     #  operand2      ExprNode
     #  cascade       CascadedCmpNode
 
-    child_attrs = ['operand2', 'cascade']
+    child_attrs = ['operand2', 'coerced_operand2', 'cascade']
 
     cascade = None
+    coerced_operand2 = None
     constant_result = constant_value_not_set # FIXME: where to calculate this?
 
     def infer_type(self, env):
@@ -9101,7 +9106,9 @@ class CascadedCmpNode(Node, CmpNode):
             if not operand1.type.is_pyobject:
                 operand1 = operand1.coerce_to_pyobject(env)
         if self.cascade:
-            self.operand2 = self.cascade.optimise_comparison(self.operand2, env)
+            operand2 = self.cascade.optimise_comparison(self.operand2, env)
+            if operand2 is not self.operand2:
+                self.coerced_operand2 = operand2
         return operand1
 
     def coerce_operands_to_pyobjects(self, env):
@@ -9117,18 +9124,24 @@ class CascadedCmpNode(Node, CmpNode):
             self.operand2 = self.operand2.coerce_to_simple(env)
             self.cascade.coerce_cascaded_operands_to_temp(env)
 
-    def generate_evaluation_code(self, code, result, operand1):
+    def generate_evaluation_code(self, code, result, operand1, needs_evaluation=False):
         if self.type.is_pyobject:
             code.putln("if (__Pyx_PyObject_IsTrue(%s)) {" % result)
             code.put_decref(result, self.type)
         else:
             code.putln("if (%s) {" % result)
+        if needs_evaluation:
+            operand1.generate_evaluation_code(code)
         self.operand2.generate_evaluation_code(code)
         self.generate_operation_code(code, result,
             operand1, self.operator, self.operand2)
         if self.cascade:
             self.cascade.generate_evaluation_code(
-                code, result, self.operand2)
+                code, result, self.coerced_operand2 or self.operand2,
+                needs_evaluation=self.coerced_operand2 is not None)
+        if needs_evaluation:
+            operand1.generate_disposal_code(code)
+            operand1.free_temps(code)
         # Cascaded cmp result is always temp
         self.operand2.generate_disposal_code(code)
         self.operand2.free_temps(code)

Cython/Compiler/Parsing.pxd

@@ -49,7 +49,7 @@ cpdef p_call_parse_args(PyrexScanner s, bint allow_genexp = *)
 cdef p_call_build_packed_args(pos, positional_args, keyword_args, star_arg, starstar_arg)
 cdef p_call(PyrexScanner s, function)
 cdef p_index(PyrexScanner s, base)
-cdef p_subscript_list(PyrexScanner s)
+cdef tuple p_subscript_list(PyrexScanner s)
 cdef p_subscript(PyrexScanner s)
 cdef p_slice_element(PyrexScanner s, follow_set)
 cdef expect_ellipsis(PyrexScanner s)

Cython/Compiler/Parsing.py

@@ -503,14 +503,14 @@ def p_index(s, base):
     # s.sy == '['
     pos = s.position()
     s.next()
-    subscripts = p_subscript_list(s)
-    if len(subscripts) == 1 and len(subscripts[0]) == 2:
+    subscripts, is_single_value = p_subscript_list(s)
+    if is_single_value and len(subscripts[0]) == 2:
         start, stop = subscripts[0]
         result = ExprNodes.SliceIndexNode(pos,
             base = base, start = start, stop = stop)
     else:
         indexes = make_slice_nodes(pos, subscripts)
-        if len(indexes) == 1:
+        if is_single_value:
             index = indexes[0]
         else:
             index = ExprNodes.TupleNode(pos, args = indexes)
@@ -520,13 +520,15 @@ def p_index(s, base):
     return result
 
 def p_subscript_list(s):
+    is_single_value = True
     items = [p_subscript(s)]
     while s.sy == ',':
+        is_single_value = False
         s.next()
         if s.sy == ']':
             break
         items.append(p_subscript(s))
-    return items
+    return items, is_single_value
 
 #subscript: '.' '.' '.' | test | [test] ':' [test] [':' [test]]
 
@@ -2112,7 +2114,7 @@ def p_memoryviewslice_access(s, base_type_node):
     # s.sy == '['
     pos = s.position()
     s.next()
-    subscripts = p_subscript_list(s)
+    subscripts, _ = p_subscript_list(s)
     # make sure each entry in subscripts is a slice
     for subscript in subscripts:
         if len(subscript) < 2:

tests/run/cmp.pyx

@@ -38,6 +38,28 @@ def cascaded_c(double a, double b, double c):
     """
     return a < b < c
 
+def cascaded_mix_pyleft(a, double b, double c):
+    """
+    >>> cascaded_mix_pyleft(1, 2, 3)
+    True
+    >>> cascaded_mix_pyleft(1, 2, -1)
+    False
+    >>> cascaded_mix_pyleft(10, 2, 3)
+    False
+    """
+    return a < b < c
+
+def cascaded_mix_pyright(double a, double b, c):
+    """
+    >>> cascaded_mix_pyright(1, 2, 3)
+    True
+    >>> cascaded_mix_pyright(1, 2, -1)
+    False
+    >>> cascaded_mix_pyright(10, 2, 3)
+    False
+    """
+    return a < b < c
+
 def typed_cmp(list L):
     """
     >>> typed_cmp([1,2,3])

tests/run/dict_getitem.pyx

@@ -30,6 +30,14 @@ def test(dict d, index):
     """
     return d[index]
 
+def getitem_tuple(dict d, index):
+    """
+    >>> d = {1: 1, (1,): 2}
+    >>> getitem_tuple(d, 1)
+    (1, 2)
+    """
+    return d[index], d[index,]
+
 def getitem_in_condition(dict d, key, expected_result):
     """
     >>> d = dict(a=1, b=2)

tests/run/inop.pyx

@@ -376,3 +376,52 @@ def test_inop_cascaded(x):
     False
     """
     return 1 != x in [2]
+
+### The following tests are copied from CPython's test_grammar.py.
+### They look stupid, but the nice thing about them is that Cython
+### treats '1' as a C integer constant that triggers Python object
+### coercion for the 'in' operator here, whereas the left side of
+### the cascade can be evaluated entirely in C space.
+
+def test_inop_cascaded_one():
+    """
+    >>> test_inop_cascaded_one()
+    False
+    """
+    return 1 < 1 > 1 == 1 >= 1 <= 1 != 1 in 1 not in 1 is 1 is not 1
+
+def test_inop_cascaded_int_orig(int x):
+    """
+    >>> test_inop_cascaded_int_orig(1)
+    False
+    """
+    return 1 < 1 > 1 == 1 >= 1 <= 1 != x in 1 not in 1 is 1 is not 1
+
+def test_inop_cascaded_one_err():
+    """
+    >>> test_inop_cascaded_one_err()   # doctest: +ELLIPSIS
+    Traceback (most recent call last):
+    TypeError:... itera...
+    """
+    return 1 == 1 >= 1 <= 1 in 1 not in 1 is 1 is not 1
+
+def test_inop_cascaded_int_orig_err(int x):
+    """
+    >>> test_inop_cascaded_int_orig_err(1)   # doctest: +ELLIPSIS
+    Traceback (most recent call last):
+    TypeError:... itera...
+    """
+    return 1 == 1 >= 1 <= 1 == x in 1 not in 1 is 1 is not 1
+
+###
+
+def test_inop_cascaded_int(int x):
+    """
+    >>> test_inop_cascaded_int(1)
+    False
+    >>> test_inop_cascaded_int(2)
+    True
+    >>> test_inop_cascaded_int(3)
+    False
+    """
+    return 1 != x in [1,2]