Add cpdef declarations in pxd files when overwriting a cdef method

[roed314]

See cython/cython#1732.

Upstream: Fixed upstream, but not in a stable release.

CC: @saraedum

Component: cython

Author: David Roe

Branch/Commit: 919de01

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/23227

[roed314]

Upstream: Reported upstream. No feedback yet.

[jdemeyer]

comment:3

How did you come up with this? Is there a specific concrete problem or just a theoretical bug?

By the way, the gist link talks about "cpdef'd without a corresponding entry in their pxd file" while the Cython bug talks about "cpdef override of a cdef method". These are obviously different things, so what is the problem exactly? If the problem is only cpdef overrides of cdef methods, I guess that a large majority of the entries in the gist link are false positives, making that link not very informative.

[roed314]

comment:4

Replying to @jdemeyer:

How did you come up with this? Is there a specific concrete problem or just a theoretical bug?

Yeah, this caused a segfault that saraedum and I spent 10 hours yesterday debugging, while working on #23218. Because the vtable is screwed up, function calls behave unpredictably.

By the way, the gist link talks about "cpdef'd without a corresponding entry in their pxd file" while the Cython bug talks about "cpdef override of a cdef method". These are obviously different things, so what is the problem exactly? If the problem is only cpdef overrides of cdef methods, I guess that a large majority of the entries in the gist link are false positives, making that link not very informative.

Yeah, I agree that the link is not that informative. I wrote something easy to try to get an idea of the scope of the problem, then determined that it wasn't easy to actually find the cases where the problem actually occurs (which is, indeed, where a cdef method is overwritten by a cpdef method without a corresponding entry in the pxd file). I decided that I wanted to move on and work on other stuff, so I uploaded the gist as a place to start looking.

[roed314]

comment:5

Replying to @roed314:

Replying to @jdemeyer:

How did you come up with this? Is there a specific concrete problem or just a theoretical bug?

Yeah, this caused a segfault that saraedum and I spent 10 hours yesterday debugging, while working on #23218. Because the vtable is screwed up, function calls behave unpredictably.

In particular, we noticed the problem for Polynomial_generic_dense, where _add_, _mul_ and _floordiv_ have this issue.

[jdemeyer]

comment:6

Honestly, I don't think it is realistic to fix the general problem in Sage since the underlying problem is in upstream Cython. Of course, we can fix specific cases like the Polynomial_generic_dense that you mentioned.

[roed314]

comment:7

Replying to @jdemeyer:

Honestly, I don't think it is realistic to fix the general problem in Sage since the underlying problem is in upstream Cython. Of course, we can fix specific cases like the Polynomial_generic_dense that you mentioned.

Sure, and I'm fine waiting to see what Cython does. But one possible thing for them to do is to require a cpdef declaration in the pxd file, in which case this ticket can track those additions.

[roed314]

comment:8

Replying to @roed314:

Replying to @jdemeyer:

Honestly, I don't think it is realistic to fix the general problem in Sage since the underlying problem is in upstream Cython. Of course, we can fix specific cases like the Polynomial_generic_dense that you mentioned.

Sure, and I'm fine waiting to see what Cython does. But one possible thing for them to do is to require a cpdef declaration in the pxd file, in which case this ticket can track those additions.

Cython will soon raise an error in these cases. I think the best thing to do is to wait until we update to a new version of Cython and change these pxd files then. I think it's worth leaving this ticket open until that point in case someone else runs into a similar problem.

[jdemeyer]

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

[jdemeyer]

Author: Jeroen Demeyer

[jdemeyer]

comment:10

Replying to @roed314:

Cython will soon raise an error in these cases.

I convinced Cython upstream to downgrade this to a warning for now. That will make our life easier, since we can first upgrade to Cython 0.26 (in beta now) and then fix this gradually.

[roed314]

Dependencies: #23360

[roed314]

Branch: u/roed/fix_cpdef

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

99b1e3e

Add cpdef _add_(self, other) and cpdef _mul_(self, other) all over

[sagetrac-git]

Commit: 99b1e3e

[jdemeyer]

Changed author from Jeroen Demeyer to David Roe

[jdemeyer]

comment:14

Ready for review?

[jdemeyer]

comment:15

Why restrict to _add_ and _mul_? (edit: you are not doing that, it's just the commit message implying that you do)

[roed314]

comment:16

Yeah, I just got lazy going through and figuring out all the functions that I'd actually changed. I used the log of warnings to find the places to edit, so I think I got them all, but I haven't rebuilt from scratch to check.

[roed314]

comment:17

Yeah, I think this is ready for review. I don't have anything else to add to it.

[jdemeyer]

comment:18

Replying to @roed314:

Yeah, I just got lazy going through and figuring out all the functions that I'd actually changed. I used the log of warnings to find the places to edit, so I think I got them all, but I haven't rebuilt from scratch to check.

Even if you didn't catch them all, that's not really a problem.

[jdemeyer]

Changed branch from u/roed/fix_cpdef to u/jdemeyer/fix_cpdef

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:20

Rebased to 8.1.beta1

---

New commits:

e6f5cc5

Add cpdef _add_(self, other) and cpdef _mul_(self, other) all over

[jdemeyer]

Changed commit from 99b1e3e to e6f5cc5

[jdemeyer]

comment:21

Why this?

-    cpdef normalize(self)
+    cpdef normalize(self, include_zeroth_moment = *)

[jdemeyer]

comment:22

In some cases, I am wondering whether it is better to add "abstract" _add_ and _mul_ methods on a lower level. For example, instead of adding those functions to QuaternionAlgebraElement_generic, QuaternionAlgebraElement_number_field and QuaternionAlgebraElement_rational_field, add them to QuaternionAlgebraElement_abstract raising NotImplementedError.

Concretely for this ticket, I would like to fix only the totally uncontroversial cases (for example, undoing the changes to quaternion algebra elements) and leave the rest for a follow-up.

[sagetrac-git]

Changed commit from e6f5cc5 to 919de01

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

919de01

Add declarations for cpdef arithmetic functions in .pxd files

[jdemeyer]

comment:24

positive_review to this strict subset of the original patch.

[roed314]

comment:26

See #23600 for the followup.

[jdemeyer]

Changed dependencies from #23360 to none

[vbraun]

Changed branch from u/jdemeyer/fix_cpdef to 919de01