Fix warnings for "implicit noexcept" when using legacy_implicit_noexcept=True

[tornaria]

Follow up to: #5999 (comment)

Currently, the warning Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword. gives false positives for functions returning python objects. This is misleading and results in naively adding incorrect noexcept clauses (as in sagemath/sage#36507 which added about 40k incorrect ones!).

With #5999, the incorrect noexcept clauses are warned, which is good. But it also means that it's not possible to "fix" warnings for some functions (when running in legacy implicit noexcept mode).

For instance in

$ cat test_noexcept.pyx 
cdef test_good():
    return 0

cdef test_bad() noexcept:
    return 0
$ cython -3 -X legacy_implicit_noexcept=True test_noexcept.pyx 
warning: test_noexcept.pyx:1:12: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
warning: test_noexcept.pyx:4:10: noexcept clause is ignored for function returning Python object

both the good and the bad versions give a warning.

---

The first commit in this PR fixes the warning to avoid these false positives, by not giving a warning for functions that return a python object.

Notes on the change:

• the current implementation of the implicit noexcept mode and the warning happens during parsing, but this is too early to know the return type
• hence we move handling of the implicit noexcept mode and the warning to the analysis phase
• the logic is: if we are in implicit noexcept mode, check that the function definition
  • does not return a python type
  • does not have an explicit except or noexcept clause
  • self.exception_check is set, meaning the function would normally check exceptions (this rules out extern definitions; the visibility != 'extern' is not enough)
• under the above conditions, trigger implicit noexcept by clearing self.exception_check with the corresponding deprecation warning.

---

The second commit in this PR adds 44 missing noexcept clauses to files in Cython/Includes. In fact, currently there are 60 implicit noexcept warnings, of which 16 are false positives and fixed by the first commit.

---

The combination of both commits results in a "clean" Cython/Includes. Testing this:

Step 1, create a test file that cimports every include file shipped in cython:

$ find Cython/Includes -name '*.pxd' | sed -e 's|^Cython/Includes/|cimport |;s|/|.|;s|\.pxd$||' > test_cimport.pyx

Step 2, test with current master:

$ PYTHONPATH=Cython/Includes ./cython.py -3+ -X legacy_implicit_noexcept=True test_cimport.pyx |& grep -c "Implicit noexcept"
60

Step 3, checkout this branch, run the test before recompiling:

$ PYTHONPATH=Cython/Includes ./cython.py -3+ -X legacy_implicit_noexcept=True test_cimport.pyx |& grep -c "Implicit noexcept"
16

Here we still get 16 false positives since we are using cython from current master.

Step 4, rebuild cython with this branch and run with -Werror:

$ make
[...]
$ PYTHONPATH=Cython/Includes ./cython.py -3+ -Werror -X legacy_implicit_noexcept=True test_cimport.pyx
$

We see no warning at all: neither "implicit noexcept" nor the warning from #5999 which is good.

---

Final words: it would be very nice to merge this and make a new release of cython asap. In fact, right now building sagemath with cython 3.0.9 gives ~ 40k warnings coming from #5999. A good warnings, I made 40k mistakes in sagemath/sage#36507. But fixing those 40k mistakes leads to 40k implicit noexcept warnings and doubt (is each of the 40k fixes ok or not ok?).

My hope is that, once we get rid of all the warnings related to noexcept (in sagemath and all dependencies, e.g. numpy) we'll be more comfortable switching off the legacy mode.

[tornaria]

@scoder here's my PR. I haven't run tests since I don't really understand how to do it.

I think some tests in tests/run/legacy_implicit_noexcept.pyx that warn "implicit noexcept" are false positives and would be removed by this PR, so that should be fixed.

A quick test seems to indicate the warnings for lines 24, 36, 39, 42 are false positives. Also lines 45 and 49 don't give a warning with current master but they give a warning with this branch which seems ok (because the parser doesn't see the @cython.cfunc but my PR does; the line 49 actually has a typo)

[tornaria]

Something like this seems it would do it:

--- a/tests/run/legacy_implicit_noexcept.pyx
+++ b/tests/run/legacy_implicit_noexcept.pyx
@@ -46,7 +46,7 @@ cdef test_noexcept_warning():
 def func_pure_implicit() -> cython.int:
     raise RuntimeError
 
-@cython.excetval(check=False)
+@cython.exceptval(check=False)
 @cython.cfunc
 def func_pure_noexcept() -> cython.int:
     raise RuntimeError
@@ -148,11 +148,8 @@ def test_pure_noexcept():
 
 _WARNINGS = """
 12:5: Unraisable exception in function 'legacy_implicit_noexcept.func_implicit'.
-12:36: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
+12:22: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
 15:5: Unraisable exception in function 'legacy_implicit_noexcept.func_noexcept'.
-24:43: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
-27:38: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
-36:43: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
-39:36: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
-42:28: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
+27:28: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
+45:0: Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.
 """

But I don't know how to test it to be sure. Note that the position of the warning also changes, it now points to the function name instead of the closing parenthesis (arguably the former behaviour is better since it's the position where the noexcept clause should be added).

[tornaria]

I added a commit to fix tests. I tested with ./runtests.py legacy_implicit_noexcept.

A fourth commit adds a couple of cdef extern functions, to test that they are implicitly noexcept without a warning. The second one cdef extern int extern_fun_fun(int (*f)(int)) shows why the line and self.exception_check in the first commit is necessary, otherwise there is a warning, not for extern_fun_fun but for the parameter function f.

[da-woods]

This looks good to me I think. I'll leave it a little bit to see if there are further comments though

[tornaria]

Would it make sense to add a test that the following can be cythonized without any warning using legacy_implicit_noexcept:

$ find Cython/Includes -name '*.pxd' | sed -e 's|^Cython/Includes/|cimport |;s|/|.|;s|\.pxd$||' > test_cimport.pyx
test_cimport.pyx
$ cat test_cimport.pyx
cimport libc.stdint
cimport libc.string
cimport libc.__init__
cimport libc.stdio
cimport libc.float
cimport libc.locale
...

Here's how I build to check I don't get any warning (on this branch)

$ PYTHONPATH=Cython/Includes ./cython.py -3+ -Werror -X legacy_implicit_noexcept=True 

I don't know how to turn this into a test.

---

Edit: maybe this is nicer as a test file:

$ find Cython/Includes -name '*.pxd' | sed -e 's|^Cython/Includes/|cimport |;s|/|.|;s|\(\.__init__\)\?\.pxd$||' | sort > test_cimport.pyx
$ cat test_cimport.pyx
cimport cpython
cimport cpython.array
cimport cpython.bool
cimport cpython.buffer
cimport cpython.bytearray
cimport cpython.bytes
...

Also, maybe you want to separate the libcpp part and try the rest in C mode and only the libcpp part in c++ mode.

[matusvalo]

I think we should include also memoryviews. But this should be done in future PR adding warnings for noexcept functions returning memoryviews.

[tornaria]

I don't know about memoryviews, sorry.

Thanks for approving the PR. It'd be great to have a released version of cython with this PR merged, to help with fixing sagemath missing noexcept so we can eventually remove legacy_implicit_noexcept for good.

[matusvalo]

3.0.x commit: 4e9f730

[da-woods]

Contextlib doesn't seem to be working on Python 2.7 on the 3.0.x cherry-pick unfortunately.

[matusvalo]

I committed directly to the branch for simplicity and here is result :-(.

[matusvalo]

I have created a PR to 3.0.X reverting the changes: #6103. But I don't have python2.7 anymore so cannot test them locally. I will have more time playing with it in the eventing (I hope so).

[da-woods]

No worries. I don't think a temporarily failing branch will cause real problems.