Repository: cython/cython
Pull request: Avoid using strings for exception_value of func_type
The main advantage is that we don't end up hugely sensitive to the exact string representation of a particular number.

Fixes #5709

Problem statement:
[BUG] values used for exception_value are really inconsistent
### Describe the bug

This is something I'm running into while trying to sort out implicit exception_value for fused types. But it's a general problem not related to fused functions.

Essentially for floating point types, Cython uses a number of different representations of the exception value, and this leads to mismatches where there should be no mismatch

### Code to reproduce the behaviour:

```cython
cdef double return_double(double arg, bint fail):
    if fail:
        raise RuntimeError
    return arg

ctypedef double (*func_ptr1)(double, bint) except? -1

def test():
    # Test that we can assign to the function pointer we expect
    cdef func_ptr1 p1 = return_double
```

This fails with

```
Error compiling Cython file:
------------------------------------------------------------
...

ctypedef double (*func_ptr1)(double, bint) except? -1

def test():
    # Test that we can assign to the function pointer we expect
    cdef func_ptr1 p1 = return_double
                        ^
------------------------------------------------------------

fused_rettype3.pyx:10:24: Cannot assign type 'double (double, bool) except? -1' to 'func_ptr1'. Exception values are incompatible.
```

The reason is that the type for `return_double` has an `exception_value` of `'-1'` (note a `str`).
The type for `func_ptr1` has an `exception_value` of `'-1.0'` (note also a `str`).

Therefore they're "incompatible" and the assignment fails.

### Expected behaviour

_No response_

### OS

_No response_

### Python version

_No response_

### Cython version

Current master

### Additional context

I think the mistake is using strings to represent `exception_value`

Hints from issue discussion:
> I think the mistake is using strings to represent `exception_value`

Not entirely a mistake. You can have an exception value that is an enum name, for example. So, if we start using numeric values, we'll still have to deal with strings, but then in combination with different numeric data types.

Commit message: Avoid boolnode

Patch to review:
diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index 28daa01faca..dd075e016fb 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -610,6 +610,25 @@ def compile_time_value_error(self, e):
         error(self.pos, "Error in compile-time expression: %s: %s" % (
             e.__class__.__name__, e))
 
+    def as_exception_value(self, env):
+        # Return the compile_time_value if possible.
+        # This can be either a Python constant or a string
+        # for types that can't be represented by a Python constant
+        # (e.g. enums)
+        result = None
+        with local_errors(ignore=True):
+            result = self.compile_time_value(env)
+        if isinstance(result, Symtab.Entry):
+            result = result.cname
+        if result is None:
+            # this isn't the preferred fallback because it can end up with
+            # hard to distinguish between identical types, e.g. -1.0 vs -1
+            # for floats. However, it lets things like NULL and typecasts work
+            result = self.get_constant_c_result_code()
+        if result is not None:
+            return result
+        error(self.pos, "Exception value must be constant")
+
     # ------------- Declaration Analysis ----------------
 
     def analyse_target_declaration(self, env):
@@ -1311,6 +1330,29 @@ def calculate_result_code(self):
     def generate_result_code(self, code):
         pass
 
+    @staticmethod
+    def for_type(pos, value, type):
+        cls = ConstNode
+        if type is PyrexTypes.c_null_ptr_type or (
+                (value == "NULL" or value == 0) and type.is_ptr):
+            return NullNode(pos)  # value and type are preset here
+        # char node is deliberately skipped and treated as IntNode
+        elif type.is_int or PyrexTypes.c_bint_type:
+            # use this instead of BoolNode for c_bint_type because
+            # BoolNode handles values differently to most other ConstNode
+            # derivatives (they aren't strings).
+            cls = IntNode
+        elif type.is_float:
+            cls = FloatNode
+        elif type is bytes_type:
+            cls = BytesNode
+        elif type is unicode_type:
+            cls = UnicodeNode
+        if cls.type is type:
+            return cls(pos, value=value)
+        else:
+            return cls(pos, value=value, type=type)
+
 
 class BoolNode(ConstNode):
     type = PyrexTypes.c_bint_type
@@ -6385,7 +6427,7 @@ def c_call_code(self):
 
     def is_c_result_required(self):
         func_type = self.function_type()
-        if not func_type.exception_value or func_type.exception_check == '+':
+        if func_type.exception_value is None or func_type.exception_check == '+':
             return False  # skip allocation of unused result temp
         return True
 
diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py
index 6918082c133..111ad7673c3 100644
--- a/Cython/Compiler/Nodes.py
+++ b/Cython/Compiler/Nodes.py
@@ -740,8 +740,8 @@ def analyse(self, return_type, env, nonempty=0, directive_locals=None, visibilit
                     #   for now.
                     if not env.is_c_class_scope and not isinstance(self.base, CPtrDeclaratorNode):
                         from .ExprNodes import ConstNode
-                        self.exception_value = ConstNode(
-                            self.pos, value=return_type.exception_value, type=return_type)
+                        self.exception_value = ConstNode.for_type(
+                            self.pos, value=str(return_type.exception_value), type=return_type)
             if self.exception_value:
                 if self.exception_check == '+':
                     self.exception_value = self.exception_value.analyse_const_expression(env)
@@ -759,9 +759,7 @@ def analyse(self, return_type, env, nonempty=0, directive_locals=None, visibilit
                 else:
                     self.exception_value = self.exception_value.analyse_types(env).coerce_to(
                         return_type, env).analyse_const_expression(env)
-                    exc_val = self.exception_value.get_constant_c_result_code()
-                    if exc_val is None:
-                        error(self.exception_value.pos, "Exception value must be constant")
+                    exc_val = self.exception_value.as_exception_value(env)
                     if not return_type.assignable_from(self.exception_value.type):
                         error(self.exception_value.pos,
                               "Exception value incompatible with function return type")
@@ -2654,7 +2652,7 @@ def analyse_declarations(self, env):
                   "Function with optional arguments may not be declared public or api")
 
         if typ.exception_check == '+' and self.visibility != 'extern':
-            if typ.exception_value and typ.exception_value.is_name:
+            if typ.exception_value is not None and typ.exception_value.is_name:
                 # it really is impossible to reason about what the user wants to happens
                 # if they've specified a C++ exception translation function. Therefore,
                 # raise an error.
@@ -3125,8 +3123,8 @@ def as_cfunction(self, cfunc=None, scope=None, overridable=True, returns=None, e
 
         if exception_value is None and cfunc_type.exception_value is not None:
             from .ExprNodes import ConstNode
-            exception_value = ConstNode(
-                self.pos, value=cfunc_type.exception_value, type=cfunc_type.return_type)
+            exception_value = ConstNode.for_type(
+                self.pos, value=str(cfunc_type.exception_value), type=cfunc_type.return_type)
         declarator = CFuncDeclaratorNode(self.pos,
                                          base=CNameDeclaratorNode(self.pos, name=self.name, cname=None),
                                          args=self.args,
diff --git a/Cython/Compiler/PyrexTypes.py b/Cython/Compiler/PyrexTypes.py
index 3616b61f255..27272382820 100644
--- a/Cython/Compiler/PyrexTypes.py
+++ b/Cython/Compiler/PyrexTypes.py
@@ -608,7 +608,7 @@ def overflow_check_binop(self, binop, env, const_rhs=False):
 
     def error_condition(self, result_code):
         if self.typedef_is_external:
-            if self.exception_value:
+            if self.exception_value is not None:
                 condition = "(%s == %s)" % (
                     result_code, self.cast_code(self.exception_value))
                 if self.exception_check:
@@ -2911,7 +2911,7 @@ def assignment_failure_extra_info(self, src_type):
             if self.base_type.pointer_assignable_from_resolved_type(copied_src_type):
                 # the only reason we can't assign is because of exception incompatibility
                 msg = "Exception values are incompatible."
-                if not self.base_type.exception_check and not self.base_type.exception_value:
+                if not self.base_type.exception_check and self.base_type.exception_value is None:
                     msg += " Suggest adding 'noexcept' to type '{}'.".format(src_type)
                 return msg
         return super(CPtrType, self).assignment_failure_extra_info(src_type)
@@ -3070,7 +3070,7 @@ def __repr__(self):
         arg_reprs = list(map(repr, self.args))
         if self.has_varargs:
             arg_reprs.append("...")
-        if self.exception_value:
+        if self.exception_value is not None:
             except_clause = " %r" % self.exception_value
         else:
             except_clause = ""
@@ -3298,11 +3298,11 @@ def declaration_code(self, entity_code,
             arg_decl_code = "void"
         trailer = ""
         if (pyrex or for_display) and not self.return_type.is_pyobject:
-            if self.exception_value and self.exception_check:
+            if self.exception_value is not None and self.exception_check:
                 trailer = " except? %s" % self.exception_value
-            elif self.exception_value and not self.exception_check:
+            elif self.exception_value is not None and not self.exception_check:
                 trailer = " except %s" % self.exception_value
-            elif not self.exception_value and not self.exception_check:
+            elif self.exception_value is None and not self.exception_check:
                 trailer = " noexcept"
             elif self.exception_check == '+':
                 trailer = " except +"
@@ -3504,7 +3504,7 @@ def __init__(self, arg_name, arg_type):
             except_clause = 'except *'
         elif self.return_type.is_pyobject:
             except_clause = ''
-        elif self.exception_value:
+        elif self.exception_value is not None:
             except_clause = ('except? %s' if self.exception_check else 'except %s') % self.exception_value
         else:
             except_clause = 'except *'
diff --git a/tests/run/exceptionpropagation.pyx b/tests/run/exceptionpropagation.pyx
index 40dfdd37d30..d1cad757348 100644
--- a/tests/run/exceptionpropagation.pyx
+++ b/tests/run/exceptionpropagation.pyx
@@ -133,3 +133,80 @@ def test_ptr_func2(int failure_mode):
             print("NULL")
     except RuntimeError:
         print("exception")
+
+cdef double return_double(double arg, bint fail):
+    if fail:
+        raise RuntimeError
+    return arg
+
+ctypedef double (*func_ptr1)(double, bint) except? -1
+
+def test_return_double(fail):
+    """
+    >>> test_return_double(False)
+    2.0
+    >>> test_return_double(True)
+    exception
+    """
+    # Test that we can assign to the function pointer we expect
+    # https://github.com/cython/cython/issues/5709 - representation of the "-1" was fragile
+    cdef func_ptr1 p1 = return_double
+    try:
+        return p1(2.0, fail)
+    except RuntimeError:
+        print("exception")
+
+cdef enum E:
+    E1
+    E2
+
+cdef E return_enum1(bint fail) except? E.E1:
+    if fail:
+        raise RuntimeError
+    return E.E2
+
+cdef E return_enum2(bint fail) except? E1:
+    if fail:
+        raise RuntimeError
+    return E.E2
+
+def test_enum1(fail):
+    """
+    >>> test_enum1(False)
+    True
+    >>> test_enum1(True)
+    exception
+    """
+    try:
+        return return_enum1(fail) == E2
+    except RuntimeError:
+        print("exception")
+
+def test_enum2(fail):
+    """
+    >>> test_enum2(False)
+    True
+    >>> test_enum2(True)
+    exception
+    """
+    try:
+        return return_enum2(fail) == E2
+    except RuntimeError:
+        print("exception")
+
+cdef char return_char(fail) except 'a':
+    if fail:
+        raise RuntimeError
+    return 'b'
+
+def test_return_char(fail):
+    """
+    >>> test_return_char(False)
+    'b'
+    >>> test_return_char(True)
+    exception
+    """
+    try:
+        return chr(return_char(fail))
+    except RuntimeError:
+        print("exception")
diff --git a/tests/run/pure_py.py b/tests/run/pure_py.py
index cfe51ed4065..204cb9d0609 100644
--- a/tests/run/pure_py.py
+++ b/tests/run/pure_py.py
@@ -414,13 +414,15 @@ def ccall_except_check(x):
     return x+1
 
 
-@cython.test_fail_if_path_exists("//CFuncDeclaratorNode//IntNode[@base_10_value = '-1']")
 @cython.test_assert_path_exists("//CFuncDeclaratorNode")
 @cython.ccall
 @cython.returns(cython.long)
 @cython.exceptval(check=True)
 def ccall_except_check_always(x):
     """
+    Note that this actually takes the same shortcut as for a Cython-syntax cdef function
+    and does except ?-1
+
     >>> ccall_except_check_always(41)
     42
     >>> ccall_except_check_always(0)


Code review comments:
diff_hunk: @@ -740,8 +740,8 @@ def analyse(self, return_type, env, nonempty=0, directive_locals=None, visibilit
                     #   for now.
                     if not env.is_c_class_scope and not isinstance(self.base, CPtrDeclaratorNode):
                         from .ExprNodes import ConstNode
-                        self.exception_value = ConstNode(
-                            self.pos, value=return_type.exception_value, type=return_type)
+                        self.exception_value = ConstNode.for_type(
+                            self.pos, value=str(return_type.exception_value), type=return_type)
             if self.exception_value:
path: Cython/Compiler/Nodes.py
text: @user1:
You changed this to `is (not) None` in other places, so here as well? I still see a couple of other places where we use a plain `if … self.exception_value …:`, e.g. in the error check just a bit up.
```suggestion
            if self.exception_value is not None:
```

diff_hunk: @@ -610,6 +610,25 @@ def compile_time_value_error(self, e):
         error(self.pos, "Error in compile-time expression: %s: %s" % (
             e.__class__.__name__, e))
 
+    def as_exception_value(self, env):
+        # Return the compile_time_value if possible.
+        # This can be either a Python constant or a string
+        # for types that can't be represented by a Python constant
+        # (e.g. enums)
+        result = None
+        with local_errors(ignore=True):
+            result = self.compile_time_value(env)
+        if isinstance(result, Symtab.Entry):
+            result = result.cname
path: Cython/Compiler/ExprNodes.py
text: @user1:
`compile_time_value()` is used at parse time, which is long over here. This is called during the analysis phase. It seems better to use `self.entry` and the constant result value.

diff_hunk: @@ -1311,6 +1330,29 @@ def calculate_result_code(self):
     def generate_result_code(self, code):
         pass
 
+    @staticmethod
+    def for_type(pos, value, type):
path: Cython/Compiler/ExprNodes.py
text: @user1:
Might be worth adding a `constant_result` as well, while we're at it.

@author:
I'm slightly unclear what this means.

I'll add `calculate_constant_result()` to the value before returning it, but it's possible I've missed the point here.

@user1:
I meant to set the attribute (explicitly) when creating the node instance. Calling `calculate_constant_result()` is allowed to fail, so we should not use it outside of the `ConstantFolding` transform.

You could pass the `constant_result` also as an optional argument into this method, so that callers can decide if and how to calculate it. Default should probably be `constant_value_not_set`. Or calculate it here based on the type and value.

This is similar to `Parsing.wrap_compile_time_constant()`, BTW, although that has a slightly different focus. I'm not implying that there is room for reuse, though…