Improve performance hints for nogil + pxd (#6088)

In order to be called efficiently, these functions must
have the exception specification set in the pxd file since
Cython is unable to assume an implicit exception specification.

This improves the quality of the messages to draw attention to
this detail.

Closes #6001

Author: da-woods

Cython/Compiler/ExprNodes.py

@@ -6495,7 +6495,8 @@ def generate_result_code(self, code):
                             PyrexTypes.write_noexcept_performance_hint(
                                 self.pos, code.funcstate.scope,
                                 function_name=perf_hint_entry.name if perf_hint_entry else None,
-                                void_return=self.type.is_void, is_call=True)
+                                void_return=self.type.is_void, is_call=True,
+                                is_from_pxd=(perf_hint_entry and perf_hint_entry.defined_in_pxd))
                         code.globalstate.use_utility_code(
                             UtilityCode.load_cached("ErrOccurredWithGIL", "Exceptions.c"))
                         exc_checks.append("__Pyx_ErrOccurredWithGIL()")

Cython/Compiler/PyrexTypes.py

@@ -5475,7 +5475,9 @@ def cap_length(s, max_len=63):
     hash_prefix = hashlib.sha256(s.encode('ascii')).hexdigest()[:6]
     return '%s__%s__etc' % (hash_prefix, s[:max_len-17])
 
-def write_noexcept_performance_hint(pos, env, function_name=None, void_return=False, is_call=False):
+def write_noexcept_performance_hint(pos, env,
+                                    function_name=None, void_return=False, is_call=False,
+                                    is_from_pxd=False):
     if function_name:
         # we need it escaped everywhere we use it
         function_name = "'%s'" % function_name
@@ -5498,6 +5500,9 @@ def write_noexcept_performance_hint(pos, env, function_name=None, void_return=Fa
         solutions.append(
             "Use an 'int' return type on %s to allow an error code to be returned." %
             the_function)
+    if is_from_pxd and not void_return:
+        solutions.append(
+            "Declare any exception value explicitly for functions in pxd files.")
     if len(solutions) == 1:
         msg = "%s %s" % (msg, solutions[0])
     else:

Cython/Compiler/Symtab.py

@@ -14,7 +14,7 @@
     import builtins
 
 from ..Utils import try_finally_contextmanager
-from .Errors import warning, error, InternalError
+from .Errors import warning, error, InternalError, performance_hint
 from .StringEncoding import EncodedString
 from . import Options, Naming
 from . import PyrexTypes
@@ -907,6 +907,17 @@ def declare_cfunction(self, name, type, pos,
                 elif not in_pxd and entry.defined_in_pxd and type.compatible_signature_with(entry.type):
                     # TODO: check that this was done by a signature optimisation and not a user error.
                     #warning(pos, "Function signature does not match previous declaration", 1)
+
+                    # Cython can't assume anything about cimported functions declared without
+                    # an exception value. This is a performance problem mainly for nogil functions.
+                    if entry.type.nogil and entry.type.exception_value is None and type.exception_value:
+                        performance_hint(
+                            entry.pos,
+                            f"No exception value declared for '{entry.name}' in pxd file.\n"
+                            "Users cimporting this function and calling it without the gil "
+                            f"will always require an exception check.\n"
+                            "Suggest adding an explicit exception value.",
+                            self)
                     entry.type = type
                 else:
                     error(pos, "Function signature does not match previous declaration")

tests/compile/nogil_perf_hints.pyx

@@ -0,0 +1,20 @@
+# mode: compile
+# tag: perf_hints
+
+# Compile only to avoid needing to compile definitions of the functions
+# declared in another pxd file
+
+from nogil_perf_hints_pxd cimport f_has_except_value, f_missing_except_value, f_noexcept
+
+def test():
+    with nogil:
+        # should not generate performance hints
+        f_has_except_value()
+        f_noexcept()
+        # should generate performance hints.
+        # (Unfortunately it's difficult to check the extra information on the 2nd+ line of the hint)
+        f_missing_except_value()
+
+_PERFORMANCE_HINTS = """
+16:30: Exception check after calling 'f_missing_except_value' will always require the GIL to be acquired.
+"""

tests/compile/nogil_perf_hints_pxd.pxd

@@ -0,0 +1,3 @@
+cdef int f_has_except_value() nogil except -1
+cdef int f_missing_except_value() nogil
+cdef int f_noexcept() nogil noexcept

tests/run/nogil.pxd

@@ -1 +1,6 @@
 cdef void voidexceptnogil_in_pxd() nogil
+
+# These definitions are unhelpful to people cimporting
+# them because the exception value isn't in the pxd.
+cdef int f_in_pxd1() nogil
+cdef int f_in_pxd2() nogil

tests/run/nogil.pyx

@@ -198,8 +198,26 @@ def test_performance_hint_nogil():
         voidexceptnogil_in_other_pxd()
 
 
+cdef int f_in_pxd1() nogil except -1:
+    return 0
+
+cdef int f_in_pxd2() nogil:  # implicit except -1?
+    return 0
+
+def test_declared_in_pxd():
+    """
+    >>> test_declared_in_pxd()
+    """
+    with nogil:
+        # no warnings here because we're in the same file as the declaration
+        f_in_pxd1()
+        f_in_pxd2()
+
+
 # Note that we're only able to check the first line of the performance hint
 _PERFORMANCE_HINTS = """
+5:18: No exception value declared for 'f_in_pxd1' in pxd file.
+6:18: No exception value declared for 'f_in_pxd2' in pxd file.
 20:9: Exception check after calling 'f' will always require the GIL to be acquired.
 24:5: Exception check on 'f' will always require the GIL to be acquired.
 34:5: Exception check on 'release_gil_in_nogil' will always require the GIL to be acquired.