Error on memoryview argument capture on 0.29.x #4849
Conversation
I don't believe it's easy to fix cython#4798 on 0.29.x Therefore, generate an error message that explains two possible workarounds. This at least makes sure that people don't end up with mysterious crashes
Cython/Compiler/Nodes.py
Outdated
| if entry.type.is_memoryviewslice: | ||
| error( | ||
| self.pos, | ||
| "Putting capturing a memoryview argument in a closure is not supported. " |
There was a problem hiding this comment.
"Putting capturing" is difficult to understand for me. "Capture" doesn't feel like a common term here.
Maybe: "Referring to a memoryview typed argument directly in a nested closure function is not supported in Cython 0.x."
There was a problem hiding this comment.
"Putting capturing" doesn't make any kind of sense to me either now that I re-read it.
I've change it to your proposed wording.
|
Thanks |
|
Hi, this change broke SciPy's build see scipy/scipy#16718. I am trying to understand what we have to update. |
|
Sorry about the breakage (although it looks like you've sorted a fix). When I was testing this bug it crashed so reliably that I thought it unlikely anyone was actually using this in real code. So I'm slightly unsure how you've been getting away with it. |
|
It's not really my area TBH, so I have no clue 😅 But yes seems like we found a fix. |
|
So, apparently there were users of this before, and they got away at least without crashes. That may indicate that the new error is too broad and may cover cases that are actually safe – possibly. The effect is that users now add workarounds to their code that shouldn't be there. And that are not needed in Cython 3.0. For otherwise functional code, that's a drawback. What should we do? Should we keep this an error or change it into a warning? A warning has the advantage that it gives a hint to users writing new code that what they do might be dangerous, but allows existing usages to ignore the warning. But users would probably still end up adding workarounds. OTOH, as long as it's unclear if there are really (sufficiently) safe cases, an error seems safer. And now that the error change is out in the wild, we could also argue that a warning won't make things much better… Obviously, backporting the proper fix would be best, but is also fairly risky because it touches areas of tight conditional code generation (which makes a high path coverage very hard to achieve). That brings in a high risk of fixup releases. Thoughts? |
|
I'm having a look... First thing to do I think is to identify if the generated code for the cases in Scipy and Sklearn is actually safe or not |
|
(Still looking into the details but) the other potential option that occurs to me: we could probably make it "safe but memoryleak" in Cython 0.29.x rather than "unsafe because it might crash". |
|
I think it's probably safe to capture memoryviews in a def function but not a cdef function. We should probably loosen the error condition to reflect that. I think #4927 is probably also a bug that'll justify another release fairly soon, so fixing the both of them together may well be a good idea |
|
Which actually means it's OK generally since I fixed the issue with cdef functions... |
|
The error was removed again in 0.29.32. |
Error on memoryview argument capture on 0.29.x
I don't believe it's easy to fix
#4798 for def functions on 0.29.x Therefore,
generate an error message that explains two possible workarounds.
This at least makes sure that people don't end up with mysterious
crashes.
It is possible to fix for cdef functions though, so I've done that.
Best attempt at backporting #4848 for 0.29.x