Make libsingular multivariate polynomial rings collectible

src/sage/algebras/letterplace/free_algebra_letterplace.pxd

@@ -15,7 +15,8 @@ from sage.libs.singular.decl cimport ring
 
 
 cdef class FreeAlgebra_letterplace_libsingular():
-    cdef ring* _lp_ring
+    cdef ring *_lp_ring
+    cdef int *_lp_ring_ref
     cdef MPolynomialRing_libsingular _commutative_ring
     cdef MPolynomialRing_libsingular _lp_ring_internal
     cdef object _ngens

src/sage/algebras/letterplace/free_algebra_letterplace.pyx

@@ -894,7 +894,8 @@ cdef class FreeAlgebra_letterplace_libsingular():
         from sage.libs.singular.function import singular_function
         freeAlgebra = singular_function("freeAlgebra")
         cdef RingWrap rw = freeAlgebra(commutative_ring, degbound)
-        self._lp_ring = singular_ring_reference(rw._ring)
+        self._lp_ring_ref = rw._ring_ref
+        self._lp_ring = singular_ring_reference(rw._ring, self._lp_ring_ref)
         # `_lp_ring` viewed as `MPolynomialRing_libsingular` with additional
         # letterplace attributes set (for internal use only)
         self._lp_ring_internal = new_CRing(rw, commutative_ring.base_ring())
@@ -909,4 +910,4 @@ cdef class FreeAlgebra_letterplace_libsingular():
         (since this method can be at unpredictable times due to garbage
         collection).
         """
-        singular_ring_delete(self._lp_ring)
+        singular_ring_delete(self._lp_ring, self._lp_ring_ref)

src/sage/libs/singular/decl.pxd

@@ -212,6 +212,7 @@ cdef extern from "singular/Singular/libsingular.h":
 
         n_Procs_s* cf  # coefficient field/ring
         int ref
+        int sage_ref
 
         # return total degree of p
 

src/sage/libs/singular/function.pxd

@@ -25,6 +25,8 @@ cdef singular_ring* access_singular_ring(r) except <singular_ring*> -1
 
 cdef class RingWrap:
     cdef singular_ring *_ring
+    cdef int *_ring_ref
+
 
 cdef class Resolution:
     cdef syStrategy *_resolution

src/sage/libs/singular/function.pyx

@@ -1,3 +1,3 @@
 """
 libSingular: Functions
 
@@ -98,12 +98,16 @@
 from sage.libs.singular.option import opt_ctx
 from sage.libs.singular.polynomial cimport singular_vector_maximal_component
 from sage.libs.singular.singular cimport sa2si, si2sa, si2sa_intvec, si2sa_bigintvec, start_catch_error, check_error
+from sage.libs.singular.ring cimport singular_ring_delete, singular_ring_reference
+
 from sage.libs.singular.singular import error_messages
 
 from sage.interfaces.singular import get_docstring
 
 from sage.misc.verbose import get_verbose
 
+from cysignals.memory cimport sig_calloc
+
 
 cdef poly* sage_vector_to_poly(v, ring *r) except <poly*> -1:
     """
@@ -148,8 +152,7 @@
         return "<RingWrap>"
 
     def __dealloc__(self):
-        if self._ring != NULL:
-            self._ring.ref -= 1
+        singular_ring_delete(self._ring, self._ring_ref)
 
     def ngens(self):
         """
@@ -810,7 +813,6 @@
         Append the ring ``r`` to the list.
         """
         cdef ring *_r = access_singular_ring(r)
-        _r.ref += 1
         return self._append(<void *>_r, RING_CMD)
 
     cdef leftv *append_matrix(self, mat) except NULL:
@@ -1026,9 +1028,8 @@
 
     cdef leftv* handle_call(self, Converter argument_list, ring *_ring=NULL) noexcept:
         if _ring != currRing: rChangeCurrRing(_ring)
-        cdef bint error = iiMake_proc(self.proc_idhdl, NULL, argument_list.args)
         cdef leftv * res
-        if not error:
+        if not iiMake_proc(self.proc_idhdl, NULL, argument_list.args):
             res = <leftv*> omAllocBin(sleftv_bin)
             res.Init()
             res.Copy(&iiRETURNEXPR)
@@ -1170,8 +1171,13 @@
             currRingHdl = ggetid("my_awesome_sage_ring")
             if currRingHdl == NULL:
                 currRingHdl = enterid("my_awesome_sage_ring", 0, RING_CMD, &IDROOT, 1)
-                currRingHdl.data.uring = <ring *>omAlloc0Bin(sip_sring_bin)
-            currRingHdl.data.uring.ref += 1
+                if currRing:
+                    currRingHdl.data.uring = currRing
+                else:
+                    currRingHdl.data.uring = <ring *>omAlloc0Bin(sip_sring_bin)
+                    # deletion would result in a segfault, thus, we
+                    # prevent it from deletion:
+                    currRingHdl.data.uring.ref += 1
 
     cdef BaseCallHandler get_call_handler(self):
         """
@@ -1457,9 +1463,7 @@
     if si_ring != currRing: rChangeCurrRing(si_ring)
 
     if currRingHdl.data.uring!= currRing:
-        currRingHdl.data.uring.ref -= 1
-        currRingHdl.data.uring = currRing # ref counting?
-        currRingHdl.data.uring.ref += 1
+        currRingHdl.data.uring = currRing
 
     cdef Converter argument_list = Converter(args, R, attributes)
 
@@ -1882,9 +1886,9 @@
 
 cdef inline RingWrap new_RingWrap(ring* r):
     cdef RingWrap ring_wrap_result = RingWrap.__new__(RingWrap)
-    ring_wrap_result._ring = r
-    ring_wrap_result._ring.ref += 1
-
+    ring_wrap_result._ring_ref = <int*>sig_calloc(1, sizeof(int))
+    ring_wrap_result._ring = singular_ring_reference(r, ring_wrap_result._ring_ref)
+    ring_wrap_result._ring.ref = 1 # to prevent Singular from deleting it prematurely
     return ring_wrap_result
 
 # Add support for _instancedoc_

src/sage/libs/singular/groebner_strategy.pxd

@@ -8,13 +8,16 @@ from sage.rings.polynomial.plural cimport NCPolynomialRing_plural, NCPolynomial_
 cdef class GroebnerStrategy(SageObject):
     cdef skStrategy *_strat
     cdef ring *_parent_ring
+    cdef int *_parent_ring_ref
     cdef MPolynomialRing_libsingular _parent
     cdef object _ideal
 
     cpdef MPolynomial_libsingular normal_form(self, MPolynomial_libsingular p)
 
 cdef class NCGroebnerStrategy(SageObject):
     cdef skStrategy *_strat
+    cdef ring *_parent_ring
+    cdef int *_parent_ring_ref
     cdef NCPolynomialRing_plural _parent
     cdef object _ideal
 

src/sage/libs/singular/groebner_strategy.pyx

@@ -1,3 +1,3 @@
 """
 Singular's Groebner Strategy Objects
 
@@ -110,7 +110,8 @@
 
         cdef MPolynomialRing_libsingular R = <MPolynomialRing_libsingular>L.ring()
         self._parent = R
-        self._parent_ring = singular_ring_reference(R._ring)
+        self._parent_ring_ref = R._ring_ref
+        self._parent_ring = singular_ring_reference(R._ring, self._parent_ring_ref)
 
         if not R.term_order().is_global():
             raise NotImplementedError("The local case is not implemented yet.")
@@ -178,8 +179,7 @@
                 rChangeCurrRing(oldRing)
             else:
                 delete_skStrategy(self._strat)
-        if self._parent_ring:
-            singular_ring_delete(self._parent_ring)
+        singular_ring_delete(self._parent_ring, self._parent_ring_ref)
 
     def _repr_(self) -> str:
         """
@@ -362,6 +362,9 @@
 
         cdef NCPolynomialRing_plural R = <NCPolynomialRing_plural>L.ring()
         self._parent = R
+        assert R._ring_ref, "Underlying ring for the Groebner strategy needs to have a refcount"
+        self._parent_ring_ref = R._ring_ref
+        self._parent_ring = singular_ring_reference(R._ring, self._parent_ring_ref)
 
         if not R.term_order().is_global():
             raise NotImplementedError("The local case is not implemented yet.")
@@ -405,6 +408,8 @@
             sage: strat = NCGroebnerStrategy(I)
             sage: del strat   # indirect doctest
         """
+        # WARNING: the Cython class self._parent is no longer accessible!
+        # see http://trac.sagemath.org/sage_trac/ticket/11339
         cdef ring *oldRing = NULL
         if self._strat:
             omfree(self._strat.sevS)
@@ -418,13 +423,14 @@
             omfree(self._strat.fromQ)
             id_Delete(&self._strat.Shdl, self._parent._ring)
 
-            if self._parent._ring != currRing:
+            if self._parent_ring != currRing:
                 oldRing = currRing
-                rChangeCurrRing(self._parent._ring)
+                rChangeCurrRing(self._parent_ring)
                 delete_skStrategy(self._strat)
                 rChangeCurrRing(oldRing)
             else:
                 delete_skStrategy(self._strat)
+        singular_ring_delete(self._parent_ring, self._parent_ring_ref)
 
     def _repr_(self):
         """

src/sage/libs/singular/polynomial.pxd

@@ -20,7 +20,7 @@ cdef int singular_polynomial_check(poly *p, ring *r) except -1
 cdef int singular_polynomial_add (poly **ret, poly *p, poly *q, ring *r) noexcept
 cdef int singular_polynomial_call (poly **ret, poly *p, ring *r, list args,
                                    poly *(*get_element)(object) noexcept) noexcept
-cdef int singular_polynomial_cmp (poly *p, poly *q, ring *r) noexcept
+cdef int singular_polynomial_cmp (poly *p, poly *q, ring *r) except -2
 cdef int singular_polynomial_rmul (poly **ret, poly *p, RingElement q, ring *r) noexcept
 cdef int singular_polynomial_mul (poly **ret, poly *p, poly *q, ring *r) except -1
 cdef int singular_polynomial_sub (poly **ret, poly *p, poly *q, ring *r) noexcept

src/sage/libs/singular/polynomial.pyx

@@ -231,7 +231,7 @@ cdef int singular_polynomial_call(poly **ret, poly *p, ring *r, list args,
 
     return 0
 
-cdef int singular_polynomial_cmp(poly *p, poly *q, ring *r) noexcept:
+cdef int singular_polynomial_cmp(poly *p, poly *q, ring *r) except -2:
     """
     Compare two Singular elements ``p`` and ``q`` in ``r``.
 
@@ -285,6 +285,8 @@ cdef int singular_polynomial_cmp(poly *p, poly *q, ring *r) noexcept:
         True
     """
     cdef int tmp
+    assert r, "A valid ring must be provided"
+    assert r.ref >= 0, "This ring has %d references and thus has previously been deleted"%(r.ref)
 
     if r != currRing:
         rChangeCurrRing(r)

src/sage/libs/singular/ring.pxd

@@ -17,21 +17,27 @@ from sage.libs.singular.decl cimport ring
 
 # To work with singular rings, you need to balance singular_ring_new with
 # singular_ring_delete or singular_ring_reference with
-# singular_ring_delete. That is, either use one of the two patterns:
+# singular_ring_delete; the latter will, when appropriate, also deallocate
+# the <int*> that is being used for the reference count.
+# That is, either use one of the two patterns:
 #
 # cdef class myclass_new():
-#     cdef ring* myring;
+#     cdef ring *myring;
+#     cdef int *ref
 #     def __cinit__():
-#         self.myring = singular_ring_new(...)
+#         self.ref = <int*>sig_malloc(sizeof(int))
+#         self.myring = singular_ring_reference(singular_ring_new(...), self.ref)
 #     def __dealloc__():
-#         singular_ring_delete(self.myring)
+#         singular_ring_delete(self.myring, self.ref)
 #
 # cdef class myclass_reference():
-#     cdef ring* refring;
-#     def __cinit__(ring* some_ring):
-#         self.refring = singular_ring_reference(some_ring)
+#     cdef ring *refring
+#     cdef int *ref
+#     def __cinit__(ring *some_ring, int *refcount_for_some_ring):
+#         self.ref = refcount_for_some_ring
+#         self.refring = singular_ring_reference(some_ring, self.ref)
 #     def __dealloc__():
-#         singular_ring_delete(self.refring)
+#         singular_ring_delete(self.refring, self.ref)
 #
 # You must not refer to Python/Cython classes in the Cython
 # destructor, the following is INVALID:
@@ -48,10 +54,8 @@ from sage.libs.singular.decl cimport ring
 cdef ring *singular_ring_new(base_ring, n, names, term_order) except NULL
 
 # reference an existing ring
-cdef ring *singular_ring_reference(ring *existing_ring) except NULL
+cdef ring *singular_ring_reference(ring *existing_ring, int *refcount) except NULL
 
-# carefully delete a ring once its refcount is zero
-cdef void singular_ring_delete(ring *doomed) noexcept
-
-# Used internally for reference counting
-cdef wrap_ring(ring* R)
+# carefully delete a ring once its refcount is zero, in that case
+# also deallocating refcount
+cdef int singular_ring_delete(ring *doomed, int *refcount) except 1