[ENH] Propagate exceptions from cdef functions by default
#4670
Conversation
There was a problem hiding this comment.
Just drive-by comments, not actionable until Stefan or David agree.
I would also propose to reshuffle the order of except + and except?, because AFAICT the except + case does not currently do exception checking by default (and assuming this should be kept); that way, the processing would be grouped more consistently, à la:
exc_check = 1
if s.sy == 'except':
if s.sy == '*':
...
elif s.sy == '?':
...
### all checked exceptions above this point, all unchecked ones below ###
elif s.sy == '+':
...
else:
# case e.g. "except -1"
...
elif s.sy == 'IDENT' and s.systring == 'noexcept':
...
return exc_val, exc_check
|
…ython into propagate-exceptions-by-default
…ate-exceptions-by-default
|
I'm seeing that a lot of the current failures arise from the fact that the Cython tests and internals define or (2) declare external C functions without an exception value. Presumably, none of those are expected to raise. Due to the change in behaviour introduced in this PR, I'm needing to mark them all I'm happy to work through all the failures in this way, but wanted to check whether this is an expected side-effect before forging ahead. |
|
It is certainly an unfortunate effect on users. We're not the only ones who define cdef functions.
Were you referring to „extern” functions? Meaning, real C functions? Those are probably unlikely enough to raise Python exceptions to simply exclude them from this change.
|
|
@scoder It's more than just extern functions (although I will address extern functions in a future comment). Here's an example; the following program compiles on the main branch, but not this one. cimport cython
cdef class Foo:
cdef cython.floating method(self, cython.integral x, cython.floating y):
return x + y
cdef Foo obj = Foo()
cdef double (*func)(Foo, long, double)
func = obj.methodOn this branch, I get: Error compiling Cython file:
------------------------------------------------------------
...
return x + y
cdef Foo obj = Foo()
cdef double (*func)(Foo, long, double)
func = obj.method
^
------------------------------------------------------------
test.pyx:10:10: Type is not specializedFor reasons I don't fully understand, I have to use cimport cython
cdef class Foo:
cdef cython.floating method(self, cython.integral x, cython.floating y) noexcept:
return x + y
cdef Foo obj = Foo()
cdef double (*func)(Foo, long, double) noexcept
func = obj.methodDoes this point to a bug somewhere, or would this be an expected result from this PR? |
I think your example makes sense to me to fail since in master branch does not compile following: cimport cython
cdef class Foo:
cdef cython.floating method(self, cython.integral x, cython.floating y) except? -1:
return x + y
cdef Foo obj = Foo()
cdef double (*func)(Foo, long, double) except? -1
func = obj.methodSince we changed default to semantics of Unfortunately, it introduces backward incompatible change but still I think PR works as described in the issue. |
|
BTW this PR seems OK for me except comment by @scoder. |
This is certainly not the best error message ever. We should at least report that no matching signature was found, and preferably give hints why.
This is not how this should work. "extern" functions shouldn't raise exceptions, unless explicitly marked as such. They are just too unlikely to raise in the real world. |
This reverts commit e09fc5e.
Right now it is not clear how should be "extern" functions handled. Even bug report leaves it as open questions:
Since they are unlikely to raise exceptions, should be marked as Similarly how we should threat pointer to function:
|
That seems the biggest open question. As soon as we make a distinction between extern and non-extern functions, we end up with the problem that function pointers need to be able to refer to both. And they seem equally likely to refer to either kind of function, maybe even with a slightly higher chance of pointing to Cython implemented (i.e. non-extern) functions, even though they are a totally C-ish thing. So you'd end up with code like this: cdef extern from "someheader.h":
int cfunc(int x)
cdef int cdeffunc(int x) noexcept:
return x + 1
cdef int (*func)(int x) noexcept
func = cfunc
func = cdeffuncand cdef extern from "someheader.h":
int cfunc(int x) except? -1
cdef int cdeffunc(int x):
return x + 1
cdef int (*func)(int x)
func = cfunc
func = cdeffuncOTOH, if we make both behave the same, then we break pretty much all .pxd files out there, but end up with something more consistent: cdef extern from "someheader.h" noexcept:
int cfunc(int x)
cdef int cdeffunc(int x) noexcept:
return x + 1
cdef int (*func)(int x) noexcept
func = cfunc
func = cdeffuncIt feels like the decision is between "easy to use (in most cases)" and "easy to explain". |
|
If we went for "consistent" (i.e. require |
Co-authored-by: h-vetinari <h.vetinari@gmx.com>
I don't fully understand -- is this proposing |
No, sorry for the confusion. I was just musing that the hypothetical
would be more intuitive to me ("?" signifying more uncertainty / less checking) -- aside from also avoiding the inconsistency of
But of course, we cannot change (much less invert!) the meaning of |
|
@scoder @da-woods @h-vetinari Could I ask for what I hope would be a final review? |
There was a problem hiding this comment.
This looks good I think. The few comments I have are minor ones.
I'd definitely like to wait for @scoder to go over it too. It's a fairly significant change (in terms of the effect it'll have) so more eyes on it is probably better.
But I don't see any reason for this not to make the next release.
| """ | ||
| Parse exception value clause. | ||
|
|
||
| Maps clauses to exc_check / exc_value / exc_clause as follows: |
There was a problem hiding this comment.
exc_clause is just for the nogil order warning I think? That's fine, but I think it'd be good to mention that in the comment. Just because exc_check and exc_value are much more important to how Cython handles exceptions and I think it's worth making that clear
There was a problem hiding this comment.
While it's true that currently exc_clause is only used to correctly raise the nogil warning, it could potentially be useful for other things in the future? I just feel it's very easy for the comment here to become out of sync with how the return value is actually used outside this function.
There was a problem hiding this comment.
The comment contains a table with a rather complete specification. If anything at all changes in the implementation, it will most likely need to be adapted. I don't think more information will make that any worse.
There was a problem hiding this comment.
Thanks! Added a note.
| """ | ||
| Parse exception value clause. | ||
|
|
||
| Maps clauses to exc_check / exc_value / exc_clause as follows: |
There was a problem hiding this comment.
The comment contains a table with a rather complete specification. If anything at all changes in the implementation, it will most likely need to be adapted. I don't think more information will make that any worse.
Co-authored-by: da-woods <dw-git@d-woods.co.uk>
…ython into propagate-exceptions-by-default
|
Thanks @shwina - this should remove one of the last major blocks for 3.0 |
|
I'd forgotten about #4936... I'm not sure. I'll have a look and see how difficult it seems. At very least we should get a warning into Cython 3 about the future change of behaviour |
|
This PR broke scipy's build, bisected back to 77918c5 specifically. Not sure if the right place to fix this is cython or scipy. Do you want a new issue on the cython side? scipy/scipy#17234 is the issue on scipy. bisect log + script folded. #! /usr/bin/xonsh
import argparse
from xonsh.dirstack import with_pushd
from pathlib import Path
$RAISE_SUBPROC_ERROR = True
git clean -xfd
pip install -v . --no-build-isolation
with with_pushd('/home/tcaswell/source/p/scipy/scipy'):
git clean -xfd
pip install -v . --no-build-isolation
|
|
I've replied on the Scipy issue but I think we can probably fix this in the Cython PyCapsule definitions. Needs a bit more thought though |
|
Replied again on the Scipy issue (scipy/scipy#17234 (comment)). To summarise: with a more detailed look I think the right fix is to annotate a |
|
@da-woods @scoder I'm updating mpi4py for Cython 3, and this change hit me relatively hard. Although I can easily manage to update all of my codebase quickly and without too much pain, other users with large Cython codebases may suffer a lot. I actually agree with the change and I'm all for it. However, IMHO this change is a huge backward-compatibility fiasco. Perhaps we should consider adding a |
|
I definitely don't think we want to make it anything more granular than "per-file". And currently the work for this is done in the parser so that pretty much forces it to be done per-file file. I can see this being a big change for some codebases so I can see a switch being helpful (much as we don't like too many switches because they add combinations that need testing). So I wouldn't be opposed to this. |
I took liberty and created an issue for that. |
Closes #4280
This PR enables Python exceptions to be propagated by (non-extern)
cdeffunctions by default. The behaviour forexternfunctions is unchanged.It also adds a new
noexceptmodifier that can be used to declarecdeffunctions that will not raise.Finally, it adds to the
language-basicsandmigrating-to-cython-3.0parts of the documentation, describing the changes .