Repository: cython/cython
Pull request: Infer the result type of builtin numeric binary operations
See https://github.com/cython/cython/issues/4237
Closes https://github.com/cython/cython/issues/5743

Problem statement:
Replace `coerce_to_integer()` with `coerce_to_index()`
### Describe your issue

We use `coerce_to_integer()` in several places where we intend to convert a (Python) value to a C integer index value. However, the method coerces to C `long` internally and thus risks an incomplete value range on Windows (where `long` is 32 bits).

It feels wrong to have such a generic method in the first place since we normally have a specific C integer type that we need to coerce to.

Instead, the usages suggest to add a `coerce_to_index()` method that coerces to `Py_ssize_t`, and to leave the remaining "just need a C integer" cases to the normal `coerce_to()` method.

Hints from issue discussion:

Commit message: Set up rules for inferring the result type of builtin numeric binary operations.

See https://github.com/cython/cython/issues/4237

Patch to review:
diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index c462a03d33a..dcc960e1794 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -11840,6 +11840,12 @@ def compute_c_result_type(self, type1, type2):
         else:
             return None
 
+    def infer_builtin_types_operation(self, type1, type2):
+        if type1.is_builtin_type:
+            return PyrexTypes.result_type_of_builtin_operation(type1, type2)
+        else:
+            return PyrexTypes.result_type_of_builtin_operation(type2, type1)
+
     def may_be_none(self):
         if self.type and self.type.is_builtin_type:
             # if we know the result type, we know the operation, so it can't be None
@@ -11961,7 +11967,7 @@ def infer_builtin_types_operation(self, type1, type2):
         if type1 in string_types and type2 in string_types:
             return string_types[max(string_types.index(type1),
                                     string_types.index(type2))]
-        return None
+        return super().infer_builtin_types_operation(type1, type2)
 
     def compute_c_result_type(self, type1, type2):
         #print "AddNode.compute_c_result_type:", type1, self.operator, type2 ###
@@ -12098,7 +12104,7 @@ def infer_builtin_types_operation(self, type1, type2):
             return type2
         if type2.is_int:
             return type1
-        return None
+        return super().infer_builtin_types_operation(type1, type2)
 
 
 class MatMultNode(NumBinopNode):
@@ -12107,6 +12113,10 @@ class MatMultNode(NumBinopNode):
     def is_py_operation_types(self, type1, type2):
         return True
 
+    def infer_builtin_types_operation(self, type1, type2):
+        # We really don't know anything about this operation.
+        return None
+
     def generate_evaluation_code(self, code):
         code.globalstate.use_utility_code(UtilityCode.load_cached("MatrixMultiply", "ObjectHandling.c"))
         super().generate_evaluation_code(code)
@@ -12159,6 +12169,16 @@ def infer_type(self, env):
             self.operand1.infer_type(env),
             self.operand2.infer_type(env), env)
 
+    def infer_builtin_types_operation(self, type1, type2):
+        result_type = super().infer_builtin_types_operation(type1, type2)
+        if result_type is not None and self.operator == '/' and (self.truedivision or self.ctruedivision):
+            # Result of truedivision is not an integer
+            if result_type is Builtin.int_type:
+                return Builtin.float_type
+            elif result_type.is_int:
+                return PyrexTypes.widest_numeric_type(PyrexTypes.c_double_type, result_type)
+        return result_type
+
     def analyse_operation(self, env):
         self._check_truedivision(env)
         NumBinopNode.analyse_operation(self, env)
@@ -12330,7 +12350,7 @@ def infer_builtin_types_operation(self, type1, type2):
                 return None   # RHS might implement '% operator differently in Py3
             else:
                 return basestring_type  # either str or unicode, can't tell
-        return None
+        return super().infer_builtin_types_operation(type1, type2)
 
     def zero_division_message(self):
         if self.type.is_int:
@@ -12416,6 +12436,10 @@ def analyse_types(self, env):
         self._check_cpow(env)
         return super().analyse_types(env)
 
+    def infer_builtin_types_operation(self, type1, type2):
+        # TODO
+        return None
+
     def analyse_c_operation(self, env):
         NumBinopNode.analyse_c_operation(self, env)
         if self.type.is_complex:
diff --git a/Cython/Compiler/PyrexTypes.py b/Cython/Compiler/PyrexTypes.py
index f3a4c8ca293..034a9d69d1b 100644
--- a/Cython/Compiler/PyrexTypes.py
+++ b/Cython/Compiler/PyrexTypes.py
@@ -5161,6 +5161,36 @@ def real_type(ntype):
     return widest_type
 
 
+
+def result_type_of_builtin_operation(builtin_type, type2):
+    """
+    Try to find a suitable (C) result type for a binary operation with a known builtin type.
+    """
+    if builtin_type.name == 'float':
+        if type2.is_numeric:
+            return widest_numeric_type(c_double_type, type2)
+        elif type2.is_builtin_type and type2.name == 'int':
+            return c_double_type
+        elif type2.is_builtin_type and type2.name == 'complex':
+            return type2
+    elif builtin_type.name == 'int':
+        if type2.is_int:
+            return builtin_type
+        elif type2.is_float or type2.is_builtin_type and type2.name == 'float':
+            return c_double_type
+        elif type2.is_builtin_type and type2.name == 'complex':
+            return type2
+    elif builtin_type.name == 'complex':
+        if type2.is_complex:
+            return CComplexType(widest_numeric_type(c_double_type, type2.real_type))
+        elif type2.is_numeric:
+            return CComplexType(widest_numeric_type(c_double_type, type2))
+        elif type2.is_builtin_type and type2.name in ('int', 'float', 'complex'):
+            return CComplexType(c_double_type)
+
+    return None
+
+
 def numeric_type_fits(small_type, large_type):
     return widest_numeric_type(small_type, large_type) == large_type
 
@@ -5213,13 +5243,14 @@ def spanning_type(type1, type2):
         return py_object_type
     return span_type
 
+
 def _spanning_type(type1, type2):
     if type1.is_numeric and type2.is_numeric:
         return widest_numeric_type(type1, type2)
-    elif type1.is_builtin_type and type1.name == 'float' and type2.is_numeric:
-        return widest_numeric_type(c_double_type, type2)
-    elif type2.is_builtin_type and type2.name == 'float' and type1.is_numeric:
-        return widest_numeric_type(type1, c_double_type)
+    elif type1.is_builtin_type:
+        return result_type_of_builtin_operation(type1, type2) or py_object_type
+    elif type2.is_builtin_type:
+        return result_type_of_builtin_operation(type2, type1) or py_object_type
     elif type1.is_extension_type and type2.is_extension_type:
         return widest_extension_type(type1, type2)
     elif type1.is_pyobject or type2.is_pyobject:


Code review comments:
diff_hunk: @@ -5161,6 +5161,36 @@ def real_type(ntype):
     return widest_type
 
 
+
+def result_type_of_builtin_operation(builtin_type, type2):
+    """
+    Try to find a suitable (C) result type for a binary operation with a known builtin type.
+    """
+    if builtin_type.name == 'float':
+        if type2.is_numeric:
+            return widest_numeric_type(c_double_type, type2)
+        elif type2.is_builtin_type and type2.name == 'int':
path: Cython/Compiler/PyrexTypes.py
text: @user1:
This feels like it goes against the general rule of writing things like `if type2 is Builtin.int_type`. I assume this is just to avoid circular dependencies?